mavlink / mavros

MAVLink to ROS gateway with proxy for Ground Control Station
Other
879 stars 989 forks source link

mavros_msgs HILGPS message documentation contradiction #1628

Open JamesSewellAI opened 3 years ago

JamesSewellAI commented 3 years ago

Issue details

The documentation for MAVROS HILGPS references the MAVLINK documentation for HILGPS but does not conform to the same SI units as MAVLINK. I understand that ROS implements REP103 and REP105 for unit standards but this is extremely confusing when the referenced documentation states otherwise and can (and has...) result in an aircraft crashing when using HILGPS in a GPS assisted mode. An example of these unit differences are

Parameter / MAVLINK Defined SI Unit / MAVROS Required SI Unit

MAVROS version and platform

Mavros: 1.8.0 ROS: Melodic Ubuntu: 18.04.6

Autopilot type and version

~[ ] ArduPilot~ [ * ] PX4

Version: 1.11.0

Proposed feature

Add comment in mavros_msgs/HilGPS documentation to state the units to be used for these parameters.

vooon commented 3 years ago

Hmm, at first i supposed that message is direct representation of mavlink one, so likely shouldn't have any translation (with exception to GeoPoint), but that's not the case:

https://github.com/mavlink/mavros/blob/87f2b114206057595ef4ed72e75057aed05e71cb/mavros_extras/src/plugins/hil.cpp#L194-L222

That's looks like misunderstanding during development, as we probably should use Vector3 for velocity and float32 for other scaled items. Or leave them without any conversion at all.

Problem that either of that changes would break third party software. Change in message would be less troublesome as users would get checksum or build errors, rather silently do unexpected translations.

JamesSewellAI commented 3 years ago

@vooon thanks for the quick response. What would be the proposed solution to this issue then? I am fine to use it in the current state as I am now very aware of the issue but I don't want to progress the work further if an interface change will break everything up to that point. I guess the question is whether the interface should conform with ROS REP103/105 or MAVLINK.

vooon commented 3 years ago

@JamesSewellAI concidering that other messages in hil.cpp follow's ros convention, probbaly better do the same for gps too.

Ping @TSC21 .