BemfState encapsulation, PWM compare methods, ramp bug fix by kaidokert · Pull Request #30 · kaidokert/rm32

kaidokert · 2026-05-01T06:44:29Z

Summary

BemfState encapsulation — all 11 fields private, BEMF logic moved from
isr_logic.rs into BemfState methods: sync_config, update, zero_cross_detected,
record_zero_cross, update_timing_from_timer, record_zc_timing, reset_for_step,
reset_after_commutation, com_timer_delay. bemf.rs reduced to tests.

DutyState PWM methods — pwm_compare() and brake_compare() extract inline
duty-to-timer math from ISR.

Ramp bug fix — ramp_limit used duty.last > 500 (wrong) instead of
average_interval > 500 (C parity) for RPM-based ramp profile selection.
4 unit tests added that would have caught this.

Constants — 8 magic numbers extracted to named constants in constants.rs.
clamp_ceilings simplified with .min() chain.

state.rs pub(crate) fields: 37 → 13 (BemfState 11 + DutyState 13 made private).

Test plan

223 unit tests pass (6 new)
46 blackbox tests pass, 1 xfail
All 4 MCU cross-builds pass
Clippy clean

Summary by CodeRabbit

Refactor
- Reorganized motor control state management to centralize BEMF timing and detection logic.
Tests
- Updated motor control tests to use new state accessor API.

Make BemfState fields fully private (11 fields), move BEMF logic from isr_logic.rs and bemf.rs into BemfState methods in state.rs: - sync_config, update, zero_cross_detected, record_zero_cross - update_timing_from_timer, record_zc_timing, reset_for_step - reset_after_commutation, com_timer_delay Add DutyState PWM compare methods: - pwm_compare(tim1_arr) — duty to timer compare value - brake_compare(drag_brake_strength, tim1_arr) — brake output Fix pre-existing ramp bug: ramp_limit used duty.last > 500 instead of average_interval > 500 for RPM-based ramp profile selection (matches C). Add 4 unit tests for ramp profile selection. Extract 8 magic numbers into named constants in constants.rs. Simplify clamp_ceilings using .min() chain. state.rs pub(crate) fields: 37 → 13 (BemfState + DutyState fully private).

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-05-01T06:44:40Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: f1a7dd57-7f00-49a2-bfb0-e02ecb6ea11f

📥 Commits

Reviewing files that changed from the base of the PR and between cfe984a and 8c71c6d.

📒 Files selected for processing (1)

rm32/src/control/state.rs

📝 Walkthrough

Walkthrough

The PR refactors BEMF (Back-EMF) state management by centralizing comparator sampling, zero-cross detection, and timing logic into a cohesive BemfState API. Fields are made private, and ISR functions now invoke state methods instead of performing inline calculations. Tests are updated to use accessor methods.

Changes

Cohort / File(s)	Summary
BEMF State API Centralization `rm32/src/control/state.rs`	Encapsulates BEMF state by making fields private and introducing methods: `sync_config`, `update` (comparator sampling), `zero_cross_detected`, `record_zero_cross`, `update_timing_from_timer`, `record_zc_timing`, `com_timer_delay`, and reset helpers. Adds public test accessors for `bad_count`, `filter_level`, and `wait_time`.
ISR Logic Refactoring `rm32/src/control/isr_logic.rs`	Replaces inline BEMF counter/threshold logic with calls to `BemfState` methods. `ten_khz_tick` syncs config via `bemf.sync_config`, `bemf_polling` calls `bemf.update` and checks `zero_cross_detected`, `commutation_timer_expired` uses `bemf.update_timing_from_timer`, and `bemf_zero_cross` calls `bemf.record_zc_timing`.
Test Updates `rm32/src/control/bemf.rs`, `rm32/src/control/tests.rs`	Refactors tests to use accessor methods (`bad_count()`, `set_filter_level()`, `set_wait_time()`, `zc_found()`) instead of direct field access. Removes `BemfState::update` implementation and adds new test cases for zero-cross threshold and interval computation.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Make struct fields private, add accessor methods #24: Both PRs make BemfState fields private and add accessor/update methods to centralize BEMF logic and replace direct field access patterns.
Make harness test actual firmware code #9: Both PRs refactor ISR code paths in state.rs and isr_logic.rs by centralizing BEMF logic into BemfState methods and updating ISR functions to call those methods.
Tighten visibility to pub(crate), delete 12 dead items surfaced by compiler #22: Both PRs modify BemfState and test helpers, including tightening access to BemfState internals and refactoring how the state is accessed and updated.

