mavros_msgs HILGPS message documentation contradiction

[JamesSewellAI]

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

• Latitude / Degrees E-7 / Degrees
• Longitude / Degrees E-7 / Degrees
• Altitude / mm / m

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]

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:

mavros/mavros_extras/src/plugins/hil.cpp

Lines 194 to 222 in 87f2b11

	void gps_cb(const mavros_msgs::HilGPS::ConstPtr & req)
	{
	mavlink::common::msg::HIL_GPS gps = {};
	gps.time_usec = req->header.stamp.toNSec() / 1000;
	gps.fix_type = req->fix_type;
	gps.lat = req->geo.latitude * 1E7;
	gps.lon = req->geo.longitude * 1E7;
	// @warning geographic_msgs/GeoPoint.msg uses WGS 84 reference ellipsoid
	// @TODO: Convert altitude to AMSL to be received by the FCU
	// related to issue #529
	gps.alt = req->geo.altitude * 1E3;
	// [[[cog:
	// for f in (
	// 'eph', 'epv', 'vel', 'vn', 've', 'vd', 'cog'):
	// cog.outl("gps.%s = req->%s * 1E2;" % (f, f))
	// ]]]
	gps.eph = req->eph * 1E2;
	gps.epv = req->epv * 1E2;
	gps.vel = req->vel * 1E2;
	gps.vn = req->vn * 1E2;
	gps.ve = req->ve * 1E2;
	gps.vd = req->vd * 1E2;
	gps.cog = req->cog * 1E2;
	// [[[end]]] (checksum: b71b4e33be4574667105126a43507e82)
	gps.satellites_visible = req->satellites_visible;
	UAS_FCU(m_uas)->send_message_ignore_drop(gps);
	}

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]

@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]

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

Ping @TSC21 .