Skip to content

Unify RTC, lp ticker, and us ticker for NRF51 and NRF52 series #7172

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
Merged
merged 10 commits into from
Jun 20, 2018
Merged

Unify RTC, lp ticker, and us ticker for NRF51 and NRF52 series #7172

merged 10 commits into from
Jun 20, 2018

Conversation

mprse
Copy link
Contributor

@mprse mprse commented Jun 8, 2018

Description

Add the following ticker modifications for NRF5x boards:

  • Increase NRF52 timer width to 32 bits,
  • Remove duplicate lp ticker initialization from I2C and flash,
  • common_rtc_set_interrupt(): Wrap <ticks_count> before comparisons,
  • NRF51, NRF52: Implement us_ticker_free() function,
  • NRF5x: Add bug fix for the first timer read,
  • NRF5x: Increase lp us ticker interrupt priority,
  • Use same ticker implementation for both NRF51 and NRF52.

Most of these changes have been proposed by @marcuschangarm. Can you review this PR?

Pull request type

[ ] Fix
[ ] Refactor
[ ] New target
[X] Feature
[ ] Breaking change

mprse added 3 commits June 7, 2018 16:00
@mprse
NRF52_DK: Use 32 bit counter for us ticker (instead 16 bit counter)
e5e53e5
@mprse
NRF5x: Remove duplicate lp ticker initialization from I2C, flash, trng.
ccba52b
@mprse
common_rtc_set_interrupt: Wrap <ticks_count> before comparisons
4d56b00 
RTC counter is 24-bit. Upper layer handles counter size and wraps ticks count when interrupt is to be fired before passing it to common_rtc_set_interrupt(), but for consistency and safety reasons we can wrap it again in the NRF driver.
@0xc0170 0xc0170 requested review from marcuschangarm and a team June 8, 2018 11:11
fkjagodzinski
fkjagodzinski previously approved these changes Jun 18, 2018
View reviewed changes
Copy link
Member

@fkjagodzinski fkjagodzinski left a comment

Choose a reason for hiding this comment

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

LGTM

targets/TARGET_NORDIC/TARGET_NRF5x/us_ticker.c
@@ -100,6 +100,9 @@ void us_ticker_init(void)

nrf_timer_task_trigger(NRF_TIMER1, NRF_TIMER_TASK_START);

/* Bug fix. First value can't be trusted. */
nrf_timer_task_trigger(NRF_TIMER1, NRF_TIMER_TASK_CAPTURE1);
Copy link
Member

Choose a reason for hiding this comment

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

If this is somehow related to one of the known anomalies, it may be worth adding a reference.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it is. I just noticed some very odd behavior on my boards.

marcuschangarm
marcuschangarm reviewed Jun 18, 2018
View reviewed changes
targets/TARGET_NORDIC/TARGET_NRF5x/TARGET_NRF52/lp_ticker.h Outdated
#define LP_TICKER_COUNTER_BITS 32u
#define LP_TICKER_FREQ 31250

#endif // LP_TICKER_H
Copy link
Contributor

Choose a reason for hiding this comment

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

I can't see where this file is being used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file is added by mistake. I have removed it.

marcuschangarm
marcuschangarm reviewed Jun 18, 2018
View reviewed changes
targets/TARGET_NORDIC/TARGET_NRF5x/us_ticker.c Outdated
@@ -110,7 +110,9 @@ void us_ticker_set_interrupt(timestamp_t timestamp)
{
core_util_critical_section_enter();

nrf_timer_cc_write(NRF_TIMER1, NRF_TIMER_CC_CHANNEL0, timestamp & 0xFFFF);
const uint32_t counter_mask = ((1 << US_TICKER_COUNTER_BITS) - 1);
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this bit-shift always work? I was expecting this:

((1ULL << US_TICKER_COUNTER_BITS) - 1)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed as suggested.

marcuschangarm
marcuschangarm reviewed Jun 18, 2018
View reviewed changes
targets/TARGET_NORDIC/TARGET_NRF5x/us_ticker.c Outdated
@@ -110,7 +110,9 @@ void us_ticker_set_interrupt(timestamp_t timestamp)
{
core_util_critical_section_enter();

nrf_timer_cc_write(NRF_TIMER1, NRF_TIMER_CC_CHANNEL0, timestamp & 0xFFFF);
const uint32_t counter_mask = ((1 << US_TICKER_COUNTER_BITS) - 1);
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as above.

@marcuschangarm
Copy link
Contributor

@mprse I just got back in the office, thank you for pushing this! I've added a couple of minor comments.

mprse added 7 commits June 19, 2018 08:46
@mprse
NRF51, NRF52: Implement us_ticker_free() function.
cca4090
@mprse
Use common us_ticker.c for NRF51 and NRF52 boards
1583cbc
@mprse
NRF5x: Add bug fix for the first timer read.
7e222f2 
It has been noticed that first read value can not be trusted.
@mprse
NRF5x: Increase lp us ticker interrupt priority
b073720 
Set the second highest user level, leaving the highest for UART (we are having constant overflows) and two levels below for everything else.
This should increase the timer accuracy.
@mprse
Use common_rtc.h for NRF51 and NRF52 boards
4a8de08
@mprse
Use lp_ticker.c for NRF51 and NRF52 boards
80fabcb
@mprse
us_ticker_set_interrupt: fix bit-shift operation
02d7d25
@mprse mprse dismissed stale reviews from fkjagodzinski and maciejbocianski via 02d7d25 June 19, 2018 07:53
@mprse
Copy link
Contributor Author

mprse commented Jun 19, 2018

Provided some fixes after review.

@marcuschangarm
Copy link
Contributor

Looks good! Thank you!

@0xc0170
Copy link
Contributor

0xc0170 commented Jun 20, 2018

/morph build

@mbed-ci
Copy link

mbed-ci commented Jun 20, 2018

Build : SUCCESS

Build number : 2402
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/7172/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

@mbed-ci
Copy link

mbed-ci commented Jun 20, 2018

Exporter Build : SUCCESS

Build number : 2037
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/exporter/7172/

@mbed-ci
Copy link

mbed-ci commented Jun 20, 2018

Test : SUCCESS

Build number : 2188
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/7172/2188

@cmonr cmonr merged commit 84d6b79 into ARMmbed:master Jun 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marcuschangarm marcuschangarm marcuschangarm left review comments

@0xc0170 0xc0170 0xc0170 approved these changes

@fkjagodzinski fkjagodzinski fkjagodzinski approved these changes

@maciejbocianski maciejbocianski maciejbocianski left review comments

Assignees
No one assigned
Labels
release-version: 5.9.2
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@mprse @marcuschangarm @0xc0170 @mbed-ci @maciejbocianski @fkjagodzinski @cmonr