[2.0.x] AVR preemptive interrupts, ARM interrupt priorities

[ejtagle]

Clean up preemptive interrupt emulation for AVR.

Also, simplify logic on all ARM based ones. Now, it is REQUIRED to properly configure interrupt priority. USART should have highest priority, followed by Stepper, then all others.

Note that this is theoretical code. I tried it in Arduino Due and works. AVR compiles but have no way to test on actual hardware.

STM32F1xx is using a very strange API to program timers, and is NOT setting the priority of the interrupts.

All the other targets seem to be properly setting priority of timers

The priorities should be set
1st: USART
2nd: Stepper / Limit switches
3rd all the rest (USB, Servos, etc)
last: Temperature

[xC0000005]

I tested this combined with my code to set the NVIC_PriorityGroup and proper priorities and the temps are stable, steppers do not appear to skip, though the proof will be in much longer prints.

[thinkyhead]

Looks good! And I love anything that reduces the code size and complexity…

[thinkyhead]

Please make your commit Messages short and to the point.
Place the longer explanation in the Description.
It takes extra time for the maintainers to fix them.

BAD:

FIXED:

[p3p]

USART should have highest priority, followed by Stepper, and then all the other ones

I thought serial should be in the low priority communications group, definitely not interrupting the stepper isr ..

[AnHardt]

I thought serial should be in the low priority communications group, definitely not interrupting the stepper isr ..

Do you remember the "serial hack", where we polled the serial line during the the stepper interrupt because it lasts longer than two consecutive chars can arrive at 250000 baud?

[ejtagle]

Yes @AnHardt: That is exactly the problem. If running at 250000 bauds, you end with 25000 interrupts per second. Certainly, the Stepper interrupt would block the reception of bytes.
On AVR, there is no way to fix that, except to poll the USART at the Stepper ISR (Ouch!!) or to allow the USART to preempt and interrupt the Stepper ISR.
On ARM the problem is mostly the same, but with some caveats...

It can be workarounded by using DMA with the USART, thus the RX buffer can be much longer and interrupt latency becomes a non issue
Or you can simply use the USB CDC ... Yes, the USB stack interrupt handling can be delayed with no issues at all :)
Depending on the CPU being used, DMA is easy or complex to get running. I was planning to add DMA capabilities to the Serial port of Due.

If you print from the SD, there is no problem at all :) (a very good reason to use it). also, if you print via the USB Native port there is no problem...

I am not completely happy about polling the USART on the Stepper ISR, as that creates extra dependencies between modules..

There is no easy way around the USART issue without having DMA working. There is always the risk of latency added to the stepper ISR at some point when the USART ISR starts executing

[p3p]

I'm going to be honest and say no, my assumption was that something as slow as USART as highest priority would cause to much jitter in pulses, I don't play with AVR much (at all) and didn't realise there was no buffer, I would usually use a half buffer interrupt (on LPC1768 there is a 16byte buffer)

[ejtagle]

@p3p : If such is the case on LPC, set the priority of the USART for that platform to a lower priority than the Steppers... I
We all know, eventually 32bit archs would start offering advantages compared to the poor AVR ... ;)

[thinkyhead]

What more needs to be done for the sake of STM32F1xx before this could be merged? Is the inability to set priorities a definite limitation, or is there some extra requirement for that architecture (such as needing to disable all interrupts before priorities can be set)?

I would usually use a half buffer interrupt

If there's some way to put the UART into a mode where it only interrupts when there's a definite need to prevent lost characters, that would be useful. Of course, on these faster processors the stepper ISR should be done a lot faster, so an interrupt in the middle of it will be less likely. And of course, the UART interrupt will also take up a lot less time… The main thing is, when the stepper ISR is running at top speed, we just don't want it monopolizing the MCU so that the UART cannot even get a character in edgewise.

[p3p]

@ejtagle I was just surprised at the order you specified, but it makes sense if there is no buffering available, I didn't write the hardware serial support for LPC1768 and just had a look to see no priority specified anyway .. I should probably look into that.

[xC0000005]

STM32F1 is no more broken if you merge now than it is before you merge. I’ll submit a PR that assigns the priorities and sets NVIC Grouping soon. I’m trying to avoid copying in a chunk of CMSIS header code that already exists for LPC1768.

…

On Apr 23, 2018, at 4:31 PM, Scott Lahteine ***@***.***> wrote: What more needs to be done for the sake of STM32F1xx before this could be merged? Is the failure to set priorities a definite limitation, or is there some extra requirement for that architecture (such as needing to disable all interrupts before priorities can be set)? I would usually use a half buffer interrupt If there's some way to put the UART into a mode where it only interrupts when there's a definite need to prevent lost characters, that would be useful. Of course, on these faster processors the stepper ISR should be done a lot faster, so an interrupt in the middle of it will be less likely. Of course, the UART interrupt will also take up a lot less time… The main thing is, when the stepper ISR is running at top speed, we just don't want it monopolizing the MCU so that the UART cannot even get a character in edgewise. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#10496 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/Aeppcam8zIyTXJwFGDopMB2hzJqIm-qbks5trmRfgaJpZM4TfQ6e>.

[p3p]

The main thing is, when the stepper ISR is running at top speed, we just don't want it monopolizing the MCU so that the UART cannot even get a character in edgewise.

Isn't that why we invented STEP_TIMER_MIN_INTERVAL, though people are always going to push the step rates past what the MCU can handle.

@xC0000005 I'm not sure you should try to use code from inside the CMSIS-LPC1768 framework, it's asking for problems, and I'm always researching ways to move away from that mess, to something newer and actually supported by nxp

[ejtagle]

@p3p If LPC1768/9 has a 16byte FIFO, or for any arch whose USART has a FIFO, and if that FIFO can be enabled, then there is no danger at all setting the USART interrupt priority lower than the Stepper.

