fix: address review feedback on beacon mode PR

- Split beacon_helper.hpp into .hpp/.cpp with static library
- Use std::map for stable metadata key ordering
- Use int64_t for process_id parameter to avoid truncation
- Make entity_id a node parameter with default (reduces manifest coupling)
- Move import sys to top-level in launch file
- Increase beacon TTL from 10s to 15s (3x publish interval)
- Hardcode BEACON_MODE=none in CI service for deterministic tests

Author: bburda

demos/sensor_diagnostics/CMakeLists.txt

@@ -15,42 +15,53 @@ find_package(diagnostic_msgs REQUIRED)
 find_package(rcl_interfaces REQUIRED)
 find_package(ros2_medkit_msgs REQUIRED)
 
+# Beacon helper library (shared across sensor nodes)
+add_library(beacon_helper STATIC src/beacon_helper.cpp)
+ament_target_dependencies(beacon_helper PUBLIC
+  rclcpp
+  diagnostic_msgs
+  ros2_medkit_msgs
+)
+target_include_directories(beacon_helper PUBLIC
+  $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/src>
+)
+
 # LiDAR simulator node
 add_executable(lidar_sim_node src/lidar_sim_node.cpp)
+target_link_libraries(lidar_sim_node beacon_helper)
 ament_target_dependencies(lidar_sim_node
   rclcpp
   sensor_msgs
   diagnostic_msgs
   rcl_interfaces
-  ros2_medkit_msgs
 )
 
 # IMU simulator node
 add_executable(imu_sim_node src/imu_sim_node.cpp)
+target_link_libraries(imu_sim_node beacon_helper)
 ament_target_dependencies(imu_sim_node
   rclcpp
   sensor_msgs
   diagnostic_msgs
-  ros2_medkit_msgs
 )
 
 # GPS simulator node
 add_executable(gps_sim_node src/gps_sim_node.cpp)
+target_link_libraries(gps_sim_node beacon_helper)
 ament_target_dependencies(gps_sim_node
   rclcpp
   sensor_msgs
   diagnostic_msgs
-  ros2_medkit_msgs
 )
 
 # Camera simulator node
 add_executable(camera_sim_node src/camera_sim_node.cpp)
+target_link_libraries(camera_sim_node beacon_helper)
 ament_target_dependencies(camera_sim_node
   rclcpp
   sensor_msgs
   diagnostic_msgs
   rcl_interfaces
-  ros2_medkit_msgs
 )
 
 # Anomaly detector node

demos/sensor_diagnostics/docker-compose.yml

@@ -38,7 +38,7 @@ services:
     container_name: sensor_diagnostics_demo_ci
     environment:
       - ROS_DOMAIN_ID=40
-      - BEACON_MODE=${BEACON_MODE:-none}
+      - BEACON_MODE=none
     ports:
       - "8080:8080"
     command: >

demos/sensor_diagnostics/launch/demo.launch.py

