Skip to content

Reuse bond heartbeat watchdog timer - #116

Open
mini-1235 wants to merge 2 commits into
ros:ros2from
mini-1235:mini-ros2-local-fixes
Open

Reuse bond heartbeat watchdog timer#116
mini-1235 wants to merge 2 commits into
ros:ros2from
mini-1235:mini-ros2-local-fixes

Conversation

@mini-1235

Copy link
Copy Markdown
Contributor

This came up while I was doing some lifecycle manager performance work in Nav2. In summary, this PR includes the following changes:

Bond::heartbeatTimerReset() now reuses the existing heartbeat watchdog timer instead of destroying and recreating it on every heartbeat. The first heartbeat still creates the timer, while subsequent heartbeats call TimerBase::reset() to renew the deadline. This preserves the intended watchdog behavior without changing the heartbeat frequency or timeout duration. In my 2-minute performance test, this gives a small improvement of about 2%, but since this is related to heartbeat_timeout_, I think it is still worth optimizing.

This PR also narrows the included rclcpp headers to help reduce build time, and updates the minimum CMake version.

Assisted by Codex 5.6 Sol

@mini-1235

Copy link
Copy Markdown
Contributor Author

cc @SteveMacenski

Could you also take a look at this one? It is related to ros-navigation/navigation2#6061, and you are more familiar with this part of the code than I am.

Comment thread bondcpp/src/bond.cpp

void Bond::heartbeatTimerReset()
{
if (heartbeat_timer_) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This seems to work I think. heartbeat_timeout_ can change using setHeartbeatTimeout but since it doesn't change when started_, its OK as this only occurs after its started.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Just adding the comment for a future reviewer that I'm sure will want to track down the same thing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants