-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Update HAL Sleep manager test to cope with STM32 LPTIM HW #10700
Conversation
ede1577
to
acfb6f8
Compare
Updated to keep the code in test setup that make sure that tickers are initialized. |
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.
LGTM
In particular and as kindly suggested by Przemec S. : 1. Add setup/teardown handler’s for all cases. This disables sys-tick, so there should be no unexpected lp ticker interrupt scheduling. 2. Modify setup/teardown handler’s: remove suspension of lp/us tickers, so they can count as this is required by test_lock_unlock_test_check test case. 3. Use TEST_ASSERT_TRUE(sleep_manager_can_deep_sleep_test_check()) after setting interrupt to cope with STM specific handling (CMPOK interrupt with deep-sleep locked). This performs wait only if needed and will not affect other targets which do not need extra wait. 4. Move sleep_manager_lock_deep_sleep() after TEST_ASSERT_TRUE(sleep_manager_can_deep_sleep_test_check()) 5. Use TEST_ASSERT_TRUE(sleep_manager_can_deep_sleep_test_check()) in test_lock_unlock_test_check to let lower layers manage deep sleep.
acfb6f8
to
d4db0f3
Compare
@mprse I've updated the PR - could be great if you can help getting more reviews or starting CI |
@0xc0170 who else needs to review ? |
and avoid associated warning ...
I'll review shortly |
Hi @LMESTM @fkjagodzinski ! After the @fkjagodzinski review, I suggest modifying the test as follows: mprse@90880ff BTW I added a loop in |
@mprse Otherwise, how would you suggest to proceed : do you want to make a new PR to replace this one, or shall I cherry-pick your suggested commit if @fkjagodzinski agrees this is a better way forward ? |
83e274a
to
db57cd0
Compare
I think we should continue this one. If you agree with proposed changes and they are also accepted by @fkjagodzinski feel free to cherry-pick or use provided commits to further changes. |
@fkjagodzinski could you please provide feedback on @mprse changes before I consider including them into this PR ? @mprse I'll rely on you explaining the changes even if I take them in this PR :-) |
Changes: - restore the original form of setup/teardown handlers, - test_lock_unlock_test_check(): do not use common ticker layer (Timer, Timeout). Use only ticker HAL layer. - Increase DEEP_SLEEP_TEST_CHECK_WAIT_DELTA_US delta.
…ayer to count elapsed time.
With the DEEP_SLEEP_TEST_CHECK_WAIT_DELTA_US increased, we now have TEST_ASSERT_UINT64_WITHIN(delta=1000, expected=1000, actual=1000) so this assertion needed to be updated. What we need is the deep sleep to be enabled after the programed interrupt has fired and before a 2ms timeout expiration, which means >= 1000 and < 2000.
45a0fef
to
8476be6
Compare
Just fixed a minor astyle issue |
@adbridge I think I've done the requested changes. I guess that Label needs work could be removed. |
CI started |
Test run: FAILEDSummary: 2 of 11 test jobs failed Failed test jobs:
|
@fkjagodzinski @mprse Could you have a look at the CI failure root cause ? |
@LMESTM the
|
@fkjagodzinski Cherry-pick fails ... it seems you haven't taken my latest commit. |
Sorry about that; indeed, I used an outdated branch from your repo. Here is the new commit that should work fine: fkjagodzinski@a1436c5. BTW, I've just noticed that |
Add missing `lp_ticker_clear_interrupt()` in the interrput handler used in `test_lock_unlock_test_check()` test. Remove redefined `us_to_ticks()`.
Branch updated with your fix. |
@0xc0170 @adbridge - the issue found in CI should have been solved with fix from @fkjagodzinski now part of this PR - could you restart CI to check it's ok ? |
CI started |
Test run: SUCCESSSummary: 11 of 11 test jobs passed |
Description
Update HAL Sleep manager test to cope with STM32 LPTIM HW
those changes are generic and should not create any regression to other targets, but are required for oncoming changes on STM targets (#10701)
In particular and as kindly suggested by Przemec S. :
test_lock_unlock_test_check to let lower layers manage deep sleep.
Pull request type
Reviewers
@jeromecoutant @mprse
Release Notes