Add Support for GPS_STATUS mavlink msg #28517

MAD-CRAZY-MAN · 2024-11-04T06:57:06Z

When using the ubx and NMEA protocols, satellite information is parsed and broadcasted via the GPS_STATUS MAVLink message.

I have also developed support for the SBF protocol but did not include it due to its high bandwidth requirements. However, it can be added upon request.

MAD-CRAZY-MAN · 2024-11-04T06:58:54Z

I have verified the GPS_STATUS message in the MAVLink Inspector.

amilcarlucas · 2024-11-04T15:11:40Z

Binary Name      Text [B]         Data [B]     BSS (B)        Total Flash Change [B] (%)      Flash Free After PR (B)
---------------  ---------------  -----------  -------------  ----------------------------  -------------------------
arducopter-heli  1108 (+0.0609%)  0 (0.0000%)  -4 (-0.0015%)  1108 (+0.0608%)                                  141168
antennatracker   1580 (+0.1171%)  0 (0.0000%)  4 (+0.0015%)   1580 (+0.1169%)                                  613220
arducopter       1124 (+0.0618%)  0 (0.0000%)  -4 (-0.0015%)  1124 (+0.0616%)                                  140624
ardurover        1568 (+0.0940%)  0 (0.0000%)  0 (0.0000%)    1568 (+0.0939%)                                  294344
arduplane        2100 (+0.1156%)  0 (0.0000%)  -4 (-0.0015%)  2100 (+0.1153%)                                  143088
blimp            1668 (+0.1217%)  0 (0.0000%)  -4 (-0.0015%)  1668 (+0.1215%)                                  591104
ardusub          1636 (+0.1015%)  0 (0.0000%)  -4 (-0.0015%)  1636 (+0.1013%)                                  350004

This takes a lot of space and should therefore be behind a feature flag

peterbarker

As @amilcarlucas says, please put this behind a feature flag.

Enable it by default on boards with >2048 kB of flash

MAD-CRAZY-MAN · 2024-11-06T08:46:01Z

@peterbarker
Thank you for your review. Could you please confirm once again?

peterbarker · 2024-11-08T00:07:38Z

libraries/AP_GPS/AP_GPS.cpp

+void AP_GPS::send_mavlink_gps_status(mavlink_channel_t chan)
+{
+#if GPS_SATELITES_INFO_ENABLED
+    mavlink_gps_status_t msg{};


Suggested change

void AP_GPS::send_mavlink_gps_status(mavlink_channel_t chan)

{

#if GPS_SATELITES_INFO_ENABLED

mavlink_gps_status_t msg{};

#if AP_GPS_SEND_MAVLINK_GPS_STATUS_ENABLED

void AP_GPS::send_mavlink_gps_status(mavlink_channel_t chan)

{

mavlink_gps_status_t msg{};

correct spelling of satellite

move compilation guards to outside of function

make the define name more descriptive

peterbarker · 2024-11-08T00:10:14Z

libraries/AP_GPS/AP_GPS.h

+        uint8_t satellites_visible;
+        uint8_t satellites_used[20];
+        uint8_t satellites_elevation[20];
+        uint8_t satellites_azimuth[20];
+        uint8_t satellites_snr[20];
+        uint8_t satellites_prn[20];


Suggested change

uint8_t satellites_visible;

uint8_t satellites_used[20];

uint8_t satellites_elevation[20];

uint8_t satellites_azimuth[20];

uint8_t satellites_snr[20];

uint8_t satellites_prn[20];

struct {

uint8_t visible;

uint8_t used[20];

uint8_t elevation[20];

uint8_t azimuth[20];

uint8_t snr[20];

uint8_t prn[20];

} satellites;

The constant 20 here is dubious; Needs to be a lot bigger than that. Also needs to be a define.

We probably want to actually dynamically allocate the structure used to hold the sv if the user asks for the information to be streamed from the autopilot.

peterbarker · 2024-11-08T00:12:46Z

libraries/AP_GPS/AP_GPS_NMEA.cpp

+                    state.satellites_visible = _gsv.num_gps + _gsv.num_glonass + _gsv.num_galileo + _gsv.num_baidou + _gsv.num_others;    
+                }
+                for (int y = 0; y < end; y++) {
+                    state.satellites_prn[y + (_gsv.this_page_num - 1) * 4]        = _gsv.svid[y];


Looks like a buffer-overflow in the making here.

Create a variable to contain the offset, constrain when it is used to actually index the array.

peterbarker · 2024-11-08T00:16:13Z

libraries/AP_GPS/AP_GPS_NMEA.cpp

+        }
+    }
+}
+#endif


Suggested change

#endif

#endif // AP_GPS_NMEA_SATELITES_INFO_ENABLED

peterbarker · 2024-11-08T00:16:29Z

libraries/AP_GPS/AP_GPS_NMEA.cpp

@@ -734,6 +779,49 @@ void AP_GPS_NMEA::parse_agrica_field(uint16_t term_number, const char *term)
    }
 }

+#if AP_GPS_NMEA_SATELITES_INFO_ENABLED


Suggested change

#if AP_GPS_NMEA_SATELITES_INFO_ENABLED

#if AP_GPS_NMEA_SATELLITES_INFO_ENABLED

similarly elsewhere

