-
Notifications
You must be signed in to change notification settings - Fork 13.3k
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
mc_pos_control: pass on parameter metadata #21729
Conversation
5717b14
to
3fb82d9
Compare
Upon request, I separated the two steps of file splitting and metadata revision into two commits. Now it should be easier to review. |
Do we want to consider exposing any of this in separate parameter groups so that anyone without the source code open can possibly benefit? |
Yes and rename some parameters and remove some and make the effect they have more clear e.g. not use the same acceleration for independent things. I planned to do this in further steps covering individual parts. To me there are too many parameters to clean them up all at once that's why I started with these updated descriptions and the code only separation is to help with the next steps. |
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.
Very nice push! Just a couple of nitpicks:
- I wouldn't constrain the decimals too much, eg often I would put 1 instead of 0
- a couple of times you remove the .0 from the
@min
/@max
, but you missed a couple as well
src/modules/mc_pos_control/multicopter_position_control_gain_params.c
Outdated
Show resolved
Hide resolved
src/modules/mc_pos_control/multicopter_position_control_limits_params.c
Outdated
Show resolved
Hide resolved
src/modules/mc_pos_control/multicopter_position_control_limits_params.c
Outdated
Show resolved
Hide resolved
* | ||
* MPC_POS_MODE | ||
* 1 just deceleration | ||
* 3 acceleration and deceleration | ||
* 4 just acceleration | ||
* 4 use MPC_ACC_HOR instead |
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 it's not used with 4? Then maybe "not used, use MPC_ACC_HOR instead"
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.
Makes sense, I changed the working 👍
That's a parameter to untangle. It should just be autonomous acceleration and manual acceleration period. I reused that parameter at the time because during the transition it made sense in terms of what values were required compared to the FlightTaskManualPositionSmoothVel
(pos mode 3) and even much before that this parameter was the slew rate of the velocity controller so it was already reused and not renamed for a smooth transition before.
I'd separate those to the configuration options that have a clear name and only change one thing that the user actually cares about.
|
||
/** | ||
* Minimum manual thrust | ||
* Minimum collective thrust in Stabilized mode |
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.
Can't we actually combine this param with the one that limits the min throttle in Auto modes? Or just keep it to max range in Stabilized mode?
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.
My plan for these minimum throttle ones is to have a limited airmode up to this amount of throttle independent of the mode. We can theoretically merge them together before. I'm assuming they are different because 12% throttle is pretty high for minimum stick and 8% throttle is rather low for the position controller to go for if there's no airmode. It's all the same issue in my eyes. Minimum should be 0% should be spinning props at idle and they only spin up if torque is needed -> airmode but can't make the drone go nuts -> limited airmode.
because the original file was almost 1k lines and the order of parameters more historical than after topic.
- Adapted descriptions to be more clear - Adjusted some limits and decimals
3fb82d9
to
b34df9d
Compare
Thanks @sfuhrer for the review. You exactly hit the important points that need to be improved. I rebased on the main branch and added commits to address the feedback, see comments. I plan to follow up with any user-facing restructuring like renaming, regrouping, using different parameters.
Solved in d25938a |
@MaEtUgR I think it is nice to also remove the trailing zeros in the param meta data, though how they are currently displayed in the param reference is then a bit ugly (as the "f" is missing there). I guess it would be best if either the trailing "." in the meta data wouldn't be displayed on the website, or is changed to a ".0". Would you have an idea where to propose that? |
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.
My review points have all been addressed, looks clean!
@sfuhrer Thanks for checking again.
I don't find it too ugly but we can look into how to remove the trailing point. 👀 |
Solved Problem
When checking changes in multicopter position control parameters I realized there are too many of them and they also affect many parts of the system that are not directly related to position control itself.
Solution
In an effort to change this but not have too much back and forth in the naming I too kan initial pass and
Changelog Entry
For release notes:
Alternatives
Based on this we can remove unnecessary parameters, rename them, further separate them into their corresponding groups and make sure the same parameter is not mistaken for too many things.
Test coverage
No specific testing. I adjusted based on my past experience.