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

SITL parameters documentation update #28053

Merged
merged 2 commits into from
Sep 11, 2024

Conversation

YuxinPan
Copy link
Contributor

Partially addressing #22903

Passed Sim helper check and CI check:
image

image

Will squash commits once approved.

@peterbarker

libraries/SITL/SITL.cpp Show resolved Hide resolved

// @Param: DRIFT_SPEED
// @DisplayName: Gyro drift speed
// @Description: Gyro drift rate of change in degrees/second/minute
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these really in minutes?! That's rather odd. Not saying you're wrong, but please make sure its right!

Also, we can put units into separate fields, like this:

Suggested change
// @Description: Gyro drift rate of change in degrees/second/minute
// @Description: Gyro drift rate of change
// @Units: deg/s/s

Known units are here: https://github.com/ardupilot/ardupilot/blob/master/Tools/autotest/param_metadata/param.py#L66

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is indeed uncommon. I referred to the SITL.h where the comment states: AP_Float drift_speed; // degrees/second/minute
Also, I see double minutes = fmod(AP_HAL::micros64() / 60.0e6, period); if (minutes < period/2) { return minutes * ToRad(sitl->drift_speed); } return (period - minutes) * ToRad(sitl->drift_speed);

Copy link
Contributor

Choose a reason for hiding this comment

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

Great, thanks!

Sometimes the hard-to-believe things just make you do more work :-)

libraries/SITL/SITL.cpp Outdated Show resolved Hide resolved
libraries/SITL/SITL.cpp Outdated Show resolved Hide resolved
libraries/SITL/SITL.cpp Show resolved Hide resolved
libraries/SITL/SITL.cpp Outdated Show resolved Hide resolved
"SIM_BAR2_DISABLE", # not listed in SITL.cpp
"SIM_BAR2_DRIFT", # not referenced anywhere
"SIM_BAR2_FREEZE", # not listed in SITL.cpp
"SIM_BAR2_WCF_BAK", # not referenced anywhere
Copy link
Contributor

Choose a reason for hiding this comment

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

Everything in this list is used - we have a sanity check for that ;-)

In this case the parameters are coming from in here: https://github.com/ardupilot/ardupilot/blob/pr%2Fstop-using-python-is-python3/libraries/SITL/SITL.cpp#L589

Fixing this requires moving the BaroParm structure to a new file, probably SITL_Baro.cpp . @antholuo did the same thing to document airspeed by creating SITL_Airspeed.cpp, so use that as a model

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks I can follow the approach in #25005. Do you want that to be a separate PR or included in this one?

Copy link
Contributor

Choose a reason for hiding this comment

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

Separate PR makes sense. Makes these easy to review!

Tools/autotest/vehicle_test_suite.py Outdated Show resolved Hide resolved
Copy link
Contributor

@peterbarker peterbarker left a comment

Choose a reason for hiding this comment

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

LGTM

Tools/autotest/param_metadata/param.py Show resolved Hide resolved
Tools/autotest/param_metadata/param.py Show resolved Hide resolved

// @Param: DRIFT_SPEED
// @DisplayName: Gyro drift speed
// @Description: Gyro drift rate of change in degrees/second/minute
Copy link
Contributor

Choose a reason for hiding this comment

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

Great, thanks!

Sometimes the hard-to-believe things just make you do more work :-)

@peterbarker peterbarker force-pushed the pr/sitl_docs_update branch 2 times, most recently from 46763d6 to 7bb6241 Compare September 10, 2024 07:45
YuxinPan and others added 2 commits September 10, 2024 17:45
SITL: vehicle_test_suite.py parameters removal from whitelist

SITL: Add known unit amp hour

SITL: Add known unit Ah

Co-authored-by: Peter Barker <[email protected]>
@peterbarker peterbarker merged commit e7359c1 into ArduPilot:master Sep 11, 2024
94 checks passed
@peterbarker
Copy link
Contributor

Merged, thanks!

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.

2 participants