Add tickless to some mbed-os devices #4991

c1728p9 · 2017-08-30T01:51:29Z

Add support for tickless by replacing RTX's SysTick timer code with with code which uses an mbed timer along with suspending and resuming the kernel in the idle loop.

c1728p9 · 2017-08-30T01:51:37Z

/morph test

c1728p9 · 2017-08-30T01:53:49Z

This is roughly based on #4956 but with SysTick replaced entirely.

mbed-bot · 2017-08-30T04:32:20Z

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 1139

Build failed!

c1728p9 · 2017-08-30T15:41:20Z

/morph test

mbed-bot · 2017-08-30T15:41:48Z

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 1143

Build failed!

bulislaw · 2017-08-30T16:19:37Z

CC: @pan-

mbed-bot · 2017-08-30T16:29:12Z

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 1144

Build failed!

c1728p9 · 2017-08-30T17:27:35Z

/morph test

mbed-bot · 2017-08-30T18:59:58Z

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 1145

Test failed!

0xc0170

Looks fine in general.

0xc0170 · 2017-08-31T08:21:59Z

rtos/rtx5/mbed_rtx_idle.cpp

+using namespace mbed;
+
+#if (defined(NO_SYSTICK))
+extern "C" IRQn_Type mbed_get_m0_tick_irqn(void);


Is this public API? targets that do not have systick (very few) should port this, should it be included in ticker api (is it related to tickers or own API just for nosystick rtos?)

missing the documentation what it should return

0xc0170 · 2017-08-31T08:22:22Z

rtos/rtx5/mbed_rtx_idle.cpp

+    /**
+     * Get the time
+     *
+     * @return Current tmie in microseconds


typo time

0xc0170 · 2017-08-31T08:23:48Z

rtos/rtx5/mbed_rtx_idle.cpp

+        schedule_tick();
+    }
+
+    us_timestamp_t start_time;


private members should be prefixed _start_time

0xc0170 · 2017-08-31T08:37:06Z

rtos/rtx5/mbed_rtx_idle.cpp

+        // Ensure SysTick has the correct priority as it is still used
+        // to trigger software interrupts on each tick. The period does
+        // not matter since it will never start counting.
+        SysTick_Setup(16);


What does this do? I would expect to only set a new ticker vector for systick handler, nothing else, so why do we need this systick setup?

Looking at the weak function provided by RTX, its only used in osRtxSysTimerSetup.

This is only used to set the SysTick priority to the low value RTX expects (0xFF). I could put the code which sets the priority here, but to help with readability I called SysTick_Setup instead. Setting the priority is slightly different for each cortex variation and would look something like this:

#if ((__ARM_ARCH_8M_MAIN__ == 1U) || (defined(__CORTEX_M) && (__CORTEX_M == 7U))) SCB->SHPR[11] = 0xFFU; #elif (__ARM_ARCH_8M_BASE__ == 1U) SCB->SHPR[1] |= 0xFF000000U; #elif ((__ARM_ARCH_7M__ == 1U) || \ (__ARM_ARCH_7EM__ == 1U)) SCB->SHP[11] = 0xFFU; #elif (__ARM_ARCH_6M__ == 1U) SCB->SHP[1] |= 0xFF000000U; #endif

0xc0170 · 2017-08-31T08:41:43Z

rtos/rtx5/mbed_rtx_idle.cpp

+            // Don't update to the current tick.
+            // The RTOS must do this to properly
+            // schedule tasks again.
+            new_tick--;


It's not clear to me why we do -1 in case newtick > tick ? If ticks have increased since the last update tick, we substract one from the tick count?

The function svcRtxKernelResume doesn't trigger a task switch if a thread is ready to run. To work around this and jump start scheduling after a tick has passed, one less than the number of ticks that have occurred are passed to svcRtxKernelResume and SysTick is set to pending. The SysTick interrupt then increments the os's tick count to the right value and performs task switching as normal.

After further analysis, it appears that PendSV may be more appropriate in this situation. It will trigger a thread switch without recomputing the tick count. I'll update the PR to use this instead.

0xc0170 · 2017-08-31T08:52:05Z

rtos/rtx5/mbed_rtx_idle.cpp

+static uint64_t os_timer_data[sizeof(RtosTimer) / 8];
+
+/// Setup System Timer.
+int32_t osRtxSysTimerSetup (void)


Isn't there CMSIS-RTOS for tick API? What I found is https://github.com/ARM-software/CMSIS_5/blob/6e5621b3bc4becf50ac5f29a07f30ff2081a5e70/CMSIS/RTOS2/Include/os_tick.h , but that seems to be not in our codebase - https://www.keil.com/pack/doc/CMSIS/RTOS2/html/group__CMSIS__RTOS__TickAPI.html. I guess we get this once we update RTOS

Just to be aware

They've changed the names again, the API was there and is used by nrf51_dk

mbed-os/rtos/rtx5/TARGET_CORTEX_M/rtx_system.c

Line 212 in 5f13881

// ==== Public API ====

bulislaw · 2017-08-31T10:27:18Z

Fails for Ameba:

[1504175141.97][CONN][RXD] >>> Running case #7: 'Testing single thread with wait'...
[1504175141.98][CONN][INF] found KV pair in stream: {{__testcase_start;Testing single thread with wait}}, queued...
[1504175142.02][CONN][RXD] mbed assertation failed: os_timer->get_tick() == svcRtxKernelGetTickCount(), file: /Users/barsza01/devel/mbed/code/mbed-os/rtos?V?V??(Q???V??(Q

https://github.com/ARMmbed/mbed-os/pull/4991/files#diff-d9706ce53abe36e6bcaf49d927f4df58R169

bulislaw · 2017-08-31T12:06:52Z

@Archcady this PR fails for Ameba board, I've tried to see what the platform does around the SysTick, but I don't think it's int the sources (unless i've missed it). Can you have a look and see why would it fail?

pan-

Few observations:

If the SysTick is tied to the us_ticker then an additional latency might be present. Is it a good thing ?
RtosTimer shall take into account OS_TICK_FREQ.
It would be good to have a way to override the behavior in targets, the implementation exposed, on NRF51, is I believe significantly less precise than the old one (it would be good to benchmark it to verify that assertion).

c1728p9 · 2017-09-02T04:08:08Z

If the SysTick is tied to the us_ticker then an additional latency might be present. Is it a good thing ?

This will cause the additional latency of a second interrupt call, but this should be fairly small when compared to a 1ms tick. The reason this is done is that the interrupt that runs SysTick must be run at a low priority. On most if not all platforms, the lp/us ticker interrupt is set to a higher priority than this.

RtosTimer shall take into account OS_TICK_FREQ.

Done

It would be good to have a way to override the behavior in targets, the implementation exposed, on
NRF51, is I believe significantly less precise than the old one (it would be good to benchmark it to verify that assertion).

I'll compare this to the time reported by the us ticker as we discussed. Unfortunately I don't have a board with me so this might have to wait until another day.

c1728p9 · 2017-09-02T04:20:25Z

/morph test

c1728p9 · 2017-09-02T04:34:28Z

Summary of updates:

Addressed PR feedback
Switched from to the US to LP ticker
Removed tick -1 code and now use SetPendSV instead
Put tickless behind MBED_TICKLESS macro
Removed tick comparison assert (Disabling interrupts for too long (2ms+) in application can cause a SysTick interrupt to get dropped which triggers this assert)

mbed-bot · 2017-09-02T06:15:55Z

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 1164

Build failed!

bulislaw · 2017-09-04T11:19:12Z

Both Ameba and GG boards are fixed. Great job @c1728p9 :)

bulislaw · 2017-09-04T13:26:21Z

Ah that's because it's now disabled by default :)

0xc0170 · 2017-09-04T15:10:37Z

@pan- Can you review again please?

mbed-bot · 2017-09-08T05:06:21Z

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 1255

All builds and test passed!

bulislaw · 2017-09-08T08:16:42Z

@pan- are you ok with the state of things?

bulislaw

The idea looks good, will need some refinement in the future, but good enough for now. Ready for merge?

adbridge · 2017-09-08T16:09:06Z

Closed and re-opened to kick off travis again

saedelman · 2018-01-06T04:56:17Z

rtos/TARGET_CORTEX/mbed_rtx_idle.cpp

+
+    core_util_critical_section_enter();
+    uint32_t ticks_to_sleep = svcRtxKernelSuspend();
+    MBED_ASSERT(os_timer->get_tick() == svcRtxKernelGetTickCount());


breaks debugger

@saedelman Can you provide more details, or create an issue to reference this line there so we can review?

Using gdb and single stepping through the RTX code causes the assert to be triggered. This may be related to the "Gotcha" related to in https://github.com/nuket/mbed-memory-status although I am using SEGGER RTT debug output not serial output:

"On mbed 5.5 and up, there may be a Heisenbug when calling print_thread_info() inside of osKernelLock()!

This error sometimes appears on the serial console shortly after chip startup, but not always: mbed assertation failed: os_timer->get_tick() == svcRtxKernelGetTickCount(), file: .\mbed-os\rtos\TARGET_CORTEX\mbed_rtx_idle.c

The RTOS seems to be asserting an idle constraint violation due to the slowness of sending data through the serial port, but it does not happen consistently."

RtosTimer class introduced with tickless support in ARMmbed#4991 had to be renamed because that name was already present in rtos namespace.

c1728p9 force-pushed the tickless branch 2 times, most recently from 6286c12 to c6428a6 Compare August 30, 2017 15:40

c1728p9 force-pushed the tickless branch from c6428a6 to eb66d7b Compare August 30, 2017 17:27

0xc0170 reviewed Aug 31, 2017

View reviewed changes

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

pan- requested changes Aug 31, 2017

View reviewed changes

c1728p9 changed the title ~~Add tickless to all mbed-os devices~~ Add tickless to some mbed-os devices Sep 2, 2017

c1728p9 force-pushed the tickless branch from eb66d7b to be5442c Compare September 2, 2017 04:20

0xc0170 added needs: review and removed needs: work labels Sep 4, 2017

0xc0170 mentioned this pull request Sep 4, 2017

Add tickless idle loop #4956

Closed

c1728p9 force-pushed the tickless branch from be5442c to 8585a14 Compare September 4, 2017 15:29

pan- approved these changes Sep 8, 2017

View reviewed changes

bulislaw approved these changes Sep 8, 2017

View reviewed changes

adbridge closed this Sep 8, 2017

adbridge reopened this Sep 8, 2017

adbridge removed the needs: review label Sep 8, 2017

adbridge added the needs: CI label Sep 8, 2017

theotherjimmy closed this Sep 8, 2017

theotherjimmy reopened this Sep 8, 2017

theotherjimmy added needs: CI ready for merge and removed needs: CI labels Sep 8, 2017

theotherjimmy merged commit e0bc631 into ARMmbed:master Sep 8, 2017

theotherjimmy removed the ready for merge label Sep 8, 2017

pan- mentioned this pull request Sep 12, 2017

NRF52 Queue timer issue when built with release profile #4927

Closed

pan- mentioned this pull request Sep 25, 2017

Sleep: Disallow sleep for targets turning off the systick clock at sleep entry #5086

Merged

This was referenced Sep 28, 2017

Update CMSIS/RTX docs ARMmbed/mbed-os-5-docs#273

Closed

Update CMSIS/RTX docs ARMmbed/mbed-os-5-docs#276

Closed

Rtos changes ARMmbed/mbed-os-5-docs#278

Merged

fkjagodzinski mentioned this pull request Nov 21, 2017

Tests for SysTimer (the idle loop timer for tickless mode) #5548

Merged

saedelman reviewed Jan 6, 2018

View reviewed changes

Add tickless to some mbed-os devices #4991

Add tickless to some mbed-os devices #4991

Uh oh!

Conversation

c1728p9 commented Aug 30, 2017

Uh oh!

c1728p9 commented Aug 30, 2017

Uh oh!

c1728p9 commented Aug 30, 2017

Uh oh!

mbed-bot commented Aug 30, 2017

Output

Uh oh!

c1728p9 commented Aug 30, 2017

Uh oh!

mbed-bot commented Aug 30, 2017

Output

Uh oh!

bulislaw commented Aug 30, 2017

Uh oh!

mbed-bot commented Aug 30, 2017

Output

Uh oh!

c1728p9 commented Aug 30, 2017

Uh oh!

mbed-bot commented Aug 30, 2017

Output

Uh oh!

0xc0170 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!

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!

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

Uh oh!

Uh oh!

bulislaw commented Aug 31, 2017

Uh oh!

pan- left a comment

Choose a reason for hiding this comment

Uh oh!

c1728p9 commented Sep 2, 2017

Uh oh!

c1728p9 commented Sep 2, 2017

Uh oh!

c1728p9 commented Sep 2, 2017

Uh oh!

mbed-bot commented Sep 2, 2017

Output

Uh oh!

bulislaw commented Sep 4, 2017

Uh oh!

bulislaw commented Sep 4, 2017

Uh oh!

0xc0170 commented Sep 4, 2017

Uh oh!

mbed-bot commented Sep 8, 2017

Output

Uh oh!

bulislaw commented Sep 8, 2017

bulislaw commented Aug 31, 2017 •

edited

Loading

adbridge commented Sep 8, 2017 •

edited

Loading