cpu/atmega_common: __exit_isr thread_yield #8988

Josar · 2018-04-19T21:48:29Z

As proposed by @gebart in #8979 this PR adds thread_yield(); to __exit_isr similar to #6171.

This avoids duplicated code.
Additionaly code was uncrustified.

jnohlgard

Good improvement! For consistency, I would prefer to let the enter/exit isr calls in the UART driver to remain in each ISR instead of the central IRQ handler.

jnohlgard · 2018-04-20T03:46:13Z

cpu/atmega_common/periph/uart.c

 }

 #ifdef UART_0_ISR
 ISR(UART_0_ISR, ISR_BLOCK)
 {
-    __enter_isr();


I prefer the original design where enter isr is the first line of each ISR

Is this just a preference or are there any reasons. As i see there is no benefit in settings the __in_isr flag as early as possible, as there is no chance that code could be invoked which could use this information.
Setting the flag prevents code in the callback to sleep or do other things which schould not be done in interrupts, so setting it before the callback should be sufficient, imho.

I change it as it is like this in all other interrupt handler and thought it would also increase consistency.

Personal preference. The consistency helps when reviewing the code and also reduces the risk of missing it if someone uses this periph driver as a template for some other driver for the same CPU.

@gebart maybe we just have a language problem as consistency translates into two different meanings in german, depending of the context. Do you mean consistency in regard as it stays as it was or consistency as it is similar to other code of the atmegas?

RIOT/cpu/atmega_common/periph/gpio.c

Lines 264 to 274 in 22aa6b9

static inline void irq_handler(uint8_t pin_num)

{

__enter_isr();

config[pin_num].cb(config[pin_num].arg);

__exit_isr();

}

ISR(INT0_vect, ISR_BLOCK)

{

irq_handler(0); /**< predefined interrupt pin */

}

RIOT/cpu/atmega_common/periph/timer.c

Lines 169 to 189 in 22aa6b9

#ifdef TIMER_NUMOF

static inline void _isr(tim_t tim, int chan)

{

__enter_isr();

*ctx[tim].mask &= ~(1 << (chan + OCIE1A));

ctx[tim].cb(ctx[tim].arg, chan);

if (sched_context_switch_request) {

thread_yield();

}

__exit_isr();

}

#endif

#ifdef TIMER_0

ISR(TIMER_0_ISRA, ISR_BLOCK)

{

_isr(0, 0);

}

I guess you are right regarding consistency. I am not very engaged in the atmega development, sorry.
What I was trying to push for is to have a strict template for the ISR definitions which should be the same across all drivers for the CPU family, which you showed that this is. Consistency makes refactoring the driver code easier in the future. I am happy as long as this design matches the other periph drivers for the atmega.

kaspar030

Please split out the uncrustify commit.

kaspar030 · 2018-04-23T08:46:08Z

cpu/atmega_common/include/cpu.h

@@ -75,20 +81,20 @@ void cpu_init(void);
 /**
 * @brief   Print the last instruction's address
 */
-__attribute__((always_inline)) static inline void cpu_print_last_instruction(void)
+ static inline void __attribute__((always_inline)) cpu_print_last_instruction(void)


kaspar030 · 2018-04-23T08:46:19Z

cpu/atmega_common/include/cpu.h

 {
    uint8_t hi;
    uint8_t lo;
    uint16_t ptr;

-    __asm__ volatile( "in __tmp_reg__, __SP_H__  \n\t"
+    __asm__ volatile ("in __tmp_reg__, __SP_H__  \n\t"


jnohlgard · 2018-04-25T06:52:16Z

cpu/atmega_common/include/cpu.h

@@ -34,8 +34,10 @@
 #include <stdint.h>

 #include <avr/interrupt.h>
+#include "board.h"


is this include actually used?

jnohlgard · 2018-04-25T06:54:26Z

My concerns have been addressed.

Josar · 2018-04-25T16:14:01Z

@kaspar030 can you approve it like this?

Josar · 2018-04-27T10:35:07Z

@kaspar030 ping 😏

kaspar030 · 2018-04-27T10:41:48Z

cpu/atmega_common/include/cpu.h

 */
-static inline void __exit_isr(void)
+static inline void __attribute__((always_inline)) __exit_isr(void)


why always inline?

As it will be executed in an ISR, i thought speed could matter.
And if optimization is off it won't

GCC does not inline any functions when not optimizing unless you specify the ‘always_inline’ attribute for the function, like this:

/* Prototype. */ inline void foo (const char) __attribute__((always_inline));

But as the statement is the attribute has to be after the function, i am not sure but, i think it does not matter.

This benchmark makes me think maybe inline only is better.

kaspar030 · 2018-04-27T10:42:15Z

cpu/atmega_common/include/cpu.h

 */
-static inline void __enter_isr(void)
+static inline void __attribute__((always_inline)) __enter_isr(void)


why always inline? Don't trust the compiler?

Josar · 2018-05-07T11:32:43Z

@gebart and @kaspar030 rebased onto #8904 and added neccessary changes.

ZetaR60 · 2018-05-07T15:47:49Z

This looks okay to me, but note that it introduces a yield in the periph/gpio.c ISR where there previously was none.

Josar · 2018-05-07T23:25:22Z

@ZetaR60 i think that is not fully correct, it request a thread yield if there was a sched_context_switch_request in the callback handler. Which i would expect to happen, as the __in_isr blocks the context switch and at the end of an isr it has to be executed. Otherwise it would be lost or executed with an other thread_yield.

But thinking further is it neccessary to duplicate the code?

static inline void __exit_isr(void)
{
     if (sched_context_switch_request) {
         thread_yield();
        thread_yield_isr();
     }
   __in_isr = 0;
}

thread_yield_isr(); is the same as thread_yield_higher(); so just bringing __in_isr = 0 before the if and setting reti to the end will have the same result with less code, or not ?

static inline void __exit_isr(void)
{
     __in_isr = 0;
     if (sched_context_switch_request) {
         thread_yield();
     }
    __asm__ volatile ("reti");
}

ZetaR60 · 2018-05-08T01:12:07Z

Yes, you are right about my comment being erroneous. Sorry for the lazy thought process.

There are problems with moving __asm__ volatile("reti"); to a different area from __context_restore(); because the context change directly manipulates the program counter, and returning from a function also manipulates the program counter. You would have to add __attribute__((always_inline)) inline to thread_yield / thread_yield_isr, which means you would have a copy of all the contents of __context_save and __context_restore in every ISR.

Josar · 2018-05-08T11:12:12Z

@ZetaR60 seems it was too late for my brain, of course moving reti does not work.

Maybe we could agree on just putting the content of the if to thread_yield_isr();` ?

I don`t imagine a situation where saving the context and running the scheduler is necessary if there was not a thread change request.

Further thoughts, I think there should be a function like sched_thread_change_needed to avoid saving and restoring the context when a yield or yield higher is issued but there is no higher.
As of now sched_run checks this, after the context was allready saved.

ZetaR60 · 2018-05-08T15:39:57Z

@Josar can you be a bit more precise which things you are referring to? Currently nothing is run at the end of an ISR if there was no attempt to yield within the callback, because sched_context_switch_request is not set. Are the other things you are mentioning proposed changes to core/thread.c ? Currently this PR is a refactor; perhaps discussion of changes of behavior should be moved to another PR specific to that change? There might be some improvements to be made in the way I rewrote context swapping, but that rewrite took a lot of testing to do properly.

Josar · 2018-05-08T20:23:58Z

@ZetaR60 yes this PR should just be an refactorization.

thing i refer is changing the code to this

void thread_yield_isr(void) {
    if (sched_context_switch_request) {
        thread_yield();
         __context_save();
         sched_run();
        __context_restore();
    }
    __asm__ volatile("reti");
}

static inline void __exit_isr(void)
{
    thread_yield_isr();
     __in_isr = 0;
}

thing is just an idea which could be a improvement.
Now when a tread issues a thread_yield_higher e.g. when sending a msg, but the running thread has a higher priority then the thread receiving the msg, this will cause a context save, as the check if the context has to be change is done in sched_run and then the restore the same thread. this could be prevented, by first checking if the context has to be changed, then save, change, restore.

ZetaR60 · 2018-05-08T21:15:29Z

Those sound like reasonable changes. They would involve changing behavior though, so the extensive testing done in #8904 probably ought to be re-checked in a PR that changes it.

Josar · 2018-05-15T11:34:35Z

@ZetaR60 thinking further i think this can be optimized.

After deciding that a context swap is allowed in an ISR there is no need of preventing an context swap in an ISR, the only thing which has to be taken care of is to return with reti. The rest are remains which only add overhead.

As it is now we check for isr set the flag, return to caller, call next function, check for flag and do the swap.
Removing this will not only encrease swapping in interrupts but also increase interrupts with no yield as the flag check is removed.

void thread_yield_higher(void) {
        __context_save();
        sched_run();
        __context_restore();

    if (irq_is_in() == 0) {
        __asm__ volatile("ret");
    } else {
        __exit_isr();
        __asm__ volatile ("reti");
    }
}

and removing the if irq in the scheduler

    if (!on_runqueue || (current_prio > other_prio)) {
        // if (irq_is_in()) {
        //     DEBUG("sched_switch: setting sched_context_switch_request.\n");
        //     sched_context_switch_request = 1;
        // }
        // else {
            DEBUG("sched_switch: yielding immediately.\n");
            thread_yield_higher();
        // }
    }
    else {
        DEBUG("sched_switch: continuing without yield.\n");
    }

first test shows expected results.

test/xtimer_msg ok
sched_testing ok
mutex_order ok
posix_semaphore still falis test 4
thread_flags seems ok

The question is are there platforms which can not switch the context in an ISR?

Josar · 2018-05-15T13:34:57Z

@kYc0o would you mind having a look at this?

ZetaR60 · 2018-05-15T18:28:53Z

@Josar This change to thread_yield_higher is a pretty major change in behavior. Could you create another PR for it?

Josar · 2018-05-29T11:44:02Z

@kaspar030 maybe you could find some time?

kYc0o · 2018-05-29T14:28:43Z

Code wise looks OK. there are some "unrelated" changes due to uncrustify, but I guess they're valid. I need to test though. Will report back when I test on my available boards. @ZetaR60 can you also test on the mega-xplained? I guess timers and peripherals basic testing is enough.

ZetaR60 · 2018-05-29T17:38:47Z

I ran some tests, and got a couple failures:

thread_basic			OK
thread_cooperation		OK
thread_exit			OK
thread_flags			OK
thread_flags_xtimer		Reports failure
thread_flood			OK
thread_msg			OK
thread_msg_block_wo_queue	OK
thread_msg_block_w_queue	OK
thread_msg_seq			OK
xtimer_hang			OK
xtimer_periodic_wakeup		OK
xtimer_remove			Reports failure
xtimer_reset			OK
xtimer_usleep			OK
xtimer_usleep_short		OK

I think we need a bit more extensive examination. @Josar I suggest rebasing on trunk and running tests side by side with your code and the exact same revision without your code.

Josar · 2018-05-29T22:59:49Z

Rebased and fixed.
All above Tests run successfull.
Besides that the output of xtimer_hang stutters, which is solved in #9211

PS: it would be nice to have a readme in each test with the expected output.

ZetaR60 · 2018-05-30T04:11:53Z

Confirmed that the previously failed tests are now succeeding on mega-xplained. Also did another proofread of the changes, and they look fine. Let me know if I should run any other tests on mega-xplained.

Josar · 2018-06-04T13:50:54Z

@kaspar030 any further remarks?

Josar · 2018-06-07T14:58:20Z

@kaspar030 could you have a second look?

ZetaR60 · 2018-06-07T22:57:17Z

@Josar You are being a bit aggressive with pinging Kaspar. I know things often move slowly, but I don't think repeating requests that much is going to help.

Josar · 2018-06-08T09:23:14Z

Thank you @ZetaR60 for indicating this. Not ment to be agressiv at all.
I just tried to follow the advise in the wiki to remind the reviewer if there is no reaction for some days.

kYc0o · 2018-06-08T14:17:46Z

@Josar please squash.

kYc0o · 2018-06-08T15:19:14Z

All the tests in #8988 (comment) passed on arduino-mega2560.

Josar · 2018-06-08T15:56:09Z

I uncrustified the touched files, hope this is allright.

aabadie · 2018-06-08T20:31:45Z

cpu/atmega_common/periph/timer.c

@@ -87,17 +88,21 @@ int timer_init(tim_t tim, unsigned long freq, timer_cb_t cb, void *arg)
    uint8_t pre = 0;

    /* make sure given device is valid */
-    if (tim >= TIMER_NUMOF) {
+    if (tim >= TIMER_NUMOF)
+    {


Is this change the result of uncrustify ? Then I think there's an issue with the uncrustify configuration file. The opening curly brace should be on the same line as the if.

Seems my vscode config got messed up.

Josar · 2018-06-15T08:30:46Z

@aabadie i think that the uncrustifying went right this time.
@kYc0o rebased and squashed.

kYc0o · 2018-06-26T13:12:30Z

Perfect! Thanks for your contribution!

Josar force-pushed the atmega_isr branch 2 times, most recently from a0a1515 to 0087952 Compare April 19, 2018 21:59

Josar changed the title ~~cpu/atmega_common: uncrustified~~ cpu/atmega_common: __exit_isr thread_yield Apr 20, 2018

jnohlgard requested changes Apr 20, 2018

View reviewed changes

kYc0o self-assigned this Apr 20, 2018

kYc0o added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Platform: AVR Platform: This PR/issue effects AVR-based platforms labels Apr 20, 2018

kYc0o added this to the Release 2018.07 milestone Apr 20, 2018

kaspar030 requested changes Apr 23, 2018

View reviewed changes

jnohlgard reviewed Apr 25, 2018

View reviewed changes

jnohlgard approved these changes Apr 25, 2018

View reviewed changes

Josar force-pushed the atmega_isr branch from 0087952 to 2b90dd4 Compare April 25, 2018 15:56

kaspar030 reviewed Apr 27, 2018

View reviewed changes

Josar force-pushed the atmega_isr branch from 2b90dd4 to 07058f6 Compare May 7, 2018 11:28

Josar mentioned this pull request May 15, 2018

Interrupt context switch #9142

Closed

Josar force-pushed the atmega_isr branch from 5843d71 to d41cc36 Compare May 29, 2018 22:57

kYc0o added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label May 30, 2018

kaspar030 approved these changes Jun 8, 2018

View reviewed changes

Josar force-pushed the atmega_isr branch 2 times, most recently from c30bb2c to a8f37f4 Compare June 8, 2018 15:54

aabadie reviewed Jun 8, 2018

View reviewed changes

Josar force-pushed the atmega_isr branch from f41055c to e65590d Compare June 9, 2018 09:30

Josar added 6 commits June 14, 2018 21:47

cpu/atmega_common: exit_isr thread_yield

80b02e5

cpu/atmega_common: uncrustified

9b63117

cpu/atmega_common/include/cpu.h: uncrustified

fe92771

cpu/atmega_common/periph/timer.c: uncrustified

0e49186

cpu/atmega_common/periph/uart.c: uncrustified

40c1839

cpu/atmega_common/thread_arch.c: uncrustified

9135963

Josar force-pushed the atmega_isr branch from e65590d to 9135963 Compare June 14, 2018 19:48

kYc0o merged commit f0dce1b into RIOT-OS:master Jun 26, 2018

Josar mentioned this pull request Jul 3, 2018

cpu/atmega_common: RTT and RTC support #8842

Closed

	static inline void irq_handler(uint8_t pin_num)
	{
	__enter_isr();
	config[pin_num].cb(config[pin_num].arg);
	__exit_isr();
	}

	ISR(INT0_vect, ISR_BLOCK)
	{
	irq_handler(0); /*< predefined interrupt pin /
	}

	#ifdef TIMER_NUMOF
	static inline void _isr(tim_t tim, int chan)
	{
	__enter_isr();

	*ctx[tim].mask &= ~(1 << (chan + OCIE1A));
	ctx[tim].cb(ctx[tim].arg, chan);

	if (sched_context_switch_request) {
	thread_yield();
	}

	__exit_isr();
	}
	#endif

	#ifdef TIMER_0
	ISR(TIMER_0_ISRA, ISR_BLOCK)
	{
	_isr(0, 0);
	}

cpu/atmega_common: __exit_isr thread_yield #8988

cpu/atmega_common: __exit_isr thread_yield #8988

Conversation

Josar commented Apr 19, 2018

jnohlgard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Josar Apr 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaspar030 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jnohlgard commented Apr 25, 2018

Josar commented Apr 25, 2018

Josar commented Apr 27, 2018

Choose a reason for hiding this comment

Josar Apr 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Josar commented May 7, 2018

ZetaR60 commented May 7, 2018

Josar commented May 7, 2018

ZetaR60 commented May 8, 2018

Josar commented May 8, 2018

ZetaR60 commented May 8, 2018

Josar commented May 8, 2018

ZetaR60 commented May 8, 2018

Josar commented May 15, 2018 • edited Loading

Josar commented May 15, 2018

ZetaR60 commented May 15, 2018

Josar commented May 29, 2018

kYc0o commented May 29, 2018

ZetaR60 commented May 29, 2018

Josar commented May 29, 2018 • edited Loading

ZetaR60 commented May 30, 2018

Josar commented Jun 4, 2018

Josar commented Jun 7, 2018

ZetaR60 commented Jun 7, 2018

Josar commented Jun 8, 2018

kYc0o commented Jun 8, 2018

kYc0o commented Jun 8, 2018

Josar commented Jun 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Josar commented Jun 15, 2018

kYc0o commented Jun 26, 2018

Josar Apr 24, 2018 •

edited

Loading

Josar Apr 27, 2018 •

edited

Loading

Josar commented May 15, 2018 •

edited

Loading

Josar commented May 29, 2018 •

edited

Loading