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.
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
STM32 LPTICKER with LPTIM optimisation #8771
Conversation
targets/TARGET_STM/lp_ticker.c
Outdated
/* CMPOK is set by hardware to inform application that the APB bus write operation to the LPTIM_CMP register has been successfully completed */ | ||
/* Any successive write before the CMPOK flag be set, will lead to unpredictable results */ | ||
while (__HAL_LPTIM_GET_FLAG(&LptimHandle, LPTIM_FLAG_CMPOK) == RESET) { | ||
if (__HAL_LPTIM_GET_FLAG(&LptimHandle, LPTIM_FLAG_CMPOK) == RESET) { | ||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this should actually be
MBED_ASSERT(__HAL_LPTIM_GET_FLAG(&lptimHandle, LPTIM_FLAG_CMPOK));
How confident are you that it's working?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How confident are you that it's working?
I only executed all mbed-os tests
The other idea I have is to call lp_ticker_fire_interrupt if lp_ticker_set_interrupt is called too early (before LPTIM_FLAG_CMPOK) ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll leave exact behaviour up to @c1728p9, but it needs to do something other than just return silently in this case.
I would imagine assert should be okay as long as you have correctly configured LPTICKER_DELAY_TICKS
. It will be a hard run-time check that your magic 3 is correct in practice. (And that the delay code is doing its job correctly)
b7040b2
to
f9ddc5a
Compare
@kjbracey-arm @c1728p9 |
/* Need to write a compare value in order to get LPTIM_FLAG_CMPOK in set_interrupt */ | ||
__HAL_LPTIM_CLEAR_FLAG(&LptimHandle, LPTIM_FLAG_CMPOK); | ||
__HAL_LPTIM_COMPARE_SET(&LptimHandle, 0); | ||
while (__HAL_LPTIM_GET_FLAG(&LptimHandle, LPTIM_FLAG_CMPOK) == RESET) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
init
is normally called outside critical section, so this should presumably be fine usually, but I see one critical section call in ticker_resume
, but I don't see that used? Just wondering if there's a potential IRQ latency issue there.
All timer and ticker tests OK in default and TICKLESS mode (with STM32L073 and STM32L476). |
CI started. |
Test run: SUCCESSSummary: 4 of 4 test jobs passed |
Not related failure, see referenced PR above. We will restart once it is fixed |
client test restarted |
Description
This patches fixes #8714
lp_ticker_set_interrupt implementation causes some IRQ latency and then UART character loss.
Delay in lp_ticker_set_interrupt was added to ensure that we can't write LPTIM_CMP registers too often.
This delay is now removed, thanks to the LPTICKER_DELAY_TICKS feature which guarantee a minimum delay beween lp_ticker_set_interrupt calls.
@mfatiga @c1728p9 @kjbracey-arm @LMESTM
Pull request type