@@ -24,6 +24,7 @@
 """
 
 import os
+import sys
 
 from ament_index_python.packages import get_package_prefix
 from ament_index_python.packages import get_package_share_directory
@@ -59,7 +60,6 @@ def generate_launch_description():
     beacon_mode = os.environ.get('BEACON_MODE', 'none').strip().lower()
     valid_beacon_modes = ('none', 'topic', 'param')
     if beacon_mode not in valid_beacon_modes:
-        import sys
         print(
             f"WARNING: Invalid BEACON_MODE='{beacon_mode}'. "
             f"Valid values: {', '.join(valid_beacon_modes)}. "
@@ -93,9 +93,8 @@ def generate_launch_description():
             plugin_overrides['plugins.topic_beacon.path'] = topic_beacon_path
             plugin_overrides['plugins.topic_beacon.topic'] = \
                 '/ros2_medkit/discovery'
-            plugin_overrides['plugins.topic_beacon.beacon_ttl_sec'] = 10.0
+            plugin_overrides['plugins.topic_beacon.beacon_ttl_sec'] = 15.0
         else:
-            import sys
             print(
                 "WARNING: BEACON_MODE=topic but topic_beacon plugin not "
                 "found. Falling back to none.",
@@ -112,7 +111,6 @@ def generate_launch_description():
             plugin_overrides['plugins.parameter_beacon.poll_interval_sec'] = \
                 5.0
         else:
-            import sys
             print(
                 "WARNING: BEACON_MODE=param but param_beacon plugin not "
                 "found. Falling back to none.",

demos/sensor_diagnostics/src/beacon_helper.cpp

@@ -0,0 +1,101 @@
+// Copyright 2026 selfpatch
+// SPDX-License-Identifier: Apache-2.0
+
+#include "beacon_helper.hpp"
+
+#include <unistd.h>
+
+#include "diagnostic_msgs/msg/key_value.hpp"
+
+namespace sensor_diagnostics
+{
+
+BeaconHelper::BeaconHelper(rclcpp::Node * node, const Config & config)
+: node_(node), config_(config)
+{
+  node_->declare_parameter("beacon_mode", "none");
+  mode_ = node_->get_parameter("beacon_mode").as_string();
+
+  // Cache process info (constant during lifetime)
+  pid_ = static_cast<uint32_t>(getpid());
+  char hostname_buf[256];
+  hostname_buf[sizeof(hostname_buf) - 1] = '\0';
+  if (gethostname(hostname_buf, sizeof(hostname_buf) - 1) == 0) {
+    hostname_ = hostname_buf;
+  }
+
+  if (mode_ == "topic") {
+    init_topic_beacon();
+  } else if (mode_ == "param") {
+    init_param_beacon();
+  } else if (mode_ != "none") {
+    RCLCPP_WARN(
+      node_->get_logger(),
+      "Unknown beacon_mode '%s', expected none/topic/param. Beacon disabled.",
+      mode_.c_str());
+  }
+}
+
+void BeaconHelper::init_topic_beacon()
+{
+  beacon_pub_ = node_->create_publisher<ros2_medkit_msgs::msg::MedkitDiscoveryHint>(
+    "/ros2_medkit/discovery", 10);
+
+  beacon_timer_ = node_->create_wall_timer(
+    std::chrono::seconds(5),
+    [this]() { publish_beacon(); });
+
+  // First publish likely dropped due to DDS discovery delay, next arrives in 5s
+  publish_beacon();
+
+  RCLCPP_INFO(
+    node_->get_logger(), "Topic beacon enabled (entity: %s)",
+    config_.entity_id.c_str());
+}
+
+void BeaconHelper::init_param_beacon()
+{
+  node_->declare_parameter("ros2_medkit.discovery.entity_id", config_.entity_id);
+  node_->declare_parameter("ros2_medkit.discovery.display_name", config_.display_name);
+  node_->declare_parameter("ros2_medkit.discovery.component_id", config_.component_id);
+  node_->declare_parameter("ros2_medkit.discovery.function_ids", config_.function_ids);
+  node_->declare_parameter(
+    "ros2_medkit.discovery.process_id", static_cast<int64_t>(pid_));
+  node_->declare_parameter("ros2_medkit.discovery.process_name", node_->get_name());
+
+  if (!hostname_.empty()) {
+    node_->declare_parameter("ros2_medkit.discovery.hostname", hostname_);
+  }
+
+  for (const auto & [key, value] : config_.metadata) {
+    node_->declare_parameter("ros2_medkit.discovery.metadata." + key, value);
+  }
+
+  RCLCPP_INFO(
+    node_->get_logger(), "Parameter beacon enabled (entity: %s)",
+    config_.entity_id.c_str());
+}
+
+void BeaconHelper::publish_beacon()
+{
+  auto msg = ros2_medkit_msgs::msg::MedkitDiscoveryHint();
+  msg.entity_id = config_.entity_id;
+  msg.display_name = config_.display_name;
+  msg.component_id = config_.component_id;
+  msg.function_ids = config_.function_ids;
+  msg.process_id = pid_;
+  msg.process_name = node_->get_name();
+  msg.hostname = hostname_;
+  msg.stamp = node_->now();
+
+  for (const auto & [key, value] : config_.metadata) {
+    diagnostic_msgs::msg::KeyValue kv;
+    kv.key = key;
+    kv.value = value;
+    msg.metadata.push_back(kv);
+  }
+
+  beacon_pub_->publish(msg);
+}
+
+}  // namespace sensor_diagnostics

demos/sensor_diagnostics/src/beacon_helper.hpp

@@ -11,14 +11,11 @@
 #pragma once
 
 #include <chrono>
+#include <map>
 #include <memory>
 #include <string>
-#include <unordered_map>
 #include <vector>
 
-#include <unistd.h>
-
-#include "diagnostic_msgs/msg/key_value.hpp"
 #include "rclcpp/rclcpp.hpp"
 #include "ros2_medkit_msgs/msg/medkit_discovery_hint.hpp"
 
@@ -34,97 +31,15 @@ class BeaconHelper
     std::string display_name;
     std::string component_id;
     std::vector<std::string> function_ids;
-    std::unordered_map<std::string, std::string> metadata;
+    std::map<std::string, std::string> metadata;
   };
 
-  BeaconHelper(rclcpp::Node * node, const Config & config)
-  : node_(node), config_(config)
-  {
-    node_->declare_parameter("beacon_mode", "none");
-    mode_ = node_->get_parameter("beacon_mode").as_string();
-
-    // Cache process info (constant during lifetime)
-    pid_ = static_cast<uint32_t>(getpid());
-    char hostname_buf[256];
-    hostname_buf[sizeof(hostname_buf) - 1] = '\0';
-    if (gethostname(hostname_buf, sizeof(hostname_buf) - 1) == 0) {
-      hostname_ = hostname_buf;
-    }
-
-    if (mode_ == "topic") {
-      init_topic_beacon();
-    } else if (mode_ == "param") {
-      init_param_beacon();
-    } else if (mode_ != "none") {
-      RCLCPP_WARN(
-        node_->get_logger(),
-        "Unknown beacon_mode '%s', expected none/topic/param. Beacon disabled.",
-        mode_.c_str());
-    }
-  }
+  BeaconHelper(rclcpp::Node * node, const Config & config);
 
 private:
-  void init_topic_beacon()
-  {
-    beacon_pub_ = node_->create_publisher<ros2_medkit_msgs::msg::MedkitDiscoveryHint>(
-      "/ros2_medkit/discovery", 10);
-
-    beacon_timer_ = node_->create_wall_timer(
-      std::chrono::seconds(5),
-      [this]() { publish_beacon(); });
-
-    // Publish immediately on startup
-    publish_beacon();
-
-    RCLCPP_INFO(
-      node_->get_logger(), "Topic beacon enabled (entity: %s)",
-      config_.entity_id.c_str());
-  }
-
-  void init_param_beacon()
-  {
-    node_->declare_parameter("ros2_medkit.discovery.entity_id", config_.entity_id);
-    node_->declare_parameter("ros2_medkit.discovery.display_name", config_.display_name);
-    node_->declare_parameter("ros2_medkit.discovery.component_id", config_.component_id);
-    node_->declare_parameter("ros2_medkit.discovery.function_ids", config_.function_ids);
-    node_->declare_parameter(
-      "ros2_medkit.discovery.process_id", static_cast<int>(pid_));
-    node_->declare_parameter("ros2_medkit.discovery.process_name", node_->get_name());
-
-    if (!hostname_.empty()) {
-      node_->declare_parameter("ros2_medkit.discovery.hostname", hostname_);
-    }
-
-    for (const auto & [key, value] : config_.metadata) {
-      node_->declare_parameter("ros2_medkit.discovery.metadata." + key, value);
-    }
-
-    RCLCPP_INFO(
-      node_->get_logger(), "Parameter beacon enabled (entity: %s)",
-      config_.entity_id.c_str());
-  }
-
-  void publish_beacon()
-  {
-    auto msg = ros2_medkit_msgs::msg::MedkitDiscoveryHint();
-    msg.entity_id = config_.entity_id;
-    msg.display_name = config_.display_name;
-    msg.component_id = config_.component_id;
-    msg.function_ids = config_.function_ids;
-    msg.process_id = pid_;
-    msg.process_name = node_->get_name();
-    msg.hostname = hostname_;
-    msg.stamp = node_->now();
-
-    for (const auto & [key, value] : config_.metadata) {
-      diagnostic_msgs::msg::KeyValue kv;
-      kv.key = key;
-      kv.value = value;
-      msg.metadata.push_back(kv);
-    }
-
-    beacon_pub_->publish(msg);
-  }
+  void init_topic_beacon();
+  void init_param_beacon();
+  void publish_beacon();
 
   rclcpp::Node * node_;
   Config config_;

demos/sensor_diagnostics/src/camera_sim_node.cpp

@@ -74,8 +74,9 @@ class CameraSimNode : public rclcpp::Node
       std::bind(&CameraSimNode::on_parameter_change, this, std::placeholders::_1));
 
     // Initialize beacon (reads beacon_mode parameter: none/topic/param)
+    auto entity_id = this->declare_parameter("entity_id", "camera-sim");
     beacon_ = std::make_unique<BeaconHelper>(this, BeaconHelper::Config{
-      "camera-sim", "Camera Simulator", "camera-unit",
+      entity_id, "Camera Simulator", "camera-unit",
       {"sensor-monitoring"},
       {{"sensor_type", "camera"}, {"data_topic", "image_raw"}, {"frame_id", "camera_link"}},
     });

demos/sensor_diagnostics/src/gps_sim_node.cpp

@@ -74,8 +74,9 @@ class GpsSimNode : public rclcpp::Node
       std::bind(&GpsSimNode::on_parameter_change, this, std::placeholders::_1));
 
     // Initialize beacon (reads beacon_mode parameter: none/topic/param)
+    auto entity_id = this->declare_parameter("entity_id", "gps-sim");
     beacon_ = std::make_unique<BeaconHelper>(this, BeaconHelper::Config{
-      "gps-sim", "GPS Simulator", "gps-unit",
+      entity_id, "GPS Simulator", "gps-unit",
       {"sensor-monitoring"},
       {{"sensor_type", "gps"}, {"data_topic", "fix"}, {"frame_id", "gps_link"}},
     });

demos/sensor_diagnostics/src/imu_sim_node.cpp

@@ -70,8 +70,9 @@ class ImuSimNode : public rclcpp::Node
       std::bind(&ImuSimNode::on_parameter_change, this, std::placeholders::_1));
 
     // Initialize beacon (reads beacon_mode parameter: none/topic/param)
+    auto entity_id = this->declare_parameter("entity_id", "imu-sim");
     beacon_ = std::make_unique<BeaconHelper>(this, BeaconHelper::Config{
-      "imu-sim", "IMU Simulator", "imu-unit",
+      entity_id, "IMU Simulator", "imu-unit",
       {"sensor-monitoring"},
       {{"sensor_type", "imu"}, {"data_topic", "imu"}, {"frame_id", "imu_link"}},
     });

demos/sensor_diagnostics/src/lidar_sim_node.cpp

@@ -83,8 +83,9 @@ class LidarSimNode : public rclcpp::Node
       std::bind(&LidarSimNode::on_parameter_change, this, std::placeholders::_1));
 
     // Initialize beacon (reads beacon_mode parameter: none/topic/param)
+    auto entity_id = this->declare_parameter("entity_id", "lidar-sim");
     beacon_ = std::make_unique<BeaconHelper>(this, BeaconHelper::Config{
-      "lidar-sim", "LiDAR Simulator", "lidar-unit",
+      entity_id, "LiDAR Simulator", "lidar-unit",
       {"sensor-monitoring"},
       {{"sensor_type", "lidar"}, {"data_topic", "scan"}, {"frame_id", "lidar_link"}},
     });