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

Update ticker to map closely to hardware #5028

Merged
merged 4 commits into from Oct 13, 2017

Conversation

Projects
None yet
8 participants
@c1728p9
Contributor

c1728p9 commented Sep 5, 2017

Allow tickers to specify their native frequency and number of bits. This allows the conversion to happen in common code rather than in each vendor's implementation.

@c1728p9 c1728p9 added the needs: work label Sep 5, 2017

@c1728p9 c1728p9 requested a review from pan- Sep 5, 2017

@c1728p9 c1728p9 force-pushed the c1728p9:ticker_api_changes branch Sep 14, 2017

@c1728p9 c1728p9 removed the needs: work label Sep 14, 2017

@c1728p9

This comment has been minimized.

Contributor

c1728p9 commented Sep 14, 2017

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Sep 14, 2017

Result: FAILURE

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

/morph test

Output

mbed Build Number: 1303

Test failed!

@c1728p9 c1728p9 force-pushed the c1728p9:ticker_api_changes branch Sep 19, 2017

@c1728p9 c1728p9 changed the title from Add support for various ticker implementations to Update ticker to map closely to hardware Sep 19, 2017

@c1728p9

This comment has been minimized.

Contributor

c1728p9 commented Sep 19, 2017

/morph test-nightly

@mbed-bot

This comment has been minimized.

mbed-bot commented Sep 20, 2017

Result: FAILURE

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

/morph test-nightly

Output

mbed Build Number: 1341

Test failed!

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 20, 2017

Please look at failures, tickers mostly

Allow tickers to specify their native frequency and number of bits. This allows the conversion to happen in common code rather than in each vendor's implementation.

👍 for the info structure addition, still pending reviews

@pan-

Quite happy with that change, it really remove all the conversion and overflow bookkeeping from target code.

I would ask for another review to ensure nothing has been missed.

hal/mbed_ticker_api.c Outdated
uint32_t frequency = info->frequency;
if (info->frequency == 0) {
MBED_ASSERT(0);
frequency = 1;

This comment has been minimized.

@pan-

pan- Sep 20, 2017

Member

100000 ?

This comment has been minimized.

@c1728p9

c1728p9 Sep 20, 2017

Contributor

I'll update this to default to 1MHz rather than 1Hz.

hal/mbed_ticker_api.c Outdated
MBED_ASSERT(0);
bits = 32;
}
uint32_t max_delta = 0x7 << (bits - 4); // 7/16th

This comment has been minimized.

@pan-

pan- Sep 20, 2017

Member

If bits is less than 3 this operation is undefined. Might be worthy to constraint bits in the range [32:4].

This comment has been minimized.

@c1728p9

c1728p9 Sep 20, 2017

Contributor

done

hal/mbed_ticker_api.c Outdated
return;
}

uint64_t new_ticks = (ticker_time - queue->tick_last_read) & queue->bitmask;

This comment has been minimized.

@pan-

pan- Sep 20, 2017

Member

I'd use the wording elapsed_ticks and elapsed_us which seems to me more precise but I might be completely wrong here 😄 .

This comment has been minimized.

@c1728p9

c1728p9 Sep 20, 2017

Contributor

sound good. I updated the variables

@c1728p9

This comment has been minimized.

Contributor

c1728p9 commented Sep 20, 2017

PR #5096 is needed before testing will pass on this PR. This is because us_ticker_read() and lp_ticker_read() no longer returns the same value as ticker_read(ticker). The call to ticker_read(ticker) both scales the frequency and starts counting from 0.

@c1728p9 c1728p9 force-pushed the c1728p9:ticker_api_changes branch Sep 20, 2017

@c1728p9

This comment has been minimized.

Contributor

c1728p9 commented Sep 20, 2017

Addressed @pan-'s comments and rebased to master

@0xc0170

Should we add here now that deep sleep locking info?

hal/ticker_api.h Outdated
uint64_t max_delta_us; /**< Largest delta in us that can be used when scheduling */
uint32_t tick_last_read; /**< Last tick read */
uint64_t tick_remainder; /**< Ticks that have not been added to base_time */
us_timestamp_t base_time;

This comment has been minimized.

@0xc0170

0xc0170 Sep 21, 2017

Member

why are we missing docs for base time? I dont see base_time used anywhere in this patch.

what are units for max delta (might not be that clear diff vs max_delta_us for someone?).

This comment has been minimized.

@c1728p9

c1728p9 Sep 21, 2017

Contributor

This wasn't actually being used so I removed it. I also updated the comments next to max_delta to indicate that the delta was in ticks.

@c1728p9 c1728p9 force-pushed the c1728p9:ticker_api_changes branch 2 times, most recently Sep 21, 2017

@c1728p9

This comment has been minimized.

Contributor

c1728p9 commented Sep 21, 2017

Should we add here now that deep sleep locking info?

This PR is big enough as it is, it might be best as a separate PR on top of this. What do you think @0xc0170 and @pan-?

@pan-

This comment has been minimized.

Member

pan- commented Sep 21, 2017

separate PR.

@0xc0170 0xc0170 added needs: CI and removed needs: work labels Sep 22, 2017

