Make Pid and PidState fields private, extract tick methods by kaidokert · Pull Request #23 · kaidokert/rm32

kaidokert · 2026-04-30T23:53:12Z

Summary

Two encapsulation improvements:

Pid struct — all 10 fields now private (were pub(crate)). New set_gains(kp, ki, kd)
method replaces direct field writes + manual reset. Direct integral = 0 replaced
with existing reset() method.

PidState struct — all 10 fields now private (were pub/pub(crate)). PID tick logic
moved from MainState::tick() into 3 PidState methods:

tick_stall(ci) -> u16 — stall boost for ISR
tick_current_limit(current, target, min, running) -> u16 — duty ceiling
tick_speed_control(e_com, zc, running) -> Option<u16> — throttle override

MainState::tick() PID section shrinks from 30 lines to 15.

Test plan

203 unit tests pass
45 blackbox tests pass, 1 xfail
All 4 MCU cross-builds pass
Clippy clean

Summary by CodeRabbit

Refactor
- Centralized and encapsulated motor control logic for more reliable stall protection, current limiting, and speed control.
- Safer updates to PID configuration to avoid unexpected state carryover during reconfiguration.
- ISR interaction made robust by preventing direct shared-state writes.
Tests
- Added unit tests validating clamp/reset/override behaviors for control loops.

Pid struct fields were pub(crate) but only accessed directly from main_state.rs for gain updates and integral reset. Replace with: - set_gains(kp, ki, kd) — updates gains and resets accumulated state - reset() — already existed, now used instead of direct integral = 0 Also make Commutation::intervals private — only accessed via record_interval() method.

Move stall/current-limit/speed PID logic from MainState::tick() into PidState methods: tick_stall(), tick_current_limit(), tick_speed_control(). All PidState fields are now private — access only through methods. MainState::tick() PID section shrinks from 30 lines to 15, calling 3 methods that each return a value to publish to SharedComm.

sourcery-ai

Sorry @kaidokert, you have reached your weekly rate limit of 1500000 diff characters.

Please try again later or upgrade to continue using Sourcery

coderabbitai · 2026-04-30T23:53:49Z

📝 Walkthrough

Walkthrough

Made PID and commutation internals private, added PidState constructors/setters and tick methods, updated MainState and harness to use the new Pid/PidState API, and added unit tests for PID behaviors; ISR now interacts via tick-returned values and commutation intervals are written via an interface instead of direct field access.

Changes

Cohort / File(s)	Summary
PID & PidState `rm32/src/control/state.rs`, `rm32/src/pid.rs`	Removed `pub(crate)` from internal PID/PidState fields; added `PidState::with_stall_target`, setters (`set_current_gains`, `set_use_current_limit`, `set_use_speed_control`, `set_target_e_com_time`), `Pid::set_gains`, `Pid::clear_integral`, and three tick methods (`tick_stall`, `tick_current_limit`, `tick_speed_control`). Unit tests added for PID behaviors.
Main loop integration `rm32/src/main_state.rs`	Replaced direct PID field mutations and manual accumulator/clamping with calls into PidState setters and tick methods; consume tick-returned duty ceiling/boost/override instead of manual calculations.
Harness CLI `rm32/src/bin/harness.rs`	Changed handling of `use_current_limit` and `use_speed_control_loop` to call `set_use_current_limit(...)` and `set_use_speed_control(...)` on the pid object instead of writing fields directly.
Commutation privacy `rm32/src/commutation.rs`	Made `intervals` private and documented that ISR writes step intervals via `record_interval()` rather than direct field access.

Sequence Diagram(s)

sequenceDiagram
    participant Harness as "Harness (CLI)"
    participant Main as "MainState"
    participant Pid as "PidState"
    participant ISR as "ISR / Commutation"

    rect rgba(150,200,255,0.5)
    Harness->>Main: set flag (use_current_limit / use_speed_control)
    Main->>Pid: set_use_current_limit(v) / set_use_speed_control(v)
    end

    rect rgba(200,255,200,0.5)
    ISR->>Pid: record_interval() / provide samples
    Pid-->>ISR: tick_current_limit() -> duty_ceiling
    Pid-->>ISR: tick_stall() -> boost_duty
    Pid-->>ISR: tick_speed_control() -> Option<override>
    end

    rect rgba(255,230,150,0.5)
    ISR->>Main: publish ISR-facing outputs (ceiling / boost / override)
    Main->>Main: apply outputs to motor control
    end

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

