Repository navigation
Propagate feedback and command errors through the ROS hardware interface - #9
Merged
Merged
Conversation
Reviewer's GuideThe PR hardens the ROS hardware interface by serializing multipart CAN commands, validating complete command sets before transmission, propagating send and feedback-health failures as lifecycle/read/write errors, and adding focused conversion and concurrency tests. Sequence diagram for validated ROS command transmissionsequenceDiagram
participant ControllerManager
participant PiperHardware
participant PiperInterface
participant CAN
ControllerManager->>PiperHardware: write()
PiperHardware->>PiperHardware: jointCommandToMillideg()
PiperHardware->>PiperHardware: gripperCommandToMicrometres()
alt invalid command
PiperHardware-->>ControllerManager: ERROR
else valid command set
PiperHardware->>PiperInterface: controlJoints()
PiperInterface->>CAN: send multipart joint frames
PiperInterface-->>PiperHardware: send result
alt joint send failed
PiperHardware-->>ControllerManager: ERROR
else joint send succeeded
PiperHardware->>PiperInterface: controlGripper()
PiperInterface->>CAN: send gripper frame
PiperInterface-->>PiperHardware: send result
PiperHardware-->>ControllerManager: OK or ERROR
end
end
Sequence diagram for stale feedback stopping active controllerssequenceDiagram
participant ControllerManager
participant PiperHardware
participant PiperInterface
ControllerManager->>PiperHardware: read()
PiperHardware->>PiperInterface: getArmJointStates()
alt active and joint feedback unhealthy
PiperHardware-->>ControllerManager: ERROR
else joint feedback healthy
PiperHardware->>PiperInterface: getGripperStates()
alt active and gripper feedback unhealthy
PiperHardware-->>ControllerManager: ERROR
else feedback healthy
PiperHardware-->>ControllerManager: OK with current state
end
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🟡 Changes recommended
Activation checks, lifecycle fault recovery, and concurrency-test failure cleanup remain incomplete.
3 open findings
What changed in this PR
Makes the ROS hardware interface report stale feedback and command failures, building on #8’s freshness tracking.
Changes:
- Serializes multi-frame joint and Cartesian commands.
- Validates commands and propagates feedback and transmission errors.
- Adds conversion helpers, tests, and documentation.
| File | Description |
|---|---|
| piper_cpp/test/piper_interface_v2_test.cpp | Tests concurrent command serialization. |
| piper_cpp/src/interface/piper_interface_v2.cpp | Locks multi-frame transmissions. |
| piper_cpp/include/piper_cpp/interface/piper_interface_v2.h | Declares the transmission mutex. |
| piper_cpp_ros/test/command_conversion_test.cpp | Tests command validation and conversions. |
| piper_cpp_ros/src/piper_hardware_interface.cpp | Adds validation, freshness checks, and error propagation. |
| piper_cpp_ros/README.md | Documents hardware error behavior. |
| piper_cpp_ros/package.xml | Adds the GoogleTest dependency. |
| piper_cpp_ros/include/piper_cpp_ros/command_conversion.hpp | Centralizes validated unit conversions. |
| piper_cpp_ros/CMakeLists.txt | Registers conversion tests. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 5 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="piper_cpp/src/interface/piper_interface_v2.cpp" line_range="671" />
<code_context>
joint_1_millideg, joint_2_millideg, joint_3_millideg,
joint_4_millideg, joint_5_millideg, joint_6_millideg,
};
+ std::lock_guard<std::mutex> lock(multipart_tx_mutex_);
msg.type = ArmMsgType::JointCtrl12;
if (!sendPiperMsg(msg))
</code_context>
<issue_to_address>
**Failed writes leave partial targets**
When a CAN send fails after one or more target frames have been delivered, `controlJoints()` or the gripper send can fail after earlier target frames have reached the arm, and `write()` then returns `ERROR` without undoing them. The arm is left with a partial or mixed target instead of the previous target.
Make failed writes recover from already-delivered target frames so the arm is not left with a partial or mixed target.
Also at `piper_cpp_ros/src/piper_hardware_interface.cpp:485-496`, `piper_cpp_ros/README.md:248`.
</issue_to_address>
### Comment 2
<location path="piper_cpp_ros/src/piper_hardware_interface.cpp" line_range="228" />
<code_context>
}
- piper_->sendMotionCtrl2(
- piper_cpp::CtrlMode::CanCmd, piper_cpp::MoveMode::MoveJ, static_cast<uint8_t>(speed_pct_),
- piper_cpp::MitMode::PositionVelocity
- );
</code_context>
<issue_to_address>
**Enable send failure goes unchecked**
When the enable CAN send fails while the robot is already reported enabled, `on_activate()` ignores the result of `enableRobot()` and can proceed when the robot is already reported enabled, even though the enable command failed to send. The later checked motion-mode send does not detect that failure, so activation can succeed without confirming this command.
Check `enableRobot()` and return `CallbackReturn::ERROR` if it fails.
</issue_to_address>
### Comment 3
<location path="piper_cpp_ros/src/piper_hardware_interface.cpp" line_range="247" />
<code_context>
while (steady_clock::now() - zero_t0 < zero_duration)
{
- piper_->controlJoints(0, 0, 0, 0, 0, 0);
+ if (!piper_->controlJoints(0, 0, 0, 0, 0, 0))
+ {
+ RCLCPP_ERROR(rclcpp::get_logger(kLogger), "Failed to send joint targets while driving to zero.");
</code_context>
<issue_to_address>
**Gripper homing failure is ignored**
When `with_gripper` and `home_gripper_on_activate` are enabled and the CAN send for the zero command fails, `on_activate()` discards the false return from `setGripperZero()`, then continues and can report successful activation with the gripper still using its previous zero reference.
Check the return value of `setGripperZero()` and return `CallbackReturn::ERROR` when it fails.
</issue_to_address>
### Comment 4
<location path="piper_cpp/test/piper_interface_v2_test.cpp" line_range="229" />
<code_context>
+ while (frames.size() < frames.capacity())
+ {
+ pollfd pfd{bus.peer, POLLIN, 0};
+ ASSERT_GT(poll(&pfd, 1, 2000), 0) << "bus went quiet after " << frames.size() << " frames";
+ can_frame f{};
+ ASSERT_EQ(::recv(bus.peer, &f, sizeof(f), 0), static_cast<ssize_t>(sizeof(f)));
</code_context>
<issue_to_address>
**Receive failure aborts the test process**
When the receive poll times out or a socket receive fails, a fatal assertion returns from the test before threads `a` and `b` are joined; destroying either still-joinable `std::thread` calls `std::terminate`, aborting the test process instead of reporting a normal test failure.
Ensure both threads are joined even when a receive assertion fails.
</issue_to_address>
### Comment 5
<location path="piper_cpp/test/piper_interface_v2_test.cpp" line_range="225-226" />
<code_context>
+ std::thread b(sender, 2000);
+
+ std::vector<can_frame> frames;
+ frames.reserve(2 * kCallsPerThread * 3);
+ while (frames.size() < frames.capacity())
+ {
+ pollfd pfd{bus.peer, POLLIN, 0};
</code_context>
<issue_to_address>
**Reserved capacity causes false timeouts**
When `frames.capacity()` is greater than the number of frames the senders produce, the loop waits for `frames.capacity()` frames rather than the expected frame count, so `poll()` times out after all sent frames arrive and the test fails on a valid vector implementation.
Use the expected number of frames as the loop limit instead of `frames.capacity()`.
</issue_to_address>Sourcery assessment
Approval pending. 5 findings to address first.
Blocking findings: piper_cpp/src/interface/piper_interface_v2.cpp:671, piper_cpp_ros/src/piper_hardware_interface.cpp:228, piper_cpp_ros/src/piper_hardware_interface.cpp:247, piper_cpp/test/piper_interface_v2_test.cpp:229, piper_cpp/test/piper_interface_v2_test.cpp:226
justagist
added a commit
that referenced
this pull request
Oct 15, 2026
…ace (#9 #10) * serialise multipart commands so concurrent callers cannot interleave frames * validate commands and propagate send failures from the hardware interface * report stale feedback as an error while active * join the senders before asserting in the interleave test * check the enable and gripper zero sends during activation * release the CAN session on error so the component can be reconfigured
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
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.
Stacked on #8 (needs the fixed is_healthy).
callers cannot interleave into a target the arm never received from either.
metre of jaw) and returns ERROR when a CAN send fails. on_activate() checks its sends too.
reporting the cached pose as current. The controller manager then stops the controllers;
the arm holds its last target, as on deactivation.
Conversions live in piper_cpp_ros/command_conversion.hpp with an ament_cmake_gtest target.
Three new tests.
Summary by Sourcery
Harden the ROS hardware interface by validating and propagating command and feedback failures while preserving coherent arm targets.
Bug Fixes:
Enhancements:
Build:
Documentation:
Tests: