-
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
Misprints in parameters naming and description #11680
Conversation
@@ -60,7 +60,7 @@ Standard::Standard(VtolAttitudeControl *attc) : | |||
_mc_yaw_weight = 1.0f; | |||
_mc_throttle_weight = 1.0f; | |||
|
|||
_params_handles_standard.pusher_ramp_dt = param_find("VT_PSHER_RMP_DT"); | |||
_params_handles_standard.pusher_ramp_dt = param_find("VT_PUSHER_RMP_DT"); |
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.
@RomanBapst @sanderux You OK with the param name changes here? AFAIK parameters can be max 16 chars, and this is IMO better.
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.
"VT_PUSHER_RMP_DT" - there are exactly 16 characters
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.
Yes @EgoPingvina - I checked and I am happy with that, but I'm not maintainer of this code - hence query to @RomanBapst
change "higher" to "tighter" add an article before "voltage"
@dagar @hamishwillee Do we have a standard procedure how we deal with parameter renaming? |
A question for @dagar. On one hand we do need to be able to change things, but on the other, I question whether this particular change is worth the potential impact. Either way, minimally we need to accumulate the changes and clearly publish them. Even better, a script that people could run to do a migration. |
@EgoPingvina , you'll want to change the parameter name here now as well, (new since you submitted this PR). Hopefully this helps out! |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
@EgoPingvina , if you can rebase this PR against current master and fix the additional instance I linked above we could bring up your PR in the next dev call and try to get this merged in. Find me on PX4 slack if you need a hand rebasing. |
If it's OK, let's close this PR as it's getting old and not being kept updated, and I'll attempt to attend to the requisite work of dealing with renaming parameters in the months ahead and keep track with #12619. (Please re-open if anyone has a different preference!) |
I worked with QGroundControl and seen some misprints