Skip to content

Conversation

Tory9
Copy link

@Tory9 Tory9 commented Sep 4, 2025

What does this PR do?

This PR implements support for collecting GNSS resilience data from Septentrio receivers and relaying it to the Ground Control Station (GCS).


Why is this change needed?

GNSS resilience information—such as spoofing, jamming, and authentication states—is crucial for operational safety and informed decision-making. Providing this data to the GCS enhances an operator's situational awareness, allowing for better flight planning and in-flight control.


How is it implemented?

The Septentrio driver has been modified to:

  • Auto-configure the receiver to output the SBF blocks containing resilience information (if auto-config is enabled).
  • Parse these new SBF message types.
  • Map the extracted data to the MAVLink GNSS_INTEGRITY message.
  • Add the GNSS_INTEGRITY message to the STREAM_EXTENDED_STATUS to ensure it is sent to the GCS.

Related Work & Context


How has this been tested?

The feature was validated on a CubeOrange+ using a replayed SBF log file that was recorded in an environment with active jamming and spoofing.

Copy link
Contributor

@peterbarker peterbarker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice to see some effort being put in to report this stuff.

Comment on lines 1491 to 1502
uint8_t id = instance;
uint32_t system_error = get_system_errors(instance);
uint8_t authentication = get_authentication_state(instance);
uint8_t jamming = get_jamming_state(instance);
uint8_t spoofing = get_spoofing_state(instance);
uint8_t raim_state = UINT8_MAX; //not implemented yet
uint16_t raim_hfom = UINT16_MAX; //not implemented yet
uint16_t raim_vfom = UINT16_MAX; //not implemented yet
uint8_t corrections_quality = UINT8_MAX; //not implemented yet
uint8_t systems_status_summary = UINT8_MAX; //not implemented yet
uint8_t gnss_signal_quality = UINT8_MAX; //not implemented yet
uint8_t post_processing_quality = UINT8_MAX; //not implemented yet
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't bother making these locals, just throw them straight into the packet; add comments to indicate which field is being passed where things are unclear.

uint8_t systems_status_summary = UINT8_MAX; //not implemented yet
uint8_t gnss_signal_quality = UINT8_MAX; //not implemented yet
uint8_t post_processing_quality = UINT8_MAX; //not implemented yet
GCS_SEND_TEXT(MAV_SEVERITY_INFO, "gps %u with auth %u and spooof %u", id, authentication, spoofing);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Diagnostics to be removed :-)


