FW attitude control scaling fixes and cleanup #15256
Conversation
This commit aligns the scaling better with the derivations in https://dev.px4.io/master/en/flight_stack/controller_diagrams.html#airspeed-scaling Integrator terms now scale with IAS^2 (all three axes) To match the roll and pitch controllers: - Yaw integrator scale is now applied during accumulation, not to integral value (so now FW_YR_IMAX is respected more intuitively) - Yaw FF term now scale with IAS instead of IAS^2 Also made a number of small changes to make the three files (roll, pitch, yaw) 3-way diffable to be clearer about what the differences among them are.
|
@bresch FYI, applied some of the scaling fixes as suggested in your reference document. |
| /* input conditioning */ | ||
| float airspeed = ctl_data.airspeed; | ||
| /* Close the acceleration loop if _coordinated_method wants this: change body_rate setpoint */ | ||
| if (_coordinated_method == COORD_METHOD_CLOSEACC) { |
There was a problem hiding this comment.
You could remove _coordinated_method entirely.
- "Coordination method" open vs. closed code removed, since closed is never used and not actually implemented. - No change to behavior
|
Ultimately, the functionality is trivial enough that it could go in FixedWingAttitudeControl, clearing out all of this. I find the back and forth tough to follow. |
The current idea is to absorb ECL_* entirely into FixedwingAttitudeControl at some point. I think the end result would overall be much simpler and easier to follow. |
|
Not familiar with the CI tests -- one is failing, but doesn't seem like it could possibly be caused by this change: |
It actually seems fairly likely given the change modified the FW controller and only the FW mission tests is failing. Unfortunately we didn't even get a log. Have you tried SITL locally? A simple mission takeoff, waypoint, and planned landing? |
|
@dagar I'll try now. Just checking that the CI system isn't known to be fragile, or that this was a common mistake for beginner contributors. (We use a custom SITL in-house, so I'm not familiar with Gazebo.) |
|
Apologies, I've had no luck installing Gazebo (tried several methods). On our in-house simulator, these changes fly normally in SITL, but we aren't able to run I stand by my surprise that removing I can try reverting the last commit, and see if that was the problem or other commits on master. |
|
Ok after reverting, first the MAVROS rover (!) test failed for a while, but now it's passing. Intermittent issue? I'll add back the last commit. |
|
Percussive maintenance successful, ready for review again. FYI, I notice various MAVROS tests failing on several recent commits. Seems random. They all seem to involve a segfault. |
Where? |
|
@dagar: Other than mine, I see the segfaults on these CI runs: |
|
@bresch Made your requested change, and all tests passing. |
Thanks for pointing those out. Please create issues for test failures like that as they come up. I'd like to get CI to the point where we can really trust it. |
Opened issue for the CI failures here: #15300 |
|
Two flights with this pr, no issues detected: |
This commit aligns the scaling better with the derivations in
https://dev.px4.io/master/en/flight_stack/controller_diagrams.html#airspeed-scaling
Integrator terms now scale with IAS^2 (all three axes)
To match the roll and pitch controllers:
integral value (so now FW_YR_IMAX is respected more intuitively)
Also made a number of small changes to make the three files
(roll, pitch, yaw) 3-way diffable to be clearer about what the
differences among them are.
Describe problem solved by this pull request
Note, this does not handle #11975 -- according to the reference document, the FF gains should be scaled with TAS instead of IAS, but that will require some more invasive work.