-
Notifications
You must be signed in to change notification settings - Fork 19.6k
Septentrio: Add GNSS resilience reporting #31040
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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.
libraries/AP_GPS/AP_GPS.cpp
Outdated
| 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 |
There was a problem hiding this comment.
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.
libraries/AP_GPS/AP_GPS.cpp
Outdated
| 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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Diagnostics to be removed :-)
libraries/AP_GPS/AP_GPS.h
Outdated
|
|
||
| /// GPS error bits. These are kept aligned with MAVLink by | ||
| /// static_assert in AP_GPS.cpp | ||
| enum GPS_Errors { |
There was a problem hiding this comment.
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.
libraries/AP_GPS/AP_GPS.h
Outdated
| enum GPS_Authentication { | ||
| AUTHENTICATION_UNKNOWN = 0, ///< The GPS receiver does not provide GPS signal authentication info |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| 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!
libraries/AP_GPS/AP_GPS.h
Outdated
| return state[instance].gps_yaw_configured; | ||
| } | ||
|
|
||
| // general errors in the GNSS system |
There was a problem hiding this comment.
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.
libraries/AP_GPS/AP_GPS.h
Outdated
| uint32_t get_system_errors() const { | ||
| return get_system_errors(primary_instance); | ||
| } |
There was a problem hiding this comment.
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
libraries/AP_GPS/AP_GPS_SBF.cpp
Outdated
| if (RxError & INVALIDCONFIG) { | ||
| state.system_errors |= AP_GPS::GPS_Errors::CONFIGURATION; | ||
| } |
There was a problem hiding this comment.
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.
libraries/AP_GPS/AP_GPS_SBF.cpp
Outdated
| } | ||
| 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); |
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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.
b01f2a9 to
c89ee9e
Compare
|
@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? |
libraries/AP_GPS/AP_GPS.h
Outdated
|
|
||
| #if AP_MAVLINK_MSG_GNSS_INTEGRITY_ENABLED | ||
| // general errors in the GNSS system | ||
| uint32_t get_system_errors(uint8_t instance) const { |
There was a problem hiding this comment.
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.
|
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); | ||
| } |
There was a problem hiding this comment.
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
|
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? |
|
Discussion on today's Dev Call: Peter: The feature should be define-fenced for 1M flash boards. R: If the feature is off by default, we should be streaming the message in this case. |
74536d8 to
2fdd96a
Compare
|
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:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking pretty good now!
|
|
||
| #if AP_MAVLINK_MSG_GNSS_INTEGRITY_ENABLED | ||
| void AP_GPS::send_mavlink_gnss_integrity(mavlink_channel_t chan, uint8_t instance) { | ||
| if(state[instance].has_gnss_integrity) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| if(state[instance].has_gnss_integrity) { | |
| if (!state[instance].has_gnss_integrity) { | |
| return; | |
| } |
... just return early if there's nothing to do
| void AP_GPS::send_mavlink_gnss_integrity(mavlink_channel_t chan, uint8_t instance) { | ||
| if(state[instance].has_gnss_integrity) { | ||
|
|
||
| mavlink_msg_gnss_integrity_send(chan, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indenting problem here...
| bool have_gps_yaw_configured(uint8_t instance) const { | ||
| return state[instance].gps_yaw_configured; | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stray whitespace change
| #ifndef AP_GPS_GPS2_RTK_SENDING_ENABLED | ||
| #define AP_GPS_GPS2_RTK_SENDING_ENABLED HAL_GCS_ENABLED && AP_GPS_ENABLED && GPS_MAX_RECEIVERS > 1 && (AP_GPS_SBF_ENABLED || AP_GPS_ERB_ENABLED) | ||
| #endif | ||
| #endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stray whitespace change
| constexpr const char* AP_GPS_SBF::_initialisation_blob[]; | ||
| constexpr const char* AP_GPS_SBF::sbas_on_blob[]; | ||
|
|
||
| const AP_GPS_SBF::SBF_Error_Map AP_GPS_SBF::sbf_error_map[] = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| const AP_GPS_SBF::SBF_Error_Map AP_GPS_SBF::sbf_error_map[] = { | |
| static const AP_GPS_SBF::SBF_Error_Map AP_GPS_SBF::sbf_error_map[] { |
| (unsigned int)(RxError & RX_ERROR_MASK), (unsigned int)(temp.RxError & RX_ERROR_MASK)); | ||
| } | ||
| RxError = temp.RxError; | ||
| if ((ExtError & EXT_ERROR_MASK) != (temp.ExtError & EXT_ERROR_MASK)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So since these changes are outside the new AP_MAVLINK_MSG_GNSS_INTEGRITY_ENABLED, I guess these must be changes to existing behaviour?
These changes are not pulled out as a separate commit, and you haven't mentioned them anywhere, I think?
If the extent of the changes comes to "Print external error changes as they come in" then we can probably let that slide.
One thing here, 'though, is that our send-rate here is limited only to the rate at which we get messages from the external unit. That risks flooding telemetry links and making vehicles choke up on other telemetry... pre-existing in this driver, so also not a big deal.
| case RFStatus: | ||
| { | ||
| const msg4092 &temp = sbf_msg.data.msg4092u; | ||
| check_new_itow(temp.TOW, sbf_msg.length); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, change unrelated to new functionality. I think this is probably benign, but it might just be neater to not know about the message when we're not doing integrity stuff?
| const uint8_t *packet_end = (const uint8_t *)&temp + (sbf_msg.length - 8); | ||
|
|
||
| for (uint8_t i = 0; i < temp.N; i++) { | ||
| if (p + temp.SBLength > packet_end) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please also check that (p + temp.SBLength) -(&temp.RFBand) is < sizeof(sbf_msg).
We do not want to implicitly trust this packet to not be corrupt, even if it does pass a checksum. If temp.SBLength or tmp.N are wild then we run the risk of falling out of the sky. Which is bad.
... also, you can probably make this a lot neater by casting (const uint8_t *)&temp.RFBand to const RFStatusRFBandSubBlock &rf_band_data and incrementing that to move between the N entries, rather than adding temp.SBLength to a uint8_t*. You can use pointer subtraction on p and the &temp.RFBand similarly to my suggestion above to work out whether you're going to over-read the buffer.
| { | ||
| const msg4245 &temp = sbf_msg.data.msg4245u; | ||
| check_new_itow(temp.TOW, sbf_msg.length); | ||
| #if AP_MAVLINK_MSG_GNSS_INTEGRITY_ENABLED |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to the above - should we know about GALAuthStatus at all if we're not doing integrity stuff?
| #endif // AP_MAVLINK_SET_GPS_GLOBAL_ORIGIN_MESSAGE_ENABLED | ||
|
|
||
| #ifndef AP_MAVLINK_MSG_GNSS_INTEGRITY_ENABLED | ||
| #define AP_MAVLINK_MSG_GNSS_INTEGRITY_ENABLED AP_GPS_SBF_ENABLED && HAL_GCS_ENABLED && (HAL_PROGRAM_SIZE_LIMIT_KB > 1024) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll probably change this down-the-line - we can actually use this new information you're supplying in the EKF to change how ArduPilot responds to bad GPS data. But this is fine for now.
|
BTW, how does this PR relate to #27891 ? |
|
@peterbarker This is an old PR opened by a former Septentrio intern. I based this PR on it but had to create a fresh one. You can safely close that one. Sorry, I should’ve mentioned it in the description of PR |
Thanks. Note that if you based your work on someone else's unmerged work it's often a good idea to add a |
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:
GNSS_INTEGRITYmessage.GNSS_INTEGRITYmessage to theSTREAM_EXTENDED_STATUSto ensure it is sent to the GCS.Related Work & Context
GNSS_INTEGRITYmessage from the development dialect.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.