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.

Sign up for GitHub

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

Jump to bottom

Bond cleanups #87

Merged
merged 11 commits into from
Jun 13, 2022
Merged

Bond cleanups #87

merged 11 commits into from
Jun 13, 2022

Conversation

mjcarroll
Copy link
Member

Replaces #76

Split out into multiple commits for easier reviews:

  • Remove a condition variable that wasn't used c66eaf3
  • Use a type alias rather than std::function f6471a5
  • Use a delegating constructor and default initialization to reduce some boilerplate 14986f9
  • Improve locking by separating the state machine locks from the pending callbacks lock, as well as reducing the scope of locks wherever possible 5c7b65c
  • Other test improvements from Fix bond tests #76

@mjcarroll
Remove unused condition_variable
c66eaf3 
Signed-off-by: Michael Carroll <michael@openrobotics.org>
@mjcarroll
Change std::function<void(void)> to a type alias
f6471a5 
Signed-off-by: Michael Carroll <michael@openrobotics.org>
@mjcarroll
Use delegating constructor for common functions
14986f9 
Signed-off-by: Michael Carroll <michael@openrobotics.org>
@mjcarroll
Improve locking
5c7b65c 
Signed-off-by: Michael Carroll <michael@openrobotics.org>
@mjcarroll
Fix tests
5410d3f 
Signed-off-by: Michael Carroll <michael@openrobotics.org>
@mjcarroll mjcarroll self-assigned this Jun 2, 2022
@mjcarroll
Start heartbeat when connection established
e80cd77 
Signed-off-by: Michael Carroll <michael@openrobotics.org>
clalancette
clalancette requested changes Jun 2, 2022
View reviewed changes
Copy link

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

I've done a particular review and left some things to consider inline. I'll do a more thorough review tomorrow.

Besides those things, I think we can also make some of the getters (like getTopic) const methods, as well as the isBroken method.

bondcpp/src/bond.cpp Outdated Show resolved Hide resolved
bondcpp/src/bond.cpp Outdated Show resolved Hide resolved
bondcpp/include/bondcpp/bond.hpp Show resolved Hide resolved
bondcpp/src/bond.cpp Show resolved Hide resolved
bondcpp/src/bond.cpp Outdated Show resolved Hide resolved
bondcpp/include/bondcpp/bond.hpp Show resolved Hide resolved
bondcpp/src/bond.cpp Show resolved Hide resolved
bondcpp/src/bond.cpp Outdated Show resolved Hide resolved
@mjcarroll
Address reviewer feedback
a0563f9 
Signed-off-by: Michael Carroll <michael@openrobotics.org>
@mjcarroll
Restore publishing when bond dead.
ff2f427 
Also switch to rclcpp::Duration internally

Signed-off-by: Michael Carroll <michael@openrobotics.org>
clalancette
clalancette requested changes Jun 10, 2022
View reviewed changes
Copy link

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

I've left a few more comments. This is definitely getting closer; this feels a lot more robust now.

bondcpp/src/bond.cpp Outdated Show resolved Hide resolved
bondcpp/include/bondcpp/bond.hpp Outdated Show resolved Hide resolved
bondcpp/include/bondcpp/bond.hpp Show resolved Hide resolved
bondcpp/src/bond.cpp Show resolved Hide resolved
bondcpp/src/bond.cpp Show resolved Hide resolved
bondcpp/src/bond.cpp Show resolved Hide resolved
bondcpp/src/bond.cpp Show resolved Hide resolved
bondcpp/src/bond.cpp Outdated Show resolved Hide resolved
bondcpp/src/bond.cpp Outdated Show resolved Hide resolved
bondcpp/src/bond.cpp Outdated Show resolved Hide resolved
@mjcarroll
More const methods
679fdec 
Signed-off-by: Michael Carroll <michael@openrobotics.org>
@mjcarroll
Address reviewer feedback
29bc06e 
Signed-off-by: Michael Carroll <michael@openrobotics.org>
@mjcarroll mjcarroll mentioned this pull request Jun 13, 2022
Merged
clalancette
clalancette requested changes Jun 13, 2022
View reviewed changes
Copy link

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

One more fix (for what I think is a bug), and I think this is good to go.

bondcpp/src/bond.cpp Outdated Show resolved Hide resolved
@mjcarroll
Less deadlocking
9fcdd1f 
Signed-off-by: Michael Carroll <michael@openrobotics.org>
clalancette
clalancette approved these changes Jun 13, 2022
View reviewed changes
Copy link

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

Thanks for all of the iteration here. I think this leaves the code in much better shape.

@mjcarroll mjcarroll merged commit ce5346d into ros2 Jun 13, 2022
@mjcarroll mjcarroll deleted the bond_cleanups branch June 13, 2022 16:03
This was referenced Jun 13, 2022
Closed
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@clalancette clalancette clalancette approved these changes

Assignees

@mjcarroll mjcarroll

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@mjcarroll @clalancette