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.

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

[rclc] Implementation of "client with context" should be improved #115

Open
JanStaschulat opened this issue Jul 9, 2021 · 2 comments
Open
Assignees
Labels
enhancement New feature or request

Comments

@JanStaschulat
Copy link
Contributor

JanStaschulat commented Jul 9, 2021

Missing condition to call client callback with context:

@norro. It looks like that the implementation of the client callback with context is not properly called: In case the client would have a context (which is not possible now), it will call the normal callback function and not the callback function with context:

handle->client_callback(handle->data);

You never use a client callback with context?

@BrettRD
Copy link
Contributor

BrettRD commented Jul 9, 2021

I don't think this is a bug

There is no way to add a client with a context pointer, and there is no typedef for a client callback to receive a context pointer.
https://github.com/ros2/rclc/blob/master/rclc/include/rclc/executor_handle.h#L130-L138

I think there is a subtle bug in the conditions to execute the callback (WITH_CONTEXT should throw an error instead of executing the callback), but not one that could cause a crash, and I can't see a way of even running that execution pathway.

This will disappear after #116
Maybe mark it as a feature request?

@JanStaschulat JanStaschulat changed the title Bug: client does not support callback with context Improvement: implementation of "client with context" is not correct Jul 16, 2021
@norro
Copy link
Collaborator

norro commented Jul 20, 2021

Agreed, the WITH_CONTEXT callback shouldn't be silently handled without context. Since I don't know any use case for a client with context, I suggest aborting with a warning/error, if a client callback is called WITH_CONTEXT.

@JanStaschulat JanStaschulat added the enhancement New feature or request label Jul 28, 2021
@JanStaschulat JanStaschulat changed the title Improvement: implementation of "client with context" is not correct [rclc] Implementation of "client with context" should be improved Jul 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants