Skip to content

Make HAL & US tickers IRQ and idle safe #4768

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

Closed
wants to merge 2 commits into from
Closed

Make HAL & US tickers IRQ and idle safe #4768

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3202841
Make HAL & US tickers IRQ and idle safe
betzw Jul 18, 2017
a1707a0
Remove critical section in `us_ticker_set_interrupt()` & extend modif…
betzw Jul 21, 2017
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
4 changes: 2 additions & 2 deletions targets/TARGET_STM/hal_tick_16b.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,15 +161,15 @@ HAL_StatusTypeDef HAL_InitTick(uint32_t TickPriority)
return HAL_OK;
}

/* NOTE: must be called with interrupts disabled! */
void HAL_SuspendTick(void)
{
TimMasterHandle.Instance = TIM_MST;
__HAL_TIM_DISABLE_IT(&TimMasterHandle, TIM_IT_CC2);
}

/* NOTE: must be called with interrupts disabled! */
void HAL_ResumeTick(void)
{
TimMasterHandle.Instance = TIM_MST;
__HAL_TIM_ENABLE_IT(&TimMasterHandle, TIM_IT_CC2);
}

Expand Down
4 changes: 2 additions & 2 deletions targets/TARGET_STM/hal_tick_32b.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,15 +131,15 @@ HAL_StatusTypeDef HAL_InitTick(uint32_t TickPriority)
return HAL_OK;
}

/* NOTE: must be called with interrupts disabled! */
void HAL_SuspendTick(void)
{
TimMasterHandle.Instance = TIM_MST;
__HAL_TIM_DISABLE_IT(&TimMasterHandle, TIM_IT_CC2);
}

/* NOTE: must be called with interrupts disabled! */
void HAL_ResumeTick(void)
{
TimMasterHandle.Instance = TIM_MST;
__HAL_TIM_ENABLE_IT(&TimMasterHandle, TIM_IT_CC2);
}

Expand Down
13 changes: 12 additions & 1 deletion targets/TARGET_STM/sleep.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,17 +40,25 @@ extern void HAL_ResumeTick(void);

