Skip to content
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 get/settimeofday retargets #8497

Merged
merged 1 commit into from Jan 17, 2019

Conversation

Projects
None yet
9 participants
@kegilbert
Copy link
Contributor

commented Oct 22, 2018

Description

In reference to: #6988


Move the Time function implementation to get/settimeofday to match the stdlib calls and have the Time API call into the new functions.

Questions/Comments:

  1. Should the retarget get/settimeofday functions live in mbed_retarget or are they fine here?
  2. Updated tests pending once this PR gets feedback

Test code:

struct timeval tv;
time_t s = time(NULL);

wait(3.0f);

s = time(NULL);
int err = gettimeofday(&tv, NULL);

printf("Time: %d, gettimeofday: %d [Err: %d]\r\n", s, tv.tv_sec, err);

set_time(12345);

s = time(NULL);
err = gettimeofday(&tv, NULL);
printf("Time: %d, gettimeofday: %d [Err: %d]\r\n", s, tv.tv_sec, err);

wait(1.0f);
s = time(NULL);
err = gettimeofday(&tv, NULL);
printf("Time: %d, gettimeofday: %d [Err: %d]\r\n", s, tv.tv_sec, err);

const struct timeval tv_set = { 54321, 0 };
int seterr = settimeofday(&tv_set, NULL);
s = time(NULL);
err = gettimeofday(&tv, NULL);
printf("Time: %d, gettimeofday: %d [Err: %d] SetErr: %d\r\n", s, tv.tv_sec, err, seterr);

wait(1.0f);
s = time(NULL);
err = gettimeofday(&tv, NULL);
printf("Time: %d, gettimeofday: %d [Err: %d]\r\n", s, tv.tv_sec, err);
Time: 3, gettimeofday: 3 [Err: 0]
Time: 12345, gettimeofday: 12345 [Err: 0]
Time: 12346, gettimeofday: 12346 [Err: 0]
Time: 54321, gettimeofday: 54321 [Err: 0] SetErr: 0
Time: 54322, gettimeofday: 54322 [Err: 0]

Pull request type

[ ] Fix
[X] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change
@cmonr

This comment has been minimized.

Copy link
Contributor

commented Oct 23, 2018

@kegilbert Please take a look at the doxygen issues.

@kegilbert

This comment has been minimized.

Copy link
Contributor Author

commented Oct 23, 2018

Likely forgot to mark the return type in the function declaration, will update tomorrow morning.

@kjbracey-arm
Copy link
Contributor

left a comment

Seems a bit sad to be implementing this when it adds no new functionality - just another name for the second-resolution time().

Anyone who is happy with seconds could have trivially changed their code to use time, and anyone who was hoping for better (as I believe #6988 did) is going to be disappointed by this implementation.

Still, there's scope to improve the implementation later once the API is in (as long as tv_usec always being 0 isn't deemed API).

Show resolved Hide resolved platform/mbed_rtc_time.cpp Outdated
Show resolved Hide resolved platform/mbed_rtc_time.cpp Outdated
Show resolved Hide resolved platform/mbed_rtc_time.h Outdated
Show resolved Hide resolved platform/mbed_rtc_time.h Outdated
Show resolved Hide resolved platform/mbed_rtc_time.cpp Outdated
@kegilbert

This comment has been minimized.

Copy link
Contributor Author

commented Oct 23, 2018

@kjbracey-arm Thanks for the review! I agree that this doesn't really add a whole lot. I proposed this idea of get/settimeofday essentially acting as a time call with a slightly different format for users expecting get/settimeofday here as it was a rework of Marcus's old implementation using our new timer calls in Mbed OS 5.

Usec being 0 is more a byproduct of the RTC implementations at the moment, that can always be updated.

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor

commented Oct 24, 2018

I'm happy to add the API with second resolution like this, but you will get immediate pushback from users about the low quality of implementation. We will should commit to improve it at some point.

In the first pass, could be done without a change to HALs by making the default implementation always use a LPTicker/RTOS count offset added to an initial HAL RTC read, rather than performing an RTC read every time.

