Add RTC time test. #5087

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.

Sign up for GitHub

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

Jump to bottom

Merged

cmonr merged 1 commit into ARMmbed:master from mprse:rtc_test

Jan 26, 2018

Contributor

mprse commented Sep 13, 2017

Description

Add RTC time test.
Test verifies RTC time() and set_time() functions.

Status

READY

Migrations

NO

Contributor Author

mprse commented Sep 13, 2017

@bulislaw @0xc0170 The following issues have been found during test development:

set_time() function on some platforms does not set RTC time to 0. In such case specific driver function - rtc_write() sets time to 1 instead of 0. This is caused because on some platforms 0 is an invalid time. Unfortunately there is no information about this behaviour. Maybe a warning should be added to the set_time() function description?
There is no information in documentation neither header file about clock() function. Maybe this function should be removed? It is used nowhere and it looks like this is not API.
set_time() function always initialises RTC (if RTC init driver function is provided). It looks like RTC initialisation should be performed only once. I have checked few RTC init driver functions and they are not protected against second execution. Maybe extra flag should be added to control RTC init.
is it acceptable that set_time() function calls RTC write driver function (if defined) even if RTC init driver function is undefined and RTC is not initialized?
is it acceptable that time() function calls RTC read driver function (if defined) even if RTC is disabled or isenabled driver function is undefined?

Member

bulislaw commented Sep 13, 2017

set_time() function on some platforms does not set RTC time to 0. In such case specific driver function - rtc_write() sets time to 1 instead of 0. This is caused because on some platforms 0 is an invalid time. Unfortunately there is no information about this behaviour. Maybe a warning should be added to the set_time() function description?

Just a note in the doxygen should be enough.

There is no information in documentation neither header file about clock() function. Maybe this function should be removed? It is used nowhere and it looks like this is not API.

I think it's a implementation of standard library function, maybe it's not the best place for it, you could try opening new PR and moving it to mbed_retarget.cpp.

set_time() function always initialises RTC (if RTC init driver function is provided). It looks like RTC initialisation should be performed only once. I have checked few RTC init driver functions and they are not protected against second execution. Maybe extra flag should be added to control RTC init.

Init should just enable registers and the new RTC HAL spec is explicitly mentioning it can be called multiple times. So I would ignore that here, it'll be tested in HAL RTC tests

is it acceptable that set_time() function calls RTC write driver function (if defined) even if RTC init driver function is undefined and RTC is not initialized?

is it acceptable that time() function calls RTC read driver function (if defined) even if RTC is disabled or isenabled driver function is undefined?

I think it's implementation and HW specific and possibly could possibly be a valid case, we should validate if it works rather how it is done.

bulislaw suggested changes

View reviewed changes

TESTS/mbed_drivers/rtc/main.cpp Outdated

    
              void test_case_rtc_strftime() {

                  greentea_send_kv("timestamp", CUSTOM_TIME);

              static int rtc_enabled_ret;

Member

bulislaw Sep 13, 2017

Lets make them all volatile

Contributor Author

mprse Sep 14, 2017

Added volatile.

TESTS/mbed_drivers/rtc/main.cpp Outdated

    
               * When set_time/time functions are called.

               * Then set_time/time functions use attached RTC functions.

               */

              void test_attach_RTC_funtions()

Member

bulislaw Sep 13, 2017

That's too long. We should try checking one thing in a test (within reason).

Contributor Author

mprse Sep 14, 2017

Spited into two cases. First case verify attaching of stub driver functions and second verify attaching of original RTC driver functions.

TESTS/mbed_drivers/rtc/main.cpp Outdated

    
               * Note: Stubs are used instead of original RTC functions.

               *

               * Given environment has RTC functions

               * defined and RTC is enabled.

Member

bulislaw Sep 13, 2017

nit: doesn't need to be broken into two lines, we have limit of 120chars https://docs.mbed.com/docs/mbed-os-handbook/en/latest/cont/code_style/

Contributor Author

mprse Sep 14, 2017

Description of the test cases have been corrected.

TESTS/mbed_drivers/rtc/main.cpp Outdated

    
               * When set_time()/time() functions is called.

               * Then RTC time is set/retrieved.

               */

              template<uint32_t timeValue>

Member

bulislaw Sep 13, 2017

I would split that in two tests set and wait.

Contributor Author

mprse Sep 14, 2017

Spitted in to two test cases as suggested.

TESTS/mbed_drivers/rtc/main.cpp Outdated

    
                  seconds = time(NULL);

                  /* Get current time and verify that new value has been set. */

                  TEST_ASSERT_EQUAL(timeValue, seconds);

Member

bulislaw Sep 13, 2017

Maybe we should allow for +/- 1sec? As there's a chance it changes I think

Contributor Author

mprse Sep 14, 2017

To verify RTC from wider perspective, but still rational I have increased delay to 10 seconds and added delta for comparison equal to 1 sec.

mprse force-pushed the rtc_test branch from a293ff8 to 770190c Compare

September 14, 2017 07:56

This was referenced Sep 14, 2017

Move clock() function from mbed_rtc_time.cpp to mbed_retarget.cpp. #5099

Merged

Modify description of set_time() function. #5100

Closed

theotherjimmy added the needs: review label

Contributor

0xc0170 commented Sep 21, 2017

mprse force-pushed the rtc_test branch from 770190c to 5effd2f Compare

September 22, 2017 06:56

Contributor

theotherjimmy commented Sep 25, 2017

@bulislaw Have your comments been addressed?

bulislaw approved these changes

View reviewed changes

0xc0170 approved these changes

View reviewed changes

Contributor

0xc0170 left a comment

@c1728p9 Please look at this, how it aligns with another RTC specification pull request

c1728p9 approved these changes

View reviewed changes

0xc0170 added needs: CI and removed needs: review labels

Contributor

0xc0170 commented Sep 29, 2017

/morph test

mbed-bot commented Sep 29, 2017

Result: FAILURE

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

/morph test

Output

mbed Build Number: 1442

Build failed!

Member

bulislaw commented Oct 2, 2017

I think you need to add a guard against boards without RTC.

mprse force-pushed the rtc_test branch from 5effd2f to 8424896 Compare

October 2, 2017 09:31

Contributor Author

mprse commented Oct 2, 2017

Added guard against boards without RTC.

Contributor

0xc0170 commented Oct 2, 2017

/morph test

mbed-bot commented Oct 2, 2017

Result: SUCCESS

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

/morph test

Output

mbed Build Number: 1489

All builds and test passed!

0xc0170 added ready for merge and removed needs: CI labels

Contributor

theotherjimmy commented Oct 2, 2017

@mprse Could you change the sha of the last commit to kick out Circle CI? You can do this with git commit --amend with no changes.

mprse force-pushed the rtc_test branch from 8424896 to e34b4d5 Compare

October 2, 2017 18:38

Contributor Author

mprse commented Oct 2, 2017 •

edited

Loading

Could you change the sha of the last commit to kick out Circle CI? You can do this with git commit --amend with no changes.

Done

adbridge added needs: CI and removed ready for merge labels

Contributor

theotherjimmy commented Oct 5, 2017

/morph test

theotherjimmy added the needs: work label

cmonr removed the needs: preceding PR label

Contributor

cmonr commented Jan 22, 2018

The resolution of the blocking issue was that RTC will have to be disabled for the NCS36510 for the time being. A PR will be incoming today to unblock this PR.

cmonr mentioned this pull request

Remove RTC as a supported peripheral for NCS36510 #5898

Merged

Contributor

cmonr commented Jan 22, 2018

Depends on #5898

cmonr added a commit that referenced this pull request


          Merge pull request #5898 from cmonr/ncs36510-disable-rtc

669a85a

Disables RTC for NCS36510 since feature is blocking #5087 from building correctly, and issue will not be resolved soon (#5308).


          Add RTC time test.

a25bf8f

Contributor

0xc0170 commented Jan 24, 2018

@mprse Can you rebase this , it should be unblocked after?

mprse force-pushed the rtc_test branch from e34b4d5 to a25bf8f Compare

January 24, 2018 10:39

Contributor Author

mprse commented Jan 24, 2018

@mprse Can you rebase this , it should be unblocked after?

Re-based and tested on K64F, NUCLEOF070RB, NRF51_DK boards (all tool-chains).

Can we run morph?

Contributor

0xc0170 commented Jan 24, 2018

/morph build

mbed-ci commented Jan 24, 2018

Build : SUCCESS

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

Triggering tests

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

mbed-ci commented Jan 24, 2018

Exporter Build : SUCCESS

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

Contributor

0xc0170 commented Jan 26, 2018

/morph test

mbed-ci commented Jan 26, 2018

Test : FAILURE

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

Contributor

0xc0170 commented Jan 26, 2018

@mprse Can you reproduce those failures in the CI? timeouts?

From the logs

mbedhtrun -m NUCLEO_F746ZG -p DUMMY:9600 -f "BUILD/tests/NUCLEO_F746ZG/ARM/TESTS/mbed_hal/rtc_time_conv/rtc_time_conv.bin"

Starting from this test, teh rest timeout.

And can see this there

04:56:01 [1516964177.78][CONN][INF] output : STM32_STLink 0670FF544852707267161537  and error :
04:56:01 [1516964177.78][CONN][INF] updating ST link for the device ...
04:56:04 Firmware version detected: V2J30M19
04:56:13 ....................Upgrade is successful.

Contributor

cmonr commented Jan 26, 2018

/morph build

cmonr added needs: CI and removed needs: work labels

mbed-ci commented Jan 26, 2018

Build : SUCCESS

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

Triggering tests

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

mbed-ci commented Jan 26, 2018

Exporter Build : FAILURE

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

Contributor

cmonr commented Jan 26, 2018

Will rebuild. Firmware version output is symptom of earlier CI issue.
/morph build

mbed-ci commented Jan 26, 2018

Build : SUCCESS

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

Triggering tests

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

mbed-ci commented Jan 26, 2018

Exporter Build : SUCCESS

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

mbed-ci commented Jan 26, 2018

Test : SUCCESS

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

cmonr merged commit 3c793a7 into ARMmbed:master

cmonr added needs: CI release-version: 5.7.4 and removed needs: CI labels

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release-version: 5.7.4