Skip to content

fix: reset header stamps to zero in reset methods to prevent timeout#2197

Open
Ishan1923 wants to merge 4 commits intoros-controls:masterfrom
Ishan1923:fix/1774-reset-header-stamps
Open

fix: reset header stamps to zero in reset methods to prevent timeout#2197
Ishan1923 wants to merge 4 commits intoros-controls:masterfrom
Ishan1923:fix/1774-reset-header-stamps

Conversation

@Ishan1923
Copy link
Copy Markdown
Contributor

@Ishan1923 Ishan1923 commented Mar 4, 2026

Problem: When controllers reset their internal reference messages, the initialization functions assigned the current time (now()) to the new empty message's timestamp. This caused the message 'age' calculation to yield 0 seconds, which allowed the empty message to bypass safety timeout checks.

Solution:

  • Modified the reset functions across the affected controllers to set the timestamp to rclcpp::Time(0) instead of the current time. This explicitly marks the reset message as an old/invalid command, ensuring the safety timeout logic halts the robot.
  • Applied (void)node; cast to silence -Werror=unused-parameter compiler warnings without altering existing function signatures. I was unsure to remove this const std::shared_ptr<rclcpp_lifecycle::LifecycleNode> & node as I might break ABI, but I guess this might not, as it completely separate.
  • Updated EXPECT_EQ to EXPECT_GT in t_est_mecanum_drive_controller.cpp_ to correctly test the timestamp update against the new 0 baseline. The old EXPECT_EQ tested against an incorrect now() baseline; EXPECT_GT correctly ensures the timestamp advanced from 0.

Affected Controllers:

  • admittance_controller
  • diff_drive_controller
  • gpio_controllers
  • mecanum_drive_controller
  • omni_wheel_drive_controller
  • steering_controllers_library

fix #1774

…bypass

Signed-off-by: Ishan1923 <ecdev4ishan@gmail.com>
@Ishan1923 Ishan1923 changed the title fix: reset header stamps to zero in reset methods to prevent timeout … fix #1774: reset header stamps to zero in reset methods to prevent timeout Mar 4, 2026
@Ishan1923 Ishan1923 changed the title fix #1774: reset header stamps to zero in reset methods to prevent timeout fix: reset header stamps to zero in reset methods to prevent timeout #1774 Mar 4, 2026
@Ishan1923 Ishan1923 changed the title fix: reset header stamps to zero in reset methods to prevent timeout #1774 fix: reset header stamps to zero in reset methods to prevent timeout Mar 4, 2026
@codecov
Copy link
Copy Markdown

codecov bot commented Mar 4, 2026

Codecov Report

❌ Patch coverage is 96.15385% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 84.77%. Comparing base (3e66d39) to head (28d2417).

Files with missing lines Patch % Lines
gpio_controllers/src/gpio_command_controller.cpp 75.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2197   +/-   ##
=======================================
  Coverage   84.77%   84.77%           
=======================================
  Files         153      153           
  Lines       15236    15241    +5     
  Branches     1322     1324    +2     
=======================================
+ Hits        12916    12921    +5     
  Misses       1838     1838           
  Partials      482      482           
Flag Coverage Δ
unittests 84.77% <96.15%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...dmittance_controller/src/admittance_controller.cpp 73.61% <100.00%> (ø)
...iff_drive_controller/src/diff_drive_controller.cpp 81.21% <100.00%> (ø)
..._drive_controller/src/mecanum_drive_controller.cpp 85.81% <100.00%> (+0.04%) ⬆️
..._controller/test/test_mecanum_drive_controller.cpp 99.34% <ø> (ø)
...ive_controller/src/omni_wheel_drive_controller.cpp 83.01% <100.00%> (ø)
...llers_library/src/steering_controllers_library.cpp 69.53% <100.00%> (+0.44%) ⬆️
gpio_controllers/src/gpio_command_controller.cpp 83.51% <75.00%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

const std::shared_ptr<rclcpp_lifecycle::LifecycleNode> & node)
{
msg.header.stamp = node->now();
(void)node;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why don't we use [[maybe_unused]]? It was made for this exact scenario.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes using [[maybe_unused]] sounds much more better, I will use this then

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.

if we don't need the node object, why not change the function signature instead?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I thought about that, but I wanted to avoid breaking API/ABI compatibility for downstream users. If breaking the ABI is acceptable for this target branch, just let me know and I will gladly change the signature.

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.

On rolling we can break API. For the backports, we should add the new signatures but keep the old ones with a deprecation warning.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Got it, thanks for clarifying;

I'll update rolling with the direct signature change, and for the backport PRs I'll keep the old signatures with a [[deprecated]] warning delegating to the new ones.

Will push shortly

@Ishan1923
Copy link
Copy Markdown
Contributor Author

@christophfroehlich The function signatures have been updated for the rolling target.
Apologies for the delay; I was occupied with my mid-semester exams.

  • Removed the unused node parameter and (void)node; casts from the
    reset_controller_reference_msg local helper functions in:
    • admittance_controller
    • gpio_controllers
    • mecanum_drive_controller
    • steering_controllers_library
  • Local colcon test passes for all modified controllers.

@Ishan1923 Ishan1923 requested a review from thedevmystic March 27, 2026 15:30
Signed-off-by: Ishan1923 <ecdev4ishan@gmail.com>
controller_state_publisher_->try_publish(controller_state_msg_);
}

// store current ref (for open loop odometry) and update odometry
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The edits in steering_controllers_library.cpp is not related to the fix, right?

So, why here?

@mergify
Copy link
Copy Markdown
Contributor

mergify bot commented Mar 28, 2026

This pull request is in conflict. Could you fix it @Ishan1923?

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.

Reset header stamps to zero in reset methods

3 participants