Conversation
…t_firmware into 432-new-mixing-system
…and 14 equations to calculate the max thrust given a throttle setting (since RC commands come in as throttle settings, not forces).
…bility under aggressive maneuvers
…'t work in the current implementation
…calculating max thrust
bsutherland333
left a comment
bsutherland333
left a comment
There was a problem hiding this comment.
Overall really great work, I think this will be a welcome improvement to ROSflight, especially once we have an easy way to configure the mixing from a base station.
|
I think that before we merge this we should look at ways to make the unit test not freak out at the build. I wonder if we could just make the compilation a little more lax? Give an exception for Eigen? I'm not sure. |
|
I think the approach needs some more discussion. I would rather start with just a mixer matrix that can be loaded from the companion computer, or from selected standard cases, and stored as parameters. |
… array of pointers
|
@avtoku FYI I added the suggestions from our code review that I did not implement to the issues tab. These were things that I thought weren't as urgent for this PR. Thanks for the suggestions! |
JMoore5353
dismissed
bsutherland333’s stale review
Outdated... Lots has changed since this review.
There was a problem hiding this comment.
This relates to my previous comment. I think that the 1.225 should be a settable parameter.
There was a problem hiding this comment.
But again I don't think that this should necessarily be part of this PR.
| EXPECT_EQ(output.Qz.type, RATE); | ||
| EXPECT_CLOSE(output.Qz.value, 0.0); | ||
| EXPECT_EQ(output.Fz.type, THROTTLE); | ||
| EXPECT_CLOSE(output.Fz.value, 0.0); |
There was a problem hiding this comment.
The Controller::Output makes this much more clear!
iandareid
left a comment
iandareid
left a comment
There was a problem hiding this comment.
Great and very understandable changes. I think we should talk about the flags more in depth. I think I may be missing something.
bsutherland333
left a comment
bsutherland333
left a comment
There was a problem hiding this comment.
This looks like a lot of really great work. I think this will be a great improvement and don't have much else to suggest, other than we should definitely spend some time testing this on real hardware and make sure every feature works as expected.
There was a problem hiding this comment.
Are we sure we don't want to implement an array type parameter so we don't need to have this many parameters?
There was a problem hiding this comment.
I think we do. I opened a separate issue for that.
4 participants
Updated the mixer for multirotors to make it based on the Chapter 14 implementation. For multirotors, since the output of the controller is forces and torques, the mixer computes the desired angular speeds based on the motor model parameters.
For fixedwing vehicles, the mixer architectre stayed the same, since ROSplane (and RC) commands come in as control surface delta commands, or as a throttle setting.
Note that this PR does not expand the mixing definition to include forces in all three axes, and it doesn't add support for a dynamically-changing mixing matrix, both of which were mentioned in #432. The architecture currently only supports a "fixedwing" or "multirotor" mixing mode. The other changes will happen when we need to support them (i.e. for quadplane or tiltrotor).