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

AP_RPM: Giving RPM some TLC #18044

Merged
merged 7 commits into from
Aug 24, 2021
Merged

Conversation

MattKear
Copy link
Contributor

This is a minor fix to what I believe is just a hang over.

It looks like we create param arrays for _maximum, _minimum, and _quality_min yet we never give users the chance to set them on instances other than the 1st. This doesn't have any ploblems until we start trying to add more than two instances of RPM and immediatly follow on the param idx. Only noticed it whilst helping out @GiovannaAF trying to add more instances to RPM.

So we should either reduce the variable array size or add the _maximum, _minimum, and _quality_min params to the >1 instances of RPM IMHO. I opten for the former here:

Copy link
Contributor

@tridge tridge left a comment

Choose a reason for hiding this comment

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

as discussed on call, make 2_TYPE an enable param and add extra params for the 2nd instance

@tridge tridge removed the DevCallEU label Aug 4, 2021
@MattKear MattKear changed the title AP_RPM: Fix parameter memory allocation AP_RPM: Giving RPM some TLC Aug 9, 2021
@MattKear
Copy link
Contributor Author

MattKear commented Aug 9, 2021

Recommend reviewing per commit as there are a few things in this PR.

This PR now does the following:

  • Fixes the bug whereby users cannot access _MIN and _Max params for RPM2 despite there being memory allocation for them.
  • Gives each instance of RPM a _MIN_QUAL param.
  • Puts RPM params behind an enable flag
  • Re-factors the params in such a way so as to make it easier to expand the number of instances (I know of a few people wanting to do this)
  • Removes type = PWM as this is a historic hangover from the PX4 split. I need to do the param conversion for the above point anyway so I figure this is the perfect time to do it.
  • Minor whitespace tidy up.

libraries/AP_RPM/AP_RPM.cpp Outdated Show resolved Hide resolved
libraries/AP_RPM/AP_RPM.cpp Show resolved Hide resolved
@MattKear
Copy link
Contributor Author

Wiki update here: ArduPilot/ardupilot_wiki#3701

@MattKear
Copy link
Contributor Author

My apologies, I cannot attend the dev call but I think this PR is good to go. I would appriciate a review/potential merge.

@Hwurzburg Hwurzburg added the WikiNeeded needs wiki update label Aug 16, 2021
libraries/AP_RPM/AP_RPM.h Outdated Show resolved Hide resolved
tridge
tridge previously requested changes Aug 16, 2021
libraries/AP_RPM/AP_RPM.h Outdated Show resolved Hide resolved
libraries/AP_RPM/AP_RPM.cpp Outdated Show resolved Hide resolved
@MattKear MattKear removed the WikiNeeded needs wiki update label Aug 22, 2021
@MattKear
Copy link
Contributor Author

MattKear commented Aug 22, 2021

@Hwurzburg i already have a wiki pr to reflect these code changes.

@Hwurzburg
Copy link
Collaborator

I will make it delay merge until this gets merged...and you make the minor edits ...thanks

@MattKear MattKear force-pushed the pr_RPM_Param_Fix branch 3 times, most recently from 77087d8 to 538eb49 Compare August 23, 2021 22:31
@MattKear
Copy link
Contributor Author

Updated the param conversion note. Should be good to go once checks passed.

Copy link
Member

@IamPete1 IamPete1 left a comment

Choose a reason for hiding this comment

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

LGTM

@IamPete1 IamPete1 dismissed tridge’s stale review August 24, 2021 12:56

Changes have been made

@IamPete1 IamPete1 requested a review from tridge August 24, 2021 12:56
@tridge tridge merged commit c94b9e8 into ArduPilot:master Aug 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants