A few build improvements and code cleanup changes #57

rgov · 2022-04-23T00:07:02Z

This pull request includes a few code cleanup changes. If you would prefer these as separate pull requests, please let me know.

Uses CMake's built-in find_library to find libgps rather than relying on pkg-config, which is not populated by default on Debian.
Upgrades package.xml to version 2 and promotes sensor_msgs and gps_common to runtime dependencies. Previously, these were only build time dependencies, which means that ros-noetic-gpsd-client will not auto-install ros-noetic-gps-common and hence the message definitions will not be available (rostopic echo /extended_fix will error, etc.)
Changes process_data_navsat to not allocate a new NavSatFix object each time. The process_data_gps function did not have this problem.
Provides a slightly hacky workaround to the fact that gps.h pollutes the global namespace with its macro definitions like STATUS_NO_FIX. This removes the need to hardcode the enum values for NavSatStatus::STATUS_NO_FIX etc.
Removes the hardcoded default port number and instead uses DEFAULT_GPSD_PORT.
Frees the allocated gpsmm instance when stop() is called, eliminating an extremely minor memory leak.

rgov · 2022-04-23T00:44:29Z

Perhaps should remove <build_depend>pkg-config</build_depend> now that it isn't truly needed.

danthony06 · 2022-06-27T15:54:29Z

I haven't forgotten about this. I think 1, 2, and 5 make a lot of sense. I'm looking at refactoring the code significantly to help reduce the number of conditional compilation blocks we're using, and I think that might catch several of your other changes.

rgov · 2023-06-29T19:05:34Z

@danthony06 For what it's worth, this included the fix to #78 already, and did it in a way that doesn't add a lot more #ifdefs. Is there still any interest in getting this merged?

danthony06 · 2023-06-30T14:37:21Z

Sorry, I saw this and your other PR. I'll try to get to it today or Monday.

danthony06

Overall, looks good. There's been a few updates to ros2-devel to handle the newer API that obsoletes some changes, and the package.xml change isn't needed any more, but everything else looks like a valuable update.

danthony06 · 2023-07-05T14:49:05Z

gpsd_client/CMakeLists.txt

+# Try to find libgps, first with CMake's usual library search method, then by
+# querying pkg-config.
+find_library(libgps_LIBRARIES NAMES gps)
+find_path(libgps_INCLUDE_DIRS NAMES libgpsmm.h gps.h)
+
+if(NOT libgps_LIBRARIES)
+  message(STATUS "Checking pkg-config for libgps")
+  find_package(PkgConfig)
+  if(PkgConfig_FOUND)
+    pkg_check_modules(libgps libgps)
+  endif()
+endif()
+
+if(NOT libgps_LIBRARIES)
+  message(FATAL_ERROR "Could not find libgps "
+                      "(hint for Debian/Ubuntu: apt install libgps-dev)")
+else()
+  message(STATUS "Found libgps: ${libgps_LIBRARIES}")
+endif()
+


Looks good, I really like this change.

gpsd_client/package.xml

danthony06 · 2023-07-05T14:54:11Z

gpsd_client/src/client.cpp

+#include <libgpsmm.h>
+
+// gps.h defines some macros which conflict with enum values in NavSatStatus.h,
+// so we map them to new names before including other headers.
+#ifdef STATUS_FIX
+  enum {
+    GPSD_STATUS_NO_FIX   = STATUS_NO_FIX,
+    GPSD_STATUS_FIX      = STATUS_FIX,
+    GPSD_STATUS_DGPS_FIX = STATUS_DGPS_FIX,
+  };
+  #undef STATUS_NO_FIX
+  #undef STATUS_FIX
+  #undef STATUS_DGPS_FIX
+#else
+  // Renamed in gpsd >= 3.23.1 (commits d4a4d8d3, af3b7dc0, 7d7b889f) without
+  // revising the API version number.
+  enum {
+    GPSD_STATUS_NO_FIX   = STATUS_UNK,
+    GPSD_STATUS_FIX      = STATUS_GPS,
+    GPSD_STATUS_DGPS_FIX = STATUS_DGPS,
+  };
+#endif
+


I think this is obsolete with some of the other changes that have been made to ros2-devel

danthony06 · 2023-07-05T14:54:30Z

gpsd_client/src/client.cpp

