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.

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

Update references to time values to use chrono #14453

Closed
wants to merge 4 commits into from
Closed

Update references to time values to use chrono #14453

wants to merge 4 commits into from

Conversation

harmut01
Copy link
Contributor

@harmut01 harmut01 commented Mar 19, 2021 •
edited
Loading

Summary of changes

References to time should use chrono values. Refactor tests in
connectivity and drivers to remove deprecation warnings due to not
using chrono values.

Impact of changes

None

Migration actions required

None

Documentation

None

Pull request type 

[x] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results 

[] No Tests required for this change (E.g docs only update)
[x] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

Reviewers

@harrisonmutai-arm
Update references to time values to use chrono
ff0eb35 
References to time should use chrono values. Refactor tests in
connectivity and drivers to remove depcrecation warninings due to not
using chrono values.
@ciarmcom ciarmcom added the release-type: patch Indentifies a PR as containing just a patch label Mar 19, 2021
@ciarmcom ciarmcom requested a review from a team March 19, 2021 16:30
@ciarmcom
Copy link
Member

@harmut01, thank you for your changes.
@ARMmbed/mbed-os-maintainers please review.

adbridge
adbridge previously requested changes Mar 19, 2021
View reviewed changes
Copy link
Contributor

@adbridge adbridge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to revisit this, this is not the way to do it IMO, please refer to the events code for examples

drivers/tests/TESTS/mbed_drivers/lp_ticker/main.cpp Outdated
@@ -73,14 +73,14 @@ void increment_multi_counter(void)
void test_multi_ticker(void)
{
LowPowerTicker ticker[TICKER_COUNT];
const uint32_t extra_wait = 10; // extra 10ms wait time
std::chrono::milliseconds extra_wait = 10ms; // extra 10ms wait time
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rather than poluting the code with std::chrono::milliseconds in this way you should look at the definition of 'duration' in the events code and do something similar

drivers/tests/TESTS/mbed_drivers/ticker/main.cpp Outdated Show resolved Hide resolved
drivers/tests/TESTS/mbed_drivers/ticker/main.cpp Outdated Show resolved Hide resolved
drivers/tests/TESTS/mbed_drivers/ticker/main.cpp Outdated Show resolved Hide resolved
drivers/tests/TESTS/mbed_drivers/ticker/main.cpp Outdated Show resolved Hide resolved
@mergify mergify bot dismissed adbridge’s stale review March 26, 2021 12:05

Pull request has been modified.

@harrisonmutai-arm
Add test assertion for chrono values
3488dd0 
Remove microsecond macro for ticker time since ThisThread::sleep_for and
TickerBase::attach accept any type of chrono value. Add macro for chrono
value test assertion with and without tolerance.
@harmut01 harmut01 requested a review from kjbracey March 26, 2021 17:24
kjbracey
kjbracey previously approved these changes Mar 29, 2021
View reviewed changes
@mergify mergify bot added needs: CI and removed needs: work labels Mar 29, 2021
@mergify mergify bot added needs: work and removed needs: CI labels Mar 30, 2021
@harmut01 harmut01 requested a review from adbridge April 6, 2021 17:41
@ciarmcom ciarmcom added the stale Stale Pull Request label Jun 6, 2021
@0xc0170
Copy link
Contributor

0xc0170 commented Jun 9, 2021

@ARMmbed/mbed-os-core would anyone be able to pick this up as @harmut01 is no longer around to fix the failures in tests (tickers are failing, they should be related).

@ciarmcom ciarmcom added stale Stale Pull Request and removed stale Stale Pull Request labels Jun 11, 2021
@ciarmcom ciarmcom added stale Stale Pull Request and removed stale Stale Pull Request labels Jun 18, 2021
@ciarmcom ciarmcom added stale Stale Pull Request and removed stale Stale Pull Request labels Jun 25, 2021
@ciarmcom ciarmcom added stale Stale Pull Request and removed stale Stale Pull Request labels Jul 2, 2021
@ciarmcom ciarmcom added the stale Stale Pull Request label Jul 11, 2021
@ciarmcom ciarmcom added stale Stale Pull Request and removed stale Stale Pull Request labels Jul 16, 2021
@hazzlim hazzlim mentioned this pull request Jul 21, 2021
Merged
@ciarmcom ciarmcom added stale Stale Pull Request and removed stale Stale Pull Request labels Jul 23, 2021
@Patater
Copy link
Contributor

Patater commented Jul 29, 2021

Closing in favor of #14941

@Patater Patater closed this Jul 29, 2021
@mergify mergify bot removed component: connectivity component: core component: hal needs: work release-type: patch Indentifies a PR as containing just a patch stale Stale Pull Request labels Jul 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kjbracey kjbracey kjbracey left review comments

@adbridge adbridge adbridge approved these changes

@pan- pan- Awaiting requested review from pan-

@LDong-Arm LDong-Arm Awaiting requested review from LDong-Arm

Assignees

@hazzlim hazzlim

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@harmut01 @ciarmcom @0xc0170 @mbed-ci @Patater @kjbracey @adbridge @hazzlim @harrisonmutai-arm