Conversation
…ields PhasePositions: fields private, add new() constructor with 120° offsets. KissTelemPacket: struct + all 10 fields private (internal-only). TransferState: 4 internal fields private, servo stays pub. TransferActions: delete dead save_settings/play_tone (set but never read). ServoState: 7 internal fields private, 4 calibration fields stay pub. EdtScheduler: 3 internal fields private, init/deinit flags stay pub. BrushedState: 2 fields private. BrushedOutput: 2 fields pub(crate).
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @kaidokert, you have reached your weekly rate limit of 1500000 diff characters.
Please try again later or upgrade to continue using Sourcery
📝 WalkthroughWalkthroughThese changes restrict field visibility across six motor control modules (brushed, edt, servo, sine, telemetry, transfer), making internal state private or crate-scoped. A new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Review rate limit: 4/5 reviews remaining, refill in 12 minutes. Comment |
❌ 2 Tests Failed:
View the full list of 2 ❄️ flaky test(s)
To view more test analytics, go to the Test Analytics Dashboard |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
rm32/src/sine.rs (1)
213-220: ⚡ Quick winAlign
DefaultwithPhasePositions::new()to avoid split initialization semantics.Now that
new()defines the canonical 0°/120°/240° phase offsets,#[derive(Default)]still yields 0°/0°/0°. MakingDefaultdelegate tonew()avoids accidental zero-offset startup state.Suggested patch
-#[derive(Clone, Default)] +#[derive(Clone)] pub struct PhasePositions { a: i16, b: i16, c: i16, } impl PhasePositions { @@ pub fn new() -> Self { Self { a: 0, b: 120, c: 240, } } @@ } + +impl Default for PhasePositions { + fn default() -> Self { + Self::new() + } +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rm32/src/sine.rs` around lines 213 - 220, The Default implementation for PhasePositions should match the canonical offsets returned by PhasePositions::new(); replace the derived Default with an explicit impl Default for PhasePositions that returns PhasePositions::new() (or have Default call PhasePositions::new()) so constructing with Default yields 0°/120°/240° instead of 0/0/0; update/remove #[derive(Default)] and add impl Default for PhasePositions { fn default() -> Self { Self::new() } } referencing the PhasePositions type and its new() constructor.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@rm32/src/sine.rs`:
- Around line 213-220: The Default implementation for PhasePositions should
match the canonical offsets returned by PhasePositions::new(); replace the
derived Default with an explicit impl Default for PhasePositions that returns
PhasePositions::new() (or have Default call PhasePositions::new()) so
constructing with Default yields 0°/120°/240° instead of 0/0/0; update/remove
#[derive(Default)] and add impl Default for PhasePositions { fn default() ->
Self { Self::new() } } referencing the PhasePositions type and its new()
constructor.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1bf4369a-06e0-44c2-b15a-c2074750f034
📒 Files selected for processing (7)
rm32/src/brushed.rsrm32/src/edt.rsrm32/src/servo.rsrm32/src/sine.rsrm32/src/telemetry.rsrm32/src/transfer.rsrm32_stm32/src/bin/main.rs
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors several structs to restrict field visibility and removes unused fields and logic related to tone playback in the transfer state. However, the removal of the logic handling ServoResult::CalibrationDone appears to be a critical regression that discards calibration data. Additionally, the change to pub(crate) visibility for BrushedOutput fields prevents cross-crate access, and the PhasePositions struct now has an inconsistent Default implementation compared to its new new() constructor. Finally, the KissTelemPacket struct should be removed entirely if it is confirmed to be dead code.
| ServoResult::CalibrationDone { .. } => { | ||
| actions.save_settings = true; | ||
| actions.play_tone = 2; // changed tone | ||
| actions.signal_timeout_reset = true; | ||
| } |
There was a problem hiding this comment.
The removal of the logic to handle ServoResult::CalibrationDone is a regression. While the save_settings field in TransferActions might have been unused by the consumer, the CalibrationDone variant carries the actual calibration results (low_threshold_eeprom, high_threshold_eeprom). By matching with .. and removing the code that triggers a save, the calibration values are lost and will never be persisted to EEPROM. If the intent is to move this logic, it should be done in a way that doesn't discard the data.
| pub(crate) duty: u16, | ||
| pub(crate) direction: BrushedDirection, |
There was a problem hiding this comment.
Changing these fields to pub(crate) makes them inaccessible to other crates. Since brushed_tick is a public function that returns BrushedOutput, callers in other crates (like rm32_stm32) will no longer be able to access the duty and direction results. Per the general rules, visibility must be pub for cross-crate access. If the goal is encapsulation, consider providing public getters instead of pub(crate) fields.
References
- A function's visibility must be pub if it is called from another crate; pub(crate) is insufficient for cross-crate access.
- Do not restrict the visibility of fields to pub(crate) if they are accessed across crate boundaries, as this would prevent proper initialization.
- Visibility of fields should be restricted to pub(crate) if they are only accessed internally within the module, even if they contain critical status information. External access should be provided via accessor methods if needed later.
| @@ -204,12 +204,21 @@ pub fn gimbal_step( | |||
| /// Phase positions for sinusoidal drive (0-359 degrees). | |||
| #[derive(Clone, Default)] | |||
There was a problem hiding this comment.
There is an inconsistency between the derived Default implementation and the new new() constructor. Default::default() will initialize phase positions to (0, 0, 0), whereas new() initializes them to the standard three-phase set (0, 120, 240). It is safer to implement Default manually to ensure the struct is always initialized to a valid state for its intended use case.
impl Default for PhasePositions {
fn default() -> Self {
Self::new()
}
}| #[allow(dead_code)] | ||
| struct KissTelemPacket { |
There was a problem hiding this comment.
The KissTelemPacket struct appears to be dead code as it is not used within the crate and is now private with #[allow(dead_code)]. If it is no longer needed for documentation or future wire-format casting, it should be removed entirely rather than just hidden.
References
- Fields that are strictly internal to a specific logic (e.g., telemetry packet assembly) and not part of the public API should use pub(crate) visibility for encapsulation.
1 participant
Summary
Batch visibility tightening across 7 files:
new()constructorsave_settings/play_tone(set but never read)Test plan
Summary by CodeRabbit