Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix status on /extended_fix topic for DGPS and RTK. #34

Closed
wants to merge 2 commits into from
Closed

fix status on /extended_fix topic for DGPS and RTK. #34

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e4ec427
fix status on /extended_fix topic
nxdefiant Sep 15, 2020
3b3deb4
remove STATUS_RTK_FIX/FLT
nxdefiant Sep 27, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions gpsd_client/src/client.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,14 +148,14 @@ class GPSDClient {
#endif
}

if ((p->status & STATUS_FIX) && !(check_fix_by_variance && std::isnan(p->fix.epx))) {
Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

@danthony06 danthony06 Mar 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

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.

if ((p->status != STATUS_NO_FIX) && !(check_fix_by_variance && std::isnan(p->fix.epx))) {
status.status = 0; // FIXME: gpsmm puts its constants in the global
// namespace, so `GPSStatus::STATUS_FIX' is illegal.

// STATUS_DGPS_FIX was removed in API version 6 but re-added afterward
#if GPSD_API_MAJOR_VERSION != 6
if (p->status & STATUS_DGPS_FIX)
status.status |= 18; // same here
#ifdef STATUS_DGPS_FIX
if (p->status == STATUS_DGPS_FIX)
status.status = 18; // same here
#endif

#if GPSD_API_MAJOR_VERSION >= 9
Expand Down Expand Up @@ -236,7 +236,7 @@ class GPSDClient {
fix->status.status = 0; // NavSatStatus::STATUS_FIX;
break;
// STATUS_DGPS_FIX was removed in API version 6 but re-added afterward
#if GPSD_API_MAJOR_VERSION != 6
#ifdef STATUS_DGPS_FIX
case STATUS_DGPS_FIX:
fix->status.status = 2; // NavSatStatus::STATUS_GBAS_FIX;
break;
Expand Down