fix: address all PR #14 review comments

Fixes from @mfaferek93:
- Update copyright year 2025 → 2026 in all source files
- Add early return in report_fault() to avoid spamming FaultManager
- Remove unused normal_dist_ member from camera_sim_node
- Unify diagnostic topic to /diagnostics for all sensors (IMU, GPS, LiDAR, Camera)

Fixes from @Copilot:
- Add rcl_interfaces to ament_target_dependencies for lidar/camera nodes
- Fix docker-compose jq filter: .[] → .items[]
- Add exec_depend for ros2_medkit_diagnostic_bridge and fault_manager
- Fix anomaly_detector topic subscriptions to match sensor namespaces
- Add runtime scan_rate validation in lidar parameter callback
- Add num_readings validation with fallback to 360

Additional fixes:
- Update restore-normal.sh to clear IMU_SIM and GPS_SIM faults

Author: bburda

demos/sensor_diagnostics/CMakeLists.txt

@@ -21,6 +21,7 @@ ament_target_dependencies(lidar_sim_node
   rclcpp
   sensor_msgs
   diagnostic_msgs
+  rcl_interfaces
 )
 
 # IMU simulator node
@@ -45,6 +46,7 @@ ament_target_dependencies(camera_sim_node
   rclcpp
   sensor_msgs
   diagnostic_msgs
+  rcl_interfaces
 )
 
 # Anomaly detector node

demos/sensor_diagnostics/docker-compose.yml

@@ -44,5 +44,5 @@ services:
                ros2 launch sensor_diagnostics_demo demo.launch.py &
                sleep 10 &&
                curl -sf http://localhost:8080/api/v1/health &&
-               curl -sf http://localhost:8080/api/v1/apps | jq '.[] | .name' &&
+               curl -sf http://localhost:8080/api/v1/apps | jq '.items[] | .id' &&
                echo 'CI validation passed!'"

demos/sensor_diagnostics/include/sensor_diagnostics/simulated_sensor_base.hpp

@@ -1,4 +1,4 @@
-// Copyright 2025 selfpatch
+// Copyright 2026 selfpatch
 // SPDX-License-Identifier: Apache-2.0
 
 /// @file simulated_sensor_base.hpp

demos/sensor_diagnostics/package.xml

@@ -19,6 +19,8 @@
 
   <exec_depend>ros2launch</exec_depend>
   <exec_depend>ros2_medkit_gateway</exec_depend>
+  <exec_depend>ros2_medkit_diagnostic_bridge</exec_depend>
+  <exec_depend>ros2_medkit_fault_manager</exec_depend>
 
   <test_depend>ament_lint_auto</test_depend>
   <test_depend>ament_lint_common</test_depend>

demos/sensor_diagnostics/restore-normal.sh

@@ -49,14 +49,15 @@ curl -s -X PUT "${API_BASE}/apps/camera_sim/configurations/inject_black_frames"
   -H "Content-Type: application/json" -d '{"value": false}'
 
 # Clear all faults from FaultManager
-# Legacy path faults (via diagnostic_bridge) - reported BY diagnostic_bridge
-# The fault_code is the name from DiagnosticStatus.name (e.g., LIDAR_SIM, CAMERA_SIM)
+# All sensors now publish to /diagnostics, so all faults come through diagnostic_bridge
 echo ""
 echo "Clearing all faults from FaultManager..."
 curl -s -X DELETE "${API_BASE}/apps/diagnostic_bridge/faults/LIDAR_SIM" > /dev/null 2>&1
 curl -s -X DELETE "${API_BASE}/apps/diagnostic_bridge/faults/CAMERA_SIM" > /dev/null 2>&1
+curl -s -X DELETE "${API_BASE}/apps/diagnostic_bridge/faults/IMU_SIM" > /dev/null 2>&1
+curl -s -X DELETE "${API_BASE}/apps/diagnostic_bridge/faults/GPS_SIM" > /dev/null 2>&1
 
-# Modern path faults (via anomaly_detector direct service call)
+# Faults from anomaly_detector (modern path for anomaly detection)
 curl -s -X DELETE "${API_BASE}/apps/anomaly_detector/faults/SENSOR_TIMEOUT" > /dev/null 2>&1
 curl -s -X DELETE "${API_BASE}/apps/anomaly_detector/faults/SENSOR_NAN" > /dev/null 2>&1
 curl -s -X DELETE "${API_BASE}/apps/anomaly_detector/faults/SENSOR_OUT_OF_RANGE" > /dev/null 2>&1

demos/sensor_diagnostics/src/anomaly_detector_node.cpp

@@ -1,4 +1,4 @@
-// Copyright 2025 selfpatch
+// Copyright 2026 selfpatch
 // SPDX-License-Identifier: Apache-2.0
 
 /// @file anomaly_detector_node.cpp
@@ -43,12 +43,13 @@ class AnomalyDetectorNode : public rclcpp::Node
 
     // Create subscribers for MODERN path sensors (IMU, GPS)
     // Note: LiDAR and Camera use LEGACY path via /diagnostics → diagnostic_bridge
+    // Topics use absolute paths matching sensor namespace (/sensors/imu_sim/imu, /sensors/gps_sim/fix)
     imu_sub_ = this->create_subscription<sensor_msgs::msg::Imu>(
-      "/sensors/imu", 10,
+      "/sensors/imu_sim/imu", 10,
       std::bind(&AnomalyDetectorNode::imu_callback, this, std::placeholders::_1));
 
     gps_sub_ = this->create_subscription<sensor_msgs::msg::NavSatFix>(
-      "/sensors/fix", 10,
+      "/sensors/gps_sim/fix", 10,
       std::bind(&AnomalyDetectorNode::gps_callback, this, std::placeholders::_1));
 
     // Create publisher for detected faults (supplementary diagnostic topic)
@@ -173,8 +174,11 @@ class AnomalyDetectorNode : public rclcpp::Node
     const std::string & source, const std::string & code,
     const std::string & message, uint8_t severity = 2)
   {
-    // Track active faults for later clearing
+    // Skip if already reported (avoid spamming FaultManager)
     std::string fault_key = source + ":" + code;
+    if (active_faults_.count(fault_key)) {
+      return;
+    }
     active_faults_.insert(fault_key);
 
     // Publish to diagnostic topic (legacy)

demos/sensor_diagnostics/src/camera_sim_node.cpp

@@ -1,4 +1,4 @@
-// Copyright 2025 selfpatch
+// Copyright 2026 selfpatch
 // SPDX-License-Identifier: Apache-2.0
 
 /// @file camera_sim_node.cpp
@@ -33,7 +33,6 @@ class CameraSimNode : public rclcpp::Node
   CameraSimNode()
   : Node("camera_sim"),
     rng_(std::random_device{}()),
-    normal_dist_(0.0, 1.0),
     uniform_dist_(0.0, 1.0)
   {
     // Declare parameters
@@ -258,7 +257,6 @@ class CameraSimNode : public rclcpp::Node
 
   // Random number generation
   std::mt19937 rng_;
-  std::normal_distribution<double> normal_dist_;
   std::uniform_real_distribution<double> uniform_dist_;
 
   // Parameters

demos/sensor_diagnostics/src/gps_sim_node.cpp

@@ -1,4 +1,4 @@
-// Copyright 2025 selfpatch
+// Copyright 2026 selfpatch
 // SPDX-License-Identifier: Apache-2.0
 
 /// @file gps_sim_node.cpp
@@ -13,6 +13,7 @@
 #include <memory>
 #include <random>
 
+#include "diagnostic_msgs/msg/diagnostic_array.hpp"
 #include "diagnostic_msgs/msg/diagnostic_status.hpp"
 #include "diagnostic_msgs/msg/key_value.hpp"
 #include "rclcpp/rclcpp.hpp"
@@ -48,8 +49,9 @@ class GpsSimNode : public rclcpp::Node
 
     // Create publishers
     fix_pub_ = this->create_publisher<sensor_msgs::msg::NavSatFix>("fix", 10);
-    diag_pub_ = this->create_publisher<diagnostic_msgs::msg::DiagnosticStatus>(
-      "diagnostics", 10);
+    // Publish to absolute /diagnostics topic for legacy fault reporting via diagnostic_bridge
+    diag_pub_ = this->create_publisher<diagnostic_msgs::msg::DiagnosticArray>(
+      "/diagnostics", 10);
 
     // Create timer (with rate validation)
     double rate = this->get_parameter("rate").as_double();
@@ -190,8 +192,12 @@ class GpsSimNode : public rclcpp::Node
 
   void publish_diagnostics(const std::string & status, const std::string & message)
   {
+    auto diag_array = diagnostic_msgs::msg::DiagnosticArray();
+    diag_array.header.stamp = this->now();
+
     auto diag = diagnostic_msgs::msg::DiagnosticStatus();
     diag.name = "gps_sim";
+    diag.hardware_id = "gps_sensor";
 
     if (status == "OK") {
       diag.level = diagnostic_msgs::msg::DiagnosticStatus::OK;
@@ -216,12 +222,13 @@ class GpsSimNode : public rclcpp::Node
     kv.value = std::to_string(position_noise_stddev_) + "m";
     diag.values.push_back(kv);
 
-    diag_pub_->publish(diag);
+    diag_array.status.push_back(diag);
+    diag_pub_->publish(diag_array);
   }
 
   // Publishers
   rclcpp::Publisher<sensor_msgs::msg::NavSatFix>::SharedPtr fix_pub_;
-  rclcpp::Publisher<diagnostic_msgs::msg::DiagnosticStatus>::SharedPtr diag_pub_;
+  rclcpp::Publisher<diagnostic_msgs::msg::DiagnosticArray>::SharedPtr diag_pub_;
 
   // Timer
   rclcpp::TimerBase::SharedPtr timer_;

demos/sensor_diagnostics/src/imu_sim_node.cpp

@@ -1,4 +1,4 @@
-// Copyright 2025 selfpatch
+// Copyright 2026 selfpatch
 // SPDX-License-Identifier: Apache-2.0
 
 /// @file imu_sim_node.cpp
@@ -13,6 +13,7 @@
 #include <memory>
 #include <random>
 
+#include "diagnostic_msgs/msg/diagnostic_array.hpp"
 #include "diagnostic_msgs/msg/diagnostic_status.hpp"
 #include "diagnostic_msgs/msg/key_value.hpp"
 #include "rclcpp/rclcpp.hpp"
@@ -44,8 +45,9 @@ class ImuSimNode : public rclcpp::Node
 
     // Create publishers
     imu_pub_ = this->create_publisher<sensor_msgs::msg::Imu>("imu", 10);
-    diag_pub_ = this->create_publisher<diagnostic_msgs::msg::DiagnosticStatus>(
-      "diagnostics", 10);
+    // Publish to absolute /diagnostics topic for legacy fault reporting via diagnostic_bridge
+    diag_pub_ = this->create_publisher<diagnostic_msgs::msg::DiagnosticArray>(
+      "/diagnostics", 10);
 
     // Create timer (with rate validation)
     double rate = this->get_parameter("rate").as_double();
@@ -172,8 +174,12 @@ class ImuSimNode : public rclcpp::Node
 
   void publish_diagnostics(const std::string & status, const std::string & message)
   {
+    auto diag_array = diagnostic_msgs::msg::DiagnosticArray();
+    diag_array.header.stamp = this->now();
+
     auto diag = diagnostic_msgs::msg::DiagnosticStatus();
     diag.name = "imu_sim";
+    diag.hardware_id = "imu_sensor";
 
     if (status == "OK") {
       diag.level = diagnostic_msgs::msg::DiagnosticStatus::OK;
@@ -194,12 +200,13 @@ class ImuSimNode : public rclcpp::Node
     kv.value = std::to_string(msg_count_);
     diag.values.push_back(kv);
 
-    diag_pub_->publish(diag);
+    diag_array.status.push_back(diag);
+    diag_pub_->publish(diag_array);
   }
 
   // Publishers
   rclcpp::Publisher<sensor_msgs::msg::Imu>::SharedPtr imu_pub_;
-  rclcpp::Publisher<diagnostic_msgs::msg::DiagnosticStatus>::SharedPtr diag_pub_;
+  rclcpp::Publisher<diagnostic_msgs::msg::DiagnosticArray>::SharedPtr diag_pub_;
 
   // Timer
   rclcpp::TimerBase::SharedPtr timer_;

demos/sensor_diagnostics/src/lidar_sim_node.cpp

@@ -1,4 +1,4 @@
-// Copyright 2025 selfpatch
+// Copyright 2026 selfpatch
 // SPDX-License-Identifier: Apache-2.0
 
 /// @file lidar_sim_node.cpp
@@ -91,6 +91,14 @@ class LidarSimNode : public rclcpp::Node
     angle_min_ = this->get_parameter("angle_min").as_double();
     angle_max_ = this->get_parameter("angle_max").as_double();
     num_readings_ = this->get_parameter("num_readings").as_int();
+    if (num_readings_ <= 0) {
+      RCLCPP_WARN(
+        this->get_logger(),
+        "Invalid num_readings parameter (%d). Using default 360.",
+        num_readings_);
+      num_readings_ = 360;
+      this->set_parameters({rclcpp::Parameter("num_readings", num_readings_)});
+    }
     noise_stddev_ = this->get_parameter("noise_stddev").as_double();
     failure_probability_ = this->get_parameter("failure_probability").as_double();
     inject_nan_ = this->get_parameter("inject_nan").as_bool();
@@ -121,8 +129,17 @@ class LidarSimNode : public rclcpp::Node
         drift_rate_ = param.as_double();
         RCLCPP_INFO(this->get_logger(), "Drift rate changed to %.4f", drift_rate_);
       } else if (param.get_name() == "scan_rate") {
-        // Update timer with new rate
+        // Update timer with new rate (with validation)
         double rate = param.as_double();
+        if (rate <= 0.0) {
+          RCLCPP_WARN(
+            this->get_logger(),
+            "Invalid scan_rate parameter value (%f Hz). Rejecting change.",
+            rate);
+          result.successful = false;
+          result.reason = "scan_rate must be positive";
+          return result;
+        }
         auto period = std::chrono::duration<double>(1.0 / rate);
         timer_ = this->create_wall_timer(
           std::chrono::duration_cast<std::chrono::nanoseconds>(period),