STM32: Fix 32-bit us ticker interrupt scheduling

[bscott-zebra]

Description

For STM32 targets using a 32-bit timer for the microsecond ticker, the driver did not properly handle timestamps that are in the past. It would just blindly set the compare register to the requested timestamp, resulting in the interrupt being serviced up to 4295 seconds late (i.e. after the 32-bit timer counts all the way around to hit the timestamp again).

Now, after the compare register has been set, the timestamp is checked against the current time to see if the timestamp is in the past, and if so, the IRQ handler is immediately called.

NOTE: By checking if the timestamp is in the past after configuring the capture register, we ensure proper handling in the case where the timer updates past the timestamp while setting the capture register.

Status

READY

Migrations

NO

Related PRs

None

Todos

None

Deploy notes

None

Steps to test or reproduce

This problem can easily be reproduced by creating a Timeout object then calling the timeout's attach_us() member function to attach a callback with a timeout of 0 us. The callback will not get called for over 2147 seconds, and possibly up to 4295 seconds late if no other microsecond ticker events are getting scheduled in the meantime.

[LMESTM]

@monkiineko
Hi - thanks for reporting this point.
Note that I have just concern that has already been raised with other targets.
us_ticker_set_interrupt might be called already from an interrupt handler, and by calling again the interrupt handler we might end up in a recursive loop ...
Anyway the same limitation applies to current 16 bits timer implementation, so I'm ok to go ahead with it for now and I'll work on an alternative solution. (Basically we need to trig the CC1 interrupt from SW )

[LMESTM]

I'd rather move this code before the SET_COMPARE and ENABLE_IT calls.
If you're in this situation you can call the handler then return - don't you think ?

[bscott-zebra]

My concern with putting the check before the SET_COMPARE call is that the timer is running asynchronously, so it would be possible (though rare) for the timer counter to be < timestamp before the SET_COMPARE call such that the event is not manually generated, but then the timer counter increments before the CCR1 register write completes, hence the interrupt is missed. I think it is safer to write the compare register first and then do the check to see if the timestamp time has already passed.

[LMESTM]

let's discuss my comments

[LMESTM]

@monkiineko : I'd suggest to use the below line

HAL_TIM_GenerateEvent(&TimMasterHandle, TIM_EVENTSOURCE_CC1);

instead of calling directly to us_ticker_irq_handler()
I've been testing it with 16 bits timers and this is working ok.

[LMESTM]

FYI - the 16 bits ticker fix is visible here: https://github.com/LMESTM/mbed/tree/STM32_16bits_tickers

[bscott-zebra]

@LMESTM I actually thought the same thing (regarding software generating the CC1 event rather than directly calling the IRQ handler). My first implementation set the CC1G bit in the EGR to force the interrupt rather than directly calling the IRQ handler, and this worked well. I only changed it to the direct call when I saw the 16 bit timer code doing that. So I have no problem going back to setting the EGR register to generate the event rather than the direct call.

However, I do have a concern about using the HAL_TIM_GenerateEvent() function to write the EGR. At least for the STM32F3 implementation, the HAL_TIM_GenerateEvent() function uses the __HAL_LOCK()/__HAL_UNLOCK() macros, and __HAL_LOCK() will force a return without doing anything if the handle is already locked (e.g. interrupt while a thread was doing some HAL operation on that same timer. I don't know how likely it is for another HAL timer operation on the mbed timer to be in progress during a timer interrupt (doesn't seem like any of the other HAL timer functions being used currently make use of the __HAL_LOCK() macro...), but there's really no need for the lock when setting a bit via the EGR anyway (it's write only, just for setting event bits), so I'd be much more comfortable using "TimMasterHandle.Instance->EGR = TIM_EGR_CC1G;" (all STM32 devices appear to have the same EGR CC1G definition) or even "LL_TIM_GenerateEvent_CC1(TimMasterHandle.Instance);", but the later inline function is defined in device specific header files that don't appear to be included from any commonly named header, so a bunch of conditionalized includes would be required (ugly and difficult to maintain), or it would have to be included in something like each devices' hal_tick.h file (or similar) ; that would be a lot of files to change. Plus, directly writing the EGR register would be more efficient (single write operation, no function call...). :)

Anyway, as I mentioned in my reply in your code comment, I think it's safer to do the timestamp in past check after setting the compare register, so I would propose making the change:

void us_ticker_set_interrupt(timestamp_t timestamp)
{
    TimMasterHandle.Instance = TIM_MST;
    // Set new output compare value
    __HAL_TIM_SET_COMPARE(&TimMasterHandle, TIM_CHANNEL_1, (uint32_t)timestamp);
    // Enable IT
    __HAL_TIM_ENABLE_IT(&TimMasterHandle, TIM_IT_CC1);
    // Check if timestamp has already passed, and if so, set the event immediately
    if ((int32_t)(timestamp - TIM_MST->CNT) <= 0) {
        TimMasterHandle.Instance->EGR = TIM_EGR_CC1G;
    }
}

Thoughts?

[LMESTM]

@monkiineko :-) many points to discuss - thanks for your explanation.

First point is about generating the event. I think we agree this is the right thing to do. then about HAL or LL - I'd vote for LL. I know the include paths to add is a bit cumbersome but we already have the hal_tick.h files, so not so much of a problem. Alternatively I can do it in my 16 bits branch and you will be able to rebase on top of it.

Next point is about call to set_compare. My concern when looking at the code is that the event would be triggered twice ... First time because of set_compare, then again because time has passed since set_compare. An alternative is to take a 1µs margin as this would be in the precision range ?

TimMasterHandle.Instance = TIM_MST;
// Check if timestamp has already passed, and if so, set the event immediately
if ((int32_t)(timestamp - TIM_MST->CNT) <= 1) {
    TimMasterHandle.Instance->EGR = TIM_EGR_CC1G;
} else {
// Set new output compare value
__HAL_TIM_SET_COMPARE(&TimMasterHandle, TIM_CHANNEL_1, (uint32_t)timestamp);
// Enable IT
__HAL_TIM_ENABLE_IT(&TimMasterHandle, TIM_IT_CC1);

}

[bscott-zebra]

@LMESTM
I can go and add the #include to get the timer LL inline functions available in each device's hal_tick.h file and then use the LL_TIM_GenerateEvent_CC1() function to generate the event for this change (can I rely on the automated build checks to validate that I got all the include files correct?).

As to the possibility of a double interrupt if the comparison is done after setting the compare register, I don't think that is an issue, because the us_ticker_set_interrupt function is only called in 3 places, in the us ticker interrupt handler, in the ticker insert function, and in the ticker remove function. The later two use mbed core critical sections (mask interrupts), so in all cases, timer interrupts should be blocked for the duration of the us_ticker_set_interrupt() function execution, so even if the compare matches before the event is manually generated, only a single event interrupt should result (i.e. the compare would set the CC1 event, but the interrupt handler could not be run until after the us_ticker_set_interrupt() function has returned, so the manual generation of the CC1 event would be an effective NOP, just setting a bit that is already set).

Allowing a 1us window to add margin can work (I've done something similar in the past ; adding a safety margin to the timestamp being set if it is too close to the current time), but then you are relying on interrupts being blocked anyway to avoid an interrupt sneaking in and extending the time beyond your safety margin, and you risk the margin not being enough in some corner case (e.g. someone who decides to run their CPU at 1MHz to minimize power, so it takes longer than 1us to do anything).

So, my preference would still be do do the manual event generation after setting the compare register, and rely on the interrupts being blocked to prevent a double interrupt (though, with the way the mbed ticker interrupt works, a double interrupt wouldn't cause any problems, beyond just wasting a little extra time for the extra interrupt entry/exit, as it checks the current time against the queue and will do the right thing, which could include doing nothing in the case of a spurious interrupt).

[LMESTM]

@monkiineko Sounds good to me ! Thanks for sharing all your thoughts, I am now convinced for 2 good reasons (critical sections in mbed as well as the mbed ticker driver doing checks against queue).

And yes, I will start automated non-regression tests, which will also include compilation.

[LMESTM]

@monkiineko I will later update #4417 to also use LL instead of HAL. Also more comments are welcome on #4417.

[sg-]

@monkiineko have you signed the contributor agreement? Could you share your developer.mbed.org user account (a URL such as https://developer.mbed.org/users/sam_grove/)? The CLA can be adopted by visiting this page https://developer.mbed.org/contributor_agreement/

[bscott-zebra]

@sg- Yes, I have previously signed the CLA. My mbed.org profile is here:
https://developer.mbed.org/users/bscott/

[0xc0170]

@LMESTM happy with this patch (update the review please)

[0xc0170]

/morph test

[mbed-bot]

Result: SUCCESS

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

/morph test

Output

mbed Build Number: 445

All builds and test passed!

[LMESTM]

@monkiineko @0xc0170 @sg-
I confirm that non regression tests are PASSED OK.
MBED OS5 TESTS - 1 target per family

MBED OS2 Tickers TESTS
also 100% PASSED on ALL families with 32 bits timers.