Skip to content

Comments

feat: add software debounce filter for GPIO magnetic sensor + FTMS control point fixes#199

Open
cwklurks wants to merge 5 commits intoJaapvanEkris:0.9.7-(under-construction)from
cwklurks:feat/gpio-debounce-filter
Open

feat: add software debounce filter for GPIO magnetic sensor + FTMS control point fixes#199
cwklurks wants to merge 5 commits intoJaapvanEkris:0.9.7-(under-construction)from
cwklurks:feat/gpio-debounce-filter

Conversation

@cwklurks
Copy link

@cwklurks cwklurks commented Feb 20, 2026

This PR includes two improvements:

  • GPIO Debounce Filter (new) - magnetic flywheel sensors produce phantom ticks (~0.5ms apart) that corrupt stroke data. This filter validates delta-times against a rolling median buffer to reject them. Drops deltas under 1ms or under 40% of median. Fixed-size circular buffer, no allocations in the hot path. 15 tests included.
  • FTMS Control Point fixes - corrected error codes to match the Bluetooth FTMS spec, refactored handler with guard clauses.

The FTMS Control Point characteristic returned wrong result codes in
several cases:

- reset/startOrResume/stopOrPause without prior requestControl returned
  opCodeNotSupported (0x02) instead of controlNotPermitted (0x05)
- stopOrPause with invalid parameter (not 1 or 2) returned
  opCodeNotSupported instead of invalidParameter (0x03)
- stopOrPause with a truncated 1-byte buffer caused a RangeError crash
  (buffer underflow) that prevented indicate+callback from being called,
  hanging the BLE stack

Also removes dead fallthrough code — all handled cases now return
explicitly, so only the default case produces opCodeNotSupported.

Adds 11 uvu tests covering all control point code paths.
Hardware bounce from magnetic flywheel sensors can send phantom ticks
(~0.5ms apart) that corrupt stroke data. This middleware layer filters
bounces by validating delta-times against a rolling median buffer.

- Rejects deltas below 1ms (absolute minimum)
- Rejects deltas below 40% of median (physically impossible acceleration)
- Uses fixed-size circular buffer for performance (no allocations in hot path)
@DXCanas
Copy link

DXCanas commented Feb 20, 2026

Am I misunderstanding something, or couldn't the noise be filtered out already, via config?

In the docs, it's described as Switch Bounce, I think:
https://github.com/JaapvanEkris/openrowingmonitor/blob/main/docs/rower_settings.md#fixing-switch-bounce

@Abasz
Copy link
Collaborator

Abasz commented Feb 20, 2026

Am I misunderstanding something, or couldn't the noise be filtered out already, via config?

Yes this feature is already available. Not the 40% deviation though, but I personally dont really support this feature (on ESPRM I have something similar that can be enabled for extreme cases but it is known to cause some times more problems that solves).

The main message in terms of denounce is that try to avoid solving a hardware issue with a software solution.

@JaapvanEkris
Copy link
Owner

JaapvanEkris commented Feb 20, 2026

I'm extremely reluctant to get a rolling median filter back. Especially since we have a debounce filter in place in pigpio to fix this specific issue. 0.5ms is something that filter can handle easily (set it to 750 ns).

Our design is based on accepting noise and treating datapoints as untrusted. The new cyclic error correction filter (0.9.7) also detects outliers and gives them a low weight, which works reasonably well on noisy machines.

This proposed aporoach essentially sets our approach back three years (to version 0.8.1) where noise prediction and rejection was a thing. It never worked well on scale.

I think our current approach is more robust.

@JaapvanEkris
Copy link
Owner

JaapvanEkris commented Feb 20, 2026

Yes this feature is already available. Not the 40% deviation though, but I personally dont really support this feature (on ESPRM I have something similar that can be enabled for extreme cases but it is known to cause some times more problems that solves).

In 0.8.1 we used a similar approach. We used prediction to accept/reject datapoints. The big issue is that configuration really becomes a thing: you get weird abstract parameters like maximum change percentage in currentDt, which works well until someone faster comes along and he knows how to increase a flywheel speed better. We abandoned it as it indeed creates much more problems than it solves. A couple of noisy datapoints and everything is all over the place, often permanently.

The main message in terms of denounce is that try to avoid solving a hardware issue with a software solution.

This, and the pigpio debounce filter is there for a reason as well. Beyond that the new CECFilter will reduce its weight if it is outside the normal ranges (worked well with your Kayak) and the weighted Regression will mostly ignore it. My tests with CECFilter show that this type of noise will still create some disturbance, but it will largely be ignored.

@cwklurks
Copy link
Author

That makes total sense, especially regarding the 'someone faster' edge case. I did a bit of assuming, but I completely understand why a hard software filter is dangerous here and why the engine relies on probabilistically weighting untrusted data points instead.

I will strip the GPIO debounce filter out of this branch entirely and leave only the FTMS control point fixes. I'll push the updated commit and adjust the PR title shortly (as I'm at school now).

@JaapvanEkris
Copy link
Owner

I will strip the GPIO debounce filter out of this branch entirely and leave only the FTMS control point fixes. I'll push the updated commit and adjust the PR title shortly (as I'm at school now).

Great idea!

@cwklurks
Copy link
Author

okay all done, debounce filter removed. the PR only contains the FTMS fixes. should be all good for review

@JaapvanEkris JaapvanEkris changed the base branch from main to 0.9.7-(under-construction) February 20, 2026 19:34
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove these changes. Deleting comments we need for maintenance is not a good way to go

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

im super sorry about that! The comment removal was an unintended side effect when I stripped out the debounce filter changes, and I thought I added those back in. I've restored all the comments in GpioTimerService.js. The file should now match the base branch exactly with no changes from this PR.

Comments were unintentionally removed when stripping the debounce
filter changes. File now matches the base branch with no modifications.
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.

4 participants