gps_umd icon indicating copy to clipboard operation
gps_umd copied to clipboard

A few build improvements and code cleanup changes

Open rgov opened this issue 2 years ago • 5 comments

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

  1. Uses CMake's built-in find_library to find libgps rather than relying on pkg-config, which is not populated by default on Debian.

  2. 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.)

  3. Changes process_data_navsat to not allocate a new NavSatFix object each time. The process_data_gps function did not have this problem.

  4. 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.

  5. Removes the hardcoded default port number and instead uses DEFAULT_GPSD_PORT.

  6. Frees the allocated gpsmm instance when stop() is called, eliminating an extremely minor memory leak.

rgov avatar Apr 23 '22 00:04 rgov

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

rgov avatar Apr 23 '22 00:04 rgov

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.

danthony06 avatar Jun 27 '22 15:06 danthony06

@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?

rgov avatar Jun 29 '23 19:06 rgov

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

danthony06 avatar Jun 30 '23 14:06 danthony06

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.

danthony06 avatar Jul 05 '23 15:07 danthony06