feilipu / Arduino_FreeRTOS_Library

A FreeRTOS Library for all Arduino ATmega Devices (Uno R3, Leonardo, Mega, etc).
MIT License
848 stars 205 forks source link

What's the correct way to yield from ISR here? #28

Closed Floessie closed 4 years ago

Floessie commented 6 years ago

Hello Phillip,

The *GiveFromISR() functions have a pxHigherPriorityTaskWoken parameter. This can be optionally NULL, but the examples make use of it, stating the actual macro used here is port specific.

What's the right macro to use in this AVR port from your point of view? And if it's a macro (and not a function), can it be handled port agnostic using some #ifdefs?

Thanks, Flössie

feilipu commented 6 years ago

I think that a task context switch with a reti instruction terminating is required. I'll have to look into this at some stage. I don't have an immediate answer.

Some discussion here.

Floessie commented 6 years ago

Hi Phillip,

Thanks for looking into it!

Best, Flössie

Floessie commented 6 years ago

So this basically means, we can use vPortYield() here?

Floessie commented 6 years ago

Yet another hint in that direction.

feilipu commented 6 years ago

I think the right answer is to know the preamble for entering an ISR, and then reverse it after doing a context switch.

Since we're calling the ISR with __attribute__ ((naked)) there is no avr-libc preamble to worry about, just the return address for the call to the ISR is pushed onto the stack. We just have to unroll our own preamble and return address. That isn't calling vPortYield() since that will roll another context save on top of the return address.

I think (untested) that we just need to do the context switch, and then remember to unroll the ISR context save with a context restore. The issue is how the restore context happens. It may need to be before the context switch, or the ISR may never get re-enabled, and we're stuck in it forever.

The context switch will require the old context to be saved in the task control block, including the initial address (the return address found on the stack under the ISR context save). And push the new return address (being the last program counter or address run for the switched in task) Need to think more about it...

This discussion from avr freaks yields context.

Floessie commented 6 years ago

Any outcome? I searched the Internet once more but found nothing new...

feilipu commented 6 years ago

Need some time to think, code, and test, which is lacking currently.

The thing I’m most concerned about is the return address pushed by the ISR when it is entered, and where it has to end up if the ISR exits to a different address.

Floessie commented 6 years ago

Coincidentally, I just found this explanation from Richard, dating back to 2005 but nevertheless. Maybe it's helpful. (I mean the long explanation somewhere near the end of the page.)

Best, Flössie

feilipu commented 6 years ago

Richard’s 14 June comment is detailed. And comprehensive. But, I don’t fully understand it yet.

Will need to draw some stacks to know what to do, and it will take some time. But the fact program returns to within the ISR makes handling the stacked return address easy.

feilipu commented 6 years ago

This is the sequence provided by Richard, and edited by me, for reference.

1) Task A is running when an interrupt occurs. The interrupt causes some CPU registers to be saved including the interrupt flags. Registers are saved so as to be restored, and a return address is pushed before by C compiler (C always uses call never jmp), which is popped back to the PC register by the RETI instruction. For the interrupt to have occurred the interrupt flags must have been set with interrupts enabled - but within the ISR they are disabled.

2) The ISR code executes. During the execution it is discovered that a yield is required. The yield does not occur however - just a flag is set to say one is necessary.

3) At the end of the interrupt the flag is checked which says a yield is required, and yield is called.

4) Yield saves the context of the task A - which has been interrupted. This would seem unnecessary, as the task A context is already saved. The registers are saved to the stack on top of the registers that were already pushed onto the stack when the ISR was entered. The program counter that is saved points to the end of the ISR function. So there are stacked PCs stored now. And stacked registers, as we saved the context of the ISR, even though it was finished, which interrupted task A. Ideally we just need to modify the PC saved for the task A context or is it just push the stacked PC to the task A TCB. Not sure.

