search

swri-robotics / gps_umd

BSD 3-Clause "New" or "Revised" License
88 stars 94 forks source link

PRNs as integers are not satisfactory #90

Open peci1 opened 1 year ago

peci1 commented 1 year ago

In case the receiver is multi-constellation (most are in these days), having PRN fields as int16 becomes impractical, as multiple satellites from different constellations can have the same PRN.

Sometimes, this is resolved by prepending a character like 'E' for Galileo etc. But that's not doable here without breaking compatibility.

Other solutions define some base PRN offsets for different constellations (discussed e.g. here: https://gis.stackexchange.com/questions/163912/is-there-an-industry-standard-official-mapping-of-galileo-satellites-to-global ). I think this would be the only viable solution keeping compatibility.

Would you be inclined to specifying in comments what this mapping should be? If there'd be the defined mapping, it would be easy for users to convert between the numerical and alphanumeric PRNs. The only danger I see here is if some tools check the PRNs and use them further, but do not know about the convention, they would probably fail with unknown PRN errors.

danthony06 commented 1 year ago

We're utilizing GPSD to handle communicating with the GPS receiver, and mapping that data to ROS messages. Currently, GPSD defines PRNs as short integers, and reports whatever the GPS hardware uses: https://gitlab.com/gpsd/gpsd/-/blob/master/include/gps.h?ref_type=heads#L2411. Given the number of ways different hardware vendors report PRNs, I'm not in favor of introducing our own mapping on top of it, and would rather keep sending whatever the vendor is reporting via GPSD. If we change it, I'm concerned about two things:

  1. Breaking compatibility with the existing system.
  2. Introducing another point of confusion. Currently, you can look at your GPS receiver documentation at how it reports PRNs, and that should be what comes out of the ROS messages. If we start custom mapping them, I'm worried it's just going to introduce confusion.
peci1 commented 1 year ago

The code you refer to explicitly mentions NMEA. What about at least taking this part of the standard and rewriting it to the message definition? That would help with most cases except Galileo (of course, up to drivers that do not follow the gpsd convention).

Generally, I understand you don't want to go down the rabbit hole of maintaining a lot of device-specific code. However, the current approach goes a bit against the ROS philosophy - the ROS message should be an independent data exchange format. As there are more sources of GNSS data than gpsd, I see it kind of wrong that the message is tailored just to gpsd and does not suit other sources like RINEX. Maybe, if the package was named gpsd_common, it would be okay. But with a name as generic as you have, I see this as a wrong approach. However, there's no good way around it now without breaking compatibility. The best that can be done is extending the comments in the message definition to guide people in understanding the proprietary mess contained in the data.