Adopt PidState into MainState, remove 8 loose PID fields #16: Refactors PID ownership and updates harness to call pid setter methods — strong overlap with Pid/PidState API changes.
Fix e_com_time: compute from ISR intervals, not empty main-loop array #18: Changes to commutation interval handling and visibility — directly related to making intervals private and using record_interval().
Tighten visibility to pub(crate), delete 12 dead items surfaced by compiler #22: Similar visibility/encapsulation changes for PID and control state — likely touches the same refactor surface.

Poem

I nibbled through fields and stitched a seam,
Setters now tidy the PID dream,
Ticks hum steady, ISR taps the beat,
Intervals safe where code and hardware meet,
A rabbit hops — the motor sings. 🐇

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main changes: making Pid and PidState fields private and extracting control logic into tick methods.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch pr17_1

_{Review rate limit: 3/5 reviews remaining, refill in 15 minutes and 18 seconds.}

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

codecov · 2026-04-30T23:53:49Z

❌ 2 Tests Failed:

Tests completed	Failed	Passed	Skipped
258	2	256	1

View the full list of 2 ❄️ flaky test(s)

tests.blackbox.test_vectors::test_vector[signal_timeout]

Flake rate in main: 100.00% (Passed 0 times, Failed 22 times)

Stack Traces | 0.002s run time

harness = <harness.AM32Harness object at 0x7fb972c64890>
vector_file = PosixPath('.../blackbox/vectors/signal_timeout.txt')

    @pytest.mark.parametrize(
        "vector_file",
        get_vector_files(),
        ids=lambda p: p.stem,
    )
    def test_vector(harness, vector_file):
        """Run a single test vector file."""
        if vector_file.stem in XFAIL_VECTORS:
            pytest.xfail(f"Known Rust vs C difference: {vector_file.stem}")
>       run_test_vectors(harness, vector_file)

tests/blackbox/test_vectors.py:54: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

harness = <harness.AM32Harness object at 0x7fb972c64890>
vectors_file = PosixPath('.../blackbox/vectors/signal_timeout.txt')

    def run_test_vectors(harness, vectors_file):
        """
        Run test vectors from a file.
    
        Format:
            # Comments start with #
            # Blank lines are ignored
    
            @config
            key=value
            key=value
    
            @sequence
            # tick_spec | inputs | assertions
            tick 1      | throttle=0          |
            ticks 20000 | throttle=0          | armed=1
            tick 1      | throttle=500        | running=1 duty_cycle>0
        """
        import re
    
        section = None
        config_lines = []
        sequence_lines = []
    
        with open(vectors_file, encoding="utf-8") as f:
            for line in f:
                line = line.strip()
                if not line or line.startswith("#"):
                    continue
                if line == "@config":
                    section = "config"
                    continue
                if line == "@sequence":
                    section = "sequence"
                    continue
                if section == "config":
                    config_lines.append(line)
                elif section == "sequence":
                    sequence_lines.append(line)
    
        # Apply config
        harness.reset()
        for cl in config_lines:
            if "=" not in cl:
                continue
            k, v = cl.split("=", 1)
            harness.config(**{k.strip(): v.strip()})
    
        # Operator dispatch table
        ops = {
            "=": lambda a, b: a == b,
            ">": lambda a, b: a > b,
            "<": lambda a, b: a < b,
            ">=": lambda a, b: a >= b,
            "<=": lambda a, b: a <= b,
        }
    
        # Run sequence
        results = []
        for seq_line in sequence_lines:
            # Inline config commands
            if seq_line.startswith("config "):
                kvs = seq_line[7:]
                for token in kvs.split():
                    if "=" in token:
                        k, v = token.split("=", 1)
                        harness.config(**{k: v})
                continue
    
            # Inline commands
            if seq_line == "load_eeprom":
                harness.load_eeprom()
                continue
    
            parts = [p.strip() for p in seq_line.split("|")]
            cmd_part = parts[0]
            input_part = parts[1] if len(parts) > 1 else ""
            assert_part = parts[2] if len(parts) > 2 else ""
    
            # Parse inputs
            inputs = {}
            for token in input_part.split():
                if "=" in token:
                    k, v = token.split("=", 1)
                    inputs[k] = int(v)
    
            # Execute
            tokens = cmd_part.split()
            cmd = tokens[0]
            if cmd == "tick":
                state = harness.tick(**inputs)
            elif cmd == "ticks":
                n = int(tokens[1])
                state = harness.ticks(n, **inputs)
            elif cmd == "gcr_encode":
                com_time = int(tokens[1])
                state = harness.gcr_encode(com_time, **inputs)
            else:
                raise ValueError(f"Unknown command: {cmd}")
    
            # Check assertions
            if assert_part:
                for assertion in assert_part.split():
                    # Support: key=value, key>value, key<value, key>=value, key<=value
                    # Value can be numeric or string (for gcr= comma-separated buffers)
                    m = re.match(r"(\w+)(>=|<=|>|<|=)([\w,.-]+)", assertion)
                    if not m:
                        raise ValueError(f"Bad assertion: {assertion}")
                    key, op, expected_str = m.group(1), m.group(2), m.group(3)
                    actual = state.get(key)
                    if actual is None:
>                       raise KeyError(f"Key '{key}' not in state")
E                       KeyError: "Key 'armed' not in state"

tests/blackbox/harness.py:255: KeyError

tests.blackbox.test_vectors::test_vector[telemetry]

Flake rate in main: 100.00% (Passed 0 times, Failed 22 times)

Stack Traces | 0.002s run time

harness = <harness.AM32Harness object at 0x7fb972c6ade0>
vector_file = PosixPath('.../blackbox/vectors/telemetry.txt')

    @pytest.mark.parametrize(
        "vector_file",
        get_vector_files(),
        ids=lambda p: p.stem,
    )
    def test_vector(harness, vector_file):
        """Run a single test vector file."""
        if vector_file.stem in XFAIL_VECTORS:
            pytest.xfail(f"Known Rust vs C difference: {vector_file.stem}")
>       run_test_vectors(harness, vector_file)

tests/blackbox/test_vectors.py:54: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

harness = <harness.AM32Harness object at 0x7fb972c6ade0>
vectors_file = PosixPath('.../blackbox/vectors/telemetry.txt')

    def run_test_vectors(harness, vectors_file):
        """
        Run test vectors from a file.
    
        Format:
            # Comments start with #
            # Blank lines are ignored
    
            @config
            key=value
            key=value
    
            @sequence
            # tick_spec | inputs | assertions
            tick 1      | throttle=0          |
            ticks 20000 | throttle=0          | armed=1
            tick 1      | throttle=500        | running=1 duty_cycle>0
        """
        import re
    
        section = None
        config_lines = []
        sequence_lines = []
    
        with open(vectors_file, encoding="utf-8") as f:
            for line in f:
                line = line.strip()
                if not line or line.startswith("#"):
                    continue
                if line == "@config":
                    section = "config"
                    continue
                if line == "@sequence":
                    section = "sequence"
                    continue
                if section == "config":
                    config_lines.append(line)
                elif section == "sequence":
                    sequence_lines.append(line)
    
        # Apply config
        harness.reset()
        for cl in config_lines:
            if "=" not in cl:
                continue
            k, v = cl.split("=", 1)
            harness.config(**{k.strip(): v.strip()})
    
        # Operator dispatch table
        ops = {
            "=": lambda a, b: a == b,
            ">": lambda a, b: a > b,
            "<": lambda a, b: a < b,
            ">=": lambda a, b: a >= b,
            "<=": lambda a, b: a <= b,
        }
    
        # Run sequence
        results = []
        for seq_line in sequence_lines:
            # Inline config commands
            if seq_line.startswith("config "):
                kvs = seq_line[7:]
                for token in kvs.split():
                    if "=" in token:
                        k, v = token.split("=", 1)
                        harness.config(**{k: v})
                continue
    
            # Inline commands
            if seq_line == "load_eeprom":
                harness.load_eeprom()
                continue
    
            parts = [p.strip() for p in seq_line.split("|")]
            cmd_part = parts[0]
            input_part = parts[1] if len(parts) > 1 else ""
            assert_part = parts[2] if len(parts) > 2 else ""
    
            # Parse inputs
            inputs = {}
            for token in input_part.split():
                if "=" in token:
                    k, v = token.split("=", 1)
                    inputs[k] = int(v)
    
            # Execute
            tokens = cmd_part.split()
            cmd = tokens[0]
            if cmd == "tick":
                state = harness.tick(**inputs)
            elif cmd == "ticks":
                n = int(tokens[1])
                state = harness.ticks(n, **inputs)
            elif cmd == "gcr_encode":
                com_time = int(tokens[1])
                state = harness.gcr_encode(com_time, **inputs)
            else:
                raise ValueError(f"Unknown command: {cmd}")
    
            # Check assertions
            if assert_part:
                for assertion in assert_part.split():
                    # Support: key=value, key>value, key<value, key>=value, key<=value
                    # Value can be numeric or string (for gcr= comma-separated buffers)
                    m = re.match(r"(\w+)(>=|<=|>|<|=)([\w,.-]+)", assertion)
                    if not m:
                        raise ValueError(f"Bad assertion: {assertion}")
                    key, op, expected_str = m.group(1), m.group(2), m.group(3)
                    actual = state.get(key)
                    if actual is None:
