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

[Discuss] MAV_PROTOCOL_CAPABILITY_PARAM_FLOAT - redefine meaning #1769

Closed
wants to merge 1 commit into from

Conversation

hamishwillee
Copy link
Collaborator

@hamishwillee hamishwillee commented Jan 5, 2022

This is created for discussion. Don't merge until consensus has been reached.

MAV_PROTOCOL_CAPABILITY_PARAM_FLOAT is unclear about what it supports.

Autopilot supports the new param float message type.

However there is no obvious float message type. There are messages that take floats to set and get the protocol. My strong guess is that this was originally set when the parameter protocol was created to indicate those messages, and more broadly the parameter protocol. It is possible it means support for parameters of type MAV_PARAM_TYPE_REAL32 - while I think ArduPilot only supports INT32 parameters. This is set by PX4 but not ArduPilot.

I have assumed here it was meant to mean "supports the parameter protocol".

Whether or not it is, I recommend that we split the parameter protocol into two versions (in docs) and add new protocol flags for them:

  • v1: Ardupilot implementation that encodes the params directly in floats, leading to rounding errors. Ardupilot help confirm the differences.
  • v2: PX4 version that encodes the params bytewise. Perhaps also has the mechanism to get the cache - tbd.

If it means support for MAV_PARAM_TYPE_REAL32 we should update the docs appropriately and also add a protocol bit to indicate support for the INT32 type (though this can be assumed). Unless we have a better way to indicate the supported types?

This is first step to allowing a GCS to understand what version it is getting without having to know the autopilot. Ideally we might converge on a new version with this done. Either way though, we might end up with a useless protocol bit, but at least it will be a clearly defined useless bit :-)

Your thoughts appreciated.

DO NOT MERGE!

@hamishwillee hamishwillee marked this pull request as draft January 13, 2022 06:37
@auturgy
Copy link
Collaborator

auturgy commented Feb 13, 2022

I think we're overthinking this a bit. There are two active parameter encoding conventions, basically straight float (ArduPilot, 24bit precision if sending ints), and bytewise encoding (PX4, "union" to give 32 bit precision). I think we just need to revert #1771 and set that bit if using union params / bytewise encoding, and set the float bit if using straight floats.

@hamishwillee
Copy link
Collaborator Author

hamishwillee commented Feb 15, 2022

Thanks - I will close this, revert the other, and create a new PR that modifies these together. Because I think you're probably right about their intend, but I have no idea how you came to infer this from the documentation.

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

2 participants