Have a new attach_rtc_hires using struct timeval, and a piece of glue for any legacy attach_rtc users with the time_t<->struct timeval conversion.

I'd favour that approach as I really want time and gettimeofday to work pretty well on all platforms, regardless of hardware, so we don't have a situation where stuff with real HW RTC gets second resolution but non-RTC HW gets millisecond LPTicker/RTOS resolution.

Second stage you could add DEVICE_RTC_HIRES for an extended HAL API, for which you go back to calling directly.

@0xc0170 0xc0170 requested a review from ARMmbed/mbed-os-hal Oct 24, 2018

@kegilbert kegilbert force-pushed the kegilbert:gettimeofday_patch branch Oct 24, 2018

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Oct 25, 2018

Fyi, this didn't pass the cloud-client-test check.

[Build K66F ARM] [ERROR] "./mbed-os/platform/mbed_rtc_time.cpp", line 91: Error:  #393: pointer to incomplete class type is not allowed
[Build K66F ARM] "./mbed-os/platform/mbed_rtc_time.cpp", line 116: Error:  #393: pointer to incomplete class type is not allowed
[Build K66F ARM] "./mbed-os/platform/mbed_rtc_time.cpp", line 117: Error:  #393: pointer to incomplete class type is not allowed
[Build K66F ARM] "./mbed-os/platform/mbed_rtc_time.cpp", line 130: Error:  #70: incomplete type is not allowed
[Build K66F ARM] "./mbed-os/platform/mbed_rtc_time.cpp", line 142: Error:  #70: incomplete type is not allowed
[Build K66F ARM] ./mbed-os/platform/mbed_rtc_time.cpp: 0 warnings, 5 errors
[Build K66F ARM] 
[Build K66F ARM] [mbed] ERROR: "/usr/bin/python" returned error.
[Build K66F ARM]        Code: 1
[Build K66F ARM]        Path: "/builds/ws/mbed-os-ci_cloud-client-test/mbed-cloud-client-example"
[Build K66F ARM]        Command: "/usr/bin/python -u /builds/ws/mbed-os-ci_cloud-client-test/mbed-cloud-client-example/mbed-os/tools/make.py -t ARM -m K66F --source . --build ./BUILD/K66F/ARM"
[Build K66F ARM]        Tip: You could retry the last command with "-v" flag for verbose output
[Build K66F ARM] ---
[Build K66F ARM] [mbed] Auto-installing missing Python modules...

@kegilbert kegilbert force-pushed the kegilbert:gettimeofday_patch branch Oct 25, 2018

@kegilbert

This comment has been minimized.

Copy link
Contributor Author

commented Oct 26, 2018

@cmonr Resolved the test failure

@cmonr cmonr added needs: review and removed needs: work labels Oct 29, 2018

@donatieng
Copy link
Member

left a comment

Good stuff @kegilbert - a few remarks

Show resolved Hide resolved platform/mbed_rtc_time.cpp Outdated
Show resolved Hide resolved platform/mbed_rtc_time.h Outdated

@kegilbert kegilbert force-pushed the kegilbert:gettimeofday_patch branch Oct 31, 2018

@kegilbert

This comment has been minimized.

Copy link
Contributor Author

commented Oct 31, 2018

@kjbracey-arm @donatieng Thanks for the additional feedback! Updated the PR to match the request changes.

@0xc0170 0xc0170 requested a review from ARMmbed/mbed-os-core Nov 8, 2018

@0xc0170

This comment has been minimized.

Copy link
Member

commented Nov 8, 2018

This is ready for another round of reviews!

Show resolved Hide resolved platform/mbed_rtc_time.cpp Outdated
Show resolved Hide resolved platform/mbed_rtc_time.cpp Outdated

@cmonr cmonr added needs: work and removed needs: review labels Nov 9, 2018

@kegilbert kegilbert force-pushed the kegilbert:gettimeofday_patch branch Nov 14, 2018

@kegilbert

This comment has been minimized.

Copy link
Contributor Author

commented Nov 14, 2018

Apologies for the delay, updated.
@kjbracey-arm
@donatieng

@cmonr cmonr dismissed stale reviews from donatieng and kjbracey-arm Nov 15, 2018

