Repository navigation
Fix state freshness and make snapshots atomic - #8
Merged
Merged
Conversation
Reviewer's GuideThe PR fixes stale-health detection by using steady-clock arrival times with per-state policies, makes snapshots atomic across value and metadata, clears all cached state on disconnect, and adds targeted tests for freshness, consistency, and session boundaries. Sequence diagram for fresh state snapshotssequenceDiagram
participant CAN as CAN read thread
participant State as TimedFreqState
participant Reader as Consumer
CAN->>State: set(newValue, timestamp, caller_id)
Note over State: Records steady_clock arrival time
Reader->>State: snapshot()
State-->>Reader: value, timestamp, hz, is_valid, is_healthy
Sequence diagram for disconnect state resetsequenceDiagram
participant Client
participant Interface as PiperInterfaceV2
participant Reader as CAN read thread
participant States as Cached states
Client->>Interface: disconnectPort()
Interface->>Reader: stop and join
Interface->>States: reset()
States-->>Interface: Cached feedback cleared
Interface-->>Client: Disconnected
Client->>Interface: connectPort()
Interface-->>Client: New session starts with invalid cached state
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configuration
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 |
![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 6 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="134" />
<code_context>
}
arm_can_->close();
connected_.store(false, std::memory_order_relaxed);
+ resetStates();
+}
+
</code_context>
<issue_to_address>
**New feedback gets erased**
When `connectPort()` runs after `disconnectPort()` stores `connected_` false but before its `resetStates()` call completes, `connectPort()` starts a new reader while `disconnectPort()` is still resetting state, so `resetStates()` erases feedback received from the new session and callers see invalid or incomplete state.
Synchronize disconnect and reconnect so the old session's reset completes before the new reader can start.
Also at `piper_cpp/src/interface/piper_interface_v2.cpp:137`.
</issue_to_address>
### Comment 2
<location path="piper_cpp/src/interface/piper_interface_v2.cpp" line_range="157" />
<code_context>
+ arm_gripper_ctrl_.reset();
+ arm_motion_ctrl_code_151_.reset();
+ arm_instruction_response_.reset();
+ arm_health_.reset();
}
</code_context>
<issue_to_address>
**Previous firmware data persists**
When firmware bytes were cached before disconnect and the interface reconnects or firmware data is read after disconnect, `resetStates()` leaves `firmware_data_` intact, so `getFirmwareData()` exposes old bytes and later responses append to them; `getFirmwareVersion()` can select the old first `S-V` marker and return the previous session's version.
Clear `firmware_data_` in `resetStates()` so firmware data and version queries cannot use bytes from a closed session.
</issue_to_address>
### Comment 3
<location path="piper_cpp/src/interface/piper_interface_v2.cpp" line_range="45" />
<code_context>
+ // Query responses and command echoes arrive once, not periodically, so their age says
+ // nothing about the link; only the periodic feedback streams keep the default timeout.
+ arm_current_end_vel_acc_.setHealthTimeout(kNoHealthTimeout);
+ arm_crash_protection_rating_fb_.setHealthTimeout(kNoHealthTimeout);
+ arm_gripper_teaching_pendant_fb_.setHealthTimeout(kNoHealthTimeout);
</code_context>
<issue_to_address>
**Stale feedback stays healthy**
When periodic end-velocity/acceleration feedback is enabled and its updates stop, `arm_current_end_vel_acc_` has `kNoHealthTimeout`, so its `is_healthy` flag remains true after updates stop and callers cannot detect stale feedback.
Keep a finite health timeout for `arm_current_end_vel_acc_` instead of setting `kNoHealthTimeout`.
</issue_to_address>
### Comment 4
<location path="piper_cpp/include/piper_cpp/interface/timed_freq_state.h" line_range="157" />
<code_context>
+#include <vector>
namespace piper_cpp
{
+template <typename T> struct StateSnapshot;
</code_context>
<issue_to_address>
**Header can fail to compile**
When the header is compiled with a standard library that does not transitively declare `std::move` from those includes, `StateSnapshot` calls `std::move` without including `<utility>`. A standard-library implementation that does not expose `std::move` through the other included headers fails to compile this header and its consumers.
Include `<utility>` directly in `timed_freq_state.h`.
</issue_to_address>
### Comment 5
<location path="piper_cpp/test/timed_freq_state_test.cpp" line_range="84-89" />
<code_context>
+ for (int i = 0; i < 20000; ++i)
+ {
+ const StateSnapshot<Reading> snap(state);
+ if (snap.is_valid && static_cast<double>(snap.value.n) != snap.timestamp)
+ ++mismatches;
+ }
+ stop.store(true);
+ writer.join();
+ EXPECT_EQ(mismatches, 0);
+}
+
</code_context>
<issue_to_address>
**Invalid snapshots pass consistency check**
When `snapshot()` returns an invalid/default snapshot on every call, `SnapshotPairsValueWithItsOwnMetadata` skips every comparison at line 84, and the zero-mismatch assertion at line 89 passes even though no snapshot exposes the updated state.
Assert that at least one snapshot is valid before accepting the consistency check.
</issue_to_address>
### Comment 6
<location path="piper_cpp/test/timed_freq_state_test.cpp" line_range="30" />
<code_context>
+ TimedFreqState<Reading> state;
+ state.setHealthTimeout(50ms);
+ state.set(Reading{1}, 0.0);
+ EXPECT_TRUE(state.isHealthy());
+
+ std::this_thread::sleep_for(80ms);
</code_context>
<issue_to_address>
**Timeout tests can fail after a pause**
When the test thread is descheduled for more than 50 ms between an update and either healthy assertion, `HealthExpiresAfterTheTimeout` fails its healthy assertion at line 30, or `MultipartHealthNeedsEveryPartFresh` fails at line 63, if the test thread is descheduled for over 50 ms after an update; the correctly implemented timeout then expires before the assertion runs.
Use a controllable clock for timeout tests so advancing time does not depend on thread scheduling.
Also at `piper_cpp/test/timed_freq_state_test.cpp:63`.
</issue_to_address>Sourcery assessment
Approval pending. 6 findings to address first.
Blocking findings: piper_cpp/src/interface/piper_interface_v2.cpp:134, piper_cpp/src/interface/piper_interface_v2.cpp:157, piper_cpp/src/interface/piper_interface_v2.cpp:45, piper_cpp/include/piper_cpp/interface/timed_freq_state.h:157, piper_cpp/test/timed_freq_state_test.cpp:89, and 1 more
There was a problem hiding this comment.
🟡 Changes recommended
The firmware response cache survives disconnect and can expose an old version after reconnecting.
1 open finding
What changed in this PR
Improves CAN feedback freshness, snapshot consistency, and session isolation in the C++ interface.
Changes:
- Adds per-state health timeouts and atomic snapshots.
- Resets cached states on disconnect and documents the behavior.
- Adds six tests covering freshness, snapshots, and resets.
| File | Description |
|---|---|
| piper_cpp/test/timed_freq_state_test.cpp | Tests freshness, atomic snapshots, and reset behavior. |
| piper_cpp/test/piper_interface_v2_test.cpp | Tests cache clearing on disconnect. |
| piper_cpp/test/CMakeLists.txt | Registers the new state tests. |
| piper_cpp/src/interface/piper_interface_v2.cpp | Configures timeouts and resets session state. |
| piper_cpp/include/piper_cpp/interface/timed_freq_state.h | Implements freshness tracking, atomic snapshots, and reset. |
| piper_cpp/include/piper_cpp/interface/piper_interface_v2.h | Documents state semantics and declares reset helper. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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.
for ~1000 s. It now uses steady_clock time points, with a per-state timeout: 1 s for the
periodic feedback streams, none for query responses and command echoes.
locks; snapshot() now takes them under one.
Six new tests.
Summary by Sourcery
Ensure interface state freshness, snapshot consistency, and session isolation are handled correctly.
Bug Fixes:
Enhancements:
Documentation:
Tests: