Skip to content

Conversation

@RoseBeaupreARA
Copy link
Contributor

Protobuf message definition is defined as positive +CW, -CCW: https://github.com/mavlink/MAVSDK-Proto/blob/main/protos/manual_control/manual_control.proto#L50

MAVLink message is defined as +CCW, -CW: https://mavlink.io/en/messages/common.html#MANUAL_CONTROL

Therefore, to respect both specifications, sign must be reversed.

@hamishwillee
Copy link
Collaborator

@julianoes FWIW reminder the spec is a bit broken in respect to this message, so even if this is correct (I have not looked at it) we might have to ignore the spec. See mavlink/mavlink#1922 (comment)

@sonarqubecloud
Copy link

sonarqubecloud bot commented May 6, 2025

@julianoes
Copy link
Collaborator

julianoes commented May 7, 2025

Oh dear. You're right. However, I don't think I can change the API without a major version increase, as it would be a breaking and very surprising change!

I think we will have to rename the fields to how they are actually used in reality and then merge that before v4. That way it breaks the build and is very clearly a change.

@julianoes
Copy link
Collaborator

julianoes commented Dec 11, 2025

@hamishwillee can we talk about this on the next MAVLink dev call? I would like to get to a conclusion.

Edit: Please, thanks! :)

@hamishwillee
Copy link
Collaborator

Of course - added to next meeting https://github.com/mavlink/mavlink/wiki/20251217-Dev-Meeting - feel free to just add whatever you like to that agenda yourself.

@julianoes
Copy link
Collaborator

@RoseBeaupreARA so what happened is that PX4, ArduCopter, and QGC implement r/yaw inverted from the description, and hence that's what I tested and implemented in MAVSDK.

I am trying to "fix" the spec in mavlink/mavlink#2398.

@hamishwillee
Copy link
Collaborator

@julianoes FYI mavlink/mavlink#2398 went in. So MAVSDK is correct to spec now and IMO this can be closed.

@RoseBeaupreARA
Copy link
Contributor Author

Thanks for taking care of this!

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.

3 participants