Skip to content
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

Merged
merged 7 commits into from
Mar 26, 2018

Conversation

bkueng
Copy link
Member

@bkueng bkueng commented Mar 20, 2018

  • use ModuleBase class and add module documentation
  • move class declaration into a separate header
  • switch all params from the C API to the C++ Param class
  • add some missing orb_unsubscribe's

@dagar this will conflict a bit with #9103. I can do the rebase, should it be accepted and this be merged first.

@dagar
Copy link
Member

dagar commented Mar 21, 2018

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.

@bkueng
Copy link
Member Author

bkueng commented Mar 21, 2018

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 .hpp vs .h suffix is currently a mess and I always wonder which one I should be using. The same goes for camel case vs underscore in filenames. I tend towards .hpp and underscore, so it's easy to distinguish between C/C++ and I think we mostly already use underscores.

@MaEtUgR
Copy link
Member

MaEtUgR commented Mar 21, 2018

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.

@dagar
Copy link
Member

dagar commented Mar 21, 2018

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.

//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()));
Copy link
Member

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dagar I just saw that it was ignored by @bkueng and you before and assumed everyone knows why it's commented out. After all you commented it: d6e9287 😄

Now I found the error: #9166

@bkueng bkueng merged commit 11ea7dc into master Mar 26, 2018
@bkueng bkueng deleted the mc_att_control_module_base branch March 26, 2018 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants