Skip to content

Commit b50797e

Browse files
committed
fix: validate BEACON_MODE, cache process info, handle missing plugins
Address self-review findings: - Warn and fall back to 'none' for invalid BEACON_MODE values - Warn and fall back to 'none' when beacon plugin .so is not found - Cache pid and hostname in constructor instead of calling syscalls on every publish - Add explicit <chrono> include - Null-terminate hostname buffer to prevent truncation UB
1 parent 14f3835 commit b50797e

2 files changed

Lines changed: 47 additions & 10 deletions

File tree

demos/sensor_diagnostics/launch/demo.launch.py

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,16 @@ def generate_launch_description():
5757

5858
# Beacon mode from environment (controls both plugin loading and node behavior)
5959
beacon_mode = os.environ.get('BEACON_MODE', 'none')
60+
valid_beacon_modes = ('none', 'topic', 'param')
61+
if beacon_mode not in valid_beacon_modes:
62+
import sys
63+
print(
64+
f"WARNING: Invalid BEACON_MODE='{beacon_mode}'. "
65+
f"Valid values: {', '.join(valid_beacon_modes)}. "
66+
"Falling back to 'none'.",
67+
file=sys.stderr,
68+
)
69+
beacon_mode = 'none'
6070

6171
# Resolve plugin paths
6272
graph_provider_path = _resolve_plugin_path(
@@ -84,6 +94,14 @@ def generate_launch_description():
8494
plugin_overrides['plugins.topic_beacon.topic'] = \
8595
'/ros2_medkit/discovery'
8696
plugin_overrides['plugins.topic_beacon.beacon_ttl_sec'] = 10.0
97+
else:
98+
import sys
99+
print(
100+
"WARNING: BEACON_MODE=topic but topic_beacon plugin not "
101+
"found. Falling back to none.",
102+
file=sys.stderr,
103+
)
104+
beacon_mode = 'none'
87105
elif beacon_mode == 'param':
88106
param_beacon_path = _resolve_plugin_path(
89107
'ros2_medkit_param_beacon', 'param_beacon_plugin')
@@ -93,6 +111,14 @@ def generate_launch_description():
93111
param_beacon_path
94112
plugin_overrides['plugins.parameter_beacon.poll_interval_sec'] = \
95113
5.0
114+
else:
115+
import sys
116+
print(
117+
"WARNING: BEACON_MODE=param but param_beacon plugin not "
118+
"found. Falling back to none.",
119+
file=sys.stderr,
120+
)
121+
beacon_mode = 'none'
96122

97123
plugin_overrides['plugins'] = active_plugins
98124

demos/sensor_diagnostics/src/beacon_helper.hpp

Lines changed: 21 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010

1111
#pragma once
1212

13+
#include <chrono>
1314
#include <memory>
1415
#include <string>
1516
#include <unordered_map>
@@ -42,10 +43,23 @@ class BeaconHelper
4243
node_->declare_parameter("beacon_mode", "none");
4344
mode_ = node_->get_parameter("beacon_mode").as_string();
4445

46+
// Cache process info (constant during lifetime)
47+
pid_ = static_cast<uint32_t>(getpid());
48+
char hostname_buf[256];
49+
hostname_buf[sizeof(hostname_buf) - 1] = '\0';
50+
if (gethostname(hostname_buf, sizeof(hostname_buf) - 1) == 0) {
51+
hostname_ = hostname_buf;
52+
}
53+
4554
if (mode_ == "topic") {
4655
init_topic_beacon();
4756
} else if (mode_ == "param") {
4857
init_param_beacon();
58+
} else if (mode_ != "none") {
59+
RCLCPP_WARN(
60+
node_->get_logger(),
61+
"Unknown beacon_mode '%s', expected none/topic/param. Beacon disabled.",
62+
mode_.c_str());
4963
}
5064
}
5165

@@ -74,12 +88,11 @@ class BeaconHelper
7488
node_->declare_parameter("ros2_medkit.discovery.component_id", config_.component_id);
7589
node_->declare_parameter("ros2_medkit.discovery.function_ids", config_.function_ids);
7690
node_->declare_parameter(
77-
"ros2_medkit.discovery.process_id", static_cast<int>(getpid()));
91+
"ros2_medkit.discovery.process_id", static_cast<int>(pid_));
7892
node_->declare_parameter("ros2_medkit.discovery.process_name", node_->get_name());
7993

80-
char hostname_buf[256];
81-
if (gethostname(hostname_buf, sizeof(hostname_buf)) == 0) {
82-
node_->declare_parameter("ros2_medkit.discovery.hostname", std::string(hostname_buf));
94+
if (!hostname_.empty()) {
95+
node_->declare_parameter("ros2_medkit.discovery.hostname", hostname_);
8396
}
8497

8598
for (const auto & [key, value] : config_.metadata) {
@@ -98,15 +111,11 @@ class BeaconHelper
98111
msg.display_name = config_.display_name;
99112
msg.component_id = config_.component_id;
100113
msg.function_ids = config_.function_ids;
101-
msg.process_id = static_cast<uint32_t>(getpid());
114+
msg.process_id = pid_;
102115
msg.process_name = node_->get_name();
116+
msg.hostname = hostname_;
103117
msg.stamp = node_->now();
104118

105-
char hostname_buf[256];
106-
if (gethostname(hostname_buf, sizeof(hostname_buf)) == 0) {
107-
msg.hostname = hostname_buf;
108-
}
109-
110119
for (const auto & [key, value] : config_.metadata) {
111120
diagnostic_msgs::msg::KeyValue kv;
112121
kv.key = key;
@@ -120,6 +129,8 @@ class BeaconHelper
120129
rclcpp::Node * node_;
121130
Config config_;
122131
std::string mode_;
132+
uint32_t pid_{0};
133+
std::string hostname_;
123134
rclcpp::Publisher<ros2_medkit_msgs::msg::MedkitDiscoveryHint>::SharedPtr beacon_pub_;
124135
rclcpp::TimerBase::SharedPtr beacon_timer_;
125136
};

0 commit comments

Comments
 (0)