Skip to content

Protect pulse generation timing by disabling interrupts for longer - #27131

Merged
thinkyhead merged 2 commits into
MarlinFirmware:bugfix-2.1.xfrom
mh-dm:pulse-timing
Dec 9, 2024
Merged

Protect pulse generation timing by disabling interrupts for longer#27131
thinkyhead merged 2 commits into
MarlinFirmware:bugfix-2.1.xfrom
mh-dm:pulse-timing

Conversation

@mh-dm

@mh-dm mh-dm commented May 27, 2024

Copy link
Copy Markdown
Contributor

Description

Protect pulse generation timing by disabling interrupts within Stepper::isr() for longer.

This is a mitigation for the stepper isr jitter seen in #27115 and other sources of stepper isr jitter on other platforms. On microcontrollers with ISR prioritization, note that only higher priority ISRs can ever interrupt the stepper ISR.

Benefits

More consistent pulse timings. I've tested on LPC176x and here's some before/after logic analyzer captures:

Before:
jitter-interrupt-before

After (edit, recaptured due to forgotten local changes):
jitter-interrupt-after

Basically no improvements in actual printing quality/noise unless ADAPTIVE_STEP_SMOOTHING is enabled since ADAPTIVE_STEP_SMOOTHING reduces a larger source of speed jitter, namely bresenham aliasing for the minor axes.

Related Issues

Mostly mitigates #27115

@rondlh

rondlh commented Dec 9, 2024

Copy link
Copy Markdown
Contributor

Would this affect "INTERRUPT_FEATURE", causing inaccurate probing?

I just did a quick repeatability test, 6x M48 for each case, and found no negative impact on probing accuracy (STM32F407 motherboard).

@thinkyhead

thinkyhead commented Dec 9, 2024

Copy link
Copy Markdown
Member

I believe I resolved the merge conflict correctly. Anything that improves on timing is welcome. This should not negatively impact any of the main loop and UI processes. And it seems to cope with the possibility of affecting UART interrupts. Does it also fully resolved the linked bug report?

@thinkyhead
thinkyhead merged commit 5d591fd into MarlinFirmware:bugfix-2.1.x Dec 9, 2024
@mh-dm

mh-dm commented Dec 9, 2024

Copy link
Copy Markdown
Contributor Author

Would this affect "INTERRUPT_FEATURE", causing inaccurate probing?

It won't affect it negatively. Pulse generation is really really fast and the code change is for the most time critical section at the start of the stepper isr. If the endstop/probe interrupt triggers while the stepper isr is just starting it will get delayed by at most 1 pulse/microstep (2.5 microns at the common 400steps/mm in Z). Worrying about that is moot since if micron accuracy matters then probing is done slowly - couple mm/s or ~1 microstep per millisecond whereas pulse generation is orders of magnitude faster. The probing is done slowly not due to processing latency but due to mechanical "settling" time - in particular vibrations that can affect probing by much more than 2.5 microns.

Actually, better pulse timing means slightly smoother motion thus less vibration so in theory we could probe faster at same accuracy? :)

I just did a quick repeatability test, 6x M48 for each case, and found no negative impact on probing accuracy (STM32F407 motherboard).

Thank you for testing!

I believe I resolved the merge conflict correctly

Looks good to me.

Does it also fully resolved the linked bug report?

Not fully but it does mitigate it. I think we can call it good enough for now.

@mh-dm
mh-dm deleted the pulse-timing branch December 9, 2024 13:51
EvilGremlin pushed a commit to EvilGremlin/Marlin that referenced this pull request May 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] SysTick_Callback interferes with timing critical code - causes speed jitter - LPC176x

4 participants