Extract MotorState sub-trait from SharedComm by kaidokert · Pull Request #25 · kaidokert/rm32

kaidokert · 2026-05-01T01:39:45Z

Summary

First step of SharedComm decomposition (SHAREDCOMM_AUDIT.md):

Extract the 11 motor mode methods into a dedicated MotorState trait:

motor_mode(), set_motor_mode() — required (2 methods)
transition() — default impl using MotorEvent
armed(), running(), old_routine(), stepper_sine() — derived getters
set_armed(), set_running(), set_old_routine(), set_stepper_sine() — convenience setters

SharedComm: MotorState — super-trait requirement, so all existing
SharedComm callers get MotorState methods automatically.

SharedState and TestShared implement MotorState in separate impl blocks.

This validates the sub-trait pattern before extracting IsrTiming and
MainControl in subsequent PRs.

Test plan

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

Summary by CodeRabbit

Refactor
- Reorganized motor-mode behavior into a dedicated interface and split its implementations from the rest of the shared communication surface, with corresponding adjustments to internal state handling and documentation.
Tests
- Updated test imports so the reorganized motor-state interface is included during test compilation.

First step of SharedComm decomposition: extract the 11 motor mode methods (motor_mode, set_motor_mode, transition, armed/running/ old_routine/stepper_sine + setters) into a dedicated MotorState trait. SharedComm now requires MotorState as a super-trait. SharedState and TestShared implement MotorState separately. Callers using SharedComm as a trait bound get MotorState methods automatically; callers using concrete types need MotorState imported. This is the pilot for the sub-trait pattern described in SHAREDCOMM_AUDIT.md — validates the approach before extracting IsrTiming and MainControl.

coderabbitai · 2026-05-01T01:39:55Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 058163d7-201a-477e-8575-bb17447c0aa1

📥 Commits

Reviewing files that changed from the base of the PR and between 11c017f and 5b38956.

📒 Files selected for processing (1)

rm32/src/shared_state.rs

🚧 Files skipped from review as they are similar to previous changes (1)

rm32/src/shared_state.rs

📝 Walkthrough

Walkthrough

This PR extracts motor-mode APIs into a new MotorState trait and makes SharedComm a supertrait of MotorState. Implementations in SharedState and TestShared are split so motor-mode methods live in impl MotorState blocks, while remaining shared methods remain in impl SharedComm.

Changes

Cohort / File(s)	Summary
Trait definitions `rm32/src/shared_comm.rs`	Added public `MotorState` trait with `motor_mode()` and `set_motor_mode()` (and related motor-mode helpers). Updated `SharedComm` to be `pub trait SharedComm: MotorState`.
SharedState implementations `rm32/src/shared_state.rs`	Moved `motor_mode`/`set_motor_mode`/`transition` and overridden convenience setters into `impl crate::shared_comm::MotorState for SharedState`. `impl SharedComm for SharedState` now contains only remaining telemetry/input/control accessors.
TestShared implementations `rm32/src/control/shared_impl.rs`	Added `impl MotorState for TestShared` (providing `motor_mode` and `set_motor_mode`) and kept the rest of `impl SharedComm for TestShared` for other methods.
Tests import update `rm32/src/control/tests.rs`	Updated test module `use` to import both `MotorState` and `SharedComm` via underscore-imports (`as _`).

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Make harness test actual firmware code #9: Makes similar changes to the MotorState/SharedComm trait split and corresponding impl updates.

Poem

🐰 I hopped through traits with a curious nod,
I nudged the motor methods into a new pod,
SharedComm now leans on MotorState's might,
Cleanly divided, tidy and light. ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 5.26% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'Extract MotorState sub-trait from SharedComm' directly and clearly summarizes the main change: extracting a new MotorState trait from the existing SharedComm trait across multiple files.
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 pr19_1

_{Review rate limit: 4/5 reviews remaining, refill in 12 minutes.}

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

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

codecov · 2026-05-01T01:40:23Z

❌ 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 28 times)

Stack Traces | 0.002s run time

harness = <harness.AM32Harness object at 0x7f885d730fb0>
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 0x7f885d730fb0>
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 28 times)

Stack Traces | 0.002s run time

