From f399f76d373754efa6576ab97563c8a6ab0b5439 Mon Sep 17 00:00:00 2001 From: David Bensoussan Date: Mon, 31 Aug 2026 12:35:36 +0200 Subject: [PATCH 1/3] feat(dc_measurements): resolve robot_name from literal, hostname, or file Records already carry robot_name through custom keys, but the value had to be written literally into each robot's params file -- one hand-edited file per robot in a fleet. robot_name now resolves in order: a literal custom_keys_str.robot_name.value (unchanged), then custom_keys_str.robot_name.value_from_file, then the machine's hostname as the default when neither is set. A missing file or an empty resolved value fails node configuration with a clear error instead of shipping Records with a blank robot_name. Closes #442 Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01LgsQrndgkMK3F7bUcWoU3a Signed-off-by: David Bensoussan --- dc_measurements/src/measurement_server.cpp | 69 ++++++++++++++- .../test/test_measurement_dummy.cpp | 84 +++++++++++++++++++ doc/src/dc/measurements.md | 15 ++++ 3 files changed, 167 insertions(+), 1 deletion(-) diff --git a/dc_measurements/src/measurement_server.cpp b/dc_measurements/src/measurement_server.cpp index 6babaf75f..3c04b1344 100644 --- a/dc_measurements/src/measurement_server.cpp +++ b/dc_measurements/src/measurement_server.cpp @@ -3,11 +3,74 @@ #include "dc_measurements/measurement_server.hpp" +#include + +#include +#include + namespace measurement_server { using namespace std::chrono_literals; // NOLINT +namespace +{ + +// robot_name is the one custom key whose identity a fleet can't hand-edit per robot (#442): +// literal value, then a file's contents, then the machine's hostname as the default. +constexpr const char* kRobotNameKey = "robot_name"; + +std::string resolveRobotNameFromHostname() +{ + char hostname_buf[256] = { 0 }; + if (gethostname(hostname_buf, sizeof(hostname_buf) - 1) != 0) + { + throw std::runtime_error(std::string("robot_name: failed to resolve hostname: ") + std::strerror(errno)); + } + return std::string(hostname_buf); +} + +std::string resolveRobotNameFromFile(const std::string& path) +{ + std::ifstream ifs(path); + if (!ifs.good()) + { + throw std::runtime_error("robot_name: could not read value_from_file '" + path + "'"); + } + return dc_util::get_file_content(path); +} + +std::string resolveRobotName(const std::string& value, const std::string& value_from_file) +{ + std::string resolved; + std::string source; + + if (!value.empty()) + { + resolved = value; + source = "the literal value"; + } + else if (!value_from_file.empty()) + { + resolved = resolveRobotNameFromFile(value_from_file); + source = "value_from_file '" + value_from_file + "'"; + } + else + { + resolved = resolveRobotNameFromHostname(); + source = "the hostname"; + } + + if (resolved.empty()) + { + throw std::runtime_error("robot_name: resolved to an empty value from " + source); + } + + return resolved; +} + +} // namespace + MeasurementServer::MeasurementServer(const rclcpp::NodeOptions& options, const std::vector& measurement_plugins) : nav2_util::LifecycleNode("measurement_server", "", options) @@ -56,7 +119,11 @@ void MeasurementServer::setCustomKeys() custom_keys_[i]["key"] = key; custom_keys_[i]["override"] = force_override; - if (!value.empty()) + if (custom_key == kRobotNameKey) + { + custom_keys_[i]["value"] = resolveRobotName(value, value_from_file); + } + else if (!value.empty()) { custom_keys_[i]["value"] = value; } diff --git a/dc_measurements/test/test_measurement_dummy.cpp b/dc_measurements/test/test_measurement_dummy.cpp index ef54cc93e..393d7b1e2 100644 --- a/dc_measurements/test/test_measurement_dummy.cpp +++ b/dc_measurements/test/test_measurement_dummy.cpp @@ -1,6 +1,11 @@ // SPDX-FileCopyrightText: 2022-2026 David Bensoussan // SPDX-License-Identifier: MPL-2.0 +#include + +#include +#include + #include #include "dc_interfaces/msg/string_stamped.hpp" @@ -100,6 +105,85 @@ TEST_F(MeasurementDummyTest, CustomKeysAreDeclaredInTheRecord) EXPECT_EQ(dummy_record_["custom_keys"], nlohmann::json::array({ "site" })); } +// robot_name is the one custom key whose resolution is flexible: literal, hostname, or a +// file's contents, in that order, with hostname as the default when nothing is set (#442). + +TEST_F(MeasurementDummyTest, RobotNameLiteralValueIsUnchanged) +{ + ms_node_->declare_parameter("dummy.plugin", std::string("dc_measurements/Dummy")); + ms_node_->declare_parameter("dummy.topic_output", std::string("/dc/measurement/dummy")); + ms_node_->declare_parameter("dummy.record", std::string("{\"message\": \"My message\"}")); + ms_node_->declare_parameter("custom_key_str_list", std::vector{ "robot_name" }); + ms_node_->declare_parameter("custom_keys_str.robot_name.name", std::string("robot_name")); + ms_node_->declare_parameter("custom_keys_str.robot_name.value", std::string("C3PO")); + + startLifecycleNode(); + + while (!dummy_callback_) + { + rclcpp::spin_some(ms_node_->get_node_base_interface()); + } + + EXPECT_EQ(dummy_record_["robot_name"], "C3PO"); +} + +TEST_F(MeasurementDummyTest, RobotNameDefaultsToTheHostname) +{ + ms_node_->declare_parameter("dummy.plugin", std::string("dc_measurements/Dummy")); + ms_node_->declare_parameter("dummy.topic_output", std::string("/dc/measurement/dummy")); + ms_node_->declare_parameter("dummy.record", std::string("{\"message\": \"My message\"}")); + ms_node_->declare_parameter("custom_key_str_list", std::vector{ "robot_name" }); + ms_node_->declare_parameter("custom_keys_str.robot_name.name", std::string("robot_name")); + + startLifecycleNode(); + + while (!dummy_callback_) + { + rclcpp::spin_some(ms_node_->get_node_base_interface()); + } + + char hostname_buf[256] = { 0 }; + ASSERT_EQ(gethostname(hostname_buf, sizeof(hostname_buf) - 1), 0); + EXPECT_EQ(dummy_record_["robot_name"], std::string(hostname_buf)); +} + +TEST_F(MeasurementDummyTest, RobotNameResolvesFromFile) +{ + auto robot_name_file = (std::filesystem::temp_directory_path() / "dc_measurement_dummy_robot_name_file").u8string(); + std::ofstream(robot_name_file) << "TB-42"; + + ms_node_->declare_parameter("dummy.plugin", std::string("dc_measurements/Dummy")); + ms_node_->declare_parameter("dummy.topic_output", std::string("/dc/measurement/dummy")); + ms_node_->declare_parameter("dummy.record", std::string("{\"message\": \"My message\"}")); + ms_node_->declare_parameter("custom_key_str_list", std::vector{ "robot_name" }); + ms_node_->declare_parameter("custom_keys_str.robot_name.name", std::string("robot_name")); + ms_node_->declare_parameter("custom_keys_str.robot_name.value_from_file", robot_name_file); + + startLifecycleNode(); + + while (!dummy_callback_) + { + rclcpp::spin_some(ms_node_->get_node_base_interface()); + } + + EXPECT_EQ(dummy_record_["robot_name"], "TB-42"); + + std::filesystem::remove(robot_name_file); +} + +TEST_F(MeasurementDummyTest, RobotNameMissingFileFailsConfigureClearly) +{ + ms_node_->declare_parameter("dummy.plugin", std::string("dc_measurements/Dummy")); + ms_node_->declare_parameter("dummy.topic_output", std::string("/dc/measurement/dummy")); + ms_node_->declare_parameter("dummy.record", std::string("{\"message\": \"My message\"}")); + ms_node_->declare_parameter("custom_key_str_list", std::vector{ "robot_name" }); + ms_node_->declare_parameter("custom_keys_str.robot_name.name", std::string("robot_name")); + ms_node_->declare_parameter("custom_keys_str.robot_name.value_from_file", + std::string("/nonexistent/dc_robot_name_that_does_not_exist")); + + EXPECT_THROW(ms_node_->configure(), std::runtime_error); +} + TEST_F(MeasurementDummyTest, NoCustomKeysLeavesTheRecordUntouched) { ms_node_->declare_parameter("dummy.plugin", std::string("dc_measurements/Dummy")); diff --git a/doc/src/dc/measurements.md b/doc/src/dc/measurements.md index 07602449e..a2762bb12 100644 --- a/doc/src/dc/measurements.md +++ b/doc/src/dc/measurements.md @@ -52,6 +52,21 @@ the Record as a `tags` field, but it has no routing effect — remove it. See th | run_id.counter_path | Path to store the last run. It is expanded with environment variables id | str | "$HOME/run_id" | | run_id.uuid | Generate a new run ID by using a random UUID | str | false | +### robot_name resolution + +`robot_name` is a custom key like any other, but when it appears in `custom_key_str_list` +its value resolves in a fixed order rather than always being a literal string, so a fleet +does not need one hand-edited params file per robot: + +1. `custom_keys_str.robot_name.value` — a literal string, unchanged from before. +2. `custom_keys_str.robot_name.value_from_file` — the contents of a file, e.g. one written + by the provisioning process. +3. The machine's hostname — the default when neither of the above is set. + +A `value_from_file` that names a file that cannot be read, or any source that resolves to +an empty string, fails node configuration with a clear error rather than shipping Records +with a missing or blank `robot_name`. + ### Custom keys on Files The keys listed in `custom_key_str_list` label a Measurement's **Files** as well as its From 7a61ae09351d556df55c94e51edce0889f1ba031 Mon Sep 17 00:00:00 2001 From: David Bensoussan Date: Mon, 31 Aug 2026 12:37:53 +0200 Subject: [PATCH 2/3] style: fix clang-format violations in test_measurement_dummy.cpp Include order and continuation-line alignment CI's clang-format hook flagged on PR #456. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01LgsQrndgkMK3F7bUcWoU3a Signed-off-by: David Bensoussan --- dc_measurements/test/test_measurement_dummy.cpp | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/dc_measurements/test/test_measurement_dummy.cpp b/dc_measurements/test/test_measurement_dummy.cpp index 393d7b1e2..1c68c6129 100644 --- a/dc_measurements/test/test_measurement_dummy.cpp +++ b/dc_measurements/test/test_measurement_dummy.cpp @@ -1,13 +1,12 @@ // SPDX-FileCopyrightText: 2022-2026 David Bensoussan // SPDX-License-Identifier: MPL-2.0 +#include #include #include #include -#include - #include "dc_interfaces/msg/string_stamped.hpp" #include "dc_measurements/measurement_server.hpp" #include "dc_util/json_utils.hpp" @@ -179,7 +178,7 @@ TEST_F(MeasurementDummyTest, RobotNameMissingFileFailsConfigureClearly) ms_node_->declare_parameter("custom_key_str_list", std::vector{ "robot_name" }); ms_node_->declare_parameter("custom_keys_str.robot_name.name", std::string("robot_name")); ms_node_->declare_parameter("custom_keys_str.robot_name.value_from_file", - std::string("/nonexistent/dc_robot_name_that_does_not_exist")); + std::string("/nonexistent/dc_robot_name_that_does_not_exist")); EXPECT_THROW(ms_node_->configure(), std::runtime_error); } From 1cc3eb43ae416ec23c2b8c3baace2eb898e2b16a Mon Sep 17 00:00:00 2001 From: David Bensoussan Date: Mon, 31 Aug 2026 12:53:02 +0200 Subject: [PATCH 3/3] fix(dc_measurements): assert on lifecycle state, not a thrown exception rclcpp_lifecycle wraps on_configure() in its own catch and converts an uncaught exception into CallbackReturn::ERROR instead of propagating it to the caller of configure() -- the same behavior test_measurement_bool_equal.cpp already documents for BoolEqual's type-mismatch case. RobotNameMissingFile- FailsConfigureClearly was asserting the wrong thing (EXPECT_THROW), which CI's colcon test caught: configure() logged the "robot_name: could not read value_from_file" error exactly as intended but returned normally, so the test now asserts the node fails to reach the inactive state instead. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01LgsQrndgkMK3F7bUcWoU3a Signed-off-by: David Bensoussan --- dc_measurements/test/test_measurement_dummy.cpp | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/dc_measurements/test/test_measurement_dummy.cpp b/dc_measurements/test/test_measurement_dummy.cpp index 1c68c6129..975ed3cc8 100644 --- a/dc_measurements/test/test_measurement_dummy.cpp +++ b/dc_measurements/test/test_measurement_dummy.cpp @@ -10,6 +10,7 @@ #include "dc_interfaces/msg/string_stamped.hpp" #include "dc_measurements/measurement_server.hpp" #include "dc_util/json_utils.hpp" +#include "lifecycle_msgs/msg/state.hpp" class MeasurementDummyTest : public ::testing::Test { @@ -170,6 +171,13 @@ TEST_F(MeasurementDummyTest, RobotNameResolvesFromFile) std::filesystem::remove(robot_name_file); } +// rclcpp_lifecycle wraps every transition callback (on_configure here) in its own catch, +// converting an uncaught exception into a CallbackReturn::ERROR rather than letting it +// propagate to the caller of configure() (same behavior documented in +// test_measurement_bool_equal.cpp's MeasurementServerConfigureFailsToReachInactiveState). +// So the observable, end-to-end consequence of an unreadable value_from_file is that the +// whole MeasurementServer fails to reach the "inactive" state, with the resolveRobotName() +// error logged as the ERROR/FATAL "Original error" during the transition. TEST_F(MeasurementDummyTest, RobotNameMissingFileFailsConfigureClearly) { ms_node_->declare_parameter("dummy.plugin", std::string("dc_measurements/Dummy")); @@ -180,7 +188,9 @@ TEST_F(MeasurementDummyTest, RobotNameMissingFileFailsConfigureClearly) ms_node_->declare_parameter("custom_keys_str.robot_name.value_from_file", std::string("/nonexistent/dc_robot_name_that_does_not_exist")); - EXPECT_THROW(ms_node_->configure(), std::runtime_error); + auto result_state = ms_node_->configure(); + + EXPECT_NE(result_state.id(), lifecycle_msgs::msg::State::PRIMARY_STATE_INACTIVE); } TEST_F(MeasurementDummyTest, NoCustomKeysLeavesTheRecordUntouched)