/// GPS error bits. These are kept aligned with MAVLink by
/// static_assert in AP_GPS.cpp
enum GPS_Errors {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make these enum-class and contained within the AP_GPS class? Would allow for dropping the extra namespacing.

Comment on lines 174 to 175
enum GPS_Authentication {
AUTHENTICATION_UNKNOWN = 0, ///< The GPS receiver does not provide GPS signal authentication info
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
enum GPS_Authentication {
AUTHENTICATION_UNKNOWN = 0, ///< The GPS receiver does not provide GPS signal authentication info
enum Authentication {
UNKNOWN = 0, ///< The GPS receiver does not provide GPS signal authentication info

... and move to within AP_GPS:: namespace. This reduces a lot of duplication.

The enumeration you're modelling this on is one of three enuemations we use for status type in ArduPilot - but it's also the one I believe we're trying to remove!

return state[instance].gps_yaw_configured;
}

// general errors in the GNSS system
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure we really need these accessors, but if you do want to keep them please make them private.

Comment on lines 544 to 546
uint32_t get_system_errors() const {
return get_system_errors(primary_instance);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is used?

We have a no-dead-code policy which we mostly enforce....

Similarly for other "primary_instance" methods

Comment on lines 595 to 597
if (RxError & INVALIDCONFIG) {
state.system_errors |= AP_GPS::GPS_Errors::CONFIGURATION;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if a static const data structure here which is iterated over might make this a bit nicer / simpler to extend later / more compact byte-wise.

}
state.jamming_state = AP_GPS::GPS_Jamming::JAMMING_OK;
for (int i = 0; i < temp.N; i++) {
RFStatusRFBandSubBlock* rf_band_data = (RFStatusRFBandSubBlock*)(&temp.RFBand + i * temp.SBLength);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This construct makes me unhappy.

At the very least you can (a) create a pointer you can index into and (b) use a reference for each of the entries in the array for pulling things out.

But I'd really also like to see a belt-and-bracers check that we don't step outside the packet here. When pointer games go wrong vehicles get lost. Right here if temp.N is corrupt (which can happen if the packets gets through the checksumming while invalid, or if the GPS device itself puts a bad value in) we could try to access memory we don't have access to and fall out of the sky. We had a similar problem with massive packet loss in this driver that we fixed with #29399

static const ap_message STREAM_EXTENDED_STATUS_msgs[] = {
MSG_SYS_STATUS,
MSG_POWER_STATUS,
MSG_GNSS_INTEGRITY,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know whether we will want to stream this message by default. Telemetry bandwidth is actually one of those things that doesn't seem to be growing for a lot of users!

Users can get the autopilot to start streaming the message in many ways if we don't decide to stream this by default.

@Tory9
Copy link
Author

Tory9 commented Sep 9, 2025

@peterbarker , thank you for a detailed review. I have considered your suggestions and applied some changes. I have a question about streaming this message by default. Since I'm new to ArduPilot, could you mb hint at the best way to introduce this to the community?

@Tory9 Tory9 requested a review from peterbarker September 11, 2025 07:39

#if AP_MAVLINK_MSG_GNSS_INTEGRITY_ENABLED
// general errors in the GNSS system
uint32_t get_system_errors(uint8_t instance) const {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a blocker but one suggestion that was raised on the dev call that that it might be worth combining all of these get functions into one function as they are all called in once in the same currently.

@MattKear
Copy link
Member

A general comment that was made on the dev call was that it would be good if an accessor variable can be added. If the GPS is sending the relevant information then the variable is set true. That way any get functions could return early if the accessor if false.

#if AP_MAVLINK_MSG_GNSS_INTEGRITY_ENABLED
if (temp.Flags==0) {
state.spoofing_state = static_cast<uint8_t>(AP_GPS::Spoofing::OK);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: the brackets and else should be on the same line

@rmackay9
Copy link
Contributor

Could you double check that each commit only affects a single subsystem? Also each commit should be prefixed with the subsystem affected. E.g. "AP_GPS: add xxxx"

Finally could you rebase on master to get rid of the "merge" commit?

@peterbarker
Copy link
Contributor

@peterbarker
Copy link
Contributor

@rmackay9 rmackay9 added the WikiNeeded needs wiki update label Sep 24, 2025
@Georacer
Copy link
Contributor

Georacer commented Sep 24, 2025

Discussion on today's Dev Call:

Peter: The feature should be define-fenced for 1M flash boards.
Randy: Please split the commits into one for each subsystem.
P: What is the size-cost to the output binary when the feature is left undefined? It would be nice to have this as small as possible, ideally zero.
R: It’s very zero for small boards, it’s a lot for larger boards.
P: A couple dozen bytes extra when the feature is turned off is okay. Anything larger is a problem.
R: We also need a build option for the build server, in the build_options.py file.
Victoria: I’ve added the message on the EXTRA_3 stream, but I don’t see it in QGC.
P: Adding the message ID on the stream ID list is one way to have the message sent.
Another is to have the GCS request for this message on-demand.
QGC probably doesn’t have compiled-in
You can add a SEND_STATUSTEXT macro before the place where you try to send the message and see if this code path is being traversed.

R: If the feature is off by default, we should be streaming the message in this case.
P: Agreed.
Matt: We can have it off by default, we don’t know how much usage this feature will get.
P: It also needs a change in our Wiki, to document this feature.

@Tory9 Tory9 force-pushed the pr-resilience3 branch 2 times, most recently from 74536d8 to 2fdd96a Compare October 7, 2025 20:43
@Tory9
Copy link
Author

Tory9 commented Oct 8, 2025

Hello @peterbarker,

I was thinking about the issue of memory being occupied by this feature even if it's not used, which is a problem for boards with smaller flash memory. However, on the other hand, requiring a custom build—even with the great web tool ArduPilot has—might discourage some users from using the feature.

Thus, I've implemented the following solution:

  • The feature's code is excluded at compile time for boards with less than 1MB of memory.
  • For boards with more memory, the feature code is included, but if the connected receiver is not a Septentrio receiver, then the message is not streamed.
  • If users with smaller flash on their controller want to include the feature, they can still do so with a custom build.
    Please let me know if this approach is okay and if I correctly made the custom build implementation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

WikiNeeded needs wiki update

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants