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

fkjagodzinski · 2017-11-21T14:33:27Z

Description

Add tests for an RtosTimer class introduced with TICKLESS mode support in #4991.

extracted a RtosTimer class from

mbed-os/rtos/TARGET_CORTEX/mbed_rtx_idle.cpp

Line 49 in a519b84

class RtosTimer : private TimerEvent {

as rtos/SysTimer,
added unit tests.

Status

READY

Issues to be resolved before merge:

~~Fire interrupt function broken for nrf5x targets #5284~~

fkjagodzinski · 2017-11-21T14:46:00Z

Regarding CI test runs -- this test suite needs first to be compiled with -DMBED_TICKLESS i.e.:

mbed test -t GCC_ARM -m NUCLEO_L476RG -n tests-mbedmicro-rtos-mbed-systimer -DMBED_TICKLESS --compile

and then run without -DMBED_TICKLESS i.e.:

mbed test -t GCC_ARM -m NUCLEO_L476RG -n tests-mbedmicro-rtos-mbed-systimer --run -v

Where do I need to include this setup?

bulislaw · 2017-11-21T14:56:30Z

MBED_TICKLESS should be defined by targets that support tickless feature so I don't think you need to do anything special, it'll be tested by default for supported hw.

bulislaw

LGTM

0xc0170 · 2017-11-22T11:26:32Z

This PR confused me first. The headline is about tests, but this introduces a new API: SysTimer . Please be more detailed in the description and the commit messages (expose details, do not hide them). Previously it was internal, now it is being exposed as public API.

We still do not have here http://www.keil.com/pack/doc/CMSIS/RTOS2/html/group__CMSIS__RTOS__TickAPI.html ? Wouldn't this API completely deprecate this SysTimer ?

How does this addition stands against RTOSTimer that is deprecated, and EventQueue, see this quote from the RTOSTimer:

@deprecated
 The RtosTimer has been superseded by the EventQueue. The RtosTimer and EventQueue duplicate
 the functionality of timing events outside of interrupt context, however the EventQueue
 has additional features to handle deferring other events to multiple contexts.

This is bringing new Timer to the user space. How does this compare to RTOSTimer, EventQueue, Timer/LowPowerTimer?

fkjagodzinski · 2017-11-22T12:06:36Z

@0xc0170 This particular PR adds tests for a class named RtosTimer that was introduced with #4991 in

mbed-os/rtos/TARGET_CORTEX/mbed_rtx_idle.cpp

Line 49 in a519b84

class RtosTimer : private TimerEvent {

Previously it was internal, now it is being exposed as public API.

This is only to make this class accessible for test code.

How does this addition stands against RTOSTimer that is deprecated

Apart from the class name -- it's unrelated.

This is bringing new Timer to the user space.

True. Maybe this could be avoided by putting class files some other place. Could anyone point the appropriate directory? Would rtos/TARGET_CORTEX/SysTimer.h be ok?

theotherjimmy · 2017-11-22T15:35:07Z

I added a minor release version as minor releases may contain additions to the public set of API's. Patch releases may not contain additions to the public API set.

theotherjimmy · 2017-11-22T15:35:42Z

I think this is waiting on clarification/review from @0xc0170

bulislaw · 2017-11-22T16:11:20Z

Ah! I've missed the directory change. @0xc0170 good catch! Shall we move it to rtos/TARGET_CORTEX/ to separate it from RTOS APIs and in doxygen make explicit note that it's not part of the Mbed RTOS API and it's internal implementation of System Timer.

0xc0170 · 2017-11-24T13:10:48Z

Ah! I've missed the directory change. @0xc0170 good catch! Shall we move it to rtos/TARGET_CORTEX/ to separate it from RTOS APIs and in doxygen make explicit note that it's not part of the Mbed RTOS API and it's internal implementation of System Timer.

@fkjagodzinski What do you think? This would work (having it not part of public API, no inclusion besides the file that is using this internal API) and stating in the docs that this is internal detail.

fkjagodzinski · 2017-11-24T18:00:11Z

Thanks for the comments! Here is the update:

moved SysTimer files to rtos/TARGET_CORTEX/,

updated doxygen -- now the docs for SysTimer will generate only if

mbed-os/doxyfile_options

Lines 637 to 641 in 4e22295

    
           # The ENABLED_SECTIONS tag can be used to enable conditional documentation 
        
           # sections, marked by \if <section_label> ... \endif and \cond <section_label> 
        
           # ... \endcond blocks. 
        
           ENABLED_SECTIONS       =

contains RTOS_INTERNAL. This is my initial prposition regarding doxygen. Do you have any suggestions on how this should be handled?

bulislaw · 2017-11-27T12:25:48Z

rtos/TARGET_CORTEX/mbed_rtx_idle.cpp


 /// Enable System Timer.
 int32_t OS_Tick_Enable (void)
 {
    // Do not use SingletonPtr since this relies on the RTOS
    if (NULL == os_timer) {
-        os_timer = new (os_timer_data) RtosTimer();
+        os_timer = new (os_timer_data) rtos::SysTimer();


Why did we split the constructor into 2 calls?

I did that to avoid manipulating SysTick / IRQ registers while running tests. I also extracted set_irq_pending() from handler() for same reason.

0xc0170 · 2017-11-27T15:32:40Z

rtos/TARGET_CORTEX/SysTimer.h

+#include "platform/NonCopyable.h"
+#include "drivers/TimerEvent.h"
+
+namespace rtos {


Looking at the test code, and here. This is in the rtos namespace. Is enough just docs note ?

I would propose to create own namespace for this within rtos, to show the intent, internal namespace within rtos: rtos::internal

Ok, I added an additional internal namespace as suggested.

0xc0170 · 2017-11-30T16:31:58Z

/morph build

mbed-ci · 2017-11-30T17:04:48Z

Build : SUCCESS

Build number : 629
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/5548/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build

mbed-ci · 2017-11-30T18:30:03Z

Exporter Build : SUCCESS

Build number : 256
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/exporter/5548/

mbed-ci · 2017-11-30T19:43:35Z

Test : FAILURE

Build number : 453
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/5548/453

bulislaw · 2017-12-01T11:06:38Z

@fkjagodzinski these tests should only be enabled for platforms defining TICKLESS macro.

fkjagodzinski · 2017-12-05T10:24:09Z

Update:

as suggested, tests will run only for targets with MBED_TICKLESS defined,
rebased onto master,
squashed commits for readability.

Regarding test failures on NRF51_DK -- #5284 has to be resolved first.

mbed-ci · 2018-02-27T14:42:04Z

Exporter Build : SUCCESS

Build number : 941
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/exporter/5548/

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

fkjagodzinski · 2018-02-28T14:01:45Z

Update:

rebased onto master, which is now at 8b6a7aa,
updated tests:
- removed test_reschedule() which duplicated test_update_tick(),
- fixed test_update_tick(),
- fixed test_deepsleep().

@c1728p9, @bulislaw I can see that rebase deprecated your reviews; the SysTimer class wasn't modified.

fkjagodzinski · 2018-02-28T16:25:44Z

@cmonr, @0xc0170 Looks like this PR is ready. Could you trigger morph tests please?

cmonr · 2018-02-28T16:41:04Z

/morph build

mbed-ci · 2018-02-28T17:09:42Z

Build : SUCCESS

Build number : 1299
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/5548/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

mbed-ci · 2018-02-28T17:27:14Z

Build : SUCCESS

Build number : 1300
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/5548/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

mbed-ci · 2018-02-28T18:33:17Z

Exporter Build : SUCCESS

Build number : 960
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/exporter/5548/

mbed-ci · 2018-02-28T18:33:18Z

Exporter Build : SUCCESS

Build number : 960
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/exporter/5548/

cmonr · 2018-02-28T19:50:13Z

/morph build

cmonr · 2018-02-28T19:51:41Z

@fkjagodzinski Sorry about the multiple builds. Stale web pages don't auto-refresh, making me think I still need to invoke commands.

mbed-ci · 2018-02-28T20:23:37Z

Build : SUCCESS

Build number : 1304
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/5548/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

mbed-ci · 2018-02-28T21:05:23Z

Test : SUCCESS

Build number : 1085
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/5548/1085

mbed-ci · 2018-02-28T22:41:09Z

Exporter Build : SUCCESS

Build number : 964
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/exporter/5548/

mbed-ci · 2018-03-01T00:44:52Z

Test : SUCCESS

Build number : 1088
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/5548/1088

bulislaw previously approved these changes Nov 21, 2017

View reviewed changes

bulislaw requested a review from c1728p9 November 21, 2017 16:44

fkjagodzinski changed the title ~~SysTimer tests~~ Tests for SysTimer (the idle loop timer for tickless mode) Nov 22, 2017

theotherjimmy added BREAKING-CHANGE release-version: 5.7.0-rc1 and removed BREAKING-CHANGE labels Nov 22, 2017

theotherjimmy added the needs: review label Nov 22, 2017

0xc0170 added needs: work and removed needs: review labels Nov 24, 2017

fkjagodzinski force-pushed the test-systimer branch from 9822466 to 95b3887 Compare November 24, 2017 18:00

bulislaw reviewed Nov 27, 2017

View reviewed changes

0xc0170 reviewed Nov 27, 2017

View reviewed changes

0xc0170 added needs: review and removed needs: work labels Nov 28, 2017

fkjagodzinski force-pushed the test-systimer branch from b93e5fd to 55d06ab Compare December 5, 2017 10:11

fkjagodzinski added 4 commits February 28, 2018 14:37

RTOS: SysTimer: Extract RtosTimer as SysTimer

1054277

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

RTOS: SysTimer: Split methods for testing

d098cd6

RTOS: SysTimer: Add tests

74e0f95

RTOS: SysTimer: Fix update_tick

d83ed63

fkjagodzinski dismissed stale reviews from c1728p9 and bulislaw via d83ed63 February 28, 2018 13:46

fkjagodzinski force-pushed the test-systimer branch from 55d06ab to d83ed63 Compare February 28, 2018 13:46

bulislaw approved these changes Feb 28, 2018

View reviewed changes

c1728p9 approved these changes Feb 28, 2018

View reviewed changes

cmonr removed the needs: preceding PR label Feb 28, 2018

cmonr added ready for merge and removed needs: work labels Feb 28, 2018

cmonr added the release-version: 5.8.0-rc1 label Mar 1, 2018

cmonr merged commit acad33a into ARMmbed:master Mar 1, 2018

cmonr removed the ready for merge label Mar 1, 2018

fkjagodzinski deleted the test-systimer branch March 1, 2018 09:05

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

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

Uh oh!

Conversation

fkjagodzinski commented Nov 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Status

Uh oh!

fkjagodzinski commented Nov 21, 2017

Uh oh!

bulislaw commented Nov 21, 2017

Uh oh!

bulislaw left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

fkjagodzinski commented Nov 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theotherjimmy commented Nov 22, 2017

Uh oh!

theotherjimmy commented Nov 22, 2017

Uh oh!

bulislaw commented Nov 22, 2017

Uh oh!

0xc0170 commented Nov 24, 2017

Uh oh!

fkjagodzinski commented Nov 24, 2017

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 commented Nov 30, 2017

Uh oh!

mbed-ci commented Nov 30, 2017

Triggering tests

Uh oh!

mbed-ci commented Nov 30, 2017

Uh oh!

mbed-ci commented Nov 30, 2017

Uh oh!

bulislaw commented Dec 1, 2017

Uh oh!

fkjagodzinski commented Dec 5, 2017

Uh oh!

mbed-ci commented Feb 27, 2018

Uh oh!

fkjagodzinski commented Feb 28, 2018

Uh oh!

fkjagodzinski commented Feb 28, 2018

Uh oh!

cmonr commented Feb 28, 2018

Uh oh!

mbed-ci commented Feb 28, 2018

Triggering tests

Uh oh!

mbed-ci commented Feb 28, 2018

Triggering tests

Uh oh!

mbed-ci commented Feb 28, 2018

Uh oh!

mbed-ci commented Feb 28, 2018

Uh oh!

cmonr commented Feb 28, 2018

Uh oh!

cmonr commented Feb 28, 2018

Uh oh!

mbed-ci commented Feb 28, 2018

Triggering tests

Uh oh!

mbed-ci commented Feb 28, 2018

Uh oh!

fkjagodzinski commented Nov 21, 2017 •

edited

Loading

0xc0170 commented Nov 22, 2017 •

edited

Loading

fkjagodzinski commented Nov 22, 2017 •

edited

Loading