Fix halt safety: accumulate errors across all joints - #2468
Open
Functionhx wants to merge 2 commits into
Open
Conversation
…trols#2326) PR ros-controls#2326 introduced two regressions in the halt safety fix: 1. mecanum_drive_controller: Using || (logical OR) with blocking set_value causes short-circuit evaluation -- if the first wheel's set_value returns true (which it will with blocking), the remaining 3 wheels are never halted. Fixed by using &= (bitwise AND assignment) which evaluates all operands without short-circuit, ensuring all 4 wheels receive the halt command. 2. omni_wheel_drive_controller: Changed &= to |= with initial true value. Since true | anything = true, error detection was silently broken. Restored &= semantics which correctly tracks all-succeed. 3. All three files: Changed std::numeric_limits<unsigned int>::max() to -1 per maintainer review feedback. Test Plan: - Reviewed the logic of all three halt() code paths - Verified no remaining std::numeric_limits<unsigned int>::max() in set_value calls - mecanum: all 4 set_value calls execute (no short-circuit), error tracked via &= - omni: loop ensures all calls execute, error tracked via &= - steering: unchanged loop structure, only changed max_tries parameter Signed-off-by: Yuchen Fan <functionhx@gmail.com>
github-actions
Bot
requested review from
Juliaj,
bmagyar,
destogl,
homalozoa and
moriarty
July 10, 2026 10:15
christophfroehlich
requested changes
Jul 10, 2026
christophfroehlich
left a comment
christophfroehlich
left a comment
Member
There was a problem hiding this comment.
This PR has zero diff?
Functionhx
force-pushed
the
fix/halt-safety
branch
from
July 10, 2026 15:23
7bf192f to
ad55b5e
Compare
Author
|
Apologies for the empty commit — the patch failed to apply during push. Fix is up now. |
Revert -1 back to std::numeric_limits<unsigned int>::max() per maintainer feedback: both are functionally identical but the latter is the project's preferred style. Fix steering_controllers_library halt to use accumulate-and-report pattern instead of early-return on first failure, matching mecanum and omni fixes. Signed-off-by: Yuchen Fan <functionhx@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2325.