Skip to content

changes arising from Nordic work #454

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

Closed
wants to merge 11 commits into from
Closed

changes arising from Nordic work #454

wants to merge 11 commits into from

Conversation

rgrover
Copy link
Contributor

@rgrover rgrover commented Aug 20, 2014

Here’s a compilation of some of the changes I want to make to mbed-src. They have mostly to do with power optimizations originating from the Nordic work. Here are the highlights:

  • Replace the use of high-frequency clock with low-frequency crystal oscillator in system startup.
  • Rebase the implementation of us-timer (micro-second timer) on top of the app_timer module provided by Nordic (which uses the RTC).
  • Introduce timestamp_t as a type to represent time within TimerEvent. This helps avoid complications arising from timer wrap-around.
  • Minor improvements to the ticker implementation.

Thanks,

@0xc0170
Copy link
Contributor

0xc0170 commented Aug 20, 2014

Travis build failed.. us_ticker definition needs to be updated for each platform.

@Sissors
Copy link
Contributor

Sissors commented Aug 20, 2014

At which point the question should probably be: Do we want ticker to be 64 bit? Sure it has advantages, but it also means it cannot run directly on any timer anymore, and that there always needs to be extra synchronization code between the software and the hardware part of the timer.

@rgrover
Copy link
Contributor Author

rgrover commented Aug 20, 2014

The changes I made to TickerEvent are independent of the underlying timer implementations. I would like to be shown why the builds are failing.

@0xc0170
Copy link
Contributor

0xc0170 commented Aug 20, 2014

You can click on Travis failed build above (Details), which redirects you to the build for this pull request:
https://travis-ci.org/mbedmicro/mbed/builds/33072494

@bogdanm
Copy link
Contributor

bogdanm commented Aug 20, 2014

The build error shouldn't be hard to fix, although it involves changing code in most (or maybe all) our platforms, but @Sissors mentioned the main problem with this: it might involve additional synchronization to properly access the 64-bit variable. I'm not sure we want to do this now.

@rgrover
Copy link
Contributor Author

rgrover commented Aug 21, 2014

Adding timestamp_t is a useful thing. Currently us_timer_set_interrupt() relies on a 32-bit timer on most platforms, but not all. There are already examples where the argument to set_interrupt() needs to be translated before feeding into the configuration registers of the underlying timers. And it is also reasonable to assume that we'd like to use low-power versions of hardware timers to retarget us_ticker (such as using RTC); and such timers often won't be 32-bit. I suggest we dis-engage the argument of us_timer_set_interrupt() from the assumption that the underlying timers are 32-bit; and use an explicit timestamp_t. There is a significant gain in making timestmap_t 64-bit. I'll resubmit the pull request with definitions of us_timer_set_interrupt() updated for all platforms.

@Sissors
Copy link
Contributor

Sissors commented Aug 21, 2014

I don't have this target myself, so others will be alot more knowledgeable regarding changes to clock setups, so I stick to the timer.

First of all, with these changes you don't have microsecond resolution anymore. And that means bitbanged clockless protocols, such as one-wire, most likely won't work properly anymore. Aditionally normally you got the RTC timer for longer times. I don't know if on this target that is available. Finally you do 64-bit, but it is still read from 32-bit register, multiplied by roughly 30. So I don't think the timer will properly wrap around like a real 32-bit (or 64-bit) timer would do.

@rgrover
Copy link
Contributor Author

rgrover commented Aug 21, 2014

I am aware that following this change we will no longer get micro-second resolution when using Ticker on the Nordic nRF51822. I'm making a tradeoff between timer accuracy and power consumption. Using the RTC instead of a high-frequency timer brings down the average current from 700uAmps to 6uAmps, and this has a significant impact on battery powered BLE devices; which are the main application space for the nRF51822.

We are gradually moving towards a system where the expected accuracy and/or duration of a wait/TickerEvent determines the underlying timer. Switching the nRF51's ticker to use RTC is an initial step in that direction. Ideally, if the Ticker is configured to go at very short intervals, we should eventually be using a micro-second timer underneath; but for now, i'm trying to switch to RTC by default.

We've been using a us_timer for the sake of consistency, but this doesn't allow taking advantage of low-power hardware like the RTC. People needing micro-second accuracy for bit-banging will still be able to setup one of the high-frequency hardware TIMERs directly; but in the large majority of cases Ticker will be used as a periodic callback mechanism without the need for micro-second precision.

I'm proposing to switch the Ticker's internal calculation for timing-delays and time-for-next-callback to use 64-bit; this is to avoid wrap-arounds. I still expect most platforms to implement the Ticker using a 32-bit wide micro-second hardware timer, so the us_ticker_api is still expected to provide 32-bit resolution, but the interface for us_ticker_insert_event() and us_ticker_set_interrupt() should take in 64-bit values from the Ticker.

@xiongyihui
Copy link
Contributor

I agree with rgrover . I just replace TIMER with RTC in my project.

@Sissors
Copy link
Contributor

Sissors commented Aug 21, 2014

Ah I see, missed that the regular read is still 32-bit. Another option would be to only do the interrupt part on the other timer (obviously takes another timer, but the Freescale devices do the same). It is just that I think it should be really well considered before differences are made between mbed targets, the main idea of a common library to not have those.

But am I correct that the idea is to use the RTC interrupt instead of timer interrupt so you can place it in a lower power mode while waiting for the interrupt? Because if that is the goal, then I think it might be considered to just make a general mbed API for waking from sleep modes. Because while that is nice for this target, every single M0 target (and also some others) are of course for low power applications, and then it might make more sense to just use a specialised API for this in addition to the ticker API. Not every target would get this, but some would. Currently for example I got a library on mbed which supports quite some targets: http://mbed.org/users/Sissors/code/WakeUp/. I just wonder if it should come at the cost of the regular Ticker. If I understood it wrong, ignore this ;)

Anyway I don't have this one, and not going to get involved further. However I simply think it should be well considered before changing the accuracy compared to other targets.

@rgrover rgrover closed this Aug 26, 2014
yogpan01 pushed a commit to yogpan01/mbed that referenced this pull request Jul 21, 2016
fix tests to work correctly for synchronous operation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants