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

Add tickless to some mbed-os devices #4991

Merged
merged 4 commits into from Sep 8, 2017

Conversation

Projects
None yet
9 participants
@c1728p9
Contributor

c1728p9 commented Aug 30, 2017

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

This comment has been minimized.

Contributor

c1728p9 commented Aug 30, 2017

/morph test

@c1728p9

This comment has been minimized.

Contributor

c1728p9 commented Aug 30, 2017

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

@mbed-bot

This comment has been minimized.

mbed-bot commented Aug 30, 2017

Result: FAILURE

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

/morph test

Output

mbed Build Number: 1139

Build failed!

@c1728p9 c1728p9 force-pushed the c1728p9:tickless branch 2 times, most recently Aug 30, 2017

@c1728p9

This comment has been minimized.

Contributor

c1728p9 commented Aug 30, 2017

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Aug 30, 2017

Result: FAILURE

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

/morph test

Output

mbed Build Number: 1143

Build failed!

@bulislaw

This comment has been minimized.

Member

bulislaw commented Aug 30, 2017

CC: @pan-

@mbed-bot

This comment has been minimized.

mbed-bot commented Aug 30, 2017

Result: FAILURE

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

/morph test

Output

mbed Build Number: 1144

Build failed!

@c1728p9 c1728p9 force-pushed the c1728p9:tickless branch Aug 30, 2017

@c1728p9

This comment has been minimized.

Contributor

c1728p9 commented Aug 30, 2017

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Aug 30, 2017

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.

rtos/rtx5/mbed_rtx_idle.cpp Outdated
using namespace mbed;
#if (defined(NO_SYSTICK))
extern "C" IRQn_Type mbed_get_m0_tick_irqn(void);

This comment has been minimized.

@0xc0170

0xc0170 Aug 31, 2017

Member

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

rtos/rtx5/mbed_rtx_idle.cpp Outdated
/**
* Get the time
*
* @return Current tmie in microseconds

This comment has been minimized.

@0xc0170

0xc0170 Aug 31, 2017

Member

typo time

This comment has been minimized.

@c1728p9

c1728p9 Sep 2, 2017

Contributor

done

rtos/rtx5/mbed_rtx_idle.cpp Outdated
schedule_tick();
}
us_timestamp_t start_time;

This comment has been minimized.

@0xc0170

0xc0170 Aug 31, 2017

Member

private members should be prefixed _start_time

This comment has been minimized.

@c1728p9

c1728p9 Sep 2, 2017

Contributor

done

rtos/rtx5/mbed_rtx_idle.cpp Outdated
// 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);

This comment has been minimized.

@0xc0170

0xc0170 Aug 31, 2017

Member

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 comment has been minimized.

@c1728p9

c1728p9 Sep 2, 2017

Contributor

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
rtos/rtx5/mbed_rtx_idle.cpp Outdated
// Don't update to the current tick.
// The RTOS must do this to properly
// schedule tasks again.
new_tick--;

This comment has been minimized.

@0xc0170

0xc0170 Aug 31, 2017

Member

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?

This comment has been minimized.

@c1728p9

c1728p9 Sep 2, 2017

Contributor

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.

rtos/rtx5/mbed_rtx_idle.cpp Outdated
static uint64_t os_timer_data[sizeof(RtosTimer) / 8];
/// Setup System Timer.
int32_t osRtxSysTimerSetup (void)

This comment has been minimized.

@0xc0170

0xc0170 Aug 31, 2017

Member

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

This comment has been minimized.

@bulislaw

bulislaw Aug 31, 2017

Member

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

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

@bulislaw

This comment has been minimized.

Member

bulislaw commented Aug 31, 2017

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

This comment has been minimized.

Member

bulislaw commented Aug 31, 2017

@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?

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

@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 c1728p9 changed the title from Add tickless to all mbed-os devices to Add tickless to some mbed-os devices Sep 2, 2017

@c1728p9

This comment has been minimized.

Contributor

c1728p9 commented Sep 2, 2017

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 c1728p9 force-pushed the c1728p9:tickless branch Sep 2, 2017

@c1728p9

This comment has been minimized.

Contributor

c1728p9 commented Sep 2, 2017

/morph test

@c1728p9

This comment has been minimized.

Contributor

c1728p9 commented Sep 2, 2017

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

This comment has been minimized.

mbed-bot commented Sep 2, 2017

Result: FAILURE

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

/morph test

Output

mbed Build Number: 1164

Build failed!

@bulislaw

This comment has been minimized.

Member

bulislaw commented Sep 4, 2017

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

@bulislaw

This comment has been minimized.

Member

bulislaw commented Sep 4, 2017

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

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 4, 2017

@pan- Can you review again please?

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

@0xc0170 0xc0170 referenced this pull request Sep 4, 2017

Closed

Add tickless idle loop #4956

@c1728p9 c1728p9 force-pushed the c1728p9:tickless branch Sep 4, 2017

@bulislaw

This comment has been minimized.

Member

bulislaw commented Sep 8, 2017

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

@pan-

pan- approved these changes Sep 8, 2017

@bulislaw

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

@adbridge adbridge closed this Sep 8, 2017

@adbridge adbridge reopened this Sep 8, 2017

@adbridge adbridge removed the needs: review label Sep 8, 2017

@adbridge

This comment has been minimized.

Contributor

adbridge commented Sep 8, 2017

Closed and re-opened to kick off travis again

@adbridge adbridge added the needs: CI label Sep 8, 2017

@theotherjimmy theotherjimmy reopened this Sep 8, 2017

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

4 checks passed

Cam-CI uvisor Build & Test Success
Details
ci/morph-test Job has completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

fkjagodzinski added a commit to fkjagodzinski/mbed-os that referenced this pull request Nov 24, 2017

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

fkjagodzinski added a commit to fkjagodzinski/mbed-os that referenced this pull request Dec 5, 2017

RTOS: SysTimer: Extract RtosTimer as SysTimer
RtosTimer class introduced with tickless support in ARMmbed#4991 had to be
renamed because that name was already present in rtos namespace.
core_util_critical_section_enter();
uint32_t ticks_to_sleep = svcRtxKernelSuspend();
MBED_ASSERT(os_timer->get_tick() == svcRtxKernelGetTickCount());

This comment has been minimized.

@saedelman

saedelman Jan 6, 2018

breaks debugger

This comment has been minimized.

@0xc0170

0xc0170 Jan 8, 2018

Member

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

This comment has been minimized.

@saedelman

saedelman Jan 8, 2018

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."

fkjagodzinski added a commit to fkjagodzinski/mbed-os that referenced this pull request Feb 28, 2018

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

bulislaw added a commit to bulislaw/mbed-os that referenced this pull request Mar 1, 2018

RTOS: SysTimer: Extract RtosTimer as SysTimer
RtosTimer class introduced with tickless support in ARMmbed#4991 had to be
renamed because that name was already present in rtos namespace.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment