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

Fix bond tests #76

Closed
wants to merge 2 commits into from
Closed

Fix bond tests #76

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a007a77
Fix bond tests
mjcarroll Mar 26, 2021
1f6d6a6
Fix uncrustify
mjcarroll Mar 29, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion bondcpp/src/bond.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -473,7 +473,9 @@ void Bond::onHeartbeatTimeout()
{
{
std::unique_lock<std::mutex> lock(mutex_);
sm_.HeartbeatTimeout();
if (sm_.getState().getId() != SM::WaitingForSister.getId()) {
sm_.HeartbeatTimeout();
}
Copy link

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?

Copy link
Member Author

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.

Copy link

@hidmic hidmic Mar 31, 2021 •
edited
Loading

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).

Copy link
Member

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)

Copy link

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.

}
flushPendingCallbacks();
}
Expand Down
30 changes: 28 additions & 2 deletions test_bond/test/test_callbacks_cpp.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,12 +87,26 @@ TEST_F(TestCallbacksCpp, dieInLifeCallback)
bond::Bond a(TOPIC, id1, nh1);
bond::Bond b(TOPIC, id1, nh1);

a.setFormedCallback(std::bind(&bond::Bond::breakBond, &a));
a.setFormedCallback(
[&a]() {
a.breakBond();
});
a.start();
b.start();

std::atomic<bool> isRunning {true};
auto runThread = std::thread(
[&isRunning, &nh1]() {
while (isRunning) {
rclcpp::spin_some(nh1);
}
});

EXPECT_TRUE(a.waitUntilFormed(rclcpp::Duration(5.0s)));
EXPECT_TRUE(b.waitUntilBroken(rclcpp::Duration(3.0s)));

isRunning = false;
runThread.join();
}

TEST_F(TestCallbacksCpp, remoteNeverConnects)
Expand All @@ -102,6 +116,18 @@ TEST_F(TestCallbacksCpp, remoteNeverConnects)
bond::Bond a1(TOPIC, id2, nh2);

a1.start();
EXPECT_FALSE(a1.waitUntilFormed(rclcpp::Duration(5.0s)));

std::atomic<bool> isRunning {true};
auto runThread = std::thread(
[&isRunning, &nh2]() {
while (isRunning) {
rclcpp::spin_some(nh2);
}
});

EXPECT_FALSE(a1.waitUntilFormed(rclcpp::Duration(4.0s)));
EXPECT_TRUE(a1.waitUntilBroken(rclcpp::Duration(10.0s)));

isRunning = false;
runThread.join();
}