Add tickless idle loop #4956

0xc0170 · 2017-08-22T16:48:03Z

Use low power ticker if it is available to suspend kernel and keep ticks.
I added simple test that just waits and make sure we wake up.

cc @c1728p9 @pan- @bulislaw

Add tickless support to the rtos idle loop. Using low power ticker and RTOS suspend/resume functionality. We set the next event according to the kernel, and wake-up within this period. If lowpower ticker is not available, we fallback to the previous only sleep function.

pan-

Good submission but I'm under the impression the test included is pointless because it test the wait function, not the function putting the MCU to sleep.

If the function default_idle_hook was public it would be possible to test it directly instead of indirectly. Would it be possible to also add tests ensuring that the MCU wakeup when an IRQ is raised while it is asleep ?

pan- · 2017-08-23T09:03:38Z

rtos/rtos_idle.c

+    timestamp_t time_in_sleep = 0UL;
+
+    core_util_critical_section_enter();
+    uint32_t tick_freq = svcRtxKernelGetTickFreq();


This can be a static constant initialized once. SVC calls are not free.

We are in C world, this would need to be a constant. This is not anywhere as const, only as RTX macro that we might not want to pull in?

All the expressions in an initializer for an object that has static or thread storage duration shall be constant expressions or string literals.

😞

bulislaw

Also it would be good to at least add a note that people shouldn't use the lowpower ticker if the tickless mode is in use.

bulislaw · 2017-08-23T08:48:14Z

rtos/rtos_idle.c

-       Unfortunately, this usually requires disconnecting the interface chip (debugger).
-       This can be done, but it would break the local file system.
-    */
+#if DEVICE_LOWPOWERTIMER


I would add another device cap "TICKLESS" because to be fair I can see that device has both sleep and lowpower ticker and it still fails to do it properly.

We got test for that - lp ticker in HAL tests - sleep and wake up by lp ticker. if that fails, then this would fail also. lp ticker had always a requirement to wake up from sleep and also deepsleep. This one does at the moment only sleep, if it does not work it's a bug.

bulislaw · 2017-08-23T09:10:34Z

rtos/rtos_idle.c