>                       raise KeyError(f"Key '{key}' not in state")
E                       KeyError: "Key 'send_telemetry' not in state"

tests/blackbox/harness.py:255: KeyError

To view more test analytics, go to the Test Analytics Dashboard
_{📋 Got 3 mins? Take this short survey to help us improve Test Analytics.}

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0dab462b5d

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@rm32/src/main_state.rs`:
- Around line 356-360: The call to pid.tick_speed_control(...) is using
shared.zero_crosses() directly which can change mid-tick; instead use the
tick-scoped snapshot captured earlier in MainState::tick() (the local zc
variable) so the PID sees a stable zero-crosses value for the whole tick; update
the pid.tick_speed_control call to pass zc (the variable captured at the top of
tick()) rather than calling shared.zero_crosses() directly.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 4e30031c-c2ee-47a1-8e32-1d9429be519a

📥 Commits

Reviewing files that changed from the base of the PR and between 63be1e7 and 0dab462.

📒 Files selected for processing (5)

rm32/src/bin/harness.rs
rm32/src/commutation.rs
rm32/src/control/state.rs
rm32/src/main_state.rs
rm32/src/pid.rs

gemini-code-assist

Code Review

This pull request refactors the PidState and Pid structures to improve encapsulation by making fields private and introducing dedicated methods for stall protection, current limiting, and speed control. The logic previously found in main_state.rs has been moved into these new methods. Feedback was provided regarding a behavioral change in the speed control logic where the entire PID state is reset instead of only the integral term, which may impact parity with the original implementation.

10 tests covering stall, current limit, and speed control PID ticks: - current_limit_pid_reduces_ceiling_when_over_target (mirrors C test) - current_limit_pid_clamps_to_min_duty - current_limit_pid_resets_when_inactive - stall_pid_boosts_when_interval_above_target (mirrors C test) - stall_pid_clamps_to_150 - stall_pid_clamps_to_zero - speed_control_inactive_returns_none - speed_control_not_running_returns_none - speed_control_active_returns_override - speed_control_works_during_startup

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@rm32/src/control/state.rs`:
- Around line 54-57: The target_e_com_time field is private but used by
tick_speed_control(), so add a public setter to allow updating the PID target
after default() initialization: implement pub fn set_target_e_com_time(&mut
self, target: u32) on the struct that contains target_e_com_time (the same type
that defines tick_speed_control()), and use that setter from callers that need
to change the speed control target; alternatively, make the field pub if you
prefer direct access, but prefer the setter for encapsulation. Ensure you update
any other similar struct instances referenced in the same file (the other
occurrence noted around the 114-126 region) to provide the same setter or public
access so both usages can change the target.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 4f618209-9025-472f-99e7-fb0e463256e8

