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

Misprints in parameters naming and description #11680

Closed
wants to merge 2 commits into from

Conversation

EgoPingvina
Copy link

I worked with QGroundControl and seen some misprints

xdwgood
xdwgood previously approved these changes Mar 22, 2019
@@ -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");
Copy link
Contributor

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.

Copy link
Author

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

Copy link
Contributor

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"
@RomanBapst
Copy link
Contributor

@dagar @hamishwillee Do we have a standard procedure how we deal with parameter renaming?
If somebody flies a VTOL with a non-default value of this parameter we will introduce a change to his setting about which he is not aware of, correct?
In this case I think it's not very critical but still I'd like to discuss it.
How about accumulating these changes in a document when we merge them into master? Then those notes can be part of the next release notes.

@hamishwillee
Copy link
Contributor

@dagar @hamishwillee Do we have a standard procedure how we deal with parameter renaming?
If somebody flies a VTOL with a non-default value of this parameter we will introduce a change to his setting about which he is not aware of, correct?
In this case I think it's not very critical but still I'd like to discuss it.
How about accumulating these changes in a document when we merge them into master? Then those notes can be part of the next release notes.

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.
I don't think we do, but you're right that we need to.

@mcsauder
Copy link
Contributor

@EgoPingvina , you'll want to change the parameter name here now as well, (new since you submitted this PR). Hopefully this helps out!

@stale
Copy link

stale bot commented Jul 16, 2019

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.

@mcsauder
Copy link
Contributor

@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.

@mcsauder
Copy link
Contributor

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!)

@mcsauder mcsauder closed this Sep 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants