M2m2 simple dev #35

DingoOz · 2024-12-22T12:03:36Z

This PR brings over the Humble baseline code for the m2m2_lidar node.

I added the nlohmann and openSSL libraries to the pacakge's nix overlay.

When adding these libraries to the CMakeLists.txt I had to add a target_link_libraries explicitly as there are two libraries in OpenSSL that are needed OpenSSL::SSL and Open::Crypto and build fails unless being explicit here.

This is the relevant section:

OpenSSL has two libraries that need to linked against explicitly

target_link_libraries(
m2m2_lidar
PRIVATE OpenSSL::SSL OpenSSL::Crypto nlohmann_json::nlohmann_json
${rclcpp_LIBRARIES} ${sensor_msgs_LIBRARIES} ${std_msgs_LIBRARIES})

- Despite symlinking the "real" executable, ros2 run can't see the node, so versioning has to be removed.

Readme file is useful for communicating how this node is generally used; how it might be used independently of persues bringup and that there are features which are still be worked on.

Added the code that was baselined against ROS2 Humble and demonstated working.

Add two required libraries the package nix overlay and adjusted the CMakeLists file. Due to needing both openssl::crypto and openssl::ssl had to add an explicit target_link_libraries to CMakeLists.txt

RandomSpaceship

I've gone through and commented on most of the issues I see. Probably missed a few things, but I think I've noted the worst of it.

RandomSpaceship · 2024-12-22T12:21:51Z

software/ros_ws/src/perseus_sensors/include/m2m2_lidar/m2m2_lidar.hpp

+     * Groups all sensor-specific configuration parameters in one place,
+     * making it easier to modify settings and extend functionality
+     */
+    struct SensorConfig


naming doesn't match software standards - should be sensor_config_t

RandomSpaceship · 2024-12-22T12:29:11Z

software/ros_ws/src/perseus_sensors/package.xml

+
+  <depend>rclcpp</depend>
+  <depend>backward_ros</depend>
+  <depend>sensor_msgs</depend>


if you depend on openSSL and nlohman::json, add the dependencies here. Just standard <depend> tags in this case, openssl and nlohmann-json-dev.

RandomSpaceship · 2024-12-22T12:34:06Z

software/ros_ws/src/perseus_sensors/CMakeLists.txt

+
+# OpenSSL has two libraries that need to linked against explicitly
+target_link_libraries(
+  m2m2_lidar
+  PRIVATE OpenSSL::SSL OpenSSL::Crypto nlohmann_json::nlohmann_json
+          ${rclcpp_LIBRARIES} ${sensor_msgs_LIBRARIES} ${std_msgs_LIBRARIES})


replace all of this with:

target_link_libraries(m2m2_lidar PRIVATE OpenSSL::Crypto)

That's all you need.

target_link_libraries is not exclusive, it's additive! ament_target_dependencies already sets up linking against all of the dependencies in THIS_PACKAGE_INCLUDE_DEPENDS, getting rid of all the _LIBRARIES specifiers you have there. Its documentation shows in the example that it also links against default namespaced targets too, so that gets rid of OpenSSL::SSL and nlohmann_json::nlohmann_json (I think they're the default targets), which leaves only OpenSSL::Crypto needing manual linking.

RandomSpaceship · 2024-12-22T12:38:16Z

software/ros_ws/src/perseus_sensors/include/m2m2_lidar/m2m2_lidar.hpp

+    explicit M2M2Lidar(
+        const rclcpp::NodeOptions& options = rclcpp::NodeOptions());
+
+    /**
+     * @brief Clean up
+     */
+    ~M2M2Lidar();


heed the Rule of Three - given that you have a destructor (and are therefore probably managing resources), you almost certainly need to handle copy/move semantics as well, even if it's just deleteing them. Since you are managing resources (network socket), you definitely need to handle it.

RandomSpaceship · 2024-12-22T12:39:17Z

software/ros_ws/src/perseus_sensors/include/m2m2_lidar/m2m2_lidar.hpp

+    int _requestId;
+    static constexpr const char* REQUEST_DELIM = "\r\n\r\n";


incorrect ordering

RandomSpaceship · 2024-12-22T14:06:14Z

software/ros_ws/src/perseus_sensors/src/m2m2_lidar/m2m2_lidar.cpp

+class M2M2LidarNode : public rclcpp::Node
+{
+public:
+    M2M2LidarNode() : Node("m2m2_lidar_node")
+    {
+        publisher_ = this->create_publisher<sensor_msgs::msg::LaserScan>("m2m2_lidar/scan", 10);
+        timer_ = this->create_wall_timer(
+            100ms, std::bind(&M2M2LidarNode::publish_scan, this));
+    }
+
+private:
+    void publish_scan()
+    {
+        auto message = sensor_msgs::msg::LaserScan();
+        // Fill in the LaserScan message
+        publisher_->publish(message);
+    }
+
+    rclcpp::Publisher<sensor_msgs::msg::LaserScan>::SharedPtr publisher_;
+    rclcpp::TimerBase::SharedPtr timer_;
+};


don't use a different syntax. Declare everything which should be part of the class in the header file. Also, naming check fails again (method names + underscore placement).

RandomSpaceship · 2024-12-22T14:07:23Z

software/ros_ws/nix-packages/perseus-sensors.nix