@@ -23,12 +46,12 @@ class GPSDClient {
      privnode.param("frame_id", frame_id, frame_id);

      std::string host = "localhost";
-      int port = 2947;
+      int port = atoi(DEFAULT_GPSD_PORT);


Nice, I like this change.

danthony06 · 2023-07-05T14:54:48Z

gpsd_client/src/client.cpp

      privnode.getParam("host", host);
      privnode.getParam("port", port);

      char port_s[12];
-      snprintf(port_s, 12, "%d", port);
+      snprintf(port_s, sizeof(port_s), "%d", port);


gpsd_client/src/client.cpp

danthony06 · 2023-07-05T14:56:30Z

gpsd_client/src/client.cpp

+        case GPSD_STATUS_NO_FIX:
+          fix.status.status = NavSatStatus::STATUS_NO_FIX;
          break;
-// STATUS_DGPS_FIX was removed in API version 6 but re-added afterward and next renamed since the version 12
+        case GPSD_STATUS_FIX:
+          fix.status.status = NavSatStatus::STATUS_FIX;
+          break;
+// STATUS_DGPS_FIX was removed in API version 6 but re-added afterward


I don't think this is needed any more.

danthony06 · 2023-07-05T14:56:44Z

gpsd_client/src/client.cpp

+        case GPSD_STATUS_DGPS_FIX:
+          fix.status.status = NavSatStatus::STATUS_GBAS_FIX;


Probably not needed.

danthony06 · 2023-07-05T14:56:53Z

gpsd_client/src/client.cpp

+      fix.status.service = NavSatStatus::SERVICE_GPS;

-      fix->latitude = p->fix.latitude;
-      fix->longitude = p->fix.longitude;
-      fix->altitude = p->fix.altitude;
+      fix.latitude = p->fix.latitude;
+      fix.longitude = p->fix.longitude;
+      fix.altitude = p->fix.altitude;



Looks good.

danthony06 · 2023-07-05T14:57:05Z

gpsd_client/src/client.cpp

-      fix->position_covariance_type = NavSatFix::COVARIANCE_TYPE_DIAGONAL_KNOWN;
+      fix.position_covariance_type = std::isnan(p->fix.epx) ?
+        NavSatFix::COVARIANCE_TYPE_UNKNOWN :
+        NavSatFix::COVARIANCE_TYPE_DIAGONAL_KNOWN;


Looks good.

danthony06 · 2023-07-05T15:29:04Z

Ah, sorry, I just noticed you were making the changes for ROS1, not ROS2. Let me see if I can backport the API changes for v12 from the ros2-devel branch back to master so that we have a consistent implementation across the two.

rgov added 6 commits June 29, 2023 11:46

Prefer built-in CMake library search logic

a48c1a6

Declare runtime message type dependencies

ec71e18

Remove unnecessary heap allocation

8897599

Resolve naming conflict between gps.h and message enums

b36be02

Do not hardcode port number

e592a56

Free gpsmm instance

fa13367

rgov force-pushed the master branch from 5f5cba9 to fa13367 Compare June 29, 2023 19:01

Set COVARIANCE_TYPE_UNKNOWN when covariance is NaN

b680df6

rgov mentioned this pull request Jun 29, 2023

Covariance should be unknown when value is NaN #82

Open

danthony06 reviewed Jul 5, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

A few build improvements and code cleanup changes #57

A few build improvements and code cleanup changes #57

rgov commented Apr 23, 2022

rgov commented Apr 23, 2022

danthony06 commented Jun 27, 2022

rgov commented Jun 29, 2023

danthony06 commented Jun 30, 2023

danthony06 left a comment

danthony06 Jul 5, 2023

danthony06 Jul 5, 2023

danthony06 Jul 5, 2023

danthony06 Jul 5, 2023

danthony06 Jul 5, 2023

danthony06 Jul 5, 2023

danthony06 Jul 5, 2023

danthony06 Jul 5, 2023

danthony06 commented Jul 5, 2023

		case GPSD_STATUS_DGPS_FIX:
		fix.status.status = NavSatStatus::STATUS_GBAS_FIX;

A few build improvements and code cleanup changes #57

Are you sure you want to change the base?

A few build improvements and code cleanup changes #57

Conversation

rgov commented Apr 23, 2022

rgov commented Apr 23, 2022

danthony06 commented Jun 27, 2022

rgov commented Jun 29, 2023

danthony06 commented Jun 30, 2023

danthony06 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danthony06 commented Jul 5, 2023