Changes addressed. Please re-review.

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Nov 15, 2018

Reviewers, when y'all have a chance.
@donatieng @kjbracey-arm @ARMmbed/mbed-os-core @marcuschangarm

@bulislaw

This comment has been minimized.

Copy link
Member

commented Nov 26, 2018

All the PRs need to be engineering ready (marked as "needs: CI") by the end of the day (Austin time). Otherwise it won't make 5.11 and will need to come in the next release (5.12 for features, 5.11.1 for fixes and new platforms).

@0xc0170

This comment has been minimized.

Copy link
Member

commented Nov 26, 2018

Moved to 5.12, as there are still build failures. Please @kegilbert continue to work on this

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Dec 3, 2018

@kegilbert Any Progress?

@kegilbert

This comment has been minimized.

Copy link
Contributor Author

commented Dec 6, 2018

Sorry for the delay! Got a bit caught up in release work.

@kjbracey-arm Running into an issue were some targets (Odin, Realtek it seems like, potentially others which are pulling in LWIP by default) are including sys/time.h which has a handle for get/settimeofday in the GCC_ARM toolchain in particular: https://github.com/ARMmbed/mbed-os/blob/master/features/lwipstack/lwipopts.h#L23-L26

Does this need to be <sys/time.h> or would <time.h> work if we just need it for some struct definitions (was under the impression time.h had the required ones but could be wrong). Also leads to the question on if we could use the toolchain get/setimeofday functions. I was unable to find usable handles in IAR or the Arm compiler but might've missed them hence why I went this route.

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Dec 14, 2018

@kjbracey-arm I think @kegilbert is waiting for feedback

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor

commented Dec 14, 2018

time.h is the C header, which does not include gettimeofday, or shouldn't. sys/time.h is POSIX, and where it lives.

For other POSIX functions we have declared them locally in mbed_retarget.h, and we've made sure those definitions are compatible so that it doesn't matter if the compiler header is also included.

In this case, if there's type variation in the compiler headers, you may need to have toolchain-conditional - either to make sure you match, or to defer to the compiler header.

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Dec 21, 2018

@kegilbert Thoughts?

@kegilbert

This comment has been minimized.

Copy link
Contributor Author

commented Dec 25, 2018

Sorry for the late response, still traveling so will be a bit before I can take a closer look but sounds good to me. I'll update this ASAP when I'm back at the beginning of January.

@kegilbert kegilbert force-pushed the kegilbert:gettimeofday_patch branch 3 times, most recently Jan 7, 2019

@kegilbert

This comment has been minimized.

Copy link
Contributor Author

commented Jan 10, 2019

@cmonr Changed the function declaration to match the conflicting one. Seems to run alright now.

@cmonr cmonr added needs: CI and removed needs: work labels Jan 10, 2019

@0xc0170

This comment has been minimized.

Copy link
Member

commented Jan 11, 2019

CI started

@mbed-ci

This comment has been minimized.

Copy link

commented Jan 11, 2019

Test run: FAILED

Summary: 1 of 7 test jobs failed
Build number : 9
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_build-ARM

@kegilbert kegilbert force-pushed the kegilbert:gettimeofday_patch branch to dafb01c Jan 11, 2019

@kegilbert

This comment has been minimized.

Copy link
Contributor Author

commented Jan 11, 2019

Didn't realize ARMC6 doesn't set the __CC_ARM macro, this should work now.

@NirSonnenschein

This comment has been minimized.

Copy link
Contributor

commented Jan 13, 2019

restart CI

@mbed-ci

This comment has been minimized.

Copy link

commented Jan 13, 2019

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 10
Build artifacts

@cmonr cmonr merged commit 5dbb4b9 into ARMmbed:master Jan 17, 2019

21 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/build-ARM Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(+0 bytes) RAM(+0 bytes)
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARM Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Local astyle testing has passed
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 10103 cycles (+1057 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 8372B (+0.00%)
Details
travis-ci/psa-autogen Local psa-autogen testing has passed
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details

@cmonr cmonr removed the ready for merge label Jan 17, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.