5) Yield then switches the stack pointer to point to the saved context of task B - and proceeds to pop the registers from task B context. When all the general purpose registers have been popped the task B interrupt flags and return address are left on the stack. The flags are popped first. This will return the interrupt enable status to whatever it was when task B last ran (could be enabled or disabled). Finally RET is executed which causes the return address to be popped and the program counter set to the correct place to run task B.

6) At some point task B blocks. In our example assume this makes task A the candidate to run again. Having saved the task B context the context switch this time sets the stack pointer back to the saved context of task A (no the ISR status of the task A, as that is top of stack) - the task that was interrupted.

7) The general purpose registers for task A (in the ISR) are restored. Next the interrupt flags are restored. The interrupt flags were stored with interrupts disabled as they were saved from within the ISR. Finally the RET instruction is executed which sets the program counter back to the place where task A was last executing. In this case task A was last executing in the ISR. The next items on the stack are the registers pushed onto the stack by the processor when the interrupt occurred. [NB there may actually be some registers on the stack from the function prologue generated by the compiler, which will be popped by the epilogue generated by the compiler].

8) Task A returns to within the ISR. The ISR processing has already been performed but the ISR itself has not finished. The next instruction that task A executes is from the function epilogue - in a RETI. This pops the interrupt flags and return address from the stack - these are those placed there when the ISR was entered. The interrupt flags re-enables interrupts (remember they must have been enabled for the interrupt to get called at all) and returns task A to the place it was prior to it getting interrupt in the first place.

I would like to write the answer so that the program execution is not bounced back to the ISR, but rather the real task A SREG and return address replace the SREG and return address of the ISR. This should be quite easily possible, just by popping the real task A return address and pushing it back in the right sequence instead of the ISR program counter during the yield.

Floessie commented 6 years ago

I think, your edition above is correct. So I conclude the following:

Bottom line: Until those optimizations are in place, one can safely use taskYield() to yield from an ISR in this very port.

Thanks, Flössie

feilipu commented 6 years ago

What about those registers on stack generated by the prologue mentioned in 7.? Can they be ignored?

As the ISR created with the __attribute__((naked)) decoration, the only register that would be pushed is the PC at the time when the ISR interrupted normal program flow. And, this is the very register that we need to pop off and store with the task A context to avoid bouncing back via the ISR.

But, still we need to have a clear think about this, and check the asm code generated.

JoshuaKim6502 commented 5 years ago

Hi everyone, If the parameter xHigherPriorityTaskWoken in the function *GiveFromISR() results pdTRUE, it means that the *GiveFromISR() causes a task to leave the Blocked state, and the unblocked task has a higher priority than the interrupted task. In this case, ending the ISR with taskYIELD() function let the unblocked task run immediately after the ISR. Otherwise, the interrupted task resumes after the ISR, and then the unblocked task runs. Thanks a lot for Phillip’s latest update version 10.2.0-3, with which I have tested. It works fine as expected.

feilipu commented 4 years ago

I think this is now done, with an addition coming from the ATmegaxxxx effort.

Floessie commented 4 years ago

I'll adapt to portYIELD_FROM_ISR() ASAP. :+1:

feilipu commented 4 years ago

I should do one more alignment release, now that ATmegaxxxx has been merged. Still need to get portYIELD_FROM_ISR() into a release. EDIT Done.

Floessie commented 4 years ago

As portYIELD_FROM_ISR is a macro, I think I'll #ifdef it, so it doesn't matter if it's already present or not...

Thanks for you efforts! :+1: The new AVR128DA/DB series will be a perfect fit for FreeRTOS. Looking forward to Arduino and FreeRTOS support for it, which will surely need some time to mature.

Best, Flössie

feilipu commented 4 years ago

The AVR128DA28/AVR128DB28 is like someone put most of the Goldilocks Analogue features in one chip. Finally.

But, the integrated DAC is a bit weak, given what they could easily have done by integrating the MCP product range, now its even part of the same company.