Better parsing and merging of GSA messages #149
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #149 +/- ##
==========================================
+ Coverage 79.24% 80.39% +1.14%
==========================================
Files 39 39
Lines 1465 1459 -6
==========================================
+ Hits 1161 1173 +12
+ Misses 304 286 -18 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
elpiel
left a comment
elpiel
left a comment
There was a problem hiding this comment.
Thank you very much for working on this!
I've left a few questions and requests for changes.
PS
I'd actually love to hear if you find the NMEA struct useful?
For embedded systems it's actually too big to be useful as it takes up quite a bit of space to re-allocate if you want to use it.
Do you find any feature missing or things that could be improved?
The suggested PR has breaking changing on the PR so it's good time to think about the NMEA struct and improvements we can implement.
| let (i, _) = char(',')(i)?; | ||
| let (i, vdop) = float(i)?; | ||
| let (i, _) = char(',')(i)?; | ||
| let (i, gnss_type) = number::<u8>(i)?; |
There was a problem hiding this comment.
If I understand correctly this is the System ID:
https://gpsd.gitlab.io/gpsd/NMEA.html#_gsa_gps_dop_and_active_satellites
At least from this documentation I don't see 5 and 6 but if your receiver uses these numbers I'm ok with including them.
There was a problem hiding this comment.
gpsd is describing system id 5 and 6 here: https://gpsd.gitlab.io/gpsd/NMEA.html#_nmea_4_11_system_id_and_signal_id
Our receiver does not, afaik, produce these numbers
| let gnss_type = match talker_id { | ||
| "GA" => GnssType::Galileo, | ||
| "GP" => GnssType::Gps, | ||
| "GL" => GnssType::Glonass, | ||
| "BD" | "GB" => GnssType::Beidou, | ||
| "GI" => GnssType::NavIC, | ||
| "GQ" | "PQ" | "QZ" => GnssType::Qzss, | ||
| "GN" => tail.4.unwrap_or(GnssType::Gps), | ||
| _ => tail.4.unwrap_or(GnssType::Gps), |
There was a problem hiding this comment.
-
Maybe an
Optional field makes more sense, as this system id was introduced in NMEA 4.11? -
Orrr... return an error in
GNtalker with no tail. I also think we can match the talker to the parsed System Id exists in the sentence (sort of a second validation) -
Also, could you please document this feature in the struct itself including the NMEA 4.11 version and how the system id is handled by us.
Not sure how it's handled by receivers in previous versions and multi-constelation view.
Here's what I found (https://gpsd.gitlab.io/gpsd/NMEA.html#_satellite_ids):
GLONASS satellite numbers come in two flavors. If a sentence has a GL talker ID, expect the skyviews to be GLONASS-only and in the range 1-32; you must add 64 to get a globally-unique NMEA ID. If the sentence has a GN talker ID, the device emits a multi-constellation skyview with GLONASS IDs already in the 65-96 range.
There was a problem hiding this comment.
So the trouble here, according to gpsd, is that before NMEA 4.11 there was no standard way of knowing which satellite id corresponded to which system. So the ids used in GSA messages with talker GN is product dependent, and thus we have no way of distinguishing between satellites from different systems. NMEA 4.11 added the system id(last field of GSA messages) which is optional. If the system id is present all the satellites in a GSA message have the system assigned ids.
gpsd does this whole thing to decide which number each satellite has: https://gitlab.com/gpsd/gpsd/-/blob/master/drivers/driver_nmea0183.c#L549
Its been some time since I made this pull request and my memory is no longer clear about exactly what is going on.
But this seems like a hard problem to solve due to lack of accurate information.
There was a problem hiding this comment.
If I understand the issue correctly, I'd suggest to use a None in the case of GN with no System id/gnss type.
Seems the safest approach instead of providing false information to the user or their hardware.
E.g. if they know their hardware only outputs X or Y talkers but doesn't support NMEA 4.11 system id, they will see Gps which is incorrect.
There was a problem hiding this comment.
Could you explain the changes in Gsv?
At least in the example here: https://gpsd.gitlab.io/gpsd/NMEA.html#_gsv_satellites_in_view
I don't see the use of floating numbers in the string, could you verify that a float can be returned and include a test for it?
I'd prefer an actual messages being used rather than a fake one to make sure that a receiver is actually returning a float.
There was a problem hiding this comment.
Yes, we are seeing floats(or at least fixedpoint numbers with 2 decimals), and we had parse failures due to this. Note that it does not change the type of the Satellite struct as it already had f32, now we are just also parsing those floats.
I can get our GNSS to produce those messages for testing.
|
I'm also doing some updates in #153 taking this opportunity for breaking change to bump dependencies, MSRV, etc. FYI: You would need to rebase your changes once I merge the PR |
Thanks.
Yes, we find this NMEA parser very useful.
Our software is running on a embedded Linux system, so memory is not really an issue for us.
Unsure, we have no problem with maintaining our fork of the project if our usecase does not have enough overlap with yours. However we would prefer to be able to upstream patches. |
Maybe we can change the API to return the data and the NmeaSentence? |
2 participants
This fixes #148