Skip to content
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 : Fix tickless and LPTICKER_DELAY_TICKS #8242

Merged
merged 3 commits into from
Oct 5, 2018

Conversation

jeromecoutant
Copy link
Collaborator

Description

This should fix #7858,
and is needed to pass new test case from #8129

@c1728p9

Test result

With that patch, all tests are OK in default mode and in TICKLESS mode for targets using LPTICKER with LPTIM.
I will push a separate PR to enable TICKLESS for them.

Restriction

In tickless mode, for targets using LPTICKER with RTC, it seems there is still some issue
with the RTC write procedure.
I will push a new PR when solution is found.

Pull request type

[x] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Breaking change

@jeromecoutant
Copy link
Collaborator Author

Note that my TICKLESS tests was including #8223

@jeromecoutant
Copy link
Collaborator Author

0xc0170 added the needs: preceding PR

I don't think so, you can start CI

@c1728p9 c1728p9 self-requested a review September 25, 2018 22:59
},
"lpticker_delay_ticks": {
"help": "https://os.mbed.com/docs/v5.9/reference/low-power-ticker.html",
"value": 1,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is only one cycle needed?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good idea for the link. Just maybe replace "v5.9" with "latest".

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In some previous patch, I have removed this delay tick, but it seems that not all the tests can pass...
Then I set back value to 1 as the default value.

}
},
"overrides": {"lpticker_delay_ticks": 4},
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are 4 needed for the L series?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Default value 1 seems not enough for low speed targets.
4 is the experimental result to pass speed test.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there anything in the reference manual for these devices which state the number of cycles which must elapse before writing to the match register?

@@ -263,7 +258,6 @@ uint32_t lp_ticker_read(void)

void lp_ticker_set_interrupt(timestamp_t timestamp)
{
lp_ticker_disable_interrupt();
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you have any details on how this fixes the test in #8129?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, I should add the details you have added in #8129

@@ -194,21 +191,22 @@ void lp_ticker_set_interrupt(timestamp_t timestamp)
LptimHandle.Instance = LPTIM1;
irq_handler = (void (*)(void))lp_ticker_irq_handler;

__HAL_LPTIM_CLEAR_FLAG(&LptimHandle, LPTIM_FLAG_CMPOK);
__HAL_LPTIM_COMPARE_SET(&LptimHandle, timestamp);
/* 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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this bring back the blocking behavior?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, it is the same behavior as you have done in #8129

Copy link
Contributor

Choose a reason for hiding this comment

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

The change in #8129 caused blocking behavior as mentioned by @LMESTM.

Copy link
Contributor

Choose a reason for hiding this comment

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

@c1728p9 my mistake - Jérôme explained to me and I actually misunderstood CMPOK (programing ok) with CMPM (comparator match) - looks ok to me now

@c1728p9
Copy link
Contributor

c1728p9 commented Sep 25, 2018

@0xc0170 why is this marked as "needs: preceding PR"? PR #8223 is pending on this.

@0xc0170
Copy link
Contributor

0xc0170 commented Sep 26, 2018

Removing, I understood it otherway around

This fixes issue with mbed_hal/lp_ticker/lp_ticker_early_match_race_test
When both tickless and LPTICKER_DELAY_TICKS are enabled some ST
devices randomly get stuck sleeping forever. This is because the
wake up time passed to the rtc is ignored if the previous match is
about to occur. This causes the device to get stuck in sleep.

This patch prevents matches from getting dropped by the rtc by
deactivating the rtc wake up timer before setting a new value.

Events leading up to this failure for the RTC:

-1st call to lp_ticker_set_interrupt
-delay until ticker interrupt is about to fire
-2nd call to lp_ticker_set_interrupt
-interrupt for 1st call fires and match time for 2nd call is dropped
-LowPowerTickerWrapper gets ticker interrupt but treats it as a
 spurious interrupt and drops it since it comes in too early
-device enters sleep without a wakeup source and locks up
For both implementation, RTC and LPTIM, there is some delay in the
set_interrupt function due to HW constraints.

Value has been set to 4 for STM32L0,
because SystemClock is slower than other families.
@jeromecoutant
Copy link
Collaborator Author

Rebase done.

@c1728p9 maybe you can approve and start CI ?

Thx

@0xc0170
Copy link
Contributor

0xc0170 commented Oct 1, 2018

/morph build

@mbed-ci
Copy link

mbed-ci commented Oct 1, 2018

Build : SUCCESS

Build number : 3203
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/8242/

Triggering tests

/morph test
/morph export-build
/morph mbed2-build

@mbed-ci
Copy link

mbed-ci commented Oct 1, 2018

@mbed-ci
Copy link

mbed-ci commented Oct 2, 2018

@0xc0170
Copy link
Contributor

0xc0170 commented Oct 2, 2018

Test needs to be restarted (device was power cycled) and exporters as well, will do once we fix exporters CI job

@0xc0170
Copy link
Contributor

0xc0170 commented Oct 4, 2018

/morph test

@mbed-ci
Copy link

mbed-ci commented Oct 4, 2018

Test : SUCCESS

Build number : 3038
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/8242/3038

@0xc0170
Copy link
Contributor

0xc0170 commented Oct 5, 2018

/morph export-build

@mbed-ci
Copy link

mbed-ci commented Oct 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Recent LP Ticker Changes Break Tickless
6 participants