a rosout publisher of a node might not exist (#1115)

* a rosout publisher of a node might not exist to return OK if add/remove sublogger if the rosout publisher not exist rather than using a new `get` function. Signed-off-by: Chen Lihui <[email protected]> Co-authored-by: Chris Lalancette <[email protected]>
ros2 · Nov 15, 2023 · f7efa0b · f7efa0b
1 parent bf5d4bc
commit f7efa0b
Show file tree

Hide file tree

Showing 5 changed files with 25 additions and 14 deletions.
diff --git a/rcl/include/rcl/logging_rosout.h b/rcl/include/rcl/logging_rosout.h
@@ -213,7 +213,7 @@ rcl_logging_rosout_output_handler(
  * \param[in] sublogger_name a sublogger name
  * \return #RCL_RET_OK if the subordinate logger was created successfully, or
  * \return #RCL_RET_INVALID_ARGUMENT if any arguments are invalid, or
- * \return #RCL_RET_SUBLOGGER_ALREADY_EXIST if the subordinate logger already exists, or
+ * \return #RCL_RET_NOT_FOUND if the parent logger does not exist, or
  * \return #RCL_RET_BAD_ALLOC if allocating memory failed, or
  * \return #RCL_RET_ERROR if an unspecified error occurs.
  */
@@ -242,6 +242,7 @@ rcl_logging_rosout_add_sublogger(
  * \param[in] sublogger_name a sublogger name
  * \return #RCL_RET_OK if the subordinate logger was finalized successfully, or
  * \return #RCL_RET_INVALID_ARGUMENT if any arguments are invalid, or
+ * \return #RCL_RET_NOT_FOUND if the sublogger does not exist, or
  * \return #RCL_RET_BAD_ALLOC if allocating memory failed, or
  * \return #RCL_RET_ERROR if an unspecified error occurs.
  */

diff --git a/rcl/include/rcl/types.h b/rcl/include/rcl/types.h
@@ -51,6 +51,8 @@ typedef rmw_ret_t rcl_ret_t;
 #define RCL_RET_UNKNOWN_SUBSTITUTION 105
 /// rcl_shutdown() already called return code.
 #define RCL_RET_ALREADY_SHUTDOWN 106
+/// Resource not found
+#define RCL_RET_NOT_FOUND 107
 
 // rcl node specific ret codes in 2XX
 /// Invalid rcl_node_t given return code.

diff --git a/rcl/src/rcl/common.c b/rcl/src/rcl/common.c
@@ -57,6 +57,8 @@ rcl_convert_rcutils_ret_to_rcl_ret(rcutils_ret_t rcutils_ret)
       return RCL_RET_INVALID_ARGUMENT;
     case RCUTILS_RET_NOT_INITIALIZED:
       return RCL_RET_NOT_INIT;
+    case RCUTILS_RET_NOT_FOUND:
+      return RCL_RET_NOT_FOUND;
     default:
       return RCL_RET_ERROR;
   }

diff --git a/rcl/src/rcl/logging_rosout.c b/rcl/src/rcl/logging_rosout.c
@@ -15,6 +15,7 @@
 #include "rcl/logging_rosout.h"
 
 #include "rcl/allocator.h"
+#include "rcl/common.h"
 #include "rcl/error_handling.h"
 #include "rcl/node.h"
 #include "rcl/publisher.h"
@@ -438,20 +439,23 @@ rcl_logging_rosout_add_sublogger(
   rosout_map_entry_t entry;
   rosout_sublogger_entry_t sublogger_entry;
 
+  RCL_CHECK_ARGUMENT_FOR_NULL(logger_name, RCL_RET_INVALID_ARGUMENT);
+  RCL_CHECK_ARGUMENT_FOR_NULL(sublogger_name, RCL_RET_INVALID_ARGUMENT);
+  rcutils_ret_t rcutils_ret = rcutils_hash_map_get(&__logger_map, &logger_name, &entry);
+  if (RCUTILS_RET_OK != rcutils_ret) {
+    if (RCUTILS_RET_NOT_FOUND == rcutils_ret) {
+      RCL_SET_ERROR_MSG_WITH_FORMAT_STRING("Failed to get logger entry for '%s'.", logger_name);
+    }
+    return rcl_convert_rcutils_ret_to_rcl_ret(rcutils_ret);
+  }
+
   status =
     _rcl_logging_rosout_get_full_sublogger_name(logger_name, sublogger_name, &full_sublogger_name);
   if (RCL_RET_OK != status) {
     // Error already set
     return status;
   }
 
-  rcutils_ret_t rcutils_ret = rcutils_hash_map_get(&__logger_map, &logger_name, &entry);
-  if (RCUTILS_RET_OK != rcutils_ret) {
-    RCL_SET_ERROR_MSG_WITH_FORMAT_STRING("The entry of logger '%s' not exist.", logger_name);
-    status = RCL_RET_ERROR;
-    goto cleanup;
-  }
-
   if (rcutils_hash_map_key_exists(&__logger_map, &full_sublogger_name)) {
     // To get the entry and increase the reference count
     status = rcl_ret_from_rcutils_ret(
@@ -517,6 +521,8 @@ rcl_logging_rosout_remove_sublogger(
   rcl_ret_t status = RCL_RET_OK;
   char * full_sublogger_name = NULL;
   rosout_sublogger_entry_t sublogger_entry;
+  RCL_CHECK_ARGUMENT_FOR_NULL(logger_name, RCL_RET_INVALID_ARGUMENT);
+  RCL_CHECK_ARGUMENT_FOR_NULL(sublogger_name, RCL_RET_INVALID_ARGUMENT);
 
   status =
     _rcl_logging_rosout_get_full_sublogger_name(logger_name, sublogger_name, &full_sublogger_name);
@@ -525,13 +531,13 @@ rcl_logging_rosout_remove_sublogger(
     return status;
   }
 
-  // remove the entry from the map
   if (!rcutils_hash_map_key_exists(&__logger_map, &full_sublogger_name)) {
-    RCL_SET_ERROR_MSG_WITH_FORMAT_STRING("Sub-logger '%s' not exist.", full_sublogger_name);
-    status = RCL_RET_ERROR;
+    status = RCL_RET_NOT_FOUND;
+    RCL_SET_ERROR_MSG_WITH_FORMAT_STRING("Logger for '%s' not found.", full_sublogger_name);
     goto cleanup;
   }
 
+  // remove the entry from the map
   status = rcl_ret_from_rcutils_ret(
     rcutils_hash_map_get(&__sublogger_map, &full_sublogger_name, &sublogger_entry));
   if (RCL_RET_OK != status) {

diff --git a/rcl/test/rcl/test_logging_rosout.cpp b/rcl/test/rcl/test_logging_rosout.cpp
@@ -380,9 +380,9 @@ TEST_F(
   rcl_reset_error();
   EXPECT_EQ(RCL_RET_INVALID_ARGUMENT, rcl_logging_rosout_add_sublogger(logger_name, ""));
   rcl_reset_error();
-  EXPECT_EQ(RCL_RET_INVALID_ARGUMENT, rcl_logging_rosout_add_sublogger("", "child"));
+  EXPECT_EQ(RCL_RET_NOT_FOUND, rcl_logging_rosout_add_sublogger("", "child"));
   rcl_reset_error();
-  EXPECT_EQ(RCL_RET_ERROR, rcl_logging_rosout_add_sublogger("no_exist", "child"));
+  EXPECT_EQ(RCL_RET_NOT_FOUND, rcl_logging_rosout_add_sublogger("no_exist", "child"));
   rcl_reset_error();
 
   EXPECT_EQ(RCL_RET_INVALID_ARGUMENT, rcl_logging_rosout_remove_sublogger(nullptr, nullptr));
@@ -407,7 +407,7 @@ TEST_F(
 
   EXPECT_EQ(RCL_RET_OK, rcl_logging_rosout_add_sublogger(logger_name, "child2"));
   EXPECT_EQ(RCL_RET_OK, rcl_logging_rosout_remove_sublogger(logger_name, "child2"));
-  EXPECT_EQ(RCL_RET_ERROR, rcl_logging_rosout_remove_sublogger(logger_name, "child2"));
+  EXPECT_EQ(RCL_RET_NOT_FOUND, rcl_logging_rosout_remove_sublogger(logger_name, "child2"));
   rcl_reset_error();
 }