Conversation
|
I was going to add the option to disable the ROS specific prefix in this pull request, but it makes it harder to review in my opinion (I've been applying those changes locally) because it touches a lot of otherwise unrelated stuff like how options are propagated for the publishers and subscribers. So I'd rather merge this set of pr's and open new ones for the new option. I'm putting these in review, here's a first set of CI:
|
|
Another set of CI with some small test related fixes:
The remaining macOS failure is a test timeout. I think that timeout is just too close to a normal runtime. Each Connext test takes like 3-6 seconds to shutdown before then next can run. On my machine it is flaky I am going to up the timeout from 30 to 60 seconds and rerun the macOS CI, but I don't consider that a regression atm. Please review these if you have time. Thanks. |
|
See ros2/system_tests@ba60562 for the updated timeout. |
Karsten1987
left a comment
There was a problem hiding this comment.
I do +1 this, but I believe this expansion could be excluded in a separate function called get_fully_expanded_topic_name or similar as the changes in publisher, subscriber, .. are all pretty much the same given a topic/service name and a node namespace
| // Expand the given service name. | ||
| rcutils_allocator_t rcutils_allocator = rcutils_get_default_allocator(); | ||
| rcutils_string_map_t substitutions_map = rcutils_get_zero_initialized_string_map(); | ||
| rcutils_ret_t rcutils_ret = rcutils_string_map_init(&substitutions_map, 0, rcutils_allocator); |
There was a problem hiding this comment.
why not using the allocator from above?
There was a problem hiding this comment.
Well, my original reasoning was that the rcutils allocator and rcl allocator might be different in the future, but I think that's not as important as using the given allocator as much as possible. I'll change this (and look for other places where I made this choice).
|
Updated macOS CI is green (with updated timeout): |
That's true, but they are repeated because they return different errors based on some things (especially between pub/sub and client/service). In the interest of moving this forward I'll ticket this and do it after we merge this. |
|
CI after addressing comments and getting approval from @Karsten1987:
|
Using ament_target_dependencies where possible
This pr is part of the final set of changes needed to support namespaces. It actually change the
rcl_create_*functions which take a topic or service name to expand those names and validate them before passing the fully qualified version down tormwfunctions.I still need to expose this stuff into
rclcppandrclpy, as well as implement the "avoid ros prefix option".