Skip to content

cpu/atmega_common: __exit_isr thread_yield #8988

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
kYc0o merged 6 commits into from
Jun 26, 2018
Merged

cpu/atmega_common: __exit_isr thread_yield #8988

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
80b02e5
cpu/atmega_common: exit_isr thread_yield
Josar Apr 25, 2018
9b63117
cpu/atmega_common: uncrustified
Josar Apr 25, 2018
fe92771
cpu/atmega_common/include/cpu.h: uncrustified
Josar Jun 8, 2018
0e49186
cpu/atmega_common/periph/timer.c: uncrustified
Josar Jun 8, 2018
40c1839
cpu/atmega_common/periph/uart.c: uncrustified
Josar Jun 8, 2018
9135963
cpu/atmega_common/thread_arch.c: uncrustified
Josar Jun 8, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 31 additions & 24 deletions cpu/atmega_common/include/cpu.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,15 +35,17 @@

#include <avr/interrupt.h>
#include "cpu_conf.h"

#include "sched.h"
#include "thread.h"
/**
* For downwards compatibility with old RIOT code.
* TODO: remove once core was adjusted
*/
#include "irq.h"

#ifdef __cplusplus
extern "C" {
extern "C"
{
#endif

/**
Expand All @@ -52,18 +54,30 @@ extern "C" {
extern volatile uint8_t __in_isr;

/**
* @brief Flag entering of an ISR
* @brief Run this code on entering interrupt routines
*/
static inline void __enter_isr(void)
{
__in_isr = 1;
}

/**
* @brief Flag exiting of an ISR
* @brief Exit ISR mode and yield with a return from interrupt. Use at the
* end of ISRs in place of thread_yield_higher. If thread_yield is needed, use
* thread_yield followed by thread_yield_isr instead of thread_yield alone.
*/
void thread_yield_isr(void);

/**
* @brief Run this code on exiting interrupt routines
*/
static inline void __exit_isr(void)
{
if (sched_context_switch_request) {
thread_yield();
__in_isr = 0;
thread_yield_isr();
}
__in_isr = 0;
}

Expand All @@ -75,20 +89,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)
{
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"

@kaspar030 kaspar030 Apr 23, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unrelated

"mov %0, __tmp_reg__ \n\t"
: "=g"(hi) );
: "=g" (hi));

__asm__ volatile( "in __tmp_reg__, __SP_L__ \n\t"
__asm__ volatile ("in __tmp_reg__, __SP_L__ \n\t"
"mov %0, __tmp_reg__ \n\t"
: "=g"(lo) );
ptr = hi<<8 | lo;
: "=g" (lo));
ptr = hi << 8 | lo;
printf("Stack Pointer: 0x%04x\n", ptr);
}

Expand All @@ -98,13 +112,13 @@ __attribute__((always_inline)) static inline void cpu_print_last_instruction(voi
* Some CPUs may not support the highest prescaler settings
*/
enum {
CPU_ATMEGA_CLK_SCALE_DIV1 = 0,
CPU_ATMEGA_CLK_SCALE_DIV2 = 1,
CPU_ATMEGA_CLK_SCALE_DIV4 = 2,
CPU_ATMEGA_CLK_SCALE_DIV8 = 3,
CPU_ATMEGA_CLK_SCALE_DIV16 = 4,
CPU_ATMEGA_CLK_SCALE_DIV32 = 5,
CPU_ATMEGA_CLK_SCALE_DIV64 = 6,
CPU_ATMEGA_CLK_SCALE_DIV1 = 0,
CPU_ATMEGA_CLK_SCALE_DIV2 = 1,
CPU_ATMEGA_CLK_SCALE_DIV4 = 2,
CPU_ATMEGA_CLK_SCALE_DIV8 = 3,
CPU_ATMEGA_CLK_SCALE_DIV16 = 4,
CPU_ATMEGA_CLK_SCALE_DIV32 = 5,
CPU_ATMEGA_CLK_SCALE_DIV64 = 6,
CPU_ATMEGA_CLK_SCALE_DIV128 = 7,
CPU_ATMEGA_CLK_SCALE_DIV256 = 8,
CPU_ATMEGA_CLK_SCALE_DIV512 = 9,
Expand All @@ -128,13 +142,6 @@ static inline void atmega_set_prescaler(uint8_t clk_scale)
*/
void atmega_stdio_init(void);

/**
* @brief Exit ISR mode and yield with a return from interrupt. Use at the
* end of ISRs in place of thread_yield_higher. If thread_yield is needed, use
* thread_yield followed by thread_yield_isr instead of thread_yield alone.
*/
void thread_yield_isr(void);

#ifdef __cplusplus
}
#endif
Expand Down
39 changes: 17 additions & 22 deletions cpu/atmega_common/periph/timer.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,18 +29,18 @@
#include "periph/timer.h"
#include "periph_conf.h"

#define ENABLE_DEBUG (0)
#define ENABLE_DEBUG (0)
#include "debug.h"

/**
* @brief All timers have three channels
*/
#define CHANNELS (3)
#define CHANNELS (3)

/**
* @brief We have 5 possible prescaler values
*/
#define PRESCALE_NUMOF (5U)
#define PRESCALE_NUMOF (5U)

/**
* @brief Possible prescaler values, encoded as 2 ^ val
Expand All @@ -51,13 +51,13 @@ static const uint8_t prescalers[] = { 0, 3, 6, 8, 10 };
* @brief Timer state context
*/
typedef struct {
mega_timer_t *dev; /**< timer device */
volatile uint8_t *mask; /**< address of interrupt mask register */
volatile uint8_t *flag; /**< address of interrupt flag register */
timer_cb_t cb; /**< interrupt callback */
void *arg; /**< interrupt callback argument */
uint8_t mode; /**< remember the configured mode */
uint8_t isrs; /**< remember the interrupt state */
mega_timer_t *dev; /**< timer device */
volatile uint8_t *mask; /**< address of interrupt mask register */
volatile uint8_t *flag; /**< address of interrupt flag register */
timer_cb_t cb; /**< interrupt callback */
void *arg; /**< interrupt callback argument */
uint8_t mode; /**< remember the configured mode */
uint8_t isrs; /**< remember the interrupt state */
} ctx_t;

/**
Expand Down Expand Up @@ -109,8 +109,8 @@ int timer_init(tim_t tim, unsigned long freq, timer_cb_t cb, void *arg)
ctx[tim].dev->CNT = 0;

/* save interrupt context and timer mode */
ctx[tim].cb = cb;
ctx[tim].arg = arg;
ctx[tim].cb = cb;
ctx[tim].arg = arg;
ctx[tim].mode = (pre + 1);

/* enable timer with calculated prescaler */
Expand All @@ -128,7 +128,7 @@ int timer_set_absolute(tim_t tim, int channel, unsigned int value)

ctx[tim].dev->OCR[channel] = (uint16_t)value;
*ctx[tim].flag &= ~(1 << (channel + OCF1A));
*ctx[tim].mask |= (1 << (channel + OCIE1A));
*ctx[tim].mask |= (1 << (channel + OCIE1A));

return 1;
}
Expand Down Expand Up @@ -167,11 +167,6 @@ static inline void _isr(tim_t tim, int chan)
*ctx[tim].mask &= ~(1 << (chan + OCIE1A));
ctx[tim].cb(ctx[tim].arg, chan);

