-
Notifications
You must be signed in to change notification settings - Fork 13.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Change ecl controllers to not transform body angular rates #2669
Conversation
…data.angle_rate to angular acceleration as the angular rates are already angular accelerations as calculated by ekf_att_pos_estimator
Awesome, very much appreciated! We did an internal review of the same code location this afternoon and came to the same conclusion. |
/* calculate the offset in the rate resulting from rolling */ | ||
//xxx needs explanation and conversion to body angular rates or should be removed | ||
float turn_offset = fabsf((CONSTANTS_ONE_G / airspeed) * | ||
tanf(roll) * sinf(roll)) * _roll_ff; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The roll feed forward is required to prevent the nose from dipping down during turns. Its not "right" but "works" very effectively. Do you have a proposal how to retain this compensation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to be testing this with and without feed forward compensation this week as well. Even if feed forward compensation is necessary I think _pitch_ff should be applied as I'm guessing people will not think to adjust roll gains after seeing the nose pitch down or bad pitch tracking from log files.
merging master so pull request remains current. Also will be able to test PR in HIL thanks to PR PX4#2683
Availible on google books 8/11/2015: | ||
https://books.google.com/books?id=ubcczZUDCsMC&pg=PA175#v=onepage&q&f=false*/ | ||
float body_fixed_turn_offset = (fabsf((CONSTANTS_ONE_G / airspeed) * | ||
tanf(roll) * sinf(roll))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this is the same compensation as above but without _roll_ff param? Is the param not needed anymore to tune this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, this is the same compensation above without the feed forward term though you will notice that it then gets transformed from a body fixed offset (e.g angular velocity) to an euler rate offset later. I'll be looking into the affect of the lack of feed forward this week, though theoretically it should not be applied here. _ptich_ff would of course still drastically affect the performance, as this is only adding in compensation pitch when the vehicle is rolling.
adding in new functionality from PX4 Firmware master. This will allow testing controllers in HIL sim
For anyone interested I have run a few sims with the proposed changes. I am going to try to draw some conclusions from them with MATLAB analysis but here are the munched logs: I know its a lot to look at, but I'm hoping data reduction will tell us more. |
Thanks! Looks like the proposed ECL changes as-is reduce the jitter in the roll reference and have similarly good tracking as the version without jacobians. This is the opposite of what I expected. I have some idea why this could still make sense, but definitely worth more discussion. |
I agree that the roll/pitch rates look less noisy in the runs with jacobians, but I don't understand what those transforms are supposed to do. Also, it looks like the inverted flag should be set in the inverted branch of the roll constraint block for turn_offset to work correctly in ecl_pitch_controller. |
Hi @scott-eddy Thanks a lot for the pr, it was definitely wrong to transform the angular rates from the ekf. I had a look at the data and it is tough for me to have a clear favorite, althgough personally I would go for your changes + pitch feedforward. |
I did a testflight flying the same auto mission with two versions:
|
ok - so how is the attitude and rate tracking? Please just upload them to log muncher for a more complete picture. |
Log files:
|
@LorenzMeier Attitude tracking looks very similar. |
@Tumbili Thanks for actually flight testing this, unfortunately getting actual flights in for me is very difficult so I'm stuck to the simulation environment for most development. I'm doing some additional MATLAB processing on my logs and yours will provide some more insight. Looking the aggressive roll case at 8:59:32-9:01:02 and 1:04:14-1:05:44 respectively, I see the transformation achieving better pitch angular rate tracking and the setpoint looks like it is slightly less noisy. @boosfelm Interesting idea handling the turn offset in the bodyrate controller. I think this is a really good idea and I will run a sim with that logic. I'm hesitant to keep applying the _pitch_ff term to this turning offset. If a user needs a relatively large feed forward gain in order to achieve pitch tracking with no roll angle when in a turn they will now be applying that large gain they tuned for pitch tracking to a term they probably aren't aware of. However, I'd be willing to include it if the majority of devs think it best to keep it in. |
Okay, hopefully this will put the issue to bed. I will post images of the MATLAB graphs I generated below this comment and list the statistics here and finally follow with a conclusion. Sorry for being verbose but I suppose more info is always better than less
mean pitch error std pitch error mean rate error std rate error mean pitch error std pitch error mean rate error std rate error Now, I then ran two simulations with the proposed changes as they stand and with the turn offset being handled in the rate controller. Here the sims suggest that handling the turn offset in the rate controller is better. This intuitively makes sense as you are no longer adding noise to the system by multiplying by the yawrate, but you are handling the offset as a body angular rate instead of as an euler angle rate
mean pitch error std pitch error mean rate error std rate error mean pitch error std pitch error mean rate error std rate error |
Conclusions: I will push the changes with the turn offset in the rate controller if everyone agrees with this, I'd also be happy to wait to merge the changes until someone can flight test the new code. Let me know if there are more questions or issues. |
Sounds good. I'm very, very happy about the level of analysis done here, this is really pushing things forward. Happy to merge the changes as proposed. |
Okay, I will clean up some whitespace and comments and push the implementation with turn offset handeled in the rate controller after lunch. It will be up in less than one hour. |
…of pitch controller. Also set inverted flag to true if rolled more than 90 degrees
Rebased and applies, thanks! |
Updated ecl_roll/pitch/yaw_controller.cpp to not transform estimator angular rate measurements to the body frame as these are output in the body frame by the ekf.
Also updated and explained the logic for coordinated turns in the ecl_pitch_controller, which currently has a comment "//xxx needs explanation and conversion to body angular rates or should be removed"