A 16byte FIFO, or DMA with a 16byte buffer, reduces the interrupt rate to 25000/16 = 1565.5 interrupts per second. That is absolutely NOTHING for a 32 bit CPU.

If the stepper interrupt monopolizes the MCU for such long times, then we are probably trying to output more pulses per second than possible with that MCU

At that point, pulse timing is completely unreliable.

[ejtagle]

@thinkyhead : On ARM based MCUs, as there is preemption enabled, if the USART interrupt is set to a lower priority (because it has an internal FIFO...) the Stepper ISR will interrupt the USART interrupt, essentially giving no processing latency to the Stepper pulses, and thus perfect timing.
That is why if there is a way to enable RX FIFOS on any platform, it should be done 🥁

[p3p]

Can HAL_timer_isr_prologue and possibly HAL_timer_isr_epilogue (would need implemented) be used instead of the #ifndef CPU_32_BIT blocks? it hurts seeing abstractions broken.

[thinkyhead]

Can HAL_timer_isr_prologue and possibly HAL_timer_isr_epilogue (would need implemented) be used instead of the #ifndef CPU_32_BIT blocks…?

Done!

[thinkyhead]

@p3p, @ejtagle — I have a hard time imagining any case where this would be false right at the start of processing the ISR. Could it be that this flag is unnecessary?

[ejtagle]

There is a reason for that flag:
Imagine the following scenario:
The Temperature ISR was executing and the Stepper interrupts it: At that point, the temperature ISR is disabled (that is the way we prevent inmediate reentry as soon as we reenable global interrupts). When the Stepper ISR ends execution, the Temperature ISR must be kept disabled. Otherwise, as soon as the Stepper ISR reenables it, the Temperature ISR will be recalled recursively, without waiting for the previous Temperature ISR (that was interrupted by the Stepper ISR) to end.
We end with a recursive call, a stack overflow and a system crash.
So, yes, the exact sequence is extremely important here

[ejtagle]

We need the flag. Admittedly, if using C++, we could use a simple object to solve this problem. In the constructor we do the prologue, and in the destructor we do the epilogue
Also, the prologue and epilogue are not exactly the same for the Stepper and for the Temperature timers.
On the Stepper we disable both, temperature and Stepper ISRs on entry (to emulate priorization) and then reenable them (but temperature ISR is only enabled if it was enabled previously)
On the Temperature isr, we just disable temperature ISR, as said before, to prevent inmediate reentry as soon as we enable the global interrupt flags.
That avoids recursive calls of the ISRs.

[ejtagle]

If you want to hide it inside those macros, you don´t even need to use epilogue_0/epilogue_1 ... You can use several ifs or even a switch statement. GCC constant propagation will handle and inline only the appropiate to the timer being used

[thinkyhead]

I tend to prefer the macros because it ensures the bool flag only exists where it's needed. Other approaches either create an extra copy where it's not needed or force it to be defined elsewhere as a static global (e.g., Temperature::temp_isr_flag).

[ejtagle]

I have no problems with the macros at all ;)
It would be nice to do a custom ISR wrapper in assembly for AVR, so we can reduce interrupt processing latency as much as possible...

[Roxy-3D]

I'm not entirely convinced we need to run the serial at 250,000 baud. With the default Max7219 Debug LED's enabled... I can watch how many of the serial buffer slots are filled as I print. Pretty much, it stays pegged at the totally full mark. The tail pointer is always one behind the head pointer.

If we were to relax the baud rate (maybe even by 8x or 16x)... I don't think I would have a problem with the kind of printing I do. Maybe there is a need for the super high baud rate for some people and they way they slice their .STL files. But I don't think it matters for me.

Or maybe the baud rate isn't real anyway... Its an artifact of emulating serial over the USB bus. But the bottom line is the serial characters come in MUCH faster than I need to keep the printer busy.

More comments are welcome!

[ejtagle]

@Roxy-3D : The main problem is with AVR. As said before, on ARM there are very good solutions.

Use the USB native port (that emulates a serial port and whose interrupt has lower priority than Stepper)
Use the SD card
Use the USART, just assuming the platform has a FIFO so it can accept higher interrupt latencies.
If all the other things are not true, for AVR, let´s do some simple measurements and estimations:

Assume you have enabled both 3 ISRs (temperature/stepper/serial)

Let´s assume first the best scenario: No interrupt routine is executing... The time arrives to create a new step pulse. The Stepper isr launches. Let´s assume it does not reenable interrupts while executing. Then latency from the timer overflow to the pulse creation is none.

Now, the 2nd scenario: The USART Isr is running. And we enable global interrupts (something that this patch does NOT do). AVR must wait until the USART isr ends execution. By inspection of the assembly code, USART ISR can take 50 cycles to execute. At 16mhz, that means 3.12uS of delay from where pulses should be to where they are actually sent... Ouch!!

Assume we reenable global interrupts in the middle of the USART Isr. We can cut latency in half.

With the Temperature ISR is mostly the same: While AVR is serving that interrupt, the time from the start of the execution of the ISR to the place Interrupts are reenabled, is about 22 cycles, maybe more.

On AVR there is no way to reduce latency and/or pulse jitter except maybe disable all interrupts except the stepper ISR. That is the only way.

The only ways to REMOVE unpredictable latency without using polling for all the other things would be: Use the stepper ISR also to do Temperature measurements, servo, UARTs...

The way to minimize (but not reduce) latency would be to write a custom ASM wrapper for the ISR handlers, so we can reenable interrupts as soon as possible (could probably cut latency in half, but not remove it)

Its actually hard on AVR!!

And i also agree that there is no need to run USART at such high speeds, but running it slower just minimizes the problem, but does not completely solve it.