Skip to content

Commit 8e11ddf

Browse files
ccli8ccli8
committed
[Nuvoton] Fix trap in lp_ticker ISR with non-blocking "clear interrupt flag"
1 parent fe627cb commit 8e11ddf

File tree

4 files changed

+144
-124
lines changed

4 files changed

+144
-124
lines changed

‎targets/TARGET_NUVOTON/TARGET_M451/lp_ticker.c

Lines changed: 36 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -42,23 +42,14 @@ static const struct nu_modinit_s timer1_modinit = {TIMER_1, TMR1_MODULE, CLK_CLK
4242

4343
#define TIMER_MODINIT timer1_modinit
4444

45-
/* S/W interrupt enable/disable
45+
/* Timer interrupt enable/disable
4646
*
47-
* Because H/W interrupt enable/disable (TIMER_EnableInt/TIMER_DisableInt) needs delay for lp_ticker,
48-
* we introduce S/W interrupt enable/disable to avoid blocking code. With S/W interrupt enable/disable,
49-
* H/W interrupt is always enabled after ticker_init. A S/W flag is used to tell whether or not
50-
* ticker_irq_handler is ready to call.
47+
* Because Timer interrupt enable/disable (TIMER_EnableInt/TIMER_DisableInt) needs wait for lp_ticker,
48+
* we call NVIC_DisableIRQ/NVIC_EnableIRQ instead.
5149
*/
5250

53-
/* Ticker uninitialized */
54-
#define NU_TICKER_UNINIT 0
55-
/* Ticker initialized with interrupt disabled */
56-
#define NU_TICKER_INIT_INTR_DIS 1
57-
/* Ticker initialized with interrupt enabled */
58-
#define NU_TICKER_INIT_INTR_EN 2
59-
6051
/* Track ticker status */
61-
static volatile uint16_t ticker_stat = NU_TICKER_UNINIT;
52+
static volatile uint16_t ticker_inited = 0;
6253

6354
#define TMR_CMP_MIN 2
6455
#define TMR_CMP_MAX 0xFFFFFFu
@@ -68,14 +59,15 @@ static volatile uint16_t ticker_stat = NU_TICKER_UNINIT;
6859

6960
void lp_ticker_init(void)
7061
{
71-
if (ticker_stat) {
62+
if (ticker_inited) {
7263
/* By HAL spec, ticker_init allows the ticker to keep counting and disables the
7364
* ticker interrupt. */
7465
lp_ticker_disable_interrupt();
7566
lp_ticker_clear_interrupt();
67+
NVIC_ClearPendingIRQ(TIMER_MODINIT.irq_n);
7668
return;
7769
}
78-
ticker_stat = NU_TICKER_INIT_INTR_DIS;
70+
ticker_inited = 1;
7971

8072
// Reset module
8173
SYS_ResetModule(TIMER_MODINIT.rsetidx);
@@ -106,7 +98,7 @@ void lp_ticker_init(void)
10698
// Set vector
10799
NVIC_SetVector(TIMER_MODINIT.irq_n, (uint32_t) TIMER_MODINIT.var);
108100

109-
NVIC_EnableIRQ(TIMER_MODINIT.irq_n);
101+
NVIC_DisableIRQ(TIMER_MODINIT.irq_n);
110102

111103
TIMER_EnableInt(timer_base);
112104
wait_us((NU_US_PER_SEC / NU_TMRCLK_PER_SEC) * 3);
@@ -145,12 +137,12 @@ void lp_ticker_free(void)
145137
/* Disable IP clock */
146138
CLK_DisableModuleClock(TIMER_MODINIT.clkidx);
147139

148-
ticker_stat = NU_TICKER_UNINIT;
140+
ticker_inited = 0;
149141
}
150142

151143
timestamp_t lp_ticker_read()
152144
{
153-
if (ticker_stat == NU_TICKER_UNINIT) {
145+
if (! ticker_inited) {
154146
lp_ticker_init();
155147
}
156148

@@ -161,9 +153,6 @@ timestamp_t lp_ticker_read()
161153

162154
void lp_ticker_set_interrupt(timestamp_t timestamp)
163155
{
164-
/* We can call ticker_irq_handler now. */
165-
ticker_stat = NU_TICKER_INIT_INTR_EN;
166-
167156
/* In continuous mode, counter will be reset to zero with the following sequence:
168157
* 1. Stop counting
169158
* 2. Configure new CMP value
@@ -181,12 +170,15 @@ void lp_ticker_set_interrupt(timestamp_t timestamp)
181170

182171
/* NOTE: Rely on LPTICKER_DELAY_TICKS to be non-blocking. */
183172
timer_base->CMP = cmp_timer;
173+
174+
/* We can call ticker_irq_handler now. */
175+
NVIC_EnableIRQ(TIMER_MODINIT.irq_n);
184176
}
185177

186178
void lp_ticker_disable_interrupt(void)
187179
{
188180
/* We cannot call ticker_irq_handler now. */
189-
ticker_stat = NU_TICKER_INIT_INTR_DIS;
181+
NVIC_DisableIRQ(TIMER_MODINIT.irq_n);
190182
}
191183

192184
void lp_ticker_clear_interrupt(void)
@@ -203,12 +195,12 @@ void lp_ticker_clear_interrupt(void)
203195

204196
void lp_ticker_fire_interrupt(void)
205197
{
206-
/* We can call ticker_irq_handler now. */
207-
ticker_stat = NU_TICKER_INIT_INTR_EN;
208-
209198
// NOTE: This event was in the past. Set the interrupt as pending, but don't process it here.
210199
// This prevents a recursive loop under heavy load which can lead to a stack overflow.
211200
NVIC_SetPendingIRQ(TIMER_MODINIT.irq_n);
201+
202+
/* We can call ticker_irq_handler now. */
203+
NVIC_EnableIRQ(TIMER_MODINIT.irq_n);
212204
}
213205

214206
const ticker_info_t* lp_ticker_get_info()
@@ -222,15 +214,28 @@ const ticker_info_t* lp_ticker_get_info()
222214

223215
static void tmr1_vec(void)
224216
{
225-
/* NOTE: We need to clear interrupt flag earlier to reduce possibility of dummy interrupt.
226-
* This is because "clear interrupt flag" needs delay which isn't added here to avoid
227-
* blocking in ISR code. */
217+
/* NOTE: Avoid blocking in ISR due to wait for "clear interrupt flag"
218+
*
219+
* "clear interrupt flag" needs wait to take effect which isn't added here to avoid
220+
* blocking in ISR.
221+
*
222+
* Continuing above, we will get stuck in ISR due to dummy interrupt until
223+
* "clear interrupt flag" takes effect. To avoid it, we disable interrupt here and enable
224+
* interrupt in lp_ticker_fire_interrupt/lp_ticker_set_interrupt. There is another risk
225+
* that we may get stuck in a loop of ISR and lp_ticker_fire_interrupt/
226+
* lp_ticker_set_interrupt (called by lp_ticker_irq_handler), but actually we don't:
227+
* 1. When lp_ticker_fire_interrupt gets called, it means there is a past event and so this
228+
* interrupt isn't dummy.
229+
* 2. With LPTICKER_DELAY_TICKS enabled, it is lp_ticker_set_interrupt_wrapper rather than
230+
* lp_ticker_set_interrupt that gets straight called. lp_ticker_set_interrupt_wrapper
231+
* guarantees that lp_ticker_set_interrupt won't get re-called in LPTICKER_DELAY_TICKS ticks
232+
* which is just enough for "clear interrupt flag" to take effect.
233+
*/
228234
lp_ticker_clear_interrupt();
235+
lp_ticker_disable_interrupt();
229236

230237
// NOTE: lp_ticker_set_interrupt() may get called in lp_ticker_irq_handler();
231-
if (ticker_stat == NU_TICKER_INIT_INTR_EN) {
232-
lp_ticker_irq_handler();
233-
}
238+
lp_ticker_irq_handler();
234239
}
235240

236241
#endif

‎targets/TARGET_NUVOTON/TARGET_M480/lp_ticker.c

Lines changed: 36 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -42,23 +42,14 @@ static const struct nu_modinit_s timer1_modinit = {TIMER_1, TMR1_MODULE, CLK_CLK
4242

4343
#define TIMER_MODINIT timer1_modinit
4444

45-
/* S/W interrupt enable/disable
45+
/* Timer interrupt enable/disable
4646
*
47-
* Because H/W interrupt enable/disable (TIMER_EnableInt/TIMER_DisableInt) needs delay for lp_ticker,
48-
* we introduce S/W interrupt enable/disable to avoid blocking code. With S/W interrupt enable/disable,
49-
* H/W interrupt is always enabled after ticker_init. A S/W flag is used to tell whether or not
50-
* ticker_irq_handler is ready to call.
47+
* Because Timer interrupt enable/disable (TIMER_EnableInt/TIMER_DisableInt) needs wait for lp_ticker,
48+
* we call NVIC_DisableIRQ/NVIC_EnableIRQ instead.
5149
*/
5250

53-
/* Ticker uninitialized */
54-
#define NU_TICKER_UNINIT 0
55-
/* Ticker initialized with interrupt disabled */
56-
#define NU_TICKER_INIT_INTR_DIS 1
57-
/* Ticker initialized with interrupt enabled */
58-
#define NU_TICKER_INIT_INTR_EN 2
59-
6051
/* Track ticker status */
61-
static volatile uint16_t ticker_stat = NU_TICKER_UNINIT;
52+
static volatile uint16_t ticker_inited = 0;
6253

6354
#define TMR_CMP_MIN 2
6455
#define TMR_CMP_MAX 0xFFFFFFu
@@ -68,14 +59,15 @@ static volatile uint16_t ticker_stat = NU_TICKER_UNINIT;
6859

6960
void lp_ticker_init(void)
7061
{
71-
if (ticker_stat) {
62+
if (ticker_inited) {
7263
/* By HAL spec, ticker_init allows the ticker to keep counting and disables the
7364
* ticker interrupt. */
7465
lp_ticker_disable_interrupt();
7566
lp_ticker_clear_interrupt();
67+
NVIC_ClearPendingIRQ(TIMER_MODINIT.irq_n);
7668
return;
7769
}
78-
ticker_stat = NU_TICKER_INIT_INTR_DIS;
70+
ticker_inited = 1;
7971

8072
// Reset module
8173
SYS_ResetModule(TIMER_MODINIT.rsetidx);
@@ -106,7 +98,7 @@ void lp_ticker_init(void)
10698
// Set vector
10799
NVIC_SetVector(TIMER_MODINIT.irq_n, (uint32_t) TIMER_MODINIT.var);
108100

109-
NVIC_EnableIRQ(TIMER_MODINIT.irq_n);
101+
NVIC_DisableIRQ(TIMER_MODINIT.irq_n);
110102

111103
TIMER_EnableInt(timer_base);
112104
wait_us((NU_US_PER_SEC / NU_TMRCLK_PER_SEC) * 3);
@@ -145,12 +137,12 @@ void lp_ticker_free(void)
145137
/* Disable IP clock */
146138
CLK_DisableModuleClock(TIMER_MODINIT.clkidx);
147139

148-
ticker_stat = NU_TICKER_UNINIT;
140+
ticker_inited = 0;
149141
}
150142

151143
timestamp_t lp_ticker_read()
152144
{
153-
if (ticker_stat == NU_TICKER_UNINIT) {
145+
if (! ticker_inited) {
154146
lp_ticker_init();
155147
}
156148

@@ -161,9 +153,6 @@ timestamp_t lp_ticker_read()
161153

162154
void lp_ticker_set_interrupt(timestamp_t timestamp)
163155
{
164-
/* We can call ticker_irq_handler now. */
165-
ticker_stat = NU_TICKER_INIT_INTR_EN;
166-
167156
/* In continuous mode, counter will be reset to zero with the following sequence:
168157
* 1. Stop counting
169158
* 2. Configure new CMP value
@@ -181,12 +170,15 @@ void lp_ticker_set_interrupt(timestamp_t timestamp)
181170

182171
/* NOTE: Rely on LPTICKER_DELAY_TICKS to be non-blocking. */
183172
timer_base->CMP = cmp_timer;
173+
174+
/* We can call ticker_irq_handler now. */
175+
NVIC_EnableIRQ(TIMER_MODINIT.irq_n);
184176
}
185177

186178
void lp_ticker_disable_interrupt(void)
187179
{
188180
/* We cannot call ticker_irq_handler now. */
189-
ticker_stat = NU_TICKER_INIT_INTR_DIS;
181+
NVIC_DisableIRQ(TIMER_MODINIT.irq_n);
190182
}
191183

192184
void lp_ticker_clear_interrupt(void)
@@ -203,12 +195,12 @@ void lp_ticker_clear_interrupt(void)
203195

204196
void lp_ticker_fire_interrupt(void)
205197
{
206-
/* We can call ticker_irq_handler now. */
207-
ticker_stat = NU_TICKER_INIT_INTR_EN;
208-
209198
// NOTE: This event was in the past. Set the interrupt as pending, but don't process it here.
210199
// This prevents a recursive loop under heavy load which can lead to a stack overflow.
211200
NVIC_SetPendingIRQ(TIMER_MODINIT.irq_n);
201+
202+
/* We can call ticker_irq_handler now. */
203+
NVIC_EnableIRQ(TIMER_MODINIT.irq_n);
212204
}
213205

214206
const ticker_info_t* lp_ticker_get_info()
@@ -222,15 +214,28 @@ const ticker_info_t* lp_ticker_get_info()
222214

223215
static void tmr1_vec(void)
224216
{
225-
/* NOTE: We need to clear interrupt flag earlier to reduce possibility of dummy interrupt.
226-
* This is because "clear interrupt flag" needs delay which isn't added here to avoid
227-
* blocking in ISR code. */
217+
/* NOTE: Avoid blocking in ISR due to wait for "clear interrupt flag"
218+
*
219+
* "clear interrupt flag" needs wait to take effect which isn't added here to avoid
220+
* blocking in ISR.
221+
*
222+
* Continuing above, we will get stuck in ISR due to dummy interrupt until
223+
* "clear interrupt flag" takes effect. To avoid it, we disable interrupt here and enable
224+
* interrupt in lp_ticker_fire_interrupt/lp_ticker_set_interrupt. There is another risk
225+
* that we may get stuck in a loop of ISR and lp_ticker_fire_interrupt/
226+
* lp_ticker_set_interrupt (called by lp_ticker_irq_handler), but actually we don't:
227+
* 1. When lp_ticker_fire_interrupt gets called, it means there is a past event and so this
228+
* interrupt isn't dummy.
229+
* 2. With LPTICKER_DELAY_TICKS enabled, it is lp_ticker_set_interrupt_wrapper rather than
230+
* lp_ticker_set_interrupt that gets straight called. lp_ticker_set_interrupt_wrapper
231+
* guarantees that lp_ticker_set_interrupt won't get re-called in LPTICKER_DELAY_TICKS ticks
232+
* which is just enough for "clear interrupt flag" to take effect.
233+
*/
228234
lp_ticker_clear_interrupt();
235+
lp_ticker_disable_interrupt();
229236

230237
// NOTE: lp_ticker_set_interrupt() may get called in lp_ticker_irq_handler();
231-
if (ticker_stat == NU_TICKER_INIT_INTR_EN) {
232-
lp_ticker_irq_handler();
233-
}
238+
lp_ticker_irq_handler();
234239
}
235240

236241
#endif

0 commit comments

Comments
 (0)