Skip to content

Conversation

@mprse
Copy link
Contributor

@mprse mprse commented May 21, 2018

Description

This PR:

  • enables us tickrer and lp ticker support for NRF52 boards,
  • updates sleep driver for NRF52 boards (disable us ticker in deep-sleep mode). It has been verified that hal sleep test passes after these changes,
  • adapts some tests (please see commits description).

In current version we will use same driver as for NRF51_DK. If this step will be successful, then in the next step we can try to use 32 bit counter for us ticker (instead 16 bit counter) and low power counter instead of RTC for lp ticker.

@bulislaw @0xc0170 Can we run morph and perform review here ASAP in order to get it merged before Friday?

Pull request type

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

@0xc0170 0xc0170 requested a review from bulislaw May 21, 2018 12:02
@mprse mprse force-pushed the nrf52_dk_tickers branch from f1a4058 to 34c3a8b Compare May 21, 2018 12:47
@mprse
Copy link
Contributor Author

mprse commented May 21, 2018

I have restarted Jenkins and got the same failure with mbed-os-cliapp which looks not related to this PR.
@0xc0170 Do we maybe know about some issues with Jenkins?

Copy link
Contributor

Choose a reason for hiding this comment

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

We don't claim mbed 2 support for the nrf52 series.

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Please keep the list alphabetically sorted.

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

Copy link
Contributor

Choose a reason for hiding this comment

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

This is no longer compatible with SDK 14.2. Please see the email I sent you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since we need that as soon as possible I will add your changes in the next step.

@mprse mprse force-pushed the nrf52_dk_tickers branch from 34c3a8b to 25655cb Compare May 22, 2018 06:01
mprse added 7 commits May 22, 2018 08:42
It is possible that the difference between base and next tick count on some platforms is greater than 1, in this case we need to repeat counting with the reduced number of cycles (for slower boards).
If number of cycles has been reduced than base tick count needs to be redefined. This operation is missing and is added by this patch.
Test inserts event into the TimerEvent object which is scheduled 50 ms in the future. Then test thread hangs on the semaphore for 51 ms and after that time we expect that event handler has been executed. This test fails sometimes on NRF51_DK, NRF52_DK since different clocks are used for event handling and delay. TimerEvent class uses fast us ticker and semaphores are based on lp ticker (NRF51_DK) or System Tick Timer (NRF52_DK).
We assume that ticker measurement error is 10% and our 1 [ms] extra corresponds to 2% error. I suggest to increase delay to 2 [ms] (4%). This should be enough for all boards at this moment.
Represent tolerance value as it is done in `tests-mbed_drivers_timer`.
Use 5% tolerance for lp ticker.
NRF52_DK is now based on fast and accurate 1MHz counter.
This function should perform instruction cycles count in critical section.
Additionally remove redundant code.
This functionality is required by new sleep standards.
@mprse mprse force-pushed the nrf52_dk_tickers branch from 25655cb to 318063e Compare May 22, 2018 06:42
@bulislaw
Copy link
Member

@OPpuolitaival I've restarted jenkins but we were getting some weird errors:

[K82F_arm-none-eabi-gcc_minimal] 0418c61771456[mbed] ERROR: Unknown Error: [('/home/mbedjenkins/.mbed/mbed-cache/github.com/ARMmbed/mcr20a-rf-driver/.git', '/builds/ws/d-os-cliapp_master-L2X4U6MCIG7IS377UB6AF57YXVKFL@2/mbed-os-cliapp/libs/TARGET_MCR20A_RF_DRIVER/.git', "[Errno 13] Permission denied: '/home/mbedjenkins/.mbed/mbed-cache/github.com/ARMmbed/mcr20a-rf-driver/.git'")]

@OPpuolitaival
Copy link
Contributor

Investigating

@OPpuolitaival
Copy link
Contributor

@bulislaw probably related to this: ARMmbed/mbed-cli#660

@0xc0170
Copy link
Contributor

0xc0170 commented May 23, 2018

@mprse We will run CI as soon as reviewers approve this . I'll do my review now

Copy link
Contributor

@0xc0170 0xc0170 left a comment

Choose a reason for hiding this comment

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

One question about nrf51 in nrf52 implementation, not blocking

}

#if defined(TARGET_MCU_NRF51822)
void common_rtc_irq_handler(void)
Copy link
Contributor

Choose a reason for hiding this comment

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

This file is for NRF52, can this TARGET_MCU_NRF51822 be actually set?

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 will be moved to TARGET_NRF5x. But I would like to do it in the next step together with some changes proposed by @marcuschangarm.

@mprse mprse force-pushed the nrf52_dk_tickers branch from b22d22a to 5156b6a Compare May 23, 2018 12:03
@mprse
Copy link
Contributor Author

mprse commented May 23, 2018

Enabled ticker support for entire MCU_NRF52832 family (not only NRF52_DK).

@mprse mprse force-pushed the nrf52_dk_tickers branch from 5156b6a to 727d50b Compare May 23, 2018 12:16
Copy link
Member

@bulislaw bulislaw left a comment

Choose a reason for hiding this comment

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

Please run CI.

@bulislaw bulislaw merged commit 254ec30 into ARMmbed:feature-hal-spec-ticker May 23, 2018
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.

5 participants