Fix corner cases in STM32 16bit tickers #4424

LMESTM · 2017-06-01T16:50:47Z

Description

This Pull requests contains several fixes around 16 bits tickers.
Those fixes are addressing points that were raised in #1701 , #2519, #2598, #4217

Dependencies

Changes will also be aligned with changes ongoing in #4417. (needs LL include)
So this PR will be rebased after #4417 is merged.

Status

READY

Tests

F0

F1

L0

bscott-zebra · 2017-06-01T19:04:21Z

targets/TARGET_STM/us_ticker_16b.c

- set_compare(0xFFFF);
- oc_rem_part = oc_rem_part - (0xFFFF - cval);
- }
+ set_compare(timestamp & 0xFFFF);
 }
 }



While you're reworking this file, it would be good to get rid of the if (__HAL_TIM_GET_FLAG(&TimMasterHandle, TIM_FLAG_CC1) == SET) { check from the us_ticker_clear_interrupt() function below. If the flag being cleared is not set, then the clear operation will just be a NOP, and not having the check will make the code more efficient. Note that the us_ticker_32b.c implementation is already working without the check.

Ok - good point

bscott-zebra · 2017-06-01T19:17:32Z

targets/TARGET_STM/us_ticker_16b.c

@@ -25,7 +25,6 @@ TIM_HandleTypeDef TimMasterHandle;

 volatile uint32_t SlaveCounter = 0;
 volatile uint32_t oc_int_part = 0;
-volatile uint16_t oc_rem_part = 0;

 static int us_ticker_inited = 0;



With your changes, set_compare() is now called from only a single place. It might be better to just put the contents of this function inline at that one place. You could then make the TimMasterHandle.Instance = TIM_MST; assignment once at the top of the us_ticker_set_interrupt() function, rather than separately in both the "if" and "else" cases. You may go even further and just provide a static initializer for TimMasterHandle to set Instance, so it doesn't have to be done on every function call (e.g. TIM_HandleTypeDef TimMasterHandle = { TIM_MST }; // Initialize Instance).

bscott-zebra · 2017-06-01T19:18:28Z

targets/TARGET_STM/hal_tick_16b.c

@@ -24,20 +24,19 @@ extern TIM_HandleTypeDef TimMasterHandle;

 extern volatile uint32_t SlaveCounter;
 extern volatile uint32_t oc_int_part;
-extern volatile uint16_t oc_rem_part;

 volatile uint32_t PreviousVal = 0;

 void us_ticker_irq_handler(void);
 void set_compare(uint16_t count);


set_compare() is no longer used in this file, so this prototype can be removed.

bscott-zebra · 2017-06-01T19:26:06Z

targets/TARGET_STM/us_ticker_16b.c

@@ -52,22 +50,32 @@ void us_ticker_init(void)

 uint32_t us_ticker_read()
 {
- uint32_t counter;
-
 TimMasterHandle.Instance = TIM_MST;


This TimMasterHandle.Instance initialization can be removed from here, because it will either have been already done previously, or it will be done in the us_ticker_init() call immediately below.

bscott-zebra · 2017-06-01T19:36:29Z

targets/TARGET_STM/us_ticker_16b.c

+ * there are 2 possible cases of wrap around
+ * 1) in case this function is interrupted by timer_irq_handler and
+ * the SlaveCounter is updated. In that case we will loop again.
+ * 2) in case this function is called from context interrupt during


"context interrupt" should be "interrupt context"

yes ... I'm French ;-)

bscott-zebra · 2017-06-01T19:37:24Z

targets/TARGET_STM/us_ticker_16b.c

+ * the SlaveCounter is updated. In that case we will loop again.
+ * 2) in case this function is called from context interrupt during
+ * wrap-around condtion. That would prevent/delay the timer_irq_handler
+ * to be called so we need to locally check the FLAG_UPDATE and


"to be called" should be "from being called"

bscott-zebra · 2017-06-01T22:42:32Z

targets/TARGET_STM/us_ticker_16b.c

- set_compare(0xFFFF);
- oc_rem_part = oc_rem_part - (0xFFFF - cval);
- }
+ set_compare(timestamp & 0xFFFF);
 }


