fix status on /extended_fix topic for DGPS and RTK.

[nxdefiant]

When status is DGPS or RTK fix the field status.status in the /extended_fix topic currently returns -1/STATUS_NO_FIX. This PR should fix that.

For DGPS: My gpsd is GPSD_API_MAJOR_VERSION=6 and has STATUS_DGPS_FIX so use ifdef instead

Disclaimer: My gpsd does not have STATUS_RTK_FIX and STATUS_RTK_FLT yet, so this part is untested. At least it now reports 0/STATUS_FIX for RTK.

[nxdefiant]

most important change:
p->status is not a bitfield, values see
https://gitlab.com/gpsd/gpsd/-/blob/master/gps.h#L151

[kriskozak]

This seems reasonable, but my only concern would be whether this change would affect any existing users of the gpsd_client. I have no idea how many people might be using it -- the package was abandoned for a time and then adopted several years ago (mostly to preserve the GPSFix and GPSStatus messages). The gpsd_client hasn't received much attention in that time, but people pop up from time to time with issues that indicate that there are at least some users still. In any case, it looks like this change should be fine, but I haven't looked closely to see whether there might be some potential undesired side-effect of the change.

[nxdefiant]

When there is a problem, feel free to
notify me and I'll see if I can help.

[danthony06]

I took a quick look at how this is defined, and this is it:

#define STATUS_NO_FIX   0       // Unknown status, maybe no fix.
/* yes, plain GPS (SPS Mode), without DGPS, PPS, RTK, DR, etc. */
#define STATUS_FIX      1
#define STATUS_DGPS_FIX 2       /* yes, with DGPS */
#define STATUS_RTK_FIX  3       /* yes, with RTK Fixed */
#define STATUS_RTK_FLT  4       /* yes, with RTK Float */
#define STATUS_DR       5       /* yes, with dead reckoning */
#define STATUS_GNSSDR   6       /* yes, with GNSS + dead reckoning */
#define STATUS_TIME     7       /* yes, time only (surveyed in, manual) */
// Note that STATUS_SIM and MODE_NO_FIX can go together.
#define STATUS_SIM      8       /* yes, simulated */
/* yes, Precise Positioning Service (PPS)
 * Not to be confused with Pulse per Second (PPS)
 * PPS is the encrypted military P(Y)-code */
#define STATUS_PPS_FIX  9

So it looks like previously, we would enter this block on STATUS_FIX, STATUS_RTK_FIX, STATUS_DR, STATUS_TIME, and STATUS_PPS_FIX. We would have missed a couple of cases where we actually had valid fixed, like STATUS_DGPS_FIX. So I think this change is okay, because we will enter this block in more cases where we do have a GPS fix, and we will be as restrictive as before when we did not have a GPS fix.

The source file is here now: https://gitlab.com/gpsd/gpsd/-/blob/master/include/gps.h

[peci1]

There is, actually, a way to add the constants without changing the MD5 sum. You would start with the commented out lines in .msg file, and then add a "plugin" that would define the commented out constants at least for C++: http://wiki.ros.org/roscpp/Overview/MessagesSerializationAndAdaptingTypes#Customizing_generated_message_headers_for_C.2B-.2B- . I'm not sure there is a similar mechanism for Python, though. And this whole approach is a bit hacky, but it works.

[kriskozak]

Adding these explicit enum values is a reasonable suggested change; however, the md5 sum will change and that will likely cause unexpected compatibility issues for anyone who isn't building this package from source. I notice that the existing constants (e.g. GPSStatus::STATUS_DGPS_FIX) are not referenced by name (in client.cpp at least), so there isn't much pressing need (just looking at the present changes) to include named constants in the message file. Let me suggest instead adding these two new lines as comments instead. Unfortunately, they won't show up in rosmsg show gps_common/GPSStatus, but they will appear in the message definitions and online documentation (e.g. http://docs.ros.org/melodic/api/gps_common/html/msg/GPSStatus.html). More importantly, though, adding them as comments won't change the md5 sum. You can verify this with rosmsg md5 gps_common/GPSStatus

[nxdefiant]

client.cpp can not use the constants
because of the global STATUS_DGPS_FIX
from gpsd. When adding both as
comments people won't be able to use
GPSStatus::STATUS_RTK_FIX and will
fallback to the number values
which I would really like to avoid.

