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

Parameter: CRASH_DETECT Param doc update to add 1 in value set #27070

Closed
wants to merge 1 commit into from

Conversation

loki077
Copy link
Contributor

@loki077 loki077 commented May 16, 2024

As this was causing a CI check we made locally to compare param values to param_metadata generated xml file.

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.

I've corrected the commit message and marked this for DevCall in case it doesn't get merged sooner.

Note that some of these fields are guidelines only; not necessarily Values, but certainly Ranges.

Copy link
Contributor

@magicrub magicrub left a comment

Choose a reason for hiding this comment

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

Why not update the bitmask field too?

@peterbarker
Copy link
Contributor

Why not update the bitmask field too?

Fair call. In fact, that is the correct fix, as both MAVProxy and MissionPlanner ignore the @values field if @bitmask is present!

I have created #27082 to enforce the same rule as we have had in libraries for years - you can only have one of @values or @bitmask

@loki077 you could simply remove the @values as part of this PR - or we could merge mine which will close this one as Merged.

As this was causing a CI check fail, which we made locally to compare param range from metadata. We only need bitmask or values and not both, in this case we are removing values.
@loki077
Copy link
Contributor Author

loki077 commented May 17, 2024

@peterbarker I have forced pushed the change. You can also make it a part of your PR.

@peterbarker
Copy link
Contributor

@peterbarker I have forced pushed the change. You can also make it a part of your PR.

I've merged that other PR now - so closing this one.

Thanks!

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.

None yet

3 participants