Poem

🐰 The BEMF state now hops with care,
With private fields tucked away fair,
Through methods we dance, no direct access,
ISR logic clean—less messy, less stress! ✨

🚥 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 directly aligns with the main changes: BemfState encapsulation (making 11 fields private and adding accessor methods), PWM compare methods added to DutyState, and a ramp bug fix for RPM-based profile selection.
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 pr24_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.}

codecov · 2026-05-01T06:44:57Z

❌ 2 Tests Failed:

Tests completed	Failed	Passed	Skipped
269	2	267	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 38 times)

Stack Traces | 0.002s run time

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

Stack Traces | 0.002s run time

harness = <harness.AM32Harness object at 0x7f52c7cbf530>
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 0x7f52c7cbf530>
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 BEMF zero-cross detection logic by encapsulating state and behavior within the BemfState struct, moving logic out of the ISR and improving modularity. Two critical bugs were identified in the timing calculations where u32 values are truncated to u16 before division or shifting, which could lead to incorrect timing at low RPM.

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 430-434: reset_for_step currently resets only counter and
bad_count but must also clear the zero-cross flag; update the method
reset_for_step to set self.zc_found = false so that subsequent
zero_cross_detected() and zero_crosses() logic works correctly after calling
record_zero_cross() and bemf_polling(); locate reset_for_step in the same state
struct and add clearing of zc_found (referenced symbols: reset_for_step,
zc_found, record_zero_cross, bemf_polling, zero_cross_detected, zero_crosses,
MIN_ZC_FOR_ADVANCE).

🪄 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: 04516634-22b7-4e3f-a304-1048eeeaa3c2

📥 Commits

Reviewing files that changed from the base of the PR and between 0f94360 and cfe984a.

📒 Files selected for processing (5)

.gitignore
rm32/src/bemf.rs
rm32/src/control/isr_logic.rs
rm32/src/control/state.rs
rm32/src/control/tests.rs

Two wait_time bugs: record_zero_cross and update_timing_from_timer truncated new_ci to u16 BEFORE dividing by 2. At low RPM (new_ci > 65535), this produces completely wrong wait_time. C code divides the u32 value first, then truncates on assignment to u16. reset_for_step was missing zc_found = false. C code clears zcfound together with bemfcounter after every commutation step. Without this, zero_cross_detected() stays false after the first detection in the startup path, stalling old_routine startup.

kaidokert added 2 commits April 30, 2026 23:42

Remove accidentally tracked working docs from repo

cfe984a

sourcery-ai Bot reviewed May 1, 2026

View reviewed changes

Revert .gitignore additions for working docs

0b401f3

gemini-code-assist Bot reviewed May 1, 2026

View reviewed changes

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

Comment thread rm32/src/control/state.rs

coderabbitai Bot reviewed May 1, 2026

View reviewed changes

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

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

This was referenced May 1, 2026

Move voltage_based_ramp from TickCounters to MotorContext #31

Merged

Embed TickCounters in IsrState, eliminate copy boilerplate #39

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BemfState encapsulation, PWM compare methods, ramp bug fix#30

BemfState encapsulation, PWM compare methods, ramp bug fix#30
kaidokert merged 4 commits into
mainfrom
pr24_1

kaidokert commented May 1, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

sourcery-ai Bot left a comment

Uh oh!

coderabbitai Bot commented May 1, 2026 •

edited

Loading

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Poem

Uh oh!

codecov Bot commented May 1, 2026 •

edited

Loading

Uh oh!

gemini-code-assist Bot left a comment

Uh oh!

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Uh oh!

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!

sourcery-ai Bot left a comment

Choose a reason for hiding this comment

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

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!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

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