STM32: fix us ticker set event timestamp double ISR possibility

[0xc0170]

This fixes setting the new event for us ticker. As it was, the interrupt would
fire 2x. It should either fire ISR immediately or set it in the future.

Tested with ODIN target and a test case for setting events in the past that will come as a separate PR.

As I understood the code, we either generate event and enable it OR set new compare value and enable it. This fixes the failures we have seen in #4628 and possibly in our CI for basic timer tests.

before this patch:

+-----------------------+-------------------+----------------------------+--------+--------------------+-------------+
| target                | platform_name     | test suite                 | result | elapsed_time (sec) | copy_method |
+-----------------------+-------------------+----------------------------+--------+--------------------+-------------+
| UBLOX_EVK_ODIN_W2-ARM | UBLOX_EVK_ODIN_W2 | tests-mbed_hal-ticker_port | FAIL   | 9.3                | shell       |
+-----------------------+-------------------+----------------------------+--------+--------------------+-------------+

after this patch:

+-----------------------+-------------------+----------------------------+--------+--------------------+-------------+
| target                | platform_name     | test suite                 | result | elapsed_time (sec) | copy_method |
+-----------------------+-------------------+----------------------------+--------+--------------------+-------------+
| UBLOX_EVK_ODIN_W2-ARM | UBLOX_EVK_ODIN_W2 | tests-mbed_hal-ticker_port | OK     | 9.93               | shell       |
+-----------------------+-------------------+----------------------------+--------+--------------------+-------------+

This was the failure [1498475248.69][CONN][RXD] Testing interrupt 0 us in the past [1498475248.77][CONN][RXD] :86::FAIL: Expected 1 Was 2 . we can see that first event in the past should produce just one IRQ call, but actually was 2x.

Is this the same problem for 16 bit ticker, I can see negative delta handling but prior that we are again setting the new match value (timestamp), is that correct?

I can rebase this on top of the test addition, and t hen rebase once the test is integrated if that helps.

cc @bcostm @adustm @LMESTM @jeromecoutant @c1728p9 @andreaslarssonublox

[LMESTM]

@0xc0170 this piece of code you're fixing here has been discussed in details in #4417 and the reasoning for the sequence is also explained in the PR / commit message of @monkiineko . Extract below:

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.

By introducing your change, the risk exists to fall in the above case.

I made the same comment that interrupt could be triggered twice and @monkiineko answered a full and detailed explanation to be read in #4417. The final argument being that

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).

Isn't the last part about mbed ticker checking the time not valid anymore ?

[0xc0170]

@LMESTM I might reread the entire thread, I remember some details of it.

We are testing directly HAL ticker layer in the test referenced above. I don't think it is acceptable to generate two events, even though the above layer handles it. How can we prevent that from happening? Can we? I would assume that generating multiple events is possible if we are setting an event in far future, for instance using 16bit timer but having event with a period of waiting longer than that, it would genereate two interrupts then at least. In this case however what I have seen the event is in the past, thus should fire asap, and should be just one.

What I am after is this bug that appeared recently for some platforms, this affects for instance as I recall odin in this case and also f429 as well http://mbedosci.cloudapp.net/results/pr/4163/625/FAIL/UBLOX_EVK_ODIN_W2/ARM/test_log_UBLOX_EVK_ODIN_W2_ARM.txt , basic rtos tests fails due to time drift. This will need to be investigated.
Update: I realized there is PR that is fixing this time drift, #4599 :-)

[LMESTM]

We are testing directly HAL ticker layer in the test referenced above. I don't think it is acceptable to generate two events, even though the above layer handles it. How can we prevent that from happening? Can we?

I think it's better to generate 2 events with one being discarded in higher layer than missing an event.
Especially the appearance of 2 events would only happen in case we're trying to setup a timer in very near future or in the past - so we're already in a corner case where higher layers are a bit late ... so isn't that acceptable ?

[0xc0170]

@LMESTM I updated the fix, this way it should generate just one. What do you think?

[LMESTM]

@0xc0170 even though the risk is reduced now, there is still a chance that TIM_MST->CNT is read before reaching timestamp, then in the following clock cycles, during if/ else comparison it reaches TIM_MST->CNT .... in this case, interrupt would be missed.

If you only keep the call to disable but keep the sequence like below, the the risk of missing interrupt disappears, and the risk of raising interrupt twice is minimized.

TimMasterHandle.Instance = TIM_MST;
 +    // disable IT while we are handling the correct timestamp
 +    __HAL_TIM_DISABLE_IT(&TimMasterHandle, TIM_IT_CC1);
      // 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) {
          LL_TIM_GenerateEvent_CC1(TimMasterHandle.Instance);
      }
  }

[bscott-zebra]

Hi @0xc0170 and @LMESTM ,
The __HAL_TIM_DISABLE_IT() and __HAL_TIM_ENABLE_IT() macros only clear/set the timer's DIER register. As such, the __HAL_TIM_DISABLE_IT() call does not prevent the CC1IF bit from being set in the timer's SR register if a match occurs while the DIER bit is clear ; it just prevents it from propagating the signal to the interrupt controller. If this occurs, and the CC1IF bit is set while the DIER bit is clear, then when __HAL_TIM_ENABLE_IT() sets the DIER bit, it should immediately result in an interrupt. There should not be a chance of missing an interrupt.
The critical part is that the CCR1 register must be set prior to checking if the time has already passed, to avoid a window where the interrupt is thought to be in the past, but the CCR1 register wasn't set before the timer incremented to match (or pass) the interrupt time. This is still being done with the current commit (12ca9020431cdd94d7d9a884f9b3df8a7a1f5553).
So I am okay with the current commit submitted by @0xc0170 (with the __HAL_TIM_ENABLE_IT() call after the potential generate event).

[LMESTM]

@monkiineko @0xc0170 ok indeed we would not miss the interrupt.
The fact disabling then enabling again the interrupt makes sense in case this function isn't called from within a critical section right ? This is not the case as of now in MBED but this makes the function more robust / future proof I guess. So OK ! thanks to both of you

[0xc0170]

Thanks for all the inputs ! 👍

This should be ready for CI.

Update: I'll rebase to place the comment above enable IT as it was misplaced, no more code changes

[0xc0170]

/morph test

[mbed-bot]

Result: SUCCESS

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

/morph test

Output

mbed Build Number: 672

All builds and test passed!

[0xc0170]

retest uvisor