-
Notifications
You must be signed in to change notification settings - Fork 13.6k
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
Refactor MC attitude controller: use module base and Param classes #9113
Conversation
Don't worry about the conflict, I'm fine either way and this looks more important. One smaller thing it might be time to discuss is naming. We have a mix of headers with .h (c & c++) and .hpp extensions. Some headers are named to match the module, some named to match the only class they contain. Personally I have a minor preference for c++ headers that are named to match the containing class, but really the only thing I can't stand is inconsistency. I don't want to get carried about with a detailed style guide, let's just try and get the main contributors roughly on the same page. |
Same for me. Naming after the class name is not that important, as you can have multiple classes. But the |
Looks very good on the first glance! I volunteer to switch completely to matrix library as a follow up to this pr. This was the plan for some time already. |
@MaEtUgR that would be great! Converting the multicopter controllers should be straightforward (and safe). After that and a few outstanding PRs (#9121, #9104, #9103) we're actually very close to being able to drop mathlib matrix entirely. That will help free up the flash needed for flight tasks on fmu-v2. |
0481f78
to
529fd1f
Compare
//math::Vector<3> rates_i_scaled = _rate_i.emult(pid_attenuations(_tpa_breakpoint_i.get(), _tpa_rate_i.get())); | ||
math::Vector<3> rates_d_scaled = _rate_d.emult(pid_attenuations(_tpa_breakpoint_d.get(), _tpa_rate_d.get())); | ||
Vector3f rates_p_scaled = _rate_p.emult(pid_attenuations(_tpa_breakpoint_p.get(), _tpa_rate_p.get())); | ||
//Vector3f rates_i_scaled = _rate_i.emult(pid_attenuations(_tpa_breakpoint_i.get(), _tpa_rate_i.get())); |
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.
@MaEtUgR we don't have to resolve it in this PR, but did you notice this line? We shouldn't carry around commented out code, so it would be good to address at some point.
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.
@dagar this will conflict a bit with #9103. I can do the rebase, should it be accepted and this be merged first.