Add test for Timer class. #4971

mprse · 2017-08-24T12:00:29Z

Description

Add unit tests for Timer class.

Status

READY

Migrations

NO

pan-

Few comments:

I wonder if the intermediary variables read_val, read_ms_val, read_us_val and read_hr_us_val are necessary. In my opinion, calling the timer functions in the assertion is clear enough.
The comment about the timer existence present in every test is unnecessary.
Would it be possible to test accumulation with the stub while moving the counter value between stop/start call.
Would it be possible to split unit test from integration tests (the tests using the real ticker).
The Timer class is underspecified and inconsistent regards to the unit returned by read* function, it would be interesting to specify behavior in boundary case and add test for these case.

pan- · 2017-08-24T13:11:10Z

TESTS/mbedmicro-rtos-mbed/timer/main.cpp

+static uint32_t curr_ticker_us_val;
+
+/* User ticker interface function. */
+static void my_interface_init()


my_ functions are stubs. I suggest replacing my_ prefix by stub_ prefix.

pan- · 2017-08-24T13:16:27Z

TESTS/mbedmicro-rtos-mbed/timer/main.cpp

+ * system clock frequency = 32 678 Hz (~32 kHz) => 1 tick = 31.25 us.
+ * delta = truncate(31.25 * 10 + 5) = truncate(317.5) = 317 us.
+ */
+#define DELTA_SYS_CLK_US ((int)((1.0f / ((float)osKernelGetSysTimerFreq() / US_PER_SEC) ) * 10 + 5))


Would it be possible to use a constant variable instead of a macro.

It is possible, but then it would have to be a very large value (e.g. 500 us), to be able to successfully execute test on each board. It looks like the better option is to define delta value based on board clock frequency.

I meant:

static const int delta_sys_clk_us = (...);

pan- · 2017-08-24T13:20:41Z

TESTS/mbedmicro-rtos-mbed/timer/main.cpp

+ * ticker data provided by the user.
+ *
+ * */
+utest::v1::status_t timer_user_ticker_setup_h(const Case *const source, const size_t index_of_case)


What the suffix _h stands for ? _handler ? If so then given there is no such suffix define by greentea_*_handler I suggest to use the _handler suffix instead of its shortened form.

pan- · 2017-08-24T13:21:17Z

TESTS/mbedmicro-rtos-mbed/timer/main.cpp

