Fix e_com_time: compute from ISR intervals, not empty main-loop array

[kaidokert]

Summary

• Bug fix: commutation_intervals array was never populated by the firmware ISR, causing e_com_time, e_rpm, and average_interval to always be zero in production
• Move intervals array into Commutation struct (ISR-owned), add record_interval() at each step advance matching C main.c:882
• ISR now computes and publishes e_com_time to SharedComm; main loop reads instead of computing from stale local array
• Remove commutation_intervals from TimingState (no longer needed in main loop)

Test plan

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

Summary by CodeRabbit

• Improvements
  • Per-step commutation intervals are now recorded at interrupt time and used as the authoritative source for commutation timing, improving RPM and duty calculations.
• Bug Fixes
  • Ensures published commutation time is populated correctly during operation, preventing it from remaining near zero.
• Tests
  • Added a blackbox test to verify commutation time becomes populated during a short run.

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

📝 Walkthrough

Walkthrough

Per-step commutation interval recording was moved into the ISR/Commutation; ISR now publishes computed e_com_time to SharedState. The main loop reads e_com_time from shared state instead of maintaining local commutation_intervals. The harness no longer backfills main timing intervals during zero-crossing.

Changes

Cohort / File(s)	Summary
Commutation & ISR rm32/src/commutation.rs, rm32/src/control/isr_logic.rs	Add Commutation.intervals: [u16;6] and Commutation::record_interval(...); ISR records per-step intervals and publishes summed e_com_time to SharedState.
State & Main Loop rm32/src/control/state.rs, rm32/src/main_state.rs	Remove TimingState.commutation_intervals; main loop now reads shared.e_com_time() and uses it for timing/derived calculations.
Harness rm32/src/bin/harness.rs	Removed code that copied shared.commutation_interval() into main_state.timing.commutation_intervals during "zc" handling.
Tests tests/blackbox/vectors/e_com_time_populated.txt	Add blackbox vector exercising ISR-driven interval recording and asserting e_com_time becomes >100.

Sequence Diagram(s)

sequenceDiagram
    participant ISR
    participant Commutation
    participant SharedState
    participant MainLoop

    ISR->>Commutation: record_interval(step_interval)
    Commutation-->>ISR: store intervals[step-1]
    ISR->>Commutation: compute sum/avg -> e_com_time_raw
    Commutation-->>ISR: e_com_time_raw
    ISR->>SharedState: set_e_com_time((sum + 4) >> 1)
    MainLoop->>SharedState: read e_com_time()
    SharedState-->>MainLoop: e_com_time
    MainLoop->>MainLoop: update timing (average, eRPM, duty)

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

Possibly related PRs

• Make harness test actual firmware code #9: Touches ISR/shared-state plumbing and adds SharedComm setters/getters overlapping with set_e_com_time work.
• Adopt TimingState into MainState, remove loose timing fields #17: Modifies timing/commutation data flow and harness zero-crossing handling similar to these changes.

Poem

🐰 I counted six hops, one by one,

ISR drums and the intervals hum.
SharedState sings the averaged chime,
Main loop listens, keeping time.
🥕 Little rabbit, code in rhyme.

🚥 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 describes the main change: moving e_com_time computation from a main-loop array (commutation_intervals in TimingState) to ISR-based intervals tracking, fixing a production bug where the array was never populated.
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 pr12_1

---

_{Review rate limit: 2/5 reviews remaining, refill in 28 minutes and 30 seconds.}

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

[codecov]

❌ 2 Tests Failed:

Tests completed	Failed	Passed	Skipped
252	2	250	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 10 times)

Stack Traces | 0.002s run time

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

Stack Traces | 0.002s run time

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

[coderabbitai]

🧹 Nitpick comments (1)

rm32/src/commutation.rs (1)

25-30: ⚡ Quick win

Add direct unit tests for record_interval() index mapping.

This is a new correctness-critical helper; add focused tests for forward/reverse and wrap cases to prevent off-by-one regressions.

