search

swri-robotics / gps_umd

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

A few build improvements and code cleanup changes #57

Open rgov opened 2 years ago

rgov commented 2 years ago

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 commented 2 years ago

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

danthony06 commented 2 years ago

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 commented 1 year ago

@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 commented 1 year ago

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

danthony06 commented 1 year ago

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.