+ */
+void test_timer_creation()
+{
+    float read_val;


Would it be possible to declare variables at initialization point ?

Fixed - removed local variables to hold measured time. Instead call read function inside assertion.

pan- · 2017-08-24T13:27:38Z

TESTS/mbedmicro-rtos-mbed/timer/main.cpp

+    read_hr_us_val = p_timer->read_high_resolution_us();
+
+    /* Check results. */
+    TEST_ASSERT_EQUAL(0, read_val);


read_val is a float, might be good to use the float assertion.

pan- · 2017-08-24T13:27:46Z

TESTS/mbedmicro-rtos-mbed/timer/main.cpp

+    read_hr_us_val = p_timer->read_high_resolution_us();
+
+    /* Check results. */
+    TEST_ASSERT_EQUAL(0, read_val);


read_val is a float, might be good to use the float assertion.

pan- · 2017-08-24T13:38:30Z

TESTS/mbedmicro-rtos-mbed/timer/main.cpp

+    /* Note that timers are based on 32-bit int microsecond counters,
+     * so can only time up to a maximum of 2^31-1 microseconds i.e.
+     * 2147483647 us (about 35 minutes). */
+    curr_ticker_us_val = 2147483647;


Serious question, what is supposed to happens when curr_ticker_us_val.read returns a value outside the [0 : MAX_INT] range ?

curr_ticker_us_val is of unsigned integer type. In case when the value would be greater than MAX_INT, then we have inconsistency. In such case read functions would return negative values.

I have assumed that the max tested measured time can not exceed MAX_INT based on information from mBed handbook (see Warning):
https://developer.mbed.org/handbook/Timer
https://docs.mbed.com/docs/mbed-os-api-reference/en/latest/APIs/tasks/Timer/

According to the documentation Timer is dedicated to measure small times (between microseconds and seconds).

@bulislaw Could you clarify the behavior of the Timer class when the time measured exceeds the value returned ?

There is a warning in the handbook but nothing in the class declaration. The range returned by read, read_ms, read_us and read_high_resolution_us is also unspecified. According to the warning in the handbook, the maximum value returned by read_ms will be 2147483 which is not very intuitive.

I'm not sure, best would be to check how it actually works. Confirm it makes sense and update the docs/handbook.

@bulislaw @pan- I created test based on documentation provided in handbook. So the boundary case for max measured time is MAX_INT. I agree that values returned by the read* functions are not very intuitive. Currently when measured time is equal to MAX_INT (2147483647) read* functions return the following values:
read(): 2147.483643 - ok
read_ms(): 2147483 - ok
read_us(): 2147483647 - ok
read_high_resolution_us(): 2147483647 - ok
When measured time is equal to MAX_INT+1 (2147483648), then we have the following results:
read(): -2147.483643 - wrong
read_ms(): 2147483 - ok
read_us(): -2147483648 - wrong
read_high_resolution_us(): 2147483648 - ok.

It is very strange that returned time can be negative, but warning in handbook informs about Timer time measurement limitations. On the other hand maybe we could modify Timer::read_us()
and Timer::read_ms() function declarations and change returned type to unsigned int. This way measured time limit would be increased and we would also rid of negative results.
In such case handbook and documents need to be also updated.

0xc0170 · 2017-08-31T12:59:25Z

Please look at the failure in jenkins ci : Error: main.cpp@6,0: #5: cannot open source input file “timer.h”: No such file or directory

0xc0170 · 2017-09-04T09:37:00Z

@mprse Bump

mprse · 2017-09-14T05:54:28Z

@pan- @0xc0170 @bulislaw All issues have been fixed. Please re-review.

0xc0170 · 2017-09-21T13:32:32Z

I restarted jenkins CI, as the last failure is not available anymore to review, @mprse please review if it fails again

0xc0170 · 2017-09-21T13:33:08Z

@pan- @0xc0170 @bulislaw All issues have been fixed. Please re-review.

adding @c1728p9

bump for review

0xc0170 · 2017-09-26T14:04:59Z

retest uvisor

0xc0170 · 2017-09-26T14:05:19Z

bump for review round !

bulislaw · 2017-09-26T15:34:30Z

@pan- are you happy with the fixes?

0xc0170 · 2017-09-27T12:11:27Z

retest uvisor

0xc0170 · 2017-09-28T12:33:40Z

@pan- are you happy with the fixes?

bump

pan- · 2017-09-28T12:38:44Z

TESTS/mbedmicro-rtos-mbed/timer/main.cpp

+#define DELTA_US delta_sys_clk_us
+#define DELTA_S  ((float)delta_sys_clk_us/US_PER_SEC)
+
+static Timer *p_timer;


Please initialize p_timer to NULL so the test in timer_user_ticker_setup_handler is valid.

pan- · 2017-09-28T12:39:45Z

TESTS/mbedmicro-rtos-mbed/timer/main.cpp

+ * */
+utest::v1::status_t cleanup_handler(const Case *const source, const size_t passed, const size_t failed, const failure_t reason)
+{
+    delete p_timer;


please reset p_timer to NULL.

theotherjimmy · 2017-09-29T15:40:07Z

/morph test

mbed-bot · 2017-09-30T08:32:23Z

/morph test

mbed-bot · 2017-09-30T13:16:43Z

Result: FAILURE

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

/morph test

Output

mbed Build Number: 1457

Test failed!

mbed-bot · 2017-09-30T20:27:07Z

Result: FAILURE

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

/morph test

Output

mbed Build Number: 1457

Test failed!

Output

mbed Build Number: 1462

Build failed!

mbed-bot · 2017-10-01T06:22:44Z

Result: FAILURE

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

/morph test

Output

mbed Build Number: 1457

Test failed!

Output

mbed Build Number: 1462

Build failed!

Output

mbed Build Number: 1467

Test failed!

mbed-bot · 2017-10-01T15:45:39Z

Result: FAILURE

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

/morph test

Output

mbed Build Number: 1457

Test failed!

Output

mbed Build Number: 1462

Build failed!

Output

mbed Build Number: 1467

Test failed!

Output

mbed Build Number: 1473

Test failed!

mbed-bot · 2017-10-02T00:51:49Z

Result: FAILURE

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

/morph test
Output

mbed Build Number: 1457

Test failed!

Output

mbed Build Number: 1462

Build failed!

Output

mbed Build Number: 1467

Test failed!

Output

mbed Build Number: 1473

Test failed!

Output

mbed Build Number: 1478

Test failed!

mbed-bot · 2017-10-02T06:36:03Z

Result: ABORTED

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

 /morph test
Output

mbed Build Number: 1457

Test failed!

Output

mbed Build Number: 1462

Build failed!

Output

mbed Build Number: 1467

Test failed!

Output

mbed Build Number: 1473

Test failed!

Output

mbed Build Number: 1478

Test failed!

0xc0170 · 2017-10-02T06:36:31Z

@studavekar Just to be aware of cyclic running CI (it seems to me) ?

@mprse Please look at the failures, there are related to this patch

I aborted the last job

studavekar · 2017-10-02T07:26:45Z

@0xc0170 the recursive trigger was due to an experimental scrip where bot triggered the aborted jobs, however when bot makes a comment of results it has a test trigger morph command in it causing recursive trigger.

this is disabled and we shouldn't see this now.

mprse · 2017-10-03T08:27:00Z

Increased measured time tolerance (for NRF51_DK).

theotherjimmy · 2017-10-05T16:08:24Z

/morph test

mbed-bot · 2017-10-05T19:04:02Z

Result: SUCCESS

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

/morph test

Output

mbed Build Number: 1531

All builds and test passed!

0xc0170 · 2017-10-07T14:30:48Z

/morph test-nightly

mbed-bot · 2017-10-08T06:23:37Z

Result: SUCCESS

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

/morph test-nightly

Output

mbed Build Number: 1543

All builds and test passed!

theotherjimmy · 2017-10-09T16:18:00Z

We're going to make sure this is not flaky by running it a bunch.

0xc0170 · 2017-10-11T07:03:59Z

/morph test-nightly

mbed-ci · 2017-10-11T07:19:12Z

Build : SUCCESS

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

Triggering tests

/test mbed-os

mbed-bot · 2017-10-11T07:27:22Z

Result: FAILURE

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

/morph test-nightly

Output

mbed Build Number: 1562

Build failed!

mprse · 2017-10-11T13:03:07Z

Result: FAILURE

It looks like there was IAR licence problem:
Fatal error[LMS001]: License check failed.

0xc0170 · 2017-10-11T13:43:06Z

@mprse The build trigger the test that is equal to nightly, wait for the next status, should be OK , license problem was resolved

studavekar · 2017-10-11T20:03:37Z

Updated the status of "morph-test-nightly" with last passing run #4971 (comment).

pan- reviewed Aug 24, 2017

View reviewed changes

theotherjimmy added the needs: review label Aug 24, 2017

mprse force-pushed the test_timer branch from ecf7057 to ed8a508 Compare August 24, 2017 14:34

0xc0170 added needs: work and removed needs: review labels Aug 31, 2017

mprse force-pushed the test_timer branch 3 times, most recently from 8e11d6a to 4255a54 Compare September 14, 2017 05:51

mprse force-pushed the test_timer branch from 4255a54 to 0ab44bd Compare September 22, 2017 07:21

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

pan- approved these changes Sep 28, 2017

View reviewed changes

theotherjimmy added needs: CI and removed needs: review labels Sep 29, 2017

mprse force-pushed the test_timer branch from 0ab44bd to 55177c2 Compare October 3, 2017 08:29

Add test for Timer class.

4509165

mprse force-pushed the test_timer branch from 55177c2 to 4509165 Compare October 3, 2017 09:55

0xc0170 added ready for merge and removed needs: CI labels Oct 9, 2017

theotherjimmy added needs: CI and removed ready for merge labels Oct 9, 2017

0xc0170 added ready for merge and removed needs: CI labels Oct 12, 2017

theotherjimmy merged commit bf9c507 into ARMmbed:master Oct 13, 2017

theotherjimmy added release-version: 5.6.3 and removed ready for merge labels Oct 13, 2017

Add test for Timer class. #4971

Add test for Timer class. #4971

Conversation

mprse commented Aug 24, 2017

Description

Status

Migrations

pan- left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xc0170 commented Aug 31, 2017

0xc0170 commented Sep 4, 2017

mprse commented Sep 14, 2017

0xc0170 commented Sep 21, 2017

0xc0170 commented Sep 21, 2017

0xc0170 commented Sep 26, 2017

0xc0170 commented Sep 26, 2017

bulislaw commented Sep 26, 2017

0xc0170 commented Sep 27, 2017

0xc0170 commented Sep 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theotherjimmy commented Sep 29, 2017

mbed-bot commented Sep 30, 2017

mbed-bot commented Sep 30, 2017

Output

mbed-bot commented Sep 30, 2017

Output

Output

mbed-bot commented Oct 1, 2017

Output

Output

Output

mbed-bot commented Oct 1, 2017

Output

Output

Output

Output

mbed-bot commented Oct 2, 2017

Output

Output

Output

Output

Output

mbed-bot commented Oct 2, 2017

Output

Output

Output

Output

Output

0xc0170 commented Oct 2, 2017 • edited Loading

studavekar commented Oct 2, 2017

mprse commented Oct 3, 2017

theotherjimmy commented Oct 5, 2017

mbed-bot commented Oct 5, 2017

Output

0xc0170 commented Oct 7, 2017

mbed-bot commented Oct 8, 2017

Output

theotherjimmy commented Oct 9, 2017

0xc0170 commented Oct 11, 2017

mbed-ci commented Oct 11, 2017

0xc0170 commented Oct 2, 2017 •

edited

Loading

studavekar commented Oct 11, 2017 •

edited

Loading