Shouldn't the compatibility only be an issue when mixing different ROS distributions?

However there is a third option: I revert both constants. I do not need STATUS_RTK_FIX/STATUS_RTK_FLT that much since I trust the covariance values more then the status value. The "p->status & STATUS_FIX" is the important change for me.

[kriskozak]

client.cpp can not use the constants
because of the global STATUS_DGPS_FIX
from gpsd. When adding both as
comments people won't be able to use
GPSStatus::STATUS_RTK_FIX and will
fallback to the number values
which I would really like to avoid.

I agree with this statement, but I think that compatibility is a bigger issue than the use of hard-coded numbers. Comments in place of constants in the message definition is not ideal, but not the worst solution either.

Shouldn't the compatibility only be an issue when mixing different ROS distributions?

As far as I know, all supported distributions are currently using master, so the change would affect all distributions. It may be reasonable to make changes for a future (as yet unreleased) distribution, but that's a different story. The main compatibility issue will be that any mixing of the new GPSStatus (and also GPSFix) messages with the old definition will result in problems -- like failure to pass messages -- you'll probably see WARN level log messages in these cases. There are a few ways that mixing would/could occur, but basically any nodes that use the new definition (including those that are built against the new definition, or that would dynamically update, like python nodes) couldn't communicate with those that don't (any nodes not built against the new definition, which would include any binaries distributed via apt that have not been updated with the new message definitions). Additionally, this change also causes headaches for analyzing bagged data that includes GPSStatus and GPSFix messages of both types.

However there is a third option: I revert both constants. I do not need STATUS_RTK_FIX/STATUS_RTK_FLT that much since I trust the covariance values more then the status value. The "p->status & STATUS_FIX" is the important change for me.

I would definitely recommend prioritizing not changing the message definition over everything else here, so this sounds like it may be your best option. @pjreed, any thoughts, since you're probably the ultimate decider here?

[nxdefiant]

done. STATUS_RTK_FIX/STATUS_RTK_FLT removed

[kriskozak]

This seems reasonable, but my only concern would be whether this change would affect any existing users of the gpsd_client. I have no idea how many people might be using it -- the package was abandoned for a time and then adopted several years ago (mostly to preserve the GPSFix and GPSStatus messages). The gpsd_client hasn't received much attention in that time, but people pop up from time to time with issues that indicate that there are at least some users still. In any case, it looks like this change should be fine, but I haven't looked closely to see whether there might be some potential undesired side-effect of the change.

[nxdefiant]

Any news on this?

[danthony06]

I think it looks good. The change to checking the GPS fix status is more correct, and will not make people who were previously entering into that block fail, and will correctly enter that block on more types of valid GPS fixes.

[danthony06]

I took a quick look at how this is defined, and this is it:

#define STATUS_NO_FIX   0       // Unknown status, maybe no fix.
/* yes, plain GPS (SPS Mode), without DGPS, PPS, RTK, DR, etc. */
#define STATUS_FIX      1
#define STATUS_DGPS_FIX 2       /* yes, with DGPS */
#define STATUS_RTK_FIX  3       /* yes, with RTK Fixed */
#define STATUS_RTK_FLT  4       /* yes, with RTK Float */
#define STATUS_DR       5       /* yes, with dead reckoning */
#define STATUS_GNSSDR   6       /* yes, with GNSS + dead reckoning */
#define STATUS_TIME     7       /* yes, time only (surveyed in, manual) */
// Note that STATUS_SIM and MODE_NO_FIX can go together.
#define STATUS_SIM      8       /* yes, simulated */
/* yes, Precise Positioning Service (PPS)
 * Not to be confused with Pulse per Second (PPS)
 * PPS is the encrypted military P(Y)-code */
#define STATUS_PPS_FIX  9

So it looks like previously, we would enter this block on STATUS_FIX, STATUS_RTK_FIX, STATUS_DR, STATUS_TIME, and STATUS_PPS_FIX. We would have missed a couple of cases where we actually had valid fixed, like STATUS_DGPS_FIX. So I think this change is okay, because we will enter this block in more cases where we do have a GPS fix, and we will be as restrictive as before when we did not have a GPS fix.

The source file is here now: https://gitlab.com/gpsd/gpsd/-/blob/master/include/gps.h

[danthony06]

Fixed via #93 and #74.