hal/mbed_ticker_api.c Outdated
if (match_tick > prev_tick) {
return (cur_tick >= match_tick) || (cur_tick < prev_tick);
} else {
return (cur_tick < match_tick) && (cur_tick >= prev_tick);

This comment has been minimized.

@jiandeng

jiandeng Sep 26, 2017

If match_tick <= prev_tick, how could prev_tick <= cur_tick < match_tick?

This comment has been minimized.

@c1728p9

c1728p9 Sep 27, 2017

Contributor

Good catch! This is a bug.

@c1728p9 c1728p9 force-pushed the c1728p9:ticker_api_changes branch Sep 27, 2017

@c1728p9

This comment has been minimized.

Contributor

c1728p9 commented Sep 27, 2017

Rebased to master and fixed ticker mismatch calculation. Diff for the new changes can be found in 6712718

@pan-

pan- approved these changes Sep 29, 2017

@mbed-bot

This comment has been minimized.

mbed-bot commented Sep 30, 2017

Result: FAILURE

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

/morph test-nightly

Output

mbed Build Number: 1458

Test failed!

@mbed-bot

This comment has been minimized.

mbed-bot commented Oct 1, 2017

Result: FAILURE

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

/morph test-nightly

Output

mbed Build Number: 1458

Test failed!

Output

mbed Build Number: 1463

Test failed!

@mbed-bot

This comment has been minimized.

mbed-bot commented Oct 1, 2017

Result: FAILURE

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

/morph test-nightly

Output

mbed Build Number: 1458

Test failed!

Output

mbed Build Number: 1463

Test failed!

Output

mbed Build Number: 1469

Test failed!

@mbed-bot

This comment has been minimized.

mbed-bot commented Oct 1, 2017

Result: FAILURE

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

/morph test-nightly

Output

mbed Build Number: 1458

Test failed!

Output

mbed Build Number: 1463

Test failed!

Output

mbed Build Number: 1469

Test failed!

Output

mbed Build Number: 1474

Test failed!

@mbed-bot

This comment has been minimized.

mbed-bot commented Oct 2, 2017

Result: FAILURE

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

/morph test-nightly

Output

mbed Build Number: 1458

Test failed!

Output

mbed Build Number: 1463

Test failed!

Output

mbed Build Number: 1469

Test failed!

Output

mbed Build Number: 1474

Test failed!

Output

mbed Build Number: 1479

Test failed!

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Oct 2, 2017

Besides couple of nightly runs, I reviewed few, ticker or lp_timeout fail, please review the latest test results

@c1728p9

This comment has been minimized.

Contributor

c1728p9 commented Oct 3, 2017

This PR won't pass testing without #5242

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Oct 5, 2017

@c1728p9 It looks like this needs a rebase.

c1728p9 added some commits Sep 13, 2017

Update ticker to map closely to hardware
Allow tickers to specify their native frequency and number of bits.
This allows the conversion to happen in common code rather than in
each vendor's implementation.
Optimize 1MHz and 32KHz use cases
Add optimizations for the most common use cases of the
us ticker, 1MHz,  and the lp ticker, 32KHz.
Add default implementation of timer info
Add weak implementations of *_ticker_get_info which returns 1MHz and
a width of 32 bits. This allows the updated Ticker API to work with
existing devices.

Note - in the future when all targets have implemented
*_ticker_get_info these weak functions will be removed.
Increase ticker test time for slower devices
Increase the hal ticker test time from 30s to 60s to prevent
a timeout from occurring on slower devices, such as the nrf51.

@c1728p9 c1728p9 force-pushed the c1728p9:ticker_api_changes branch to 77dd420 Oct 5, 2017

@c1728p9

This comment has been minimized.

Contributor

c1728p9 commented Oct 5, 2017

rebased onto master

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Oct 6, 2017

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Oct 9, 2017

/morph test-nightly

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Oct 9, 2017

retest uvisor

@mbed-bot

This comment has been minimized.

mbed-bot commented Oct 10, 2017

Result: SUCCESS

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

/morph test-nightly

Output

mbed Build Number: 1553

All builds and test passed!

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels Oct 10, 2017

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Oct 10, 2017

It is now ready for integration.

@c1728p9 This is backward compatible, thus not required to be minor, is that correct?

@c1728p9

This comment has been minimized.

Contributor

c1728p9 commented Oct 10, 2017

Hi @0xc0170, yes this is backwards compatible.

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Oct 10, 2017

@AnotherButler More new APIs here.

@mbed-ci

This comment has been minimized.

mbed-ci commented Oct 11, 2017

@AnotherButler

This comment has been minimized.

Contributor

AnotherButler commented Oct 12, 2017

@theotherjimmy Thanks. I've made a note of that and talked to @c1728p9 about docs. This should not affect this PR being merged.

@theotherjimmy theotherjimmy merged commit fad6755 into ARMmbed:master Oct 13, 2017

5 of 6 checks passed

ci-morph-build build completed
Details
Cam-CI uvisor Build & Test Success
Details
ci-morph-build
Details
ci/morph-test-nightly Job has completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment