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.

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

Add wait_ns API #9812

Merged
merged 5 commits into from Mar 1, 2019

Conversation

Projects
None yet
8 participants
@kjbracey-arm
Copy link
Contributor

commented Feb 22, 2019

Description

This provides the ability to generate really small delays - it's often
the case that wait_us() takes multiple microseconds to set up, so
having an alternative suitable for <10us delays is useful.

There have been a few local implementations - it makes sense to
centralise them as they need retuning for each new ARM core.

Based on the local implementation inside the Atmel 802.15.4 driver.

Pull request type

[ ] Fix
[ ] Refactor
[ ] Target update
[X] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

@c1728p9, @bulislaw

Release Notes

  • wait_ns() API added for <10us small software-loop based delays.

@ciarmcom ciarmcom requested review from bulislaw and c1728p9 Feb 22, 2019

@ciarmcom

This comment has been minimized.

Copy link
Member

commented Feb 22, 2019

@ciarmcom ciarmcom requested review from ARMmbed/mbed-os-maintainers Feb 22, 2019

@kjbracey-arm kjbracey-arm force-pushed the kjbracey-arm:wait_ns branch 3 times, most recently Feb 22, 2019

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor Author

commented Feb 22, 2019

Added a timing test - this will test both that my tuning number (LOOP_SCALER) is correct for each core type, and also that the targets are setting SystemCoreClock correctly, which we rely on. I don't think we have any existing tests for this.

@kjbracey-arm kjbracey-arm force-pushed the kjbracey-arm:wait_ns branch Feb 22, 2019

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor Author

commented Feb 22, 2019

Testing shows a bunch of failures: https://mbed-os.mbedcloudtesting.com/job/mbed-os-ci_greentea-test/1244/testReport/

  • RTL8195 (M3) is running 35% slower than SystemCoreClock indicates according to lp-timer.
  • Nucleo F7s (M7) are sometimes running twice as fast. (F746ZG-IAR was correct, other 2 F746ZG builds failed and alll F767ZI builds ran double speed). I think this is probably an alignment issue - I suspect the M7 is sometimes dual issuing and managing a 1 cycle loop, depending on alignment.
  • NRF51_DK (M0) is running 26% slower than SystemCoreClock indicates according to lp-timer. Might just be some interrupt load though? Seems it should be locked at 16MHz.

Looks like I need to do something to make the M7 timing predictable - the nRF51 and RTL8195 seem to have some sort of target-specific problem - either a clock error or unexpectedly high interrupt load.

@kjbracey-arm kjbracey-arm force-pushed the kjbracey-arm:wait_ns branch 3 times, most recently Feb 22, 2019

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor Author

commented Feb 25, 2019

Updated with something that I hope stabilises the M7 timing - SUBS NOP NOP BCS seems to reliably take 2 cycles, whereas SUBS BCS seemed to take either 1 or 2.

Still need to figure out RTL8195 and NRF51_DK - maybe some input from relevant teams might be useful.

@0xc0170

This comment has been minimized.

Copy link
Member

commented Feb 25, 2019

Still need to figure out RTL8195 and NRF51_DK - maybe some input from relevant teams might be useful.

@ARMmbed/team-realtek Can you review time differences for target?

@ARMmbed/mbed-os-pan or @TacoGrandeTX could comment on NRF51_DK ?

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor Author

commented Feb 25, 2019

In my latest run RTL8195 and M7 devices passed, nRF51 still fails, and now Nucleo L073 and F072 failed too.

https://mbed-os.mbedcloudtesting.com/job/mbed-os-ci_greentea-test/1257/testReport/

It's possible it's down to alignment again, rather than specific platforms - flash sometimes not able to keep up with the instruction flow? Rerunning that one to see if consistent for one build, and will keep fiddling.

If I can't force alignment (seems to be hard to do), may be able to get it down to a tolerable difference between fast and slow alignment by putting enough NOPs in - smoothing out the flow.

@kjbracey-arm kjbracey-arm force-pushed the kjbracey-arm:wait_ns branch Feb 26, 2019

@kjbracey-arm kjbracey-arm force-pushed the kjbracey-arm:wait_ns branch Feb 26, 2019

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor Author

commented Feb 26, 2019

Some platforms seem to be quite persistently running 30% slower than expected, but not been able to lock down a cause. Falling back to making the upper tolerance bound higher, to try to make sure the test is stable. It would be nice to get it more precise, but for the sort of use case envisaged, taking 40% longer is acceptable.

Trying some repeated test system runs to get confidence.

kjbracey-arm added some commits Jan 31, 2019

Add wait_ns API
This provides the ability to generate really small delays - it's often
the case that wait_us() takes multiple microseconds to set up, so
having an alternative suitable for <10us delays is useful.

There have been a few local implementations - it makes sense to
centralise them as they need retuning for each new ARM core.

Based on the local implementation inside the Atmel 802.15.4 driver.
M2351: include core_cm23.h
Nuvoton M2351 was including generic core_armv8mbl.h from CMSIS - we
need it to be more specific to identify the specific core for
wait_ns. Change to core_cm23.h.
#elif (__CORTEX_M == 0 && defined __CM0PLUS_REV) || __CORTEX_M == 3 || __CORTEX_M == 4 || \
__CORTEX_M == 23 || __CORTEX_M == 33
// Cortex-M0+, M3, M4, M23 and M33 take 6 cycles per iteration - SUBS = 1, 3xNOP = 2, BCS = 2
// TODO - check M33

This comment has been minimized.

Copy link
@bulislaw

bulislaw Feb 27, 2019

Member

Could we resolve the TODO before committing this?

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Feb 28, 2019

Author Contributor

We don't actually have any M33 targets yet, do we? So don't think I've got anything I can test this on.

My belief is that the M33 is basically an M4 in terms of internal architecture, but no firm evidence. Could leave the __CORTEX_M == 33 case out and deal with it when we have the first actual target.

@bulislaw
Copy link
Member

left a comment

Looks good, but I don't like the TODO labels, we should either do it now or remove them as it won't happen later.

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Feb 27, 2019

CI started

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Feb 27, 2019

@bulislaw A quick search of the Mbed OS repo found that we have 330 files with TODO (command used: rg TODO --count-matches | wc -l).

If that's the only problem with this PR, I wouldn't suggest holding it up.

@kjbracey-arm It would be good to know why this is still here though.

@mbed-ci

This comment has been minimized.

Copy link

commented Feb 27, 2019

Test run: FAILED

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

Failed test jobs:

  • jenkins-ci/mbed-os-ci_build-ARM
@cmonr

This comment has been minimized.

Copy link
Contributor

commented Feb 27, 2019

Info: A CI config issue appears to be affecting NUMAKER_PFM_M2351 builds. Please ignore build errors against the target for now.

Other build failures should still be investigated, if any. Will restart CI when appropriate.

@bulislaw

This comment has been minimized.

Copy link
Member

commented Feb 28, 2019

@bulislaw A quick search of the Mbed OS repo found that we have 330 files with TODO (command used: rg TODO --count-matches | wc -l).

If that's the only problem with this PR, I wouldn't suggest holding it up.

I approved the PR, so not holding it back, but I find it pointless to add TODO as it's never going to be resolved as your grep just proved :P

@0xc0170

This comment has been minimized.

Copy link
Member

commented Feb 28, 2019

CI restarted

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor Author

commented Feb 28, 2019

Do I need to rebase this or something?

@0xc0170

This comment has been minimized.

Copy link
Member

commented Feb 28, 2019

Do I need to rebase this or something?

Not needed, it's in the CI queue (hooks here will be updated once it is executing).

@mbed-ci

This comment has been minimized.

Copy link

commented Feb 28, 2019

Test run: FAILED

Summary: 1 of 13 test jobs failed
Build number : 2
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_exporter
@0xc0170

This comment has been minimized.

Copy link
Member

commented Feb 28, 2019

Ci will be restarted (iar8 exporter issue we will resolve separately).

@deepikabhavnani
Copy link
Contributor

left a comment

Thanks for adding this @kjbracey-arm

Based on the local implementation inside the Atmel 802.15.4 driver

It will be good to record somewhere that this local implementation should be removed and use the generic one implemented here.

@cmonr cmonr added ready for merge and removed needs: CI labels Feb 28, 2019

@cmonr cmonr merged commit e6caa12 into ARMmbed:master Mar 1, 2019

30 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/build-ARM Success
Details
jenkins-ci/build-ARMC5 Success
Details
jenkins-ci/build-ARMC6 Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR8 Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(+0 bytes) RAM(+0 bytes)
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARM Success
Details
jenkins-ci/mbed2-build-ARMC5 Success
Details
jenkins-ci/mbed2-build-ARMC6 Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR8 Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Local astyle testing has passed
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/doxy-spellcheck Local doxy-spellcheck testing has passed
Details
travis-ci/events Passed, runtime is 9120 cycles (-822 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/include_check Local include_check testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 8408B (+0.00%)
Details
travis-ci/psa-autogen Local psa-autogen testing has passed
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details
travis-ci/tools-py3.5 Local tools-py3.5 testing has passed
Details
travis-ci/tools-py3.6 Local tools-py3.6 testing has passed
Details
travis-ci/tools-py3.7 Local tools-py3.7 testing has passed
Details

@cmonr cmonr removed the ready for merge label Mar 1, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.