Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

aligh with rcl that a rosout publisher of a node might not exist #1196

Merged
merged 5 commits into from
Nov 15, 2023
Merged

aligh with rcl that a rosout publisher of a node might not exist #1196

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1ee90e6
aligh with rcl that a rosout publisher of a node might not exist
Nov 3, 2023
e62d0c4
check for RCL_RET_NOT_FOUND
Nov 6, 2023
059bac6
print detailed message for exception
Nov 9, 2023
fc84c5e
test for a node with rosout disabled
Nov 14, 2023
e79106a
single quotes preferred
Nov 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions rclpy/rclpy/impl/rcutils_logger.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,13 +230,12 @@ def get_child(self, name):

if self.name:
# Prepend the name of this logger
_rclpy.rclpy_logging_rosout_add_sublogger(self.name, name)
fullname = self.name + _rclpy.rclpy_logging_get_separator_string() + name
else:
fullname = name

logger = RcutilsLogger(name=fullname)
if self.name:
if self.name and _rclpy.rclpy_logging_rosout_add_sublogger(self.name, name):
logger.logger_sublogger_namepair = (self.name, name)
return logger

Expand Down
14 changes: 9 additions & 5 deletions rclpy/src/rclpy/_rclpy_logging.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,14 +206,18 @@ rclpy_logging_get_logging_directory()
}

/// Add a subordinate logger based on a logger
void
bool
rclpy_logging_rosout_add_sublogger(const char * logger_name, const char * sublogger_name)
{
rclpy::LoggingGuard scoped_logging_guard;
rcl_ret_t rcl_ret = rcl_logging_rosout_add_sublogger(logger_name, sublogger_name);
if (RCL_RET_OK != rcl_ret) {
rcutils_reset_error();
throw std::runtime_error("Failed to call rcl_logging_rosout_add_sublogger");
if (RCL_RET_OK == rcl_ret) {
return true;
} else if (RCL_RET_NOT_FOUND == rcl_ret) {
rcl_reset_error();
return false;
} else {
throw rclpy::append_rcl_error("Failed to call rcl_logging_rosout_add_sublogger");
fujitatomoya marked this conversation as resolved.
Show resolved Hide resolved
}
}

Expand All @@ -225,7 +229,7 @@ rclpy_logging_rosout_remove_sublogger(const char * logger_name, const char * sub
rcl_ret_t rcl_ret = rcl_logging_rosout_remove_sublogger(logger_name, sublogger_name);

if (RCL_RET_OK != rcl_ret) {
rcutils_reset_error();
rcl_reset_error();
}
}

Expand Down
7 changes: 0 additions & 7 deletions rclpy/test/test_rosout_subscription.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,13 +114,6 @@ def call_logger(logger):
self.executor.spin_until_future_complete(self.fut, 3)
self.assertTrue(self.fut.done())

def test_node_logger_not_exist(self):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we want to add a test to ensure that the situation from #1195 doesn't occur again. The following code (which needs to be adapted into a test) reproduced for me:

import rclpy
from rclpy.action import ActionServer

from example_interfaces.action import Fibonacci

class MyClass:
    def __init__(self, node):
        self.action_server = ActionServer(
            node,
            Fibonacci,
            "/fibonacci_action_1",
            self.execute_callback,
        )

    def execute_callback(self):
        print("Execute callback")

rclpy.init()
node = rclpy.create_node("mynode", enable_rosout=False)
x = MyClass(node)
node.destroy_node()
rclpy.try_shutdown()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.
I used the similar logic to test the case. Updated in fc84c5e.

node = rclpy.create_node('test_extra_node', context=self.context)
logger = node.get_logger()
node = None
with self.assertRaises(RuntimeError):
logger.get_child('child')
fujitatomoya marked this conversation as resolved.
Show resolved Hide resolved


if __name__ == '__main__':
unittest.main()