feat: add warn log when topics are sanitized (#72)

CHANGELOG.md

@@ -15,6 +15,7 @@ Release Versions:
 
 - chore: fix links in readme (#68)
 - feat: add mutex around step callback (#67)
+- feat: add warn log when topics are sanitized (#66)
 
 ## 4.0.0
 

source/modulo_components/include/modulo_components/ComponentInterface.hpp

@@ -625,12 +625,12 @@ inline void ComponentInterface::add_input(
 ) {
   using namespace modulo_core::communication;
   try {
-    std::string parsed_signal_name = utilities::parse_topic_name(signal_name);
     if (data == nullptr) {
       throw modulo_core::exceptions::NullPointerException(
-          "Invalid data pointer for input '" + parsed_signal_name + "'.");
+          "Invalid data pointer for input '" + signal_name + "'.");
     }
-    this->declare_input(parsed_signal_name, default_topic, fixed_topic);
+    this->declare_input(signal_name, default_topic, fixed_topic);
+    std::string parsed_signal_name = utilities::parse_topic_name(signal_name);
     auto topic_name = this->get_parameter_value<std::string>(parsed_signal_name + "_topic");
     RCLCPP_DEBUG_STREAM(this->node_logging_->get_logger(),
                         "Adding input '" << parsed_signal_name << "' with topic name '" << topic_name << "'.");

source/modulo_components/include/modulo_components/utilities/utilities.hpp

@@ -53,10 +53,10 @@ parse_node_name(const rclcpp::NodeOptions& options, const std::string& fallback
 [[maybe_unused]] static std::string parse_topic_name(const std::string& topic_name) {
   std::string output;
   for (char c : topic_name) {
-    if ((c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z') || (c >= '0' && c <= '9') || c == '_') {
-      if (!(c == '_' && output.empty())) {
-        output.insert(output.end(), std::tolower(c));
-      }
+    if ((c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z')) {
+      output.insert(output.end(), std::tolower(c));
+    } else if (!output.empty() && ((c >= '0' && c <= '9') || c == '_')) {
+      output.insert(output.end(), std::tolower(c));
     }
   }
   return output;

source/modulo_components/modulo_components/component_interface.py

@@ -451,6 +451,10 @@ def declare_signal(self, signal_name: str, signal_type: str, default_topic="", f
         if not parsed_signal_name:
             raise AddSignalError(f"The parsed signal name for {signal_type} '{signal_name}' is empty. Provide a "
                                  f"string with valid characters for the signal name ([a-zA-Z0-9_]).")
+        if signal_name != parsed_signal_name:
+            self.get_logger().warn(
+                f"The parsed signal name for {type} '{signal_name}' is '{parsed_signal_name}'."
+                  "Use the parsed signal name to refer to this {type} and its topic parameter.")
         if parsed_signal_name in self._inputs.keys():
             raise AddSignalError(f"Signal with name '{parsed_signal_name}' already exists as input.")
         if parsed_signal_name in self._outputs.keys():
@@ -589,6 +593,10 @@ def callback_wrapper(request, response, cb):
             if not parsed_service_name:
                 raise AddServiceError(f"The parsed signal name for service {service_name} is empty. Provide a "
                                       f"string with valid characters for the service name ([a-zA-Z0-9_]).")
+            if service_name != parsed_service_name:
+                self.get_logger().warn(
+                    f"The parsed name for service '{service_name}' is '{parsed_service_name}'."
+                    "Use the parsed name to refer to this service.")
             if parsed_service_name in self._services_dict.keys():
                 raise AddServiceError(f"Service with name '{parsed_service_name}' already exists.")
             signature = inspect.signature(callback)

source/modulo_components/modulo_components/utilities/utilities.py

@@ -13,7 +13,7 @@ def parse_topic_name(topic_name: str) -> str:
     :return: The sanitized string
     """
     sanitized_string = re.sub("\W", "", topic_name, flags=re.ASCII).lower()
-    sanitized_string = sanitized_string.lstrip("_")
+    sanitized_string = sanitized_string.lstrip("0123456789_")
     return sanitized_string
 
 

source/modulo_components/src/ComponentInterface.cpp

@@ -282,6 +282,12 @@ void ComponentInterface::declare_signal(
         "The parsed signal name for " + type + " '" + signal_name
             + "' is empty. Provide a string with valid characters for the signal name ([a-zA-Z0-9_]).");
   }
+  if (signal_name != parsed_signal_name) {
+    RCLCPP_WARN_STREAM(
+        this->node_logging_->get_logger(),
+        "The parsed signal name for " + type + " '" + signal_name + "' is '" + parsed_signal_name
+            + "'. Use the parsed signal name to refer to this " + type + " and its topic parameter");
+  }
   if (this->inputs_.find(parsed_signal_name) != this->inputs_.cend()) {
     throw exceptions::AddSignalException("Signal with name '" + parsed_signal_name + "' already exists as input.");
   }
@@ -398,6 +404,12 @@ std::string ComponentInterface::validate_service_name(const std::string& service
         "The parsed service name for service '" + service_name
             + "' is empty. Provide a string with valid characters for the signal name ([a-zA-Z0-9_]).");
   }
+  if (service_name != parsed_service_name) {
+    RCLCPP_WARN_STREAM(
+        this->node_logging_->get_logger(),
+        "The parsed name for service '" + service_name + "' is '" + parsed_service_name
+            + "'. Use the parsed name to refer to this service");
+  }
   if (this->empty_services_.find(parsed_service_name) != this->empty_services_.cend()
       || this->string_services_.find(parsed_service_name) != this->string_services_.cend()) {
     throw exceptions::AddServiceException("Service with name '" + parsed_service_name + "' already exists.");

source/modulo_components/test/cpp/test_component.cpp

@@ -52,7 +52,7 @@ TEST_F(ComponentTest, RatePeriodParameters) {
 
 TEST_F(ComponentTest, AddRemoveOutput) {
   std::shared_ptr<State> data = make_shared_state(CartesianState::Random("test"));
-  component_->add_output("_tEsT_#1@3", data);
+  component_->add_output("8_tEsT_#1@3", data);
   EXPECT_TRUE(component_->outputs_.find("test_13") != component_->outputs_.end());
   EXPECT_NO_THROW(component_->outputs_.at("test_13")->publish());
   EXPECT_THROW(component_->publish_output("test_13"), exceptions::ComponentException);
@@ -65,9 +65,9 @@ TEST_F(ComponentTest, AddRemoveOutput) {
   component_->remove_output("test_13");
   EXPECT_TRUE(component_->outputs_.find("test_13") == component_->outputs_.end());
 
-  component_->add_output("_tEsT_#1@3", data, "", true, false);
+  component_->add_output("8_tEsT_#1@3", data, "", true, false);
   EXPECT_FALSE(component_->periodic_outputs_.at("test_13"));
-  EXPECT_NO_THROW(component_->publish_output("_tEsT_#1@3"));
+  EXPECT_NO_THROW(component_->publish_output("8_tEsT_#1@3"));
   EXPECT_NO_THROW(component_->publish_output("test_13"));
   EXPECT_THROW(component_->publish_output(""), exceptions::ComponentException);
 }

source/modulo_components/test/cpp/test_component_interface.cpp

@@ -95,12 +95,12 @@ TYPED_TEST(ComponentInterfaceTest, DeclareSignal) {
 
 TYPED_TEST(ComponentInterfaceTest, AddRemoveInput) {
   auto data = std::make_shared<bool>(true);
-  EXPECT_NO_THROW(this->component_->add_input("_tEsT_#1@3", data));
+  EXPECT_NO_THROW(this->component_->add_input("8_tEsT_#1@3", data));
   EXPECT_FALSE(this->component_->inputs_.find("test_13") == this->component_->inputs_.end());
   EXPECT_EQ(this->component_->template get_parameter_value<std::string>("test_13_topic"), "~/test_13");
 
   EXPECT_NO_THROW(this->component_->template add_input<std_msgs::msg::Bool>(
-      "_tEsT_#1@5", [](const std::shared_ptr<std_msgs::msg::Bool>) {}, "/topic", true));
+      "9_tEsT_#1@5", [](const std::shared_ptr<std_msgs::msg::Bool>) {}, "/topic", true));
   EXPECT_FALSE(this->component_->inputs_.find("test_15") == this->component_->inputs_.end());
   EXPECT_EQ(this->component_->template get_parameter_value<std::string>("test_15_topic"), "/topic");
 
@@ -178,7 +178,7 @@ TYPED_TEST(ComponentInterfaceTest, AddService) {
   EXPECT_EQ(static_cast<int>(this->component_->string_services_.size()), 1);
 
   // adding a mangled service name should succeed under a sanitized name
-  EXPECT_NO_THROW(this->component_->add_service("_tEsT_#1@3", empty_callback));
+  EXPECT_NO_THROW(this->component_->add_service("8_tEsT_#1@3", empty_callback));
   EXPECT_EQ(static_cast<int>(this->component_->empty_services_.size()), 2);
   EXPECT_NE(this->component_->empty_services_.find("test_13"), this->component_->empty_services_.cend());
 }
@@ -240,9 +240,9 @@ TYPED_TEST(ComponentInterfaceTest, CreateOutput) {
   EXPECT_EQ(pub_interface->get_message_pair()->get_type(), modulo_core::communication::MessageType::BOOL);
   EXPECT_THROW(pub_interface->publish(), modulo_core::exceptions::CoreException);
 
-  EXPECT_NO_THROW(this->component_->create_output(this->pub_type_, "_tEsT_#1@3", data, "", true, false));
+  EXPECT_NO_THROW(this->component_->create_output(this->pub_type_, "8_tEsT_#1@3", data, "", true, false));
   EXPECT_FALSE(this->component_->periodic_outputs_.at("test_13"));
-  EXPECT_NO_THROW(this->component_->publish_output("_tEsT_#1@3"));
+  EXPECT_NO_THROW(this->component_->publish_output("8_tEsT_#1@3"));
   EXPECT_NO_THROW(this->component_->publish_output("test_13"));
   EXPECT_THROW(this->component_->publish_output(""), exceptions::ComponentException);
 }

source/modulo_components/test/cpp/test_lifecycle_component.cpp

@@ -68,9 +68,9 @@ TEST_F(LifecycleComponentTest, AddRemoveOutput) {
   component_->remove_output("test_13");
   EXPECT_TRUE(component_->outputs_.find("test_13") == component_->outputs_.end());
 
-  component_->add_output("_tEsT_#1@3", data, "", true, false);
+  component_->add_output("8_tEsT_#1@3", data, "", true, false);
   EXPECT_FALSE(component_->periodic_outputs_.at("test_13"));
-  EXPECT_NO_THROW(component_->publish_output("_tEsT_#1@3"));
+  EXPECT_NO_THROW(component_->publish_output("8_tEsT_#1@3"));
   EXPECT_NO_THROW(component_->publish_output("test_13"));
   EXPECT_THROW(component_->publish_output(""), exceptions::ComponentException);
 }

source/modulo_components/test/python/test_component.py

@@ -10,13 +10,13 @@ def component(ros_context):
 
 
 def test_add_remove_output(component):
-    component.add_output("_tEsT_#1@3", "test", Bool)
+    component.add_output("8_tEsT_#1@3", "test", Bool)
     assert "test_13" in component._outputs.keys()
     assert component.get_parameter_value("test_13_topic") == "~/test_13"
     with pytest.raises(ComponentError):
         component.publish_output("test_13")
 
-    component.add_output("_tEsT_#1@5", "test", Bool, default_topic="/topic")
+    component.add_output("9_tEsT_#1@5", "test", Bool, default_topic="/topic")
     assert "test_15" in component._outputs.keys()
     assert component.get_parameter_value("test_15_topic") == "/topic"
 
@@ -26,9 +26,9 @@ def test_add_remove_output(component):
     component.remove_output("test_13")
     assert "test_13" not in component._inputs.keys()
 
-    component.add_output("_tEsT_#1@3", "test", Bool, publish_on_step=False)
+    component.add_output("8_tEsT_#1@3", "test", Bool, publish_on_step=False)
     assert not component._periodic_outputs["test_13"]
-    component.publish_output("_tEsT_#1@3")
+    component.publish_output("8_tEsT_#1@3")
     component.publish_output("test_13")
     with pytest.raises(ComponentError):
         component.publish_output("")

source/modulo_components/test/python/test_component_interface.py

@@ -88,11 +88,11 @@ def test_declare_signal(component_interface):
 
 
 def test_add_remove_input(component_interface):
-    component_interface.add_input("_tEsT_#1@3", "test", Bool)
+    component_interface.add_input("8_tEsT_#1@3", "test", Bool)
     assert "test_13" in component_interface._inputs.keys()
     assert component_interface.get_parameter_value("test_13_topic") == "~/test_13"
 
-    component_interface.add_input("_tEsT_#1@5", "test", Bool, default_topic="/topic")
+    component_interface.add_input("9_tEsT_#1@5", "test", Bool, default_topic="/topic")
     assert "test_15" in component_interface._inputs.keys()
     assert component_interface.get_parameter_value("test_15_topic") == "/topic"
 
@@ -133,7 +133,7 @@ def string_callback(payload: str):
     assert len(component_interface._services_dict) == 2
 
     # adding a mangled service name should succeed under a sanitized name
-    component_interface.add_service("_tEsT_#1@3", empty_callback)
+    component_interface.add_service("8_tEsT_#1@3", empty_callback)
     assert len(component_interface._services_dict) == 3
     assert "test_13" in component_interface._services_dict.keys()
 
@@ -159,9 +159,9 @@ def test_create_output(component_interface):
     assert component_interface._outputs["test"]["message_type"] == Bool
     assert component_interface._periodic_outputs["test"]
 
-    component_interface._create_output("_tEsT_#1@3", "test", Bool, clproto.MessageType.UNKNOWN_MESSAGE, "", True, False)
+    component_interface._create_output("8_tEsT_#1@3", "test", Bool, clproto.MessageType.UNKNOWN_MESSAGE, "", True, False)
     assert not component_interface._periodic_outputs["test_13"]
-    component_interface.publish_output("_tEsT_#1@3")
+    component_interface.publish_output("8_tEsT_#1@3")
     component_interface.publish_output("test_13")
     with pytest.raises(ComponentError):
         component_interface.publish_output("")

source/modulo_components/test/python/test_lifecycle_component.py

@@ -10,13 +10,13 @@ def lifecycle_component(ros_context):
 
 
 def test_add_remove_output(lifecycle_component):
-    lifecycle_component.add_output("_tEsT_#1@3", "test", Bool)
+    lifecycle_component.add_output("8_tEsT_#1@3", "test", Bool)
     assert "test_13" in lifecycle_component._outputs.keys()
     assert lifecycle_component.get_parameter_value("test_13_topic") == "~/test_13"
     with pytest.raises(ComponentError):
         lifecycle_component.publish_output("test_13")
 
-    lifecycle_component.add_output("_tEsT_#1@5", "test", Bool, default_topic="/topic")
+    lifecycle_component.add_output("9_tEsT_#1@5", "test", Bool, default_topic="/topic")
     assert "test_15" in lifecycle_component._outputs.keys()
     assert lifecycle_component.get_parameter_value("test_15_topic") == "/topic"
 
@@ -26,9 +26,9 @@ def test_add_remove_output(lifecycle_component):
     lifecycle_component.remove_output("test_13")
     assert "test_13" not in lifecycle_component._inputs.keys()
 
-    lifecycle_component.add_output("_tEsT_#1@3", "test", Bool, publish_on_step=False)
+    lifecycle_component.add_output("8_tEsT_#1@3", "test", Bool, publish_on_step=False)
     assert not lifecycle_component._periodic_outputs["test_13"]
-    lifecycle_component.publish_output("_tEsT_#1@3")
+    lifecycle_component.publish_output("8_tEsT_#1@3")
     lifecycle_component.publish_output("test_13")
     with pytest.raises(ComponentError):
         lifecycle_component.publish_output("")