-
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
Fixing lp ticker and sleep manager tests #5063
Conversation
/morph test-nightly |
Result: FAILUREYour command has finished executing! Here's what you wrote!
OutputBuild failed! |
7a4176b
to
8e094b8
Compare
/morph test-nightly |
@@ -37,8 +39,8 @@ void sleep_manager_multithread_test() | |||
{ | |||
{ | |||
Callback<void()> cb(sleep_manager_locking_thread_test); | |||
Thread t1; | |||
Thread t2; | |||
Thread t1(osPriorityNormal, TEST_STACK_SIZE); |
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.
this multithreaded test should be guarded by MBED_RTOS_SINGLE_THREAD
. I am looking at what this macro is changing, and found only tests that use it and then toolchains python files that define it. Pointers?
It seems to me that we removed this macro from rtos code base (even if this macro is set, you can create multiple threads?), as previously it set task count to 1 as I recall, or not?
8e094b8
to
406aac5
Compare
I pushed a small update
|
This is now outdated, latest master test shows this OK :-) I am not certain what has happened previously in the CI :-) We will keep you updated if anything new with this. |
Result: FAILUREYour command has finished executing! Here's what you wrote!
OutputTest failed! |
Decrease the default stack size as it's not needed for purposes of testing.
Lock deep sleep before calling sleep to prevent the device from entering deep sleep mode.
Increase the Timeout period from 1ms to 10ms so interrupt latency has 1/10th the effect on the measurement. This prevents failures due to interrupt latency causing a drift.
The NCS36510 is not suitable for tickless, since its LP ticker cannot be scheduled fast enough. This is because it takes four 32KHz clock cycles before these writes take effect - ~120us.
406aac5
to
acf4282
Compare
/morph test-nightly |
I updated this PR by doing the following:
I also confirmed that the NRF52 was failing testing earlier due a task switch (SysTick) during a critical section. Additionally, I can confirm that the Tickless fixes this problem since SysTick never triggers an interrupt. |
👍 Changes look good to me, thanks for the update |
Result: FAILUREYour command has finished executing! Here's what you wrote!
OutputTest failed! |
The lp_ticker test overrides the default ticker handler for the low power ticker. This stops all other low power TimerEvents in the system, including the ones for tickless, from getting called. Because of this devices with tickless enabled malfunction during this test. This patch fixes this problem by passing all lp ticker events it did not trigger on to the TimerEvent irq handler.
Add NOPs after deep sleep to prevent unexpected behavior. It appears that the first one or two instructions after deep sleep do not get executed properly. Note - This is a temporary workaround. For a more robust solution the NCS36510 needs to investigate the root cause of this issue.
|
/morph test-nightly |
1 similar comment
/morph test-nightly |
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
@c1728p9 @bulislaw
target=NCS36510,toolchain=IAR Any ideas ? |
Result: FAILUREYour command has finished executing! Here's what you wrote!
OutputTest failed! |
We don't have this board in Cambridge, but looking at the sleep code the deeplseep is switching off external oscillator and lp ticker is implemented using RTC, I'm guessing we are switching off our wake up source... |
Maybe it's the same as issue #5067?!? |
/morph test-nightly |
That doesn't make sense as we use standard SysTick on this platform... |
@c1728p9 is this a bug, right? What mean Tickless fixes the problem - Will this been fixed for all Tick modes? |
Hi @nvlsianpu, tickless is on unconditionally for the nrf52, so this shouldn't be a problem. The nrf5x critical section customization makes me a bit nervous though. It doesn't mask core interrupts (like SysTick) and this was why it task switched in the middle of a critical section. |
I pushed a commit to fix the ncs36510 failures and triggered a nightly. I opened issue #5065 earlier to track this issue. |
Result: SUCCESSYour command has finished executing! Here's what you wrote!
OutputAll builds and test passed! |
Couple of fixes here:
See failures: #4987 (comment)
Whats left to do - nrf52 critical section - seems like the critical section does not work, look at sleep manager race condition. It fails. This is interesting as we have some race conditions tests, for instance also for threads. I'll review the test again, are we using it correctly, and the bug is in nordic implementation of critical section, as the rest of platforms are fine (using default implementation of critical section)
cc @nvlsianpu @anangl @pan- Any suggestions, fixes welcome !
This will require nightly as its fixing some tests.
@c1728p9 @bulislaw @sg-