-
Notifications
You must be signed in to change notification settings - Fork 70
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.
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
Add specific return type for non existent node #182
Conversation
Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com>
rmw/include/rmw/ret_types.h
Outdated
@@ -36,6 +36,10 @@ typedef int32_t rmw_ret_t; | |||
/// Incorrect rmw implementation. | |||
#define RMW_RET_INCORRECT_RMW_IMPLEMENTATION 12 | |||
|
|||
// rmw node specific ret codes in 2XX | |||
/// Failed to find node name | |||
#define RMW_RET_NON_EXISTENT_NODE_NAME 203 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why 203 instead of maybe 20?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found the matching define in rcl
so this number makes sense. Maybe mention rcl
in the existing comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it matter if they're matching? There's a convert function anyway.
Seems arbitrary to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I think the name needs to be updated according to ros2/rcl#492 (comment)
- RMW_RET_NON_EXISTENT_NODE_NAME
+ RMW_RET_NODE_NAME_NON_EXISTENT
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it matter if they're matching? There's a convert function anyway.
Seems arbitrary to me.
It's arbitrary yes, but I think it have some sense.
Also, I think the name needs to be updated according to ros2/rcl#492 (comment)
- RMW_RET_NON_EXISTENT_NODE_NAME + RMW_RET_NODE_NAME_NON_EXISTENT
Thanks, forgot to push that commit.
Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com>
@@ -36,6 +36,11 @@ typedef int32_t rmw_ret_t; | |||
/// Incorrect rmw implementation. | |||
#define RMW_RET_INCORRECT_RMW_IMPLEMENTATION 12 | |||
|
|||
// rmw node specific ret codes in 2XX | |||
/// Failed to find node name | |||
// Using same return code than in rcl |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ivanpauno why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer using the same code when possible, I don't think it's important
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Addresses ros2/ros2cli#322 (comment)