harness = <harness.AM32Harness object at 0x7f885d732f90>
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 0x7f885d732f90>
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.}

gemini-code-assist

Code Review

This pull request refactors the SharedComm trait by decomposing it into sub-traits, specifically introducing the MotorState trait to handle motor mode state transitions. The changes are applied to the test implementation, the trait definitions, and the hardware-backed SharedState. A significant issue was identified in the SharedState implementation of MotorState: it fails to override convenience setters like set_armed, which leads to the use of non-atomic default implementations. This could introduce race conditions, so these methods should delegate to the existing atomic inherent methods in SharedState.

coderabbitai

🧹 Nitpick comments (1)

rm32/src/shared_comm.rs (1)

22-26: ⚡ Quick win

Capture mode once in default transition() to avoid double-read drift.

Line 23 and Line 24 read motor_mode() separately. Snapshotting once makes the default path deterministic and slightly cheaper.

Suggested diff

     fn transition(&self, event: MotorEvent) {
-        let new = self.motor_mode().transition(event);
-        if new != self.motor_mode() {
+        let current = self.motor_mode();
+        let new = current.transition(event);
+        if new != current {
             self.set_motor_mode(new);
         }
     }

🤖 Prompt for AI Agents

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

In `@rm32/src/shared_comm.rs` around lines 22 - 26, The default transition() reads
motor_mode() twice which can drift or be non-deterministic; capture the current
mode once into a local (e.g., let current = self.motor_mode()), call
current.transition(event) (or use current to derive new), compare new !=
current, and only then call self.set_motor_mode(new); update the transition()
method to use these symbols (transition, motor_mode, set_motor_mode) so the
motor mode is snapshotted once.

🤖 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/shared_comm.rs`:
- Around line 22-26: The default transition() reads motor_mode() twice which can
drift or be non-deterministic; capture the current mode once into a local (e.g.,
let current = self.motor_mode()), call current.transition(event) (or use current
to derive new), compare new != current, and only then call
self.set_motor_mode(new); update the transition() method to use these symbols
(transition, motor_mode, set_motor_mode) so the motor mode is snapshotted once.

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 0a3ee150-6acd-43ea-a4bf-94046e01c18b

📥 Commits

Reviewing files that changed from the base of the PR and between e0eea5f and 11c017f.

📒 Files selected for processing (4)

rm32/src/control/shared_impl.rs
rm32/src/control/tests.rs
rm32/src/shared_comm.rs
rm32/src/shared_state.rs

The trait default implementations use non-atomic load+store. SharedState has atomic fetch_update inherent methods for set_armed, set_running, set_old_routine, set_stepper_sine. Override the trait methods to delegate to those, preventing race conditions when accessed through the MotorState trait.

sourcery-ai Bot reviewed May 1, 2026

View reviewed changes

gemini-code-assist Bot reviewed May 1, 2026

View reviewed changes

Comment thread rm32/src/shared_state.rs

coderabbitai Bot reviewed May 1, 2026

View reviewed changes

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

kaidokert mentioned this pull request May 1, 2026

Extract MainControl sub-trait from SharedComm #27

Merged

4 tasks

kaidokert deleted the pr19_1 branch May 1, 2026 06:47

coderabbitai Bot mentioned this pull request May 1, 2026

Gate TestShared behind #[cfg(test)] #32

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extract MotorState sub-trait from SharedComm#25

Extract MotorState sub-trait from SharedComm#25
kaidokert merged 2 commits into
mainfrom
pr19_1

kaidokert commented May 1, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented May 1, 2026 •

edited

Loading

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Poem

❌ Failed checks (1 warning)

Uh oh!

sourcery-ai Bot left a comment

Uh oh!

codecov Bot commented May 1, 2026 •

edited

Loading

Uh oh!

gemini-code-assist Bot left a comment

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

kaidokert commented May 1, 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!

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

Uh oh!

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Poem

❌ Failed checks (1 warning)

Uh oh!

sourcery-ai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

❌ 2 Tests Failed:

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

kaidokert commented May 1, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented May 1, 2026 •

edited

Loading

codecov Bot commented May 1, 2026 •

edited

Loading