-
Notifications
You must be signed in to change notification settings - Fork 158
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
use new type identification for service calls #242
Conversation
Signed-off-by: Karsten Knese <karsten@openrobotics.org>
Signed-off-by: Karsten Knese <karsten@openrobotics.org>
Why not just (without error checking)?
|
I don't have enough insights about this to answer whether or not the error checking makes sense or not. I'd naively say yes (error checking makes sense) so that we catch easier when this behavior changes in the future, but I am open for whatever is best practice here. |
I think more generally having what @dirk-thomas suggests makes sense, since we haven't settle on how many parts to the message namespace there can be. Probably a better way to error check is to wrap the whole thing in a try-except since the import_module / getattr should tell us if the name is valid, right? |
@dirk-thomas is the proposed way what you envisioned? I am happy to change it to whatever is recommended. |
My snippet was meant as this-is-how-the-logic-should-work and would get rid of all the extra steps currently being used. The snippet doesn't do any error checking - but the actual patch should certainly do that - I just left that out for brevity. |
Signed-off-by: Karsten Knese <karsten@openrobotics.org>
@jacobperron @dirk-thomas does 468289d look like what you've suggested? |
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.
Have you ensure that it continues to work for actions?
Did it work before this patch with actions? I haven't tested it with actions. Do we have a working example for this? |
I assume it worked before. Since you are changing the code dealing with action types in this patch please also check it with an action type. I don't have an example at hand. |
How do we get the type information of the actual action service?
what's the respective type to the service:
I've tried:
but this fails with |
The "send goal" service is defined in the module
|
Great! @jacobperron example does work. |
Similar problem as described here: #241
ros2 service call ...
fails because the new service types are parsed incorrectly.Signed-off-by: Karsten Knese karsten@openrobotics.org