Conversation
Signed-off-by: Michael Carroll <michael@openrobotics.org>
Signed-off-by: Michael Carroll <michael@openrobotics.org>
| sm_.HeartbeatTimeout(); | ||
| if (sm_.getState().getId() != SM::WaitingForSister.getId()) { | ||
| sm_.HeartbeatTimeout(); | ||
| } |
There was a problem hiding this comment.
@mjcarroll why the change? why the difference with bondpy?
There was a problem hiding this comment.
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
This was causing a segfault in the remoteNeverConnects test:
bond_core/test_bond/test/test_callbacks_cpp.cpp
Lines 98 to 107 in fb0fa11
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.
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.
@mjcarroll bump (this is in the list of things to triage for Galactic)
There was a problem hiding this comment.
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 <michael@openrobotics.org>
|
Going to backport some of the fixes from #87 in place of this. |
Fix unit tests for CI.
Signed-off-by: Michael Carroll michael@openrobotics.org