+# Automatically generated by: ros2nix --output-dir=ros_ws/nix-packages --output-as-nix-pkg-name --no-default --distro jazzy
+{
+  lib,
+  buildRosPackage,
+  ament-cmake,
+  backward-ros,
+  rclcpp,
+  sensor-msgs,
+  nlohmann_json,
+  openssl,
+}:
+buildRosPackage rec {
+  pname = "ros-jazzy-perseus-sensors";
+  version = "0.0.0";
+
+  src = ./../src/perseus_sensors;
+
+  buildType = "ament_cmake";
+  buildInputs = [
+    ament-cmake
+    nlohmann_json
+    openssl
+  ];
+  propagatedBuildInputs = [
+    backward-ros
+    rclcpp
+    sensor-msgs
+  ];
+  nativeBuildInputs = [ ament-cmake ];
+
+  meta = {
+    description = "TODO: Package description";
+    license = with lib.licenses; [ mit ];
+  };
+}


after fixing the package.xml, run software/scripts/nix-package.sh --no-commit to regenerate the nix packaging info

RandomSpaceship · 2024-12-22T14:09:11Z

software/ros_ws/src/perseus_sensors/include/m2m2_lidar/m2m2_lidar.hpp

+    /**
+     * @brief Clean up
+     */
+    ~M2M2Lidar();


destructors should either be protected and nonvirtual or public and virtual

(yes I'm guilty of this too, I need to fix my code as well)

RandomSpaceship · 2024-12-22T14:11:12Z

software/ros_ws/src/perseus_sensors/src/m2m2_lidar/m2m2_lidar.cpp

+#include "rclcpp/rclcpp.hpp"
+#include "sensor_msgs/msg/laser_scan.hpp"
+
+using namespace std::chrono_literals;


you might want to add using std::string, std::vector; and the like to remove some of the namespacing in this file

RandomSpaceship · 2024-12-22T14:12:04Z

software/ros_ws/src/perseus_sensors/src/m2m2_lidar/m2m2_lidar.cpp

+    scan->time_increment = 1.0 / (15.0 * points.size());  // Assuming 15Hz scan rate
+    scan->scan_time = 1.0 / 15.0;                         // 15Hz
+    scan->range_min = 0.1;                                // 10cm minimum range
+    scan->range_max = 30.0;                               // 30m maximum range


give your magic constants names, then you can also get rid of the comments (or convert them to notes on the units of the constant)

Removed references for target_link_libraries that are not needed

…ge.xml

…LE decode

RandomSpaceship and others added 8 commits December 22, 2024 19:14

ros_ws(patches): Delete unneeded

f03f177

cmake(templates): Remove ROS exe install versioning

d4d5468

- Despite symlinking the "real" executable, ros2 run can't see the node, so versioning has to be removed.

perseus_sensors(m2m2): Add package + empty node

189f86c

Merge branch 'main' into m2m2-simple-dev

dd87745

persues_sensors(m2m2)lidar): Added a README file for this node.

cfb0ad9

Readme file is useful for communicating how this node is generally used; how it might be used independently of persues bringup and that there are features which are still be worked on.

perseus_sensors(m2m2_lidar): Added baseline code from Humble

ba3399c

Added the code that was baselined against ROS2 Humble and demonstated working.

perseus_sensors(m2m2_lidar): Corrected typo in README

cb44609

perseus_sensors(m2m2_lidar): Added nlohmann and openSSL libraries

c7e6831

Add two required libraries the package nix overlay and adjusted the CMakeLists file. Due to needing both openssl::crypto and openssl::ssl had to add an explicit target_link_libraries to CMakeLists.txt

DingoOz temporarily deployed to binary-deployment December 22, 2024 12:05 — with GitHub Actions Inactive

RandomSpaceship requested changes Dec 22, 2024

View reviewed changes

DingoOz added 6 commits December 23, 2024 05:59

persues_sensors(m2m2_lidar): removed unneeded target_link_libraries

f6d84ac

Removed references for target_link_libraries that are not needed

perseus_sensors(m2m2_lidar): member variables moved to correct position

3336377

persues_sensors(m2m2_lidar): added openssl and nlohmann libs to packa…

bcc1dab

…ge.xml

perseus_sensors(m2m2_lidar): formatting change to defaultConfig

2d5490d

perseus_sensors(m2m2_lidar): used strncmp to make intent clearer re R…

6206d49

…LE decode

perseus_sensors(m2m2_lidar): removed comment that was not needed

399db15

DingoOz temporarily deployed to binary-deployment December 22, 2024 20:34 — with GitHub Actions Inactive

DingoOz deployed to binary-deployment December 22, 2024 20:36 — with GitHub Actions Active

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

M2m2 simple dev #35

M2m2 simple dev #35

DingoOz commented Dec 22, 2024

RandomSpaceship left a comment

RandomSpaceship Dec 22, 2024

RandomSpaceship Dec 22, 2024

RandomSpaceship Dec 22, 2024

RandomSpaceship Dec 22, 2024

RandomSpaceship Dec 22, 2024

RandomSpaceship Dec 22, 2024

RandomSpaceship Dec 22, 2024

RandomSpaceship Dec 22, 2024

RandomSpaceship Dec 22, 2024

RandomSpaceship Dec 22, 2024

RandomSpaceship Dec 22, 2024

		int _requestId;
		static constexpr const char* REQUEST_DELIM = "\r\n\r\n";

M2m2 simple dev #35

Are you sure you want to change the base?

M2m2 simple dev #35

Conversation

DingoOz commented Dec 22, 2024

OpenSSL has two libraries that need to linked against explicitly

RandomSpaceship 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment