-
Notifications
You must be signed in to change notification settings - Fork 64
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
Fix bond tests #76
Fix bond tests #76
Conversation
Signed-off-by: Michael Carroll <[email protected]>
Signed-off-by: Michael Carroll <[email protected]>
sm_.HeartbeatTimeout(); | ||
if (sm_.getState().getId() != SM::WaitingForSister.getId()) { | ||
sm_.HeartbeatTimeout(); | ||
} |
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.
@mjcarroll why the change? why the difference with bondpy
?
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.
The WaitingForSister
state doesn't have an implementation for HeartbeatTimeout
as you can see here:
bond_core/bondcpp/include/bondcpp/BondSM_sm.h
Lines 67 to 79 in 5a79fec
class SM_WaitingForSister : | |
public SM_Default | |
{ | |
public: | |
SM_WaitingForSister(const char *name, int stateId) | |
: SM_Default(name, stateId) | |
{}; | |
void ConnectTimeout(BondSMContext& context); | |
void Die(BondSMContext& context); | |
void SisterAlive(BondSMContext& context); | |
void SisterDead(BondSMContext& context); | |
}; |
This was causing a segfault in the remoteNeverConnects
test:
bond_core/test_bond/test/test_callbacks_cpp.cpp
Lines 98 to 107 in fb0fa11
TEST_F(TestCallbacksCpp, remoteNeverConnects) | |
{ | |
auto nh2 = rclcpp::Node::make_shared("test_callbacks_cpp_2"); | |
std::string id2 = genId(); | |
bond::Bond a1(TOPIC, id2, nh2); | |
a1.start(); | |
EXPECT_FALSE(a1.waitUntilFormed(rclcpp::Duration(5.0s))); | |
EXPECT_TRUE(a1.waitUntilBroken(rclcpp::Duration(10.0s))); | |
} |
I don't believe that this impacts behavior, just guards against an undefined condition that wasn't previously exposed from the unit tests.
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.
I'm not super familiar with bond
's architecture, but if a heartbeat timeout can occur while waiting for sister, should said state handle that condition (even if ignoring it)? By implementing HeartbeatTimeout
I mean. That applies to both bondcpp
and bondpy
(where I don't an if
clause like this one).
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.
@mjcarroll bump (this is in the list of things to triage for Galactic)
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.
IIUC ros2
bondcpp
has this new method start()
that resets the heartbeat timer, while kinetic-devel
bondcpp
only reset the heartbeat timer the first time someone called Heartbeat. It probably wasn't possible for the WaitingForSister
state to see HeartbeatTimeout
in ROS 1.
Maybe the heartbeat timer should be reset only when the connection is formed? I also agree with @hidmic that WaitingForSister
should implement that callback too, but the implementation should be a warning or error that the callback is unexpected.
@mjcarroll is this still targeting initial Galactic release, or is patch 1 more likely? |
Probably patch 1 at this point. |
* Remove a condition variable that wasn't used * Use a type alias rather than std::function * Use a delegating constructor and default initialization to reduce some boilerplate * Improve locking by separating the state machine locks from the pending callbacks lock, as well as reducing the scope of locks wherever possible * Other test improvements from Fix bond tests #76 Signed-off-by: Michael Carroll <[email protected]>
Going to backport some of the fixes from #87 in place of this. |
Fix unit tests for CI.
Signed-off-by: Michael Carroll [email protected]