Adopt TimingState into MainState, remove loose timing fields

[kaidokert]

Summary

• Trim TimingState to 4 main-loop-owned fields (e_rpm, average_interval, last_average_interval, commutation_intervals)
• Remove dead fields that duplicated SharedComm (commutation_interval, zero_crosses, e_com_time) and a hardcoded constant (polling_mode_changeover)
• Adopt as MainState::timing: TimingState replacing 4 loose scalar fields
• MainState field count: 35 → 23 (across this PR and the PidState PR)

Test plan

• 207 unit tests pass
• 44 blackbox tests pass, 1 xfail
• All 4 MCU cross-builds pass
• Clippy clean

Summary by CodeRabbit

• Refactor
  • Consolidated timing-related data structures to improve internal organization and code consistency.

[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]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 76c9b36b-5ac2-431c-9736-a44f1f893ead

📥 Commits

Reviewing files that changed from the base of the PR and between 94a4749 and 02041e7.

📒 Files selected for processing (3)

• rm32/src/bin/harness.rs
• rm32/src/control/state.rs
• rm32/src/main_state.rs

---

📝 Walkthrough

Walkthrough

This change consolidates timing-related fields in MainState by moving them into a dedicated TimingState struct. TimingState is redefined to contain only main-loop timing outputs, removing ISR-owned fields. All references in the harness and main loop are updated to access these fields through the new timing field.

Changes

Cohort / File(s)	Summary
Timing Field Consolidation rm32/src/main_state.rs, rm32/src/control/state.rs, rm32/src/bin/harness.rs	Consolidates timing fields (e_rpm, average_interval, last_average_interval, commutation_intervals) from MainState into a new timing: TimingState field. TimingState is restructured to exclude ISR-owned timing inputs and derives Default. Harness diagnostic output and command overrides are updated to access these fields via main.timing.*.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related PRs

• Make harness test actual firmware code #9 — Directly related harness and main-state refactor that changed timing field access patterns, making this PR's consolidation of timing fields into a dedicated struct a continuation of that structural evolution.

Poem

🐰 Timing fields now cuddle tight,
In their own cozy TimingState site,
No more scattered here and there,
Just .timing.* everywhere—
Hopping faster, sleek and light! ✨

🚥 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 structural change: consolidating four loose timing fields into TimingState and updating MainState accordingly.
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 pr11_1

---

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

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

[codecov]

❌ 2 Tests Failed:

Tests completed	Failed	Passed	Skipped
251	2	249	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 8 times)

Stack Traces | 0.002s run time

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

Stack Traces | 0.002s run time

harness = <harness.AM32Harness object at 0x7fe9db2427b0>
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 0x7fe9db2427b0>
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 MainState structure by encapsulating timing-related fields into a new TimingState struct and removing fields that are now managed by the ISR in SharedComm. While the refactoring improves code organization, two significant issues were identified: the commutation_intervals array in MainState is currently unpopulated in production because the ISR cannot access it, which will result in incorrect telemetry and timing calculations. Additionally, the desync reset logic for average_interval is ineffective as it is immediately overwritten by calculations derived from the un-reset commutation_intervals buffer.

[gemini-code-assist]

The commutation_intervals array in MainState is currently unpopulated in the actual firmware path. While the test harness (harness.rs) manually updates this array, the firmware's ISR logic (isr_logic.rs) only has access to SharedState, which does not contain this array. Consequently, sum will always be zero in production, causing e_com_time, e_rpm telemetry, and average_interval to be calculated incorrectly.

Consider either moving the intervals array to SharedState so the ISR can update it, or simplifying the calculation to use the singular shared.commutation_interval() (e.g., let e_com_time = (shared.commutation_interval() * 3) as i32;) if per-step averaging is not strictly required.

References

1. Modules without direct HAL access should communicate with the ISR via shared state to ensure data consistency and proper hardware interaction, as the ISR is the primary interface for hardware-level updates.

[gemini-code-assist]

Resetting average_interval here is ineffective because it is recalculated at the beginning of every tick() (line 212) based on e_com_time, which in turn is derived from the commutation_intervals array. To make this reset persistent across ticks, you must also reset the underlying intervals array.

                    self.timing.average_interval = DESYNC_RESET_INTERVAL;
                    self.timing.commutation_intervals.fill(DESYNC_RESET_INTERVAL as u16);

References

1. When updating or resetting control state, all internal buffers and terms contributing to that state must be cleared to prevent stale data from causing transient calculation errors.