if (sched_context_switch_request) {
thread_yield();
thread_yield_isr();
}

__exit_isr();
}
#endif
Expand All @@ -192,8 +187,8 @@ ISR(TIMER_0_ISRC, ISR_BLOCK)
{
_isr(0, 2);
}
#endif /* TIMER_0_ISRC */
#endif /* TIMER_0 */
#endif /* TIMER_0_ISRC */
#endif /* TIMER_0 */

#ifdef TIMER_1
ISR(TIMER_1_ISRA, ISR_BLOCK)
Expand All @@ -211,8 +206,8 @@ ISR(TIMER_1_ISRC, ISR_BLOCK)
{
_isr(1, 2);
}
#endif /* TIMER_1_ISRC */
#endif /* TIMER_1 */
#endif /* TIMER_1_ISRC */
#endif /* TIMER_1 */

#ifdef TIMER_2
ISR(TIMER_2_ISRA, ISR_BLOCK)
Expand Down
22 changes: 6 additions & 16 deletions cpu/atmega_common/periph/uart.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@

#include "periph/uart.h"


/**
* @brief Maximum percentage error in calculated baud before switching to
* double speed transmission (U2X)
Expand Down Expand Up @@ -90,6 +89,7 @@ static void _update_brr(uart_t uart, uint16_t brr, bool double_speed)
static void _set_brr(uart_t uart, uint32_t baudrate)
{
uint16_t brr;

#if defined(UART_STDIO_BAUDRATE)
/* UBRR_VALUE and USE_2X are statically computed from <util/setbaud.h> */
if (baudrate == UART_STDIO_BAUDRATE) {
Expand All @@ -115,7 +115,7 @@ int uart_init(uart_t uart, uint32_t baudrate, uart_rx_cb_t rx_cb, void *arg)

/* register interrupt context */
isr_ctx[uart].rx_cb = rx_cb;
isr_ctx[uart].arg = arg;
isr_ctx[uart].arg = arg;

/* disable and reset UART */
dev[uart]->CSRB = 0;
Expand All @@ -134,14 +134,13 @@ int uart_init(uart_t uart, uint32_t baudrate, uart_rx_cb_t rx_cb, void *arg)
dev[uart]->CSRB = (1 << TXEN0);
}


return UART_OK;
}

void uart_write(uart_t uart, const uint8_t *data, size_t len)
{
for (size_t i = 0; i < len; i++) {
while (!(dev[uart]->CSRA & (1 << UDRE0))) {};
while (!(dev[uart]->CSRA & (1 << UDRE0))) {}
dev[uart]->DR = data[i];
}
}
Expand All @@ -160,46 +159,37 @@ void uart_poweroff(uart_t uart)

static inline void isr_handler(int num)
{
__enter_isr();

isr_ctx[num].rx_cb(isr_ctx[num].arg, dev[num]->DR);

if (sched_context_switch_request) {
thread_yield();
thread_yield_isr();
}
__exit_isr();
}

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

@jnohlgard jnohlgard Apr 20, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@Josar Josar Apr 20, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@jnohlgard jnohlgard Apr 20, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@kaspar030 kaspar030 Apr 23, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@Josar Josar Apr 24, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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);
}

@jnohlgard jnohlgard Apr 25, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

isr_handler(0);
__exit_isr();
}
#endif /* UART_0_ISR */

#ifdef UART_1_ISR
ISR(UART_1_ISR, ISR_BLOCK)
{
__enter_isr();
isr_handler(1);
__exit_isr();
}
#endif /* UART_1_ISR */

#ifdef UART_2_ISR
ISR(UART_2_ISR, ISR_BLOCK)
{
__enter_isr();
isr_handler(2);
__exit_isr();
}
#endif /* UART_2_ISR */

#ifdef UART_3_ISR
ISR(UART_3_ISR, ISR_BLOCK)
{
__enter_isr();
isr_handler(3);
__exit_isr();
}
#endif /* UART_3_ISR */
Loading