SysTick_Handler very likely to cause significant jitter in timing critical code

[mh-dm]

SysTick_Handler very likely to cause significant jitter in timing critical code.

cores/arduino/main.cpp currently implements the handler as follows:

 __attribute__ ((weak)) void SysTick_Callback() {}
// [..]
volatile uint64_t _millis;
// [..]
    NVIC_SetPriority(SysTick_IRQn, NVIC_EncodePriority(0, 0, 0));
// [..]
  void SysTick_Handler(void) {
    ++_millis;
    SysTick_Callback();
  }

0 is the highest priority level which means SysTick_Handler is going to run regardless of anyone. This is absolutely fine if it was just incrementing _millis. But it's also providing that callback - which is a big footgun. At minimum it should come with warning (ideally at compile time) that it's running at the highest priority and to not use it for basically anything because it will interfere with the timing of absolutely every other interrupt. (Even a function call is probably too much but that's beside the point.)

Now for the real life example - Marlin firmware. Looking at Marlin/src/HAL/LPC1768/HAL.cpp, this is the problematic SysTick_Callback:

void SysTick_Callback() { disk_timerproc(); }

Where disk_timerproc() is actually from framework-arduino-lpc176x/system/CMSIS/lib/chanfs/mmc_ssp.c:

/*-----------------------------------------------------------------------*/
/* Device timer function                                                 */
/*-----------------------------------------------------------------------*/
/* This function must be called from timer interrupt routine in period
/  of 1 ms to generate card control timing.
*/
void disk_timerproc (void)

Now disk_timerproc() really doesn't do much (a rewrite just using _millis could get rid of the need for it to exist it in the first place but that's beside the point).

Here's a visual of the issue, namely those 6000mm/s^2 or more acceleration spikes when moving at a leisurely ~100mm/s:

The spikes are there even after I increased the priority of the stepper interrupt to the highest (0).

And to confirm that it's SysTick_Callback() I've commented it out completely, including definition and use. This is the result:

(Which also makes me wonder why disk_timerproc() exists. Everything, including read write to SD card seems to works fine without it existing. But again, that's beside the point.)

I'm also opening an issue on Marlin side to purge SysTick_Callback calls to actually fix the jitter.

[p3p]

pretty sure disk_timerproc is used to update timers that are used throughout that library, for timeouts and such. also it used to detect card removal but that's disabled. not sure just removing the timeout on error featues is a good idea.

Moving it to ram along with the isr stub would probably help considerably, I presume most of the time is spent fetching the flash into cache (there is no prefetch or prediction)

[mh-dm]

I haven't looked into it much but can't the library call millis() to update its timers when it needs to? Like every other code that requires timekeeping.

[p3p]

the lib is not built on Arduino, the sys tick callback is just generally where you plug timing related callbacks in and it was left that way.

[p3p]

there is a section defined .ramcode in the linker script, that we could move the ISR and disk_timerproc into, this may fix the problem,

[mh-dm]

_millis is extern "C" so technically it could be used by that library. That said, on marlin side I think Marlin/issues/27115 should be a decent mitigation.