+    if (ticks_to_sleep) {
+        uint64_t us_to_sleep = 1000000 * ticks_to_sleep / tick_freq;
+        // Calculate the maximum period we can sleep and stay within
+        if (us_to_sleep >= INT_MAX) {


UINT32_MAX?

Indeed the truncation is not needed because ticker_insert_event_us is used.

bulislaw · 2017-08-23T09:12:07Z

rtos/rtos_idle.c

+    timestamp_t time_in_sleep = 0UL;
+
+    core_util_critical_section_enter();
+    uint32_t tick_freq = svcRtxKernelGetTickFreq();


You shouldn't use internal RTX functions. The API is osKernel*

The reason for this is to use critical sections. I had it previously (see the earlier commit) but then there might be a race condition. osKernel function cannot be invoked within critical section :(

I don't see much harm in having interrupt there, if you think it's a problem we should create an issue for RTOS.

If the critical section is removed and svcRtxKernelSuspend is replaced by osKernelSuspend and a thread is set as ready and run between the osKernelSuspend and sleep ~~then the number of ticks acquired by osKernelSuspend is invalid and the OS might be put to sleep for an incorrect amount of time~~ then it won't be serviced immediately which can be critical for IRQ deferring the work in userland.

pan- · 2017-08-23T09:55:19Z

Also it would be good to at least add a note that people shouldn't use the lowpower ticker if the tickless mode is in use.

Why ? Ticker events are serialized in a queue, if an event registered by the user is fired before the event registered by default_idle_hook then like with any other IRQ the kernel is awaken. It might be good to remove idle_loop_event from the queue, after sleep, if it wasn't fired though.

Basic functionality, to test Thread::wait() with tickless. This is not accurancy test, just that an application wakes up within the expected period + some margin.

0xc0170 · 2017-08-23T12:21:49Z

@pan- The testing needs more love. I reworked it a bit (fixed Thread::wait(), add kernel tick check). One more addition would be to make idle loop pointer public and test that directly (one thing there to be done is to set kernel next event = that would be our expected wake up time). Any other suggestions?

0xc0170 · 2017-08-23T15:34:57Z

I run only rtos tests on one platform , this is the result:

+--------------+---------------+-------------------------------------+--------+--------------------+-------------+
| target       | platform_name | test suite                          | result | elapsed_time (sec) | copy_method |
+--------------+---------------+-------------------------------------+--------+--------------------+-------------+
| K64F-GCC_ARM | K64F          | tests-mbedmicro-rtos-mbed-basic     | OK     | 20.85              | shell       |
| K64F-GCC_ARM | K64F          | tests-mbedmicro-rtos-mbed-idle_loop | OK     | 13.29              | shell       |
| K64F-GCC_ARM | K64F          | tests-mbedmicro-rtos-mbed-isr       | OK     | 16.96              | shell       |
| K64F-GCC_ARM | K64F          | tests-mbedmicro-rtos-mbed-mail      | OK     | 14.08              | shell       |
| K64F-GCC_ARM | K64F          | tests-mbedmicro-rtos-mbed-malloc    | OK     | 26.41              | shell       |
| K64F-GCC_ARM | K64F          | tests-mbedmicro-rtos-mbed-mutex     | OK     | 17.95              | shell       |
| K64F-GCC_ARM | K64F          | tests-mbedmicro-rtos-mbed-queue     | FAIL   | 14.08              | shell       |
| K64F-GCC_ARM | K64F          | tests-mbedmicro-rtos-mbed-semaphore | FAIL   | 12.98              | shell       |
| K64F-GCC_ARM | K64F          | tests-mbedmicro-rtos-mbed-signals   | OK     | 19.92              | shell       |
| K64F-GCC_ARM | K64F          | tests-mbedmicro-rtos-mbed-threads   | FAIL   | 22.51              | shell       |
+--------------+---------------+-------------------------------------+--------+--------------------+-------------+
mbedgt: test suite results: 3 FAIL / 7 OK

Feel free to push to this branch to update if I am not around.

bulislaw · 2017-08-23T15:35:37Z

I'll have a look at the failures.

bulislaw · 2017-08-25T10:43:57Z

The tests are timing out, when the timeout is risen we get sleep accuracy errors. eg

:374::FAIL: Values Not Within Delta 50000 Expected 500000 Was 726460

:374::FAIL: Values Not Within Delta 50000 Expected 1500000 Was 259272

The differences between sleep requested and actual are up to 50%

Removing call to sleep from the idle handler doesn't change anything - still takes to much time.

bulislaw · 2017-08-25T13:32:44Z

rtos/rtos_idle.c

+        ticker_remove_event(lp_ticker_data, &idle_loop_event);
+        ticker_set_handler(lp_ticker_data, &idle_loop_handler);
+        timestamp_t start_time = lp_ticker_read();
+        ticker_insert_event_us(lp_ticker_data, &idle_loop_event, us_to_sleep, (uint32_t)&idle_loop_event);


I don't think it's correct, us_to_sleep should be a time stamp not delay to wait https://github.com/ARMmbed/mbed-os/blob/master/hal/ticker_api.h#L143

Change over os calls to use the svc equivalent. Also fixed time calculations.

0xc0170 · 2017-08-29T17:19:32Z

https://github.com/ARMmbed/mbed-os/blob/master/rtos/Thread.cpp#L321 - look at this implementation, osDelay was changed in RTOS2, the argument is ticks now, previously it was milliseconds.

If someone changes tick freq, the code becomes incorrect.

0xc0170 · 2017-09-04T15:14:49Z

Most likely replaced by Add tickless to some mbed-os devices #4991. I'll close this one.

0xc0170 force-pushed the dev_tickless_rtos branch from 7327d0f to 4b4b6f3 Compare August 22, 2017 17:00

0xc0170 added the needs: work label Aug 23, 2017

pan- requested changes Aug 23, 2017

View reviewed changes

bulislaw suggested changes Aug 23, 2017

View reviewed changes

0xc0170 force-pushed the dev_tickless_rtos branch 2 times, most recently from fc67bd2 to d812281 Compare August 23, 2017 10:24

test: add rtos idle loop

bc235d6

Basic functionality, to test Thread::wait() with tickless. This is not accurancy test, just that an application wakes up within the expected period + some margin.

0xc0170 force-pushed the dev_tickless_rtos branch from d812281 to 45f9423 Compare August 23, 2017 12:07

0xc0170 force-pushed the dev_tickless_rtos branch from 8639575 to fc069d1 Compare August 23, 2017 12:26

bulislaw reviewed Aug 25, 2017

View reviewed changes

pan- mentioned this pull request Aug 25, 2017

NRF52 Queue timer issue when built with release profile #4927

Closed

c1728p9 and others added 2 commits August 29, 2017 06:53

Update tickless code

b0b8d1e

Change over os calls to use the svc equivalent. Also fixed time calculations.

test: add tick check in idle loop

091a6aa

0xc0170 force-pushed the dev_tickless_rtos branch from fc069d1 to 091a6aa Compare August 29, 2017 05:54

c1728p9 mentioned this pull request Aug 30, 2017

Add tickless to some mbed-os devices #4991

Merged

0xc0170 closed this Sep 4, 2017

0xc0170 removed the needs: work label Sep 4, 2017

Add tickless idle loop #4956

Add tickless idle loop #4956

Uh oh!

Conversation

0xc0170 commented Aug 22, 2017

Uh oh!

pan- left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bulislaw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xc0170 Aug 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pan- Aug 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pan- commented Aug 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

0xc0170 commented Aug 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

0xc0170 commented Aug 23, 2017

Uh oh!

bulislaw commented Aug 23, 2017

Uh oh!

bulislaw commented Aug 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xc0170 commented Aug 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

0xc0170 commented Sep 4, 2017

Uh oh!

Uh oh!

0xc0170 Aug 23, 2017 •

edited

Loading

pan- Aug 23, 2017 •

edited

Loading

pan- commented Aug 23, 2017 •

edited

Loading

0xc0170 commented Aug 23, 2017 •

edited

Loading

bulislaw commented Aug 25, 2017 •

edited

Loading

0xc0170 commented Aug 29, 2017 •

edited

Loading