📥 Commits

Reviewing files that changed from the base of the PR and between 0dab462 and bc8c6d8.

📒 Files selected for processing (1)

rm32/src/control/state.rs

…etter - Add Pid::clear_integral() that only zeros integral (matching C behavior). Speed control was using reset() which also cleared derivative state — behavioral change from C. - Pass captured zc snapshot to tick_speed_control instead of reading shared.zero_crosses() directly, which could change mid-tick via desync reset. - Add PidState::set_target_e_com_time() — field was private with no setter, leaving speed PID target stuck at 0.

coderabbitai

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@rm32/src/control/state.rs`:
- Around line 107-111: Normalize the lower clamp bound before using it: compute
a safe min_bound = min_duty.clamp(0, 2000) (ensure types match i16), then call
self.current_limit_adjust = self.current_limit_adjust.clamp(min_bound, 2000) so
the lower bound cannot exceed 2000 or be negative, and only then cast
self.current_limit_adjust as u16; adjust the code around current_limit_adjust,
min_duty and the clamp call to use min_bound.
- Around line 112-115: When tick_current_limit() or tick_speed_control() detect
the controller is inactive they currently suppress output but leave the PID
internal state intact; update both functions to reset their PID state when
returning inactive by clearing the integral accumulator,
last_error/previous_error and any derivative/history or timestamp fields so no
stale state remains. Locate the PID instances used by each function (e.g. fields
referenced in tick_current_limit and tick_speed_control such as current_limit_*
or speed_* controller/accumulator variables) and implement a reset method or
inline zeroing (integral = 0.0, previous_error = None/0.0, last_timestamp =
None, derivative_history.clear()) and call it before returning the inactive
value.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 82c8d956-0c82-4a0d-b7c6-e061dc2e93c8

📥 Commits

Reviewing files that changed from the base of the PR and between bc8c6d8 and 65c82c8.

📒 Files selected for processing (3)

rm32/src/control/state.rs
rm32/src/main_state.rs
rm32/src/pid.rs

✅ Files skipped from review due to trivial changes (1)

rm32/src/main_state.rs

kaidokert added 2 commits April 30, 2026 16:37

sourcery-ai Bot reviewed Apr 30, 2026

View reviewed changes

chatgpt-codex-connector Bot reviewed Apr 30, 2026

View reviewed changes

Comment thread rm32/src/control/state.rs Outdated

coderabbitai Bot reviewed Apr 30, 2026

View reviewed changes

Comment thread rm32/src/main_state.rs Outdated

gemini-code-assist Bot reviewed Apr 30, 2026

View reviewed changes

Comment thread rm32/src/control/state.rs

coderabbitai Bot reviewed May 1, 2026

View reviewed changes

Comment thread rm32/src/control/state.rs

coderabbitai Bot reviewed May 1, 2026

View reviewed changes

Comment thread rm32/src/control/state.rs

Comment thread rm32/src/control/state.rs

kaidokert merged commit c072bc1 into main May 1, 2026
23 checks passed

This was referenced May 1, 2026

Make struct fields private, add accessor methods #24

Merged

Encapsulate DutyState and ProtectionState, remove last pub fields from state.rs #28

Merged

kaidokert deleted the pr17_1 branch May 1, 2026 06:46

Conversation

kaidokert commented Apr 30, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Summary by CodeRabbit

Uh oh!

sourcery-ai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot commented Apr 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Possibly related PRs

Poem

Uh oh!

codecov Bot commented Apr 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

❌ 2 Tests Failed:

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

kaidokert commented Apr 30, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Apr 30, 2026 •

edited

Loading

codecov Bot commented Apr 30, 2026 •

edited

Loading