Skip to content

Always trigger guard condition waitset#1923

Merged
alsora merged 5 commits intomasterfrom
asoragna/rclcpp-1917
Jun 2, 2022
Merged

Always trigger guard condition waitset#1923
alsora merged 5 commits intomasterfrom
asoragna/rclcpp-1917

Conversation

@alsora
Copy link
Copy Markdown
Collaborator

@alsora alsora commented Apr 29, 2022

Ttrigger guard condition waitset regardless of whether a trigger callback is present
This should address #1917

Also minor cleanup at the class (remove unneded lock and better use of if-else)

Signed-off-by: Alberto Soragna alberto.soragna@gmail.com

@alsora
trigger guard condition waitset regardless of whether a trigger callb…
a83d72b 
…ack is present

Signed-off-by: Alberto Soragna <alberto.soragna@gmail.com>
fujitatomoya
fujitatomoya reviewed Apr 29, 2022
View reviewed changes
@alsora
restore mutex in guard_condition.cpp
ebf0476 
Signed-off-by: Alberto Soragna <alberto.soragna@gmail.com>
@alsora
Copy link
Copy Markdown
Collaborator Author

alsora commented May 13, 2022

I restored the mutex to prevent race condition that could cause problems.

fujitatomoya
fujitatomoya approved these changes May 13, 2022
View reviewed changes
Copy link
Copy Markdown
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm with minor nit

@alsora
remove whitespace
2ac60a4 
Signed-off-by: Alberto Soragna <alberto.soragna@gmail.com>
wjwwood
wjwwood reviewed May 13, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks reasonable.

It seems like a test to avoid this regressing in the future would be good to have.

Also, if it's not already clear from the docstring what should happen in this corner case, it should be clarified there.

@alsora alsora force-pushed the asoragna/rclcpp-1917 branch from caa9f19 to 863fa04 Compare May 17, 2022 10:32
@alsora
add unit-test
20ce75e 
Signed-off-by: Alberto Soragna <alberto.soragna@gmail.com>
@alsora alsora force-pushed the asoragna/rclcpp-1917 branch from 863fa04 to 20ce75e Compare May 17, 2022 10:46
@alsora
Copy link
Copy Markdown
Collaborator Author

alsora commented May 17, 2022

Added unit-tests.

Running full CI

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@wjwwood
Copy link
Copy Markdown
Member

wjwwood commented May 17, 2022

This doc block should mention how it interacts with the "on trigger callback":

rclcpp/rclcpp/include/rclcpp/guard_condition.hpp

Lines 75 to 85 in 02802bc

/// Notify the wait set waiting on this condition, if any, that the condition had been met.
/**
* This function is thread-safe, and may be called concurrently with waiting
* on this guard condition in a wait set.
*
* \throws rclcpp::exceptions::RCLError based exceptions when underlying
* rcl functions fail.
*/
RCLCPP_PUBLIC
void
trigger();

And, I guess we missed it, but we should have some documentation on the "on trigger callback" method as well (it currently has none):

rclcpp/rclcpp/include/rclcpp/guard_condition.hpp

Lines 110 to 112 in 02802bc

RCLCPP_PUBLIC
void
set_on_trigger_callback(std::function<void(size_t)> callback);

@alsora
add documentation for trigger and set_on_trigger_callback
25c4ba7 
Signed-off-by: Alberto Soragna <alberto.soragna@gmail.com>
@alsora
Copy link
Copy Markdown
Collaborator Author

alsora commented Jun 1, 2022

@wjwwood @fujitatomoya anything else to do here?

@fujitatomoya
Copy link
Copy Markdown
Collaborator

fujitatomoya commented Jun 1, 2022

@alsora i am good to go! @wjwwood could you check your comments on #1923 (comment)?

wjwwood
wjwwood approved these changes Jun 1, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Sorry for not re-reviewing it. I don't get notifications for pushed commits, so a comment is helpful to bump a re-review.

@alsora alsora merged commit 790e529 into master Jun 2, 2022
@delete-merged-branch delete-merged-branch bot deleted the asoragna/rclcpp-1917 branch June 2, 2022 17:58
apojomovsky pushed a commit to apojomovsky/rclcpp that referenced this pull request May 29, 2024
@alsora @apojomovsky
Always trigger guard condition waitset (ros2#1923)
03163bf 
* trigger guard condition waitset regardless of whether a trigger callback is present

Signed-off-by: Alberto Soragna <alberto.soragna@gmail.com>

* restore mutex in guard_condition.cpp

Signed-off-by: Alberto Soragna <alberto.soragna@gmail.com>

* remove whitespace

Signed-off-by: Alberto Soragna <alberto.soragna@gmail.com>

* add unit-test

Signed-off-by: Alberto Soragna <alberto.soragna@gmail.com>

* add documentation for trigger and set_on_trigger_callback

Signed-off-by: Alberto Soragna <alberto.soragna@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wjwwood wjwwood wjwwood approved these changes

@fujitatomoya fujitatomoya fujitatomoya approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@alsora @wjwwood @fujitatomoya