-
Notifications
You must be signed in to change notification settings - Fork 116
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
Allow null arguments in the EventsExecutor parameters #602
Conversation
…ion" This reverts commit 8e93400. Signed-off-by: Jose Luis Rivero <jrivero@osrfoundation.org>
Signed-off-by: Jose Luis Rivero <jrivero@osrfoundation.org>
4728b35
to
5fbe71d
Compare
Signed-off-by: Jose Luis Rivero <jrivero@osrfoundation.org>
While reviewing the error logs in the past builds I noticed that probably the problem was only in checking callbacks that can be So I think that we can leave the checks on the other parameters: |
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.
Still looks OK, assuming CI comes back green.
This PR and #600 should be backported to Humble to address the build regressions on the |
* Revert "Add RMW_CHECKS to EventsExecutor implementation" * Check the non callback values for nullptr Signed-off-by: Jose Luis Rivero <jrivero@osrfoundation.org>
* Revert "Add RMW_CHECKS to EventsExecutor implementation" * Check the non callback values for nullptr Signed-off-by: Jose Luis Rivero <jrivero@osrfoundation.org>
* Revert "Add RMW_CHECKS to EventsExecutor implementation" * Check the non callback values for nullptr Signed-off-by: Jose Luis Rivero <jrivero@osrfoundation.org>
After merging #600, failing tests have appeared in rclcpp that points to problems with assertions introduced on parameter there.
I did that following the API that explicit mention when a parameter can be null (i.e.
user_data
). Seems to me like the null check and error handling is being done at a different place of the stack.Testing: