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 RTC time test. #5087

Merged
merged 1 commit into from Jan 26, 2018

Conversation

Projects
None yet
10 participants
@mprse
Member

mprse commented Sep 13, 2017

Description

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

Status

READY

Migrations

NO

@mprse

This comment has been minimized.

Member

mprse commented Sep 13, 2017

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

  1. 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?
  2. 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.
  3. 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.
  4. 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?
  5. is it acceptable that time() function calls RTC read driver function (if defined) even if RTC is disabled or isenabled driver function is undefined?
@bulislaw

This comment has been minimized.

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.

TESTS/mbed_drivers/rtc/main.cpp Outdated
void test_case_rtc_strftime() {
greentea_send_kv("timestamp", CUSTOM_TIME);
static int rtc_enabled_ret;

This comment has been minimized.

@bulislaw

bulislaw Sep 13, 2017

Member

Lets make them all volatile

This comment has been minimized.

@mprse

mprse Sep 14, 2017

Member

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()

This comment has been minimized.

@bulislaw

bulislaw Sep 13, 2017

Member

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

This comment has been minimized.

@mprse

mprse Sep 14, 2017

Member

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.

This comment has been minimized.

@bulislaw

bulislaw Sep 13, 2017

Member

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/

This comment has been minimized.

@mprse

mprse Sep 14, 2017

Member

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>

This comment has been minimized.

@bulislaw

bulislaw Sep 13, 2017

Member

I would split that in two tests set and wait.

This comment has been minimized.

@mprse

mprse Sep 14, 2017

Member

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);

This comment has been minimized.

@bulislaw

bulislaw Sep 13, 2017

Member

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

This comment has been minimized.

@mprse

mprse Sep 14, 2017

Member

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.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 21, 2017

@mprse mprse force-pushed the mprse:rtc_test branch Sep 22, 2017

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Sep 25, 2017

@bulislaw Have your comments been addressed?

@0xc0170

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

@0xc0170 0xc0170 added needs: CI and removed needs: review labels Sep 29, 2017

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 29, 2017

/morph test

@mbed-bot

This comment has been minimized.

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!

@bulislaw

This comment has been minimized.

Member

bulislaw commented Oct 2, 2017

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

@mprse mprse force-pushed the mprse:rtc_test branch Oct 2, 2017

@mprse

This comment has been minimized.

Member

mprse commented Oct 2, 2017

Added guard against boards without RTC.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Oct 2, 2017

/morph test

@mbed-bot

This comment has been minimized.

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 0xc0170 added ready for merge and removed needs: CI labels Oct 2, 2017

@theotherjimmy

This comment has been minimized.

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 mprse force-pushed the mprse:rtc_test branch to e34b4d5 Oct 2, 2017

@mprse

This comment has been minimized.

Member

mprse commented Oct 2, 2017

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 adbridge added needs: CI and removed ready for merge labels Oct 3, 2017

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Oct 5, 2017

/morph test

@cmonr

This comment has been minimized.

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

This comment has been minimized.

Contributor

cmonr commented Jan 22, 2018

Depends on #5898

cmonr added a commit that referenced this pull request Jan 23, 2018

Merge pull request #5898 from cmonr/ncs36510-disable-rtc
Disables RTC for NCS36510 since feature is blocking #5087 from building correctly, and issue will not be resolved soon (#5308).
@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jan 24, 2018

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

@mprse mprse force-pushed the mprse:rtc_test branch from e34b4d5 to a25bf8f Jan 24, 2018

@mprse

This comment has been minimized.

Member

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?

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jan 24, 2018

/morph build

@mbed-ci

This comment has been minimized.

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

This comment has been minimized.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jan 26, 2018

/morph test

@mbed-ci

This comment has been minimized.

@0xc0170

This comment has been minimized.

Member

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.
@cmonr

This comment has been minimized.

Contributor

cmonr commented Jan 26, 2018

/morph build

@cmonr cmonr added needs: CI and removed needs: work labels Jan 26, 2018

@mbed-ci

This comment has been minimized.

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

This comment has been minimized.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jan 26, 2018

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

@mbed-ci

This comment has been minimized.

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

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@cmonr cmonr merged commit 3c793a7 into ARMmbed:master Jan 26, 2018

19 checks passed

ARM mbed CI Verification build successful.
Details
AWS-CI uVisor Build & Test Success
Details
ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-test test completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Local events testing has passed
Details
travis-ci/littlefs Local littlefs testing has passed
Details
travis-ci/mbed2-ATMEL Local mbed2-ATMEL testing has passed
Details
travis-ci/mbed2-MAXIM Local mbed2-MAXIM testing has passed
Details
travis-ci/mbed2-NORDIC Local mbed2-NORDIC testing has passed
Details
travis-ci/mbed2-NUVOTON Local mbed2-NUVOTON testing has passed
Details
travis-ci/mbed2-NXP Local mbed2-NXP testing has passed
Details
travis-ci/mbed2-RENESAS Local mbed2-RENESAS testing has passed
Details
travis-ci/mbed2-SILICON_LABS Local mbed2-SILICON_LABS testing has passed
Details
travis-ci/mbed2-STM Local mbed2-STM testing has passed
Details
travis-ci/tools Local tools testing has passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment