Skip to content

fix(mcp2515): persist acceptance filter across power cycles#1403

Draft
slothish wants to merge 1 commit into
openvehicles:masterfrom
slothish:fix/mcp2515-acceptance-filter
Draft

fix(mcp2515): persist acceptance filter across power cycles#1403
slothish wants to merge 1 commit into
openvehicles:masterfrom
slothish:fix/mcp2515-acceptance-filter

Conversation

@slothish

@slothish slothish commented May 18, 2026

Copy link
Copy Markdown

Problem

SetAcceptanceFilter() correctly writes filter registers and sets RXMODE, but
the configuration is lost on every power cycle. Start() hardcodes RXMODE=3
(receive-all) in RXB0CTRL/RXB1CTRL, silently bypassing any previously
configured hardware filter after SetPowerMode(On) or ignition cycles.

This means vehicles that configure an acceptance filter (e.g. to drop
high-frequency frames that would otherwise saturate the ISR and starve the
Events task) get no filtering in practice — the filter only holds until the
next power-mode transition.

Observed symptom on VW e-Golf / J533 harness: CAN2 (FCAN) runs ~860 fps
during OBC charging. Without a working filter, all frames reach the ISR →
Events task starved → TWDT reset loop → module reboots continuously during
charging.

Fix

Three changes:

  1. Persist filter config: SetAcceptanceFilter() stores the config in
    m_filter_cfg / m_filter_set after successfully writing registers.

  2. Re-apply on Start(): After init, if a filter was previously
    configured, SetAcceptanceFilter() is called again to restore RXMODE.
    A log error is emitted if the restore fails.

  3. Add missing REG_RXB1CTRL define (0x70): was absent from
    mcp2515_regdef.h; SetAcceptanceFilter() was setting RXMODE only on
    RXB0 as a result.

Compatibility

Callers that never call SetAcceptanceFilter() are unaffected — m_filter_set
defaults to false, so Start() takes no new action. Callers that pass
all-zero masks continue to receive all frames (RXMODE=0 with an all-pass
mask is equivalent to RXMODE=3).

NIU GTevo has SetAcceptanceFilter() commented out — no impact.

Known limitation (follow-up)

The filter is re-applied after Start() has already transitioned the
chip out of CONFIG mode and called pcp::SetPowerMode(On). During the brief
window between mode-change and re-apply (typically <20 ms), the chip is in
NORMAL/LISTEN mode with RXMODE=3, so unfiltered frames will reach the ISR.
On a hot bus this can still produce a short ISR burst on every restart.

A cleaner fix would write the filter registers while still in CONFIG mode
during Start(), eliminating the window entirely. That refactor (probably
a _locked variant of SetAcceptanceFilter callable from Start) is left
as a follow-up so this PR stays focused on the persistence bug.

SetAcceptanceFilter() stores the filter config; Start() re-applies it
after init so SetPowerMode(On) / ignition cycles no longer silently
revert RXMODE to 3 (receive-all), bypassing the hardware filter.

Also adds REG_RXB1CTRL define (was missing) and switches RXB0/RXB1
to RXMODE=0 (filter-matched) after writing filter registers.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@slothish slothish marked this pull request as draft May 18, 2026 19:25
@slothish slothish marked this pull request as ready for review May 18, 2026 19:36
@markwj

markwj commented May 19, 2026

Copy link
Copy Markdown
Member

If this is a race condition between Start and SetAcceptanceFilter, surely a better solution would be to either set the filter before Start, or during the Start call?

Or am I missing something?

@slothish

Copy link
Copy Markdown
Author

I was reluctant to touch the Start sequence more than necessary in this PR, so I parked it as a known limitation. But you're right that it's the cleaner fix.

What I had in mind was to split SetAcceptanceFilter into the public entry (lock + config-mode in/out + register writes) and a private ApplyFilterRegs(cfg) helper that only does the register writes. Start() would call the helper directly while it's still in config mode (after the CNFx writes, before ChangeMode(NORMAL/LISTEN)), and the trailing re-apply block in Start can go away.

Does that approach sound right to you?

@slothish slothish marked this pull request as draft May 19, 2026 05:30
@dexterbg

Copy link
Copy Markdown
Member

The approach sounds generally right, but also depends on the application (vehicle) setting the filter before calling Start(), and SetAcceptanceFilter() probably needs to skip changing the mode if not yet started (?).

When implementing the filters, I thought about adding a Start() variant that additionally accepts a filter configuration as a parameter, so this becomes atomic on the application level. Maybe that's a sensible addition here.

@markwj

markwj commented May 19, 2026

Copy link
Copy Markdown
Member

It seems sensible to me to (a) allow the filters to be defined before the Start, and/or (b) introduce a Start variant with the filters passed as parameters. Either would avoid the race condition.

@slothish

slothish commented May 19, 2026

Copy link
Copy Markdown
Author

The "skip mode change when not yet started" branch is easy — SetAcceptanceFilter checks m_canctrl_mode (or m_mode == CAN_MODE_OFF) and skips the enter/exit config bracket. Fine for the existing runtime-reconfig case too.

The atomic Start(mode, speed, filter) is the better primitive though — no ordering contract for the caller, and post-Start reconfigs still go through SetAcceptanceFilter as today. Rough sketch:

  • New Start overload (in canbus base, I think — see below)
  • mcp2515 implementation writes filter regs while still in config mode, before the active/listen ChangeMode
  • RegisterCanBus gets a variant that forwards the filter
  • SetAcceptanceFilter gets the not-started branch for symmetry

One open question on the API shape: both mcp2515 and esp32can have hardware filters today, but with their own config structs (mcp2515_filter_config_t vs esp32can_filter_config_t). For the atomic Start to be useful at the application level it probably wants a generic filter type in canbus base — otherwise the vehicle still has to know which controller it's talking to, which mostly defeats the point. Bigger change up front, but it sets the shape correctly for both controllers.

Which way do you want to take it?

@dexterbg

Copy link
Copy Markdown
Member

There is no sensible way to remove the controller dependency regarding hardware filters. The acceptance filter capabilities and bit layouts are hardware specific, and need to be defined with the CAN controller in mind, as the capabilities and restrictions to cope with are very different.

There of course is the option to create a filter compiler that creates a working hw configuration from a software/generic filter configuration, but this will never be able to build an exact mapping for both controller types, so the application still needs to be written with the edge cases in mind the actual hardware cannot fulfill. Also that compiler would be quite complex, and hardware filters haven't really been needed for any application up to now, probably only ever will be for the SPI/MCP controlled buses. Software filters normally are sufficient, much easier to define and maintain. IOW the actual need doesn't justify the effort.

You can create a virtual base class or union type covering both filter specifications, so you can define generic API signatures, with the actual bus implementation then picking the filter spec it needs. But it would also be OK to put this on the list for the next release.

@slothish

Copy link
Copy Markdown
Author

This is starting to feel like a sidequest. Since I'm the only vehicle module currently relying on mcp2515 hardware filters, I'd rather try to remove the need for them before digging this hole any deeper. I'll run a few charging sessions on my car without the filter active to see if I can avoid the ISR storm — if so, the filter API question becomes moot.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants