Skip to content

Timer: minor revisions #12905

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

Merged
merged 2 commits into from
Jun 18, 2020
Merged

Timer: minor revisions #12905

merged 2 commits into from
Jun 18, 2020

Conversation

kjbracey
Copy link
Contributor

@kjbracey kjbracey commented Apr 30, 2020

Summary of changes

Impact of changes

Timer and LowPowerTimer are a little more flexible, and slightly optimised.

Migration actions required

n/a

Documentation

None


Pull request type

[] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[X] 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)
[] Covered by existing mbed-os tests (Greentea or Unittest)
[X] Tests / results supplied as part of this PR

Reviewers


@kjbracey
Copy link
Contributor Author

The copy/move API extension stuff is a bit pointless, but it occurred to me while cleaning this up and after a comment about NonCopyable labelling that there is no particularly strong reason for this to be totally non-copyable. Maybe someone would find a use.

One use I can immediately imagine is sticking a Timer inside a functor passed to EventQueue to measure its dispatch time.

@ciarmcom ciarmcom requested review from a team April 30, 2020 13:00
@ciarmcom
Copy link
Member

@kjbracey-arm, thank you for your changes.
@ARMmbed/mbed-os-test @ARMmbed/mbed-os-core @ARMmbed/mbed-os-maintainers please review.

@ciarmcom ciarmcom requested a review from a team April 30, 2020 13:00
evedon
evedon previously approved these changes May 1, 2020
Copy link
Contributor

@evedon evedon left a comment

Choose a reason for hiding this comment

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

I don't know if it will be useful but the code change looks good.
PR description should be updated though as I can't see some of the minor revisions described.

@mergify mergify bot added needs: CI and removed needs: review labels May 1, 2020
@kjbracey
Copy link
Contributor Author

kjbracey commented May 4, 2020

PR description should be updated though as I can't see some of the minor revisions described.

Oh, guess it was parked a while, and there was a bit of rebasing. Some of those changes already happened in the Chrono work when TimerBase came in.

@0xc0170
Copy link
Contributor

0xc0170 commented May 4, 2020

@kjbracey-arm Looking at the commit, it still has the old description - should be updated as well as Timer ? Please do and we can start CI later today.

@mergify mergify bot dismissed evedon’s stale review May 11, 2020 07:11

Pull request has been modified.

@kjbracey
Copy link
Contributor Author

Updated commit message. CI started.

@mbed-ci
Copy link

mbed-ci commented May 11, 2020

Test run: FAILED

Summary: 1 of 6 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_greentea-test

@mergify mergify bot added needs: work and removed needs: CI labels May 11, 2020
kjbracey added 2 commits May 11, 2020 16:20
* C++11-ify a little.
* Make it copy/move constructible.
@kjbracey
Copy link
Contributor Author

kjbracey commented May 11, 2020

Reworked to pass tests, which apparently I'd failed to run on the final version.

Realised while doing it that we were missing the necessary locking on the copy constructor. Which turned out to be an interesting C++ exercise - answer found at https://www.justsoftwaresolutions.co.uk/threading/thread-safe-copy-constructors.html

In GCC at least those seem to compile to optimal code, fully inlining the locking and state copying into the copy constructor, with no visible overhead from the dummy parameters.

@adbridge
Copy link
Contributor

@evedon @0xc0170 this needs a re-review please

@mergify mergify bot added needs: CI and removed needs: review labels Jun 17, 2020
@0xc0170
Copy link
Contributor

0xc0170 commented Jun 17, 2020

I'll run CI meanwhile

@mbed-ci
Copy link

mbed-ci commented Jun 17, 2020

Test run: SUCCESS

Summary: 6 of 6 test jobs passed
Build number : 2
Build artifacts

@0xc0170
Copy link
Contributor

0xc0170 commented Jun 18, 2020

Already approved, will merge now

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

Successfully merging this pull request may close these issues.

6 participants