Proposed tests

 #[cfg(test)]
 mod tests {
     use super::*;
+    
+    #[test]
+    fn record_interval_writes_current_step_slot() {
+        let mut c = Commutation::new();
+        c.step = 3;
+        c.record_interval(1234);
+        assert_eq!(c.intervals[2], 1234);
+    }
+
+    #[test]
+    fn record_interval_after_forward_wrap_writes_step1_slot() {
+        let mut c = Commutation::new();
+        c.step = 6;
+        c.advance(); // -> step 1
+        c.record_interval(777);
+        assert_eq!(c.intervals[0], 777);
+    }
+
+    #[test]
+    fn record_interval_after_reverse_wrap_writes_step6_slot() {
+        let mut c = Commutation::new();
+        c.forward = false;
+        c.step = 1;
+        c.advance(); // -> step 6
+        c.record_interval(888);
+        assert_eq!(c.intervals[5], 888);
+    }

🤖 Prompt for AI Agents

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

In `@rm32/src/commutation.rs` around lines 25 - 30, Add focused unit tests for
record_interval to verify the index mapping uses (step - 1) as usize: create
tests in commutation.rs that directly manipulate a Commutation instance's step
and intervals, call record_interval(value) and assert intervals[(step - 1) as
usize] == value. Include at least: a forward case (e.g., step = 1 updates
intervals[0]), a mid-range case (e.g., step = 3 updates intervals[2]), a
reverse-like case by setting step to a previous value and verifying the same
mapping, and a wrap case where step = 0 should update the last index of
intervals (verify the last slot was written). Reference the record_interval
method and the Commutation fields step and intervals when locating code to test.

🤖 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/commutation.rs`:
- Around line 25-30: Add focused unit tests for record_interval to verify the
index mapping uses (step - 1) as usize: create tests in commutation.rs that
directly manipulate a Commutation instance's step and intervals, call
record_interval(value) and assert intervals[(step - 1) as usize] == value.
Include at least: a forward case (e.g., step = 1 updates intervals[0]), a
mid-range case (e.g., step = 3 updates intervals[2]), a reverse-like case by
setting step to a previous value and verifying the same mapping, and a wrap case
where step = 0 should update the last index of intervals (verify the last slot
was written). Reference the record_interval method and the Commutation fields
step and intervals when locating code to test.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 927a09ad-ee40-435c-8c21-223c3d4d5cbf

📥 Commits

Reviewing files that changed from the base of the PR and between 234e811 and 6ae2510.

📒 Files selected for processing (5)

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

💤 Files with no reviewable changes (2)

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

[gemini-code-assist]

Code Review

This pull request refactors the commutation interval tracking by moving the storage logic from the main loop into the Commutation struct and updating it directly within the ISR. The review identified an indexing issue where intervals were being recorded for the incorrect step due to the order of operations in the ISR, and suggested optimizing the e_com_time calculation to avoid redundant processing in the high-frequency control loop.

[coderabbitai]

🧹 Nitpick comments (2)

rm32/src/control/isr_logic.rs (1)

60-63: ⚡ Quick win

Consider extracting a helper for record_interval + set_e_com_time.

The same 3-line sequence is duplicated in three places; a tiny helper reduces future drift risk when one advance path changes.

♻️ Suggested refactor

+fn publish_e_com_time<S: SharedComm>(commutation: &mut Commutation, shared: &S) {
+    let e_com = commutation.record_interval(shared.commutation_interval() as u16);
+    shared.set_e_com_time(e_com);
+}
...
-                let e_com = ctx
-                    .commutation
-                    .record_interval(ctx.shared.commutation_interval() as u16);
-                ctx.shared.set_e_com_time(e_com);
+                publish_e_com_time(&mut ctx.commutation, ctx.shared);
...
-            let e_com = ctx
-                .commutation
-                .record_interval(ctx.shared.commutation_interval() as u16);
-            ctx.shared.set_e_com_time(e_com);
+            publish_e_com_time(&mut ctx.commutation, ctx.shared);
...
-    let e_com = commutation.record_interval(shared.commutation_interval() as u16);
-    shared.set_e_com_time(e_com);
+    publish_e_com_time(commutation, shared);

Also applies to: 203-207, 270-271

🤖 Prompt for AI Agents

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

In `@rm32/src/control/isr_logic.rs` around lines 60 - 63, Extract the duplicated
three-line sequence into a small helper (e.g., a method named update_e_com_time
or set_e_com_from_commutation) that captures the pattern: call
commutation.record_interval(ctx.shared.commutation_interval() as u16) and then
ctx.shared.set_e_com_time(...) with the result; implement the helper to accept
&mut self or &mut ctx as appropriate and replace each duplicated occurrence (the
blocks currently calling commutation.record_interval and
ctx.shared.set_e_com_time) with a single call to that helper so all paths use
the same logic.

rm32/src/commutation.rs (1)

25-34: ⚡ Quick win

Add direct unit tests for record_interval() behavior.

This new method is core to telemetry correctness; add focused tests for slot mapping (step - 1) and expected computed value from a known 6-interval set.

✅ Example test additions

 #[cfg(test)]
 mod tests {
     use super::*;
+    #[test]
+    fn record_interval_writes_step_slot() {
+        let mut c = Commutation::new();
+        c.step = 3;
+        c.record_interval(120);
+        assert_eq!(c.intervals[2], 120);
+    }
+
+    #[test]
+    fn record_interval_recomputes_e_com_time_from_all_slots() {
+        let mut c = Commutation::new();
+        c.intervals = [100, 100, 100, 100, 100, 100];
+        c.step = 1;
+        let e = c.record_interval(100);
+        assert_eq!(e, ((600u32 + 4) >> 1) as i32);
+    }

Also applies to: 64-165

🤖 Prompt for AI Agents

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

In `@rm32/src/commutation.rs` around lines 25 - 34, Add focused unit tests for the
record_interval method: create a Commutation instance, set its step to a known
value (e.g., 1..6), call record_interval(commutation_interval) and assert that
intervals[(step - 1) as usize] was updated with the passed u16 value and that
the returned e_com_time equals the expected computation for a known 6‑interval
set (compute expected as ((sum_of_intervals + 4) >> 1) cast to i32). Include at
least one test that fills multiple slots (e.g., populate all six intervals with
known values, call record_interval for one slot, and verify both the slot
mapping and the returned aggregated value) and ensure step is set appropriately
before each call to record_interval.

🤖 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/commutation.rs`:
- Around line 25-34: Add focused unit tests for the record_interval method:
create a Commutation instance, set its step to a known value (e.g., 1..6), call
record_interval(commutation_interval) and assert that intervals[(step - 1) as
usize] was updated with the passed u16 value and that the returned e_com_time
equals the expected computation for a known 6‑interval set (compute expected as
((sum_of_intervals + 4) >> 1) cast to i32). Include at least one test that fills
multiple slots (e.g., populate all six intervals with known values, call
record_interval for one slot, and verify both the slot mapping and the returned
aggregated value) and ensure step is set appropriately before each call to
record_interval.

In `@rm32/src/control/isr_logic.rs`:
- Around line 60-63: Extract the duplicated three-line sequence into a small
helper (e.g., a method named update_e_com_time or set_e_com_from_commutation)
that captures the pattern: call
commutation.record_interval(ctx.shared.commutation_interval() as u16) and then
ctx.shared.set_e_com_time(...) with the result; implement the helper to accept
&mut self or &mut ctx as appropriate and replace each duplicated occurrence (the
blocks currently calling commutation.record_interval and
ctx.shared.set_e_com_time) with a single call to that helper so all paths use
the same logic.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: e950e7a1-8ffa-4c86-b03c-80c1396eddbe

📥 Commits

Reviewing files that changed from the base of the PR and between 1ef3758 and 1baed67.

📒 Files selected for processing (2)

• rm32/src/commutation.rs
• rm32/src/control/isr_logic.rs