void hal_sleep(void)
{
// Disable IRQs
core_util_critical_section_enter();
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't this be a case for all platforms, thus sleep and deepsleep in upper layer should include enter/exit critical section ?

Copy link
Contributor Author

@betzw betzw Jul 18, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Presumably yes, but it depends on what these platforms are doing in hal_sleep(). If they are calling just a __WFI and have no other constraints, then there is no need for a critical section.

Copy link
Contributor

@kjbracey kjbracey Jul 18, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

In my experience (although that's mainly on ARM-A), any WFI-type sleep function should always be called with interrupts disabled.

Otherwise there's a race condition - you may have been idle, which is why you were going into sleep, but as you start running the sleep routine, an interrupt wakes a thread, but then sleep goes ahead and does WFI, so we block until the next interrupt, and the woken thread doesn't run immediately.

If interrupts are disabled all the way from the "are we idle" check to the "WFI", then the sleep routine will correctly return immediately due to a pending interrupt.

So I think 0xc0170 is right - hal_sleep() should probably always be entered with interrupts disabled. (And they should have been disabled before checking if we were idle).

Copy link
Contributor Author

@betzw betzw Jul 18, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

So I think 0xc0170 is right - hal_sleep() should probably always be entered with interrupts
disabled. (And they should have been disabled before checking if we were idle).

Unfortunatley, in this moment hal_sleep() does not get called with interrupts already disabled! Not sure if this is a choice or a bug, up to you to decide.
Anyway, as it is right now, mbed-os-5.5 is not working correctly for non-debug builds, at least for STM-NUCLEO platforms.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Useless to say, that this is a critical/blocking situation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you confirm this is a change in behaviour since the previous RTX? And why the distinction between debug and non-debug builds?

We'll need someone more familiar with the RTX port to comment. My analysis above was assuming a dedicated "idle" handler in an OS scheduler - it may be somewhat different if hal_sleep is just called as "normal" code in a minimum-priority idle thread.

Copy link
Contributor Author

@betzw betzw Jul 18, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I can confirm that since mbed-os-5.5 I am facing this issue, which I could resolve with the patch of this PR. I never checked RTX (neither v4 nor v5) source code to have a real confirmation for my assumption, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I further dare to confirm that:

hal_sleep is just called as "normal" code in a minimum-priority idle thread

Copy link
Contributor

Choose a reason for hiding this comment

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

And the debug/non-debug distinction is that hal_sleep is not called at all in debug builds, for whatever reason. (Something to do with breaking semihosting mentioned in comments?)

Given that it's just called in a continuous loop from a normal minimum-priority thread, I believe that the correct pattern would be for hal_sleep to either be a simple WFE with interrupts enabled, else a more complete shutdown within a critical section using WFI - as per this patch.

So I think entry with interrupts enabled isn't necessarily wrong, but it feels like there's a clear trap for porters here. Doubly so given that hal_sleep isn't called in debug builds.

I still don't know what the change since 5.4 is though.

Copy link
Contributor

@kjbracey kjbracey Jul 18, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Okay, change since 5.4 identified.

"release" build profile has always slept, "debug" build profile has never slept.

The behaviour of the "develop" (ie default) build profile changed between 5.4 and 5.5. In 5.4 it did not sleep - it was switched on NDEBUG. In 5.5 it does sleep - it is switched on MBED_DEBUG.

I've discussed the sleep a bit with @bulislaw, I think my previous comment is correct, so I'll recheck this and probably approve.


// Stop HAL tick to avoid to exit sleep in 1ms
HAL_SuspendTick();
// Request to enter SLEEP mode
HAL_PWR_EnterSLEEPMode(PWR_MAINREGULATOR_ON, PWR_SLEEPENTRY_WFI);

// Restart HAL tick
HAL_ResumeTick();

// Enable IRQs
core_util_critical_section_exit();
}

void hal_deepsleep(void)
{
// Disable IRQs
core_util_critical_section_enter();

// Stop HAL tick
HAL_SuspendTick();
uint32_t EnterTimeUS = us_ticker_read();
Expand Down Expand Up @@ -82,6 +90,9 @@ void hal_deepsleep(void)
// Restart HAL tick
HAL_ResumeTick();

// Enable IRQs
core_util_critical_section_exit();

// After wake-up from STOP reconfigure the PLL
SetSysClock();

Expand Down
22 changes: 7 additions & 15 deletions targets/TARGET_STM/us_ticker_16b.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,22 +26,13 @@ TIM_HandleTypeDef TimMasterHandle;
volatile uint32_t SlaveCounter = 0;
volatile uint32_t oc_int_part = 0;

static int us_ticker_inited = 0;

void us_ticker_init(void)
{
if (us_ticker_inited) return;
us_ticker_inited = 1;

TimMasterHandle.Instance = TIM_MST;

HAL_InitTick(0); // The passed value is not used
/* NOTE: assuming that HAL tick has already been initialized! */
}

uint32_t us_ticker_read()
{
if (!us_ticker_inited) us_ticker_init();

uint16_t cntH_old, cntH, cntL;
do {
cntH_old = SlaveCounter;
Expand Down Expand Up @@ -73,7 +64,7 @@ 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
Expand Down Expand Up @@ -178,20 +169,21 @@ void us_ticker_set_interrupt(timestamp_t timestamp)

void us_ticker_fire_interrupt(void)
{
TimMasterHandle.Instance = TIM_MST;
HAL_TIM_GenerateEvent(&TimMasterHandle, TIM_EVENTSOURCE_CC1);
}

void us_ticker_disable_interrupt(void)
{
TimMasterHandle.Instance = TIM_MST;
core_util_critical_section_enter();
__HAL_TIM_DISABLE_IT(&TimMasterHandle, TIM_IT_CC1);
core_util_critical_section_exit();
}

void us_ticker_clear_interrupt(void)
{
TimMasterHandle.Instance = TIM_MST;
__HAL_TIM_CLEAR_FLAG(&TimMasterHandle, TIM_FLAG_CC1);
core_util_critical_section_enter();
__HAL_TIM_CLEAR_IT(&TimMasterHandle, TIM_IT_CC1);
Copy link
Contributor

@LMESTM LMESTM Jul 21, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Is this MACRO change also required ?
Looking at the code both MACROS actually access to the same Status Register (SR), where it will clear the bit "CC1IF: Capture/Compare 1 interrupt flag" - As the erased bit is a flag, it seems better to actually use __HAL_TIM_CLEAR_FLAG.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, it came in just because us_ticker_32b.c uses __HAL_TIM_CLEAR_IT

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok - better not change it here as this is not related to your change.
As I said, they actually access the same bit as far as I've seen, but I'll change it later for 32b to align code.

Copy link
Contributor Author

@betzw betzw Jul 21, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Just remember that when you are gonna change it for 32b to revert it again also for 16b.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok

core_util_critical_section_exit();
}

#endif // TIM_MST_16BIT
20 changes: 8 additions & 12 deletions targets/TARGET_STM/us_ticker_32b.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,27 +23,21 @@

TIM_HandleTypeDef TimMasterHandle;

static int us_ticker_inited = 0;

void us_ticker_init(void)
{
if (us_ticker_inited) return;
us_ticker_inited = 1;

TimMasterHandle.Instance = TIM_MST;

HAL_InitTick(0); // The passed value is not used
/* NOTE: assuming that HAL tick has already been initialized! */
Copy link
Contributor

Choose a reason for hiding this comment

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

This was not the case for a long time, we need to be sure that mbed_sdk_init is called before C++ objects creations happens, otherwise a timer/ticker object creation would fail. @0xc0170 I think that the above assumption is now ensured on all toolchains thanks to the recent rework on boot sequence - do you confirm ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that the above assumption is now ensured on all toolchains thanks to the recent rework on boot sequence - do you confirm ?

Yes, that is correct. As you are referring to mbed OS 5 boot sequence.

mbed 2 should also be fine. We shall add a test for this to be certain that sdk is called prior C++ ctor

Copy link
Contributor

Choose a reason for hiding this comment

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

We shall run MBED2 tests with this change and we will see if this fails. - this will be part of non-reg when we think the PR is ready for testing

}

uint32_t us_ticker_read()
{
if (!us_ticker_inited) us_ticker_init();
return TIM_MST->CNT;
}

void us_ticker_set_interrupt(timestamp_t timestamp)
{
TimMasterHandle.Instance = TIM_MST;
// NOTE: This function must be called with interrupts disabled to keep our
// timer interrupt setup atomic

// disable IT while we are handling the correct timestamp
__HAL_TIM_DISABLE_IT(&TimMasterHandle, TIM_IT_CC1);
// Set new output compare value
Expand All @@ -59,14 +53,16 @@ void us_ticker_fire_interrupt(void)

void us_ticker_disable_interrupt(void)
{
TimMasterHandle.Instance = TIM_MST;
core_util_critical_section_enter();
__HAL_TIM_DISABLE_IT(&TimMasterHandle, TIM_IT_CC1);
core_util_critical_section_exit();
}

void us_ticker_clear_interrupt(void)
{
TimMasterHandle.Instance = TIM_MST;
core_util_critical_section_enter();
__HAL_TIM_CLEAR_IT(&TimMasterHandle, TIM_IT_CC1);
core_util_critical_section_exit();
}

#endif // !TIM_MST_16BIT