If the counter increments past the lower 16 bits of timestamp after the us_ticker_read, and before the compare register has been set, then oc_int_part may be 1 higher than it should be, resulting in an unwanted extra 65.5 ms delay before the us_ticker_irq_handler() is called. In order to properly handle this case, I would suggest restructuring this function as:

void us_ticker_set_interrupt(timestamp_t timestamp) { // NOTE: This function must be called with interrupts disabled to keep our // timer interrupt setup atomic TimMasterHandle.Instance = TIM_MST; // Set new output compare value __HAL_TIM_SET_COMPARE(&TimMasterHandle, TIM_CHANNEL_1, timestamp & 0xFFFF); // Ensure the compare event starts clear __HAL_TIM_CLEAR_FLAG(&TimMasterHandle, TIM_FLAG_CC1); // Enable IT __HAL_TIM_ENABLE_IT(&TimMasterHandle, TIM_IT_CC1); int current_time = us_ticker_read(); int delta = (int)(timestamp - current_time); if (delta <= 0) { // This event was in the past /* Immediately set the compare event to cause the event to be handled in * the next interrupt context. This prevents calling interrupt handlers * recursively as us_ticker_set_interrupt might be called again from the * application handler */ oc_int_part = 0; LL_TIM_GenerateEvent_CC1(TimMasterHandle.Instance); } else { /* Set the number of timer wrap-around loops before the actual timestamp * is reached. If the calculated delta time is more than halfway to the * next compare event, check to see if a compare event has already been * set, and if so, add one to the wrap-around count. This is done to * ensure the correct wrap count is used in the corner cases where the * 16 bit counter passes the compare value during the process of * configuring this interrupt. * * Assumption: The time to execute this function is less than 32ms * (otherwise incorrect behaviour could result) * * Consider the following corner cases: * 1) timestamp is 1 us in the future: * oc_int_part = 0 initially * oc_int_part left at 0 because ((delta - 1) & 0xFFFF) < 0x8000 * Compare event should happen in 1 us and us_ticker_irq_handler() * called * 2) timestamp is 0x8000 us in the future: * oc_int_part = 0 initially * oc_int_part left at 0 because ((delta - 1) & 0xFFFF) < 0x8000 * There should be no possibility of the CC1 flag being set yet * (see assumption above). When the compare event does occur in * 32768 us, us_ticker_irq_handler() will be called * 3) timestamp is 0x8001 us in the future: * oc_int_part = 0 initially * ((delta - 1) & 0xFFFF) >= 0x8000 but there should be no * possibility of the CC1 flag being set yet (see assumption above), * so oc_int_part will be left at 0, and when the compare event * does occur in 32769 us, us_ticker_irq_handler() will be called * 4) timestamp is 0x10000 us in the future: * oc_int_part = 0 initially * ((delta - 1) & 0xFFFF) >= 0x8000 * There are two subcases: * a) The timer counter has not incremented past the compare * value while setting up the interrupt. In this case, the * CC1 flag will not be set, so oc_int_part will be * left at 0, and when the compare event occurs in 65536 us, * us_ticker_irq_handler() will be called * b) The timer counter has JUST incremented past the compare * value. In this case, the CC1 flag will be set, so * oc_int_part will be incremented to 1, and the interrupt will * occur immediately after this function returns, where * oc_int_part will decrement to 0 without calling * us_ticker_irq_handler(). Then about 65536 us later, the * compare event will occur again, and us_ticker_irq_handler() * will be called * 5) timestamp is 0x10001 us in the future: * oc_int_part = 1 initially * oc_int_part left at 1 because ((delta - 1) & 0xFFFF) < 0x8000 * CC1 flag will not be set (see assumption above). In 1 us the * compare event will cause an interrupt, where oc_int_part will be * decremented to 0 without calling us_ticker_irq_handler(). Then * about 65536 us later, the compare event will occur again, and * us_ticker_irq_handler() will be called * 6) timestamp is 0x18000 us in the future: * oc_int_part = 1 initially * oc_int_part left at 1 because ((delta - 1) & 0xFFFF) < 0x8000 * There should be no possibility of the CC1 flag being set yet * (see assumption above). When the compare event does occur in * 32768 us, oc_int_part will be decremented to 0 without calling * us_ticker_irq_handler(). Then about 65536 us later, the * compare event will occur again, and us_ticker_irq_handler() will * be called * 7) timestamp is 0x18001 us in the future: * oc_int_part = 1 initially * ((delta - 1) & 0xFFFF) >= 0x8000 but there should be no * possibility of the CC1 flag being set yet (see assumption above), * so oc_int_part will be left at 1, and when the compare event * does occur in 32769 us, oc_int_part will be decremented to 0 * without calling us_ticker_irq_handler(). Then about 65536 us * later, the compare event will occur again, and * us_ticker_irq_handler() will be called * * delta - 1 is used because the timer compare event happens on the * counter incrementing to match the compare value, and it won't occur * immediately when the compare value is set to the current counter * value. */ oc_int_part = ((uint32_t)delta - 1) >> 16; if ( ((delta - 1) & 0xFFFF) >= 0x8000 && __HAL_TIM_GET_FLAG(&TimMasterHandle, TIM_FLAG_CC1) == SET ) { ++oc_int_part; /* NOTE: Instead of incrementing oc_int_part here, we could clear * the CC1 flag, but then you'd have to wait to ensure the * interrupt is knocked down before returning and reenabling * interrupts. Since this is a rare case, it's not worth it * to try and optimize it, and it keeps the code simpler and * safer to just do this increment instead. */ } } }

Ok that took me longer to answer here because I need to read everything and I actually have run tests as well, with varying timeouts from 0 to 0X10000 and more and have tried to detect possible hick-ups. I tested your above code and the alternative of mine with the 32bits sequencing, both are running fine with the CPU @ 48MHz.

I'll go with your code which is well thought and very described as well.
I've been through the various cases you've listed and timings I could thought of, and could not find a failure case so far. + the tests I've run are all fine.

I'll restart the basic mbed test suite as well and post the results

Thanks a lot for this effort !

bscott-zebra · 2017-06-02T14:16:27Z

@LMESTM Changes look good to me now. Thanks for all the testing. 👍

0xc0170 · 2017-06-05T10:11:15Z

@LMESTM Can you please rebase? There's a fix that should resolve jenkins CI

Move to a single more reliable implementation of us_ticker_read() There were historically 2 versions of us_ticker_read() implementation. The one removed here was not reliable because us_ticker_read() can be called in interrupt context which means that TIM_MST->CNT would have wrapped around while SlaveCounter is not yet updated. So there is a need to check the TIM_FLAG_UPDATE inside this function, which was not done in the implementation that is removed here.

This commit simplifies ticker interrupt set function and handler. There were issues around the 16 bits timer wrap-around timing as we were aligning interrupts with wrap-around limits (0xFFFF) and then reading TIM_MST->CNT again in timer_update_irq_handler which could lead to crossing case with the wrap-around (TIM_FLAG_UPDATE) case. Now we're using the 16 lower bits of the timestamp as the reference from using in set_compare and never changing it. There is also no need to set comparator again in timer_update_irq_handler. This is more robust and also more efficient.

Following previous fixes on 16 tickers handling, the overflow flag condition will not happen anymore, so the work-around in place is not needed anymore

Not having the check will make the code more efficient.

Since rework, this prototype is not needed anymore.

TimMasterHandle.Instance initialization can be removed from here, because it will either have been already done previously, or it will be done in the us_ticker_init() call immediately below.

…rrupt The present commit comes from monkiineko mbed contributor. The comments in code explains in details all the possible case and how they are handled.

LMESTM · 2017-06-05T11:13:42Z

@0xc0170 rebase done

0xc0170 · 2017-06-05T12:32:20Z

Does this pull request changing anything - #4094 related to this PR ?

0xc0170 · 2017-06-05T12:32:31Z

/morph test

mbed-bot · 2017-06-05T15:46:05Z

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 446

All builds and test passed!

LMESTM · 2017-06-05T16:10:10Z

Tests results added to the PR message

theotherjimmy · 2017-06-06T13:37:44Z

@LMESTM We use the PR titles in the release notes. Could you add a verb to your PR title? What happened with the STM32 16bit tickers? Even if it just "Fix a few bugs in STM32 16bit tickers" I think that would help a reader of the release notes.

LMESTM · 2017-06-06T13:44:29Z

@theotherjimmy - Ok done.

bscott-zebra reviewed Jun 1, 2017

View reviewed changes

sg- added the needs: CI label Jun 3, 2017

LMESTM added 8 commits June 5, 2017 13:12

STM32 16 bits tickers, no need to check TIM_FLAG_CC1OF FLAG

1b684ca

Following previous fixes on 16 tickers handling, the overflow flag condition will not happen anymore, so the work-around in place is not needed anymore

STM32 16 bits ticker: don't check FLAG just clear it

27be2e1

Not having the check will make the code more efficient.

STM32 16 bits tickers: remove unused prototype

7bcabf7

Since rework, this prototype is not needed anymore.

STM32 16 bits tickers: save useless instance init

572a096

TimMasterHandle.Instance initialization can be removed from here, because it will either have been already done previously, or it will be done in the us_ticker_init() call immediately below.

STM32 16 bits ticker: fix grammar issues in comments

fbfbb5e

STM32 16bits tickers: consider all corner cases in us_ticker_set_inte…

ea2cc1d

…rrupt The present commit comes from monkiineko mbed contributor. The comments in code explains in details all the possible case and how they are handled.

LMESTM force-pushed the STM32_16bits_tickers branch from aa75632 to ea2cc1d Compare June 5, 2017 11:13

LMESTM mentioned this pull request Jun 5, 2017

Ticker bug on STM32F1 #4217

Closed

LMESTM changed the title ~~Stm32 16bits tickers~~ Fix a corner cases in STM32 16bit tickers Jun 6, 2017

LMESTM changed the title ~~Fix a corner cases in STM32 16bit tickers~~ Fix corner cases in STM32 16bit tickers Jun 6, 2017

sg- added the ready for merge label Jun 6, 2017

sg- added release-version: 5.5.0-rc2 and removed needs: CI labels Jun 6, 2017

sg- merged commit 4d7c045 into ARMmbed:master Jun 7, 2017

sg- removed the ready for merge label Jun 7, 2017

This was referenced Jun 9, 2017

Instable ticker on stm32f0308-DISCO #1701

Closed

NUCLEO_F091RC: tests-mbed_hal-lp_ticker and tests-mbed_drivers-lp_timeout failing #2519

Closed

Multiple tickers stop working after several seconds in a high load #2598

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix corner cases in STM32 16bit tickers #4424

Fix corner cases in STM32 16bit tickers #4424

LMESTM commented Jun 1, 2017 •

edited

Loading

bscott-zebra Jun 1, 2017

LMESTM Jun 2, 2017

bscott-zebra Jun 1, 2017

bscott-zebra Jun 1, 2017

LMESTM Jun 2, 2017

bscott-zebra Jun 1, 2017

LMESTM Jun 2, 2017

bscott-zebra Jun 1, 2017

LMESTM Jun 2, 2017

bscott-zebra Jun 1, 2017

LMESTM Jun 2, 2017

bscott-zebra Jun 1, 2017

LMESTM Jun 2, 2017

bscott-zebra commented Jun 2, 2017

0xc0170 commented Jun 5, 2017

LMESTM commented Jun 5, 2017

0xc0170 commented Jun 5, 2017

0xc0170 commented Jun 5, 2017

mbed-bot commented Jun 5, 2017

LMESTM commented Jun 5, 2017

theotherjimmy commented Jun 6, 2017

LMESTM commented Jun 6, 2017

Fix corner cases in STM32 16bit tickers #4424

Fix corner cases in STM32 16bit tickers #4424

Conversation

LMESTM commented Jun 1, 2017 • edited Loading

Description

Dependencies

Status

Tests

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bscott-zebra commented Jun 2, 2017

0xc0170 commented Jun 5, 2017

LMESTM commented Jun 5, 2017

0xc0170 commented Jun 5, 2017

0xc0170 commented Jun 5, 2017

mbed-bot commented Jun 5, 2017

Output

LMESTM commented Jun 5, 2017

theotherjimmy commented Jun 6, 2017

LMESTM commented Jun 6, 2017

LMESTM commented Jun 1, 2017 •

edited

Loading