peterbarker · 2024-11-08T00:17:55Z

libraries/GCS_MAVLink/GCS_Common.cpp

@@ -1021,6 +1021,7 @@ ap_message GCS_MAVLINK::mavlink_id_to_ap_message_id(const uint32_t mavlink_id) c
        { MAVLINK_MSG_ID_RC_CHANNELS_RAW,       MSG_RC_CHANNELS_RAW},
 #endif
        { MAVLINK_MSG_ID_RAW_IMU,               MSG_RAW_IMU},
+        { MAVLINK_MSG_ID_GPS_STATUS,            MSG_GPS_STATUS},


Suggested change

{ MAVLINK_MSG_ID_GPS_STATUS, MSG_GPS_STATUS},

#if ...

{ MAVLINK_MSG_ID_GPS_STATUS, MSG_GPS_STATUS},

peterbarker · 2024-11-08T00:18:05Z

libraries/GCS_MAVLink/GCS_Common.cpp

@@ -6339,6 +6340,11 @@ bool GCS_MAVLINK::try_send_message(const enum ap_message id)
        send_raw_imu();
        break;

+    case MSG_GPS_STATUS:


Suggested change

case MSG_GPS_STATUS:

#if ...

case MSG_GPS_STATUS:

peterbarker · 2024-11-08T00:19:23Z

AntennaTracker/GCS_Mavlink.cpp

@@ -278,6 +278,7 @@ static const ap_message STREAM_EXTENDED_STATUS_msgs[] = {
    MSG_NAV_CONTROLLER_OUTPUT,
    MSG_GPS_RAW,
    MSG_GPS_RTK,
+    MSG_GPS_STATUS,


We can't stream this by default - way too much data for a normal telemetry link.

Suggested change

MSG_GPS_STATUS,

There are many ways a user could request this information from the autopilot as required - for example, a hypothetical satellites view in MissionPlanner could poke the autopilot to start sending it the data (and the autopilot would then make dynamic allocations to start collecting the data from the GPS device.

peterbarker · 2024-11-08T00:21:03Z

libraries/AP_GPS/AP_GPS_UBLOX.cpp

+                }
+                break;
+            }
+            state.satellites_used[i]       = static_cast<uint8_t>(_buffer.sat.sat_block[i].flags & 0x01);


Again, no relevant constraint placed upon the value used to index into the array; need one.

MAD-CRAZY-MAN added 3 commits November 4, 2024 15:29

AP_GPS_NMEA: Add GSV message Parsing

496f0e1

mavlink: send GPS_STATUS msg

99b1b51

AP_GPS_UBLOX: Add nav_sat message parsing

0e412ff

remove comment

1fc9f61

peterbarker requested changes Nov 6, 2024

View reviewed changes

MAD-CRAZY-MAN added 3 commits November 6, 2024 17:05

AP_GPS: fix: Replace svid with prn variable

87cdcf8

AP_GPS_NMEA: calculate satelites_visible based on GPSV

35b3571

AP_GPS: Add feature flag for satellite information parsing

2cc88d7

MAD-CRAZY-MAN requested a review from peterbarker November 6, 2024 08:43

peterbarker requested changes Nov 8, 2024

View reviewed changes

MAD-CRAZY-MAN marked this pull request as draft November 8, 2024 02:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Support for GPS_STATUS mavlink msg #28517

Add Support for GPS_STATUS mavlink msg #28517

MAD-CRAZY-MAN commented Nov 4, 2024

MAD-CRAZY-MAN commented Nov 4, 2024

amilcarlucas commented Nov 4, 2024

peterbarker left a comment

MAD-CRAZY-MAN commented Nov 6, 2024

peterbarker Nov 8, 2024

peterbarker Nov 8, 2024

peterbarker Nov 8, 2024

peterbarker Nov 8, 2024

peterbarker Nov 8, 2024

peterbarker Nov 8, 2024

peterbarker Nov 8, 2024

peterbarker Nov 8, 2024

peterbarker Nov 8, 2024

-        uint8_t satellites_visible;
-        uint8_t satellites_used[20];
-        uint8_t satellites_elevation[20];
-        uint8_t satellites_azimuth[20];
-        uint8_t satellites_snr[20];
-        uint8_t satellites_prn[20];
+        struct {
+            uint8_t visible;
+            uint8_t used[20];
+            uint8_t elevation[20];
+            uint8_t azimuth[20];
+            uint8_t snr[20];
+            uint8_t prn[20];
+        } satellites;

	#if AP_GPS_NMEA_SATELITES_INFO_ENABLED
	#if AP_GPS_NMEA_SATELLITES_INFO_ENABLED

	{ MAVLINK_MSG_ID_GPS_STATUS, MSG_GPS_STATUS},
	#if ...
	{ MAVLINK_MSG_ID_GPS_STATUS, MSG_GPS_STATUS},

Add Support for GPS_STATUS mavlink msg #28517

Are you sure you want to change the base?

Add Support for GPS_STATUS mavlink msg #28517

Conversation

MAD-CRAZY-MAN commented Nov 4, 2024

MAD-CRAZY-MAN commented Nov 4, 2024

amilcarlucas commented Nov 4, 2024

peterbarker left a comment

Choose a reason for hiding this comment

MAD-CRAZY-MAN commented Nov 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment