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

ports/nrf: add support for time.ticks_ms() #5470

Open
wants to merge 2 commits into
base: master
from

Conversation

@hoihu
Copy link
Contributor

hoihu commented Dec 28, 2019

The nordic port lacks support for a milisecond / microsecond tick
counter (and basically most of the time module functionality..).

The ms tick counter is needed by asyncio and prevents the usage
of this module, which is my primary design goal.

So I had a go and enabled the systick irq with 1msec resolution.
This enables the usage of a 32 bit counter rather than
on relying on the hw's systick timer 24bit width. But it does
add a bit of overhead. 32bit would overflow after 50 days - although
the pyhton return value might be restriceted to only 31 bits (?). 24 bits take app.
5h to overflow, which might be suitable too.

Inreasing the counter's width to 64bit would eliminate any overflow
handling on the python side, but may add some overhead on passing
this value from C to python (but I'm not sure how much).

Another option would have been to grab a separate HW timer unit, which could
probably be used in a better, power optimized manner and possibly
without irq's since they are 32bit width.

However I was reluctant to do this since for some MCU's a timer unit is precious.

So in summary, it was a surpring amount of factors to consider for
such a small change. Looking forward to some inputs/comments.

@IveJ

This comment has been minimized.

Copy link

IveJ commented Dec 29, 2019

@glennrub

This comment has been minimized.

Copy link
Contributor

glennrub commented Dec 29, 2019

Thanks a lot for this, @hoihu!
And thanks a lot for the extensive study you have done!

In order to get the true feeling of the time.ticks_ms() be shown when running in REPL UART mode, we might also consider to remove this line: https://github.com/micropython/micropython/blob/master/ports/nrf/mphalport.c#L73.

Reasoning:
When Systick is used, going into WFI/WFE will also stop the Systick count as it is sourced by the same clock as the CPU. In case of REPL, i believe we are not needing this power optimization pointed out (WFI) as we are already running quite some power using the UART peripheral already.

With its limitations in mind, it might still be possible to use RTCounter objects to get more accurate lower power timing events in conjunction with machine.deep_sleep()/WFI to overcome the Systick gaps produced during sleep.

Do not get me wrong, I'm still in favor for this one going in :)

ports/nrf/main.c Outdated Show resolved Hide resolved
ports/nrf/main.c Outdated Show resolved Hide resolved
ports/nrf/boards/pca10059/mpconfigboard.h Outdated Show resolved Hide resolved
@hoihu

This comment has been minimized.

Copy link
Contributor Author

hoihu commented Dec 29, 2019

Reasoning:
When Systick is used, going into WFI/WFE will also stop the Systick count as it is sourced by the same clock as the CPU

oh my goodness this information just saved me ;)

Thanks.. that's good to know and is apparently different than on the stm32 port (where for example wfi is used within the systick irq handler as part of the MICROPY_EVENT_POLL_HOOK macro. And this macro is required to make select.poll work it seems.

So at the moment I have a the new asyncio version in #5332 working on the nrf52.

@hoihu

This comment has been minimized.

Copy link
Contributor Author

hoihu commented Dec 29, 2019

With its limitations in mind, it might still be possible to use RTCounter objects to get more accurate lower power timing events in conjunction with machine.deep_sleep()/WFI to overcome the Systick gaps produced during sleep.

Agreed. Essentially this means that you can't use systick with any low power setting (and that might be a popular use case e.g. for a bluetooth beacon etc). I'll have a look into RTCounter.

@glennrub

This comment has been minimized.

Copy link
Contributor

glennrub commented Dec 29, 2019

So at the moment I have a the new asyncio version in #5332 working on the nrf52.

Awesome! :)

I'll have a look into RTCounter

We might have a common interest here. I plan to replace the sdk_app_timer_mod.c fork from Nordic SDK in my nrf9160 socket support branch with a new MIT licensed RTC timer module. It has been on my TODO list for quite some time, but not yet started on it. If i'm not wrong we are looking for the same thing, right?

Some thoughts on what i had in mind:

I'm thinking that the modules/machine/rtcounter.c should reserve RTC1 peripheral instance and be able to make as many timers needed. A bit like the SDK one, limited by a config of course. Providing a public registration interface for lets say ticks_ms() and LTE timers to register. Today, the machine.RTCounter creates one object for each peripheral instance, which is a bit bad utilization of the very valuable peripheral. Also, RTC0 is not available when SoftDevice is enabled.

@hoihu

This comment has been minimized.

Copy link
Contributor Author

hoihu commented Dec 29, 2019

To recap my scan through the datasheet: There are 3 independant RTC timers. They are 24bit counters with 1/32.768 resolution. Using it for 1msec resolution is possible by setting the divider to 33, but introduces an error of 0.71% (app 25sec/h).

Overflows occurs app. after 4.6h and could be handled via irq, if necessary.

@hoihu

This comment has been minimized.

Copy link
Contributor Author

hoihu commented Dec 29, 2019

if i'm not wrong we are looking for the same thing, right?

It could overlap, I'm not sure. My primary goal is to get asyncio running on the nrf. This needs to have at least:

  • time.ticks_ms() - that could include to use RTC object as you have proposed
  • select support

some optional things I did for my testing:

  • sync manifest handling support with stm32 port (to freeze asyncio.py)
  • add CTRL-C on USB CDC REPL

should reserve RTC1 peripheral instance and be able to make as many timers needed

ok, similar as the "soft timers" on the stm32? Could be ported from there (see softimer.cetc).

RTC0 is not available when SoftDevice is enabled.

ok good to know. So maybe could RTC1 be configured for 1msec intervals that ticks_ms() can use? In parallel also a softtimer can be handled I guess (using a compare irq for example). The counter's value must not be changed manually though, ticks_ms needs a free running timer value.

@hoihu

This comment has been minimized.

Copy link
Contributor Author

hoihu commented Dec 29, 2019

I somehow like the idea to configure RTC1 as msec timer and RTC2 as a (precise) seconds timer.

What I think is a bit tricky to get right are the dependencies of the RTC objects to the various modules... so for example, if ticks_ms is enabled, RTC1 should not be used.. how can we transparently show that to the user?

@glennrub

This comment has been minimized.

Copy link
Contributor

glennrub commented Dec 29, 2019

I somehow like the idea to configure RTC1 as msec timer and RTC2 as a (precise) seconds timer.

I believe we should rule out the use of RTC2. If i recall correctly it is only available in nrf52832, nrf52833 and nrf52840. Other sub-variants of nrf52 lacks it, and also its not present in nrf51, nrf9160 and nrf53.

What I think is a bit tricky to get right are the dependencies of the RTC objects to the various modules... so for example, if ticks_ms is enabled, RTC1 should not be used.. how can we transparently show that to the user?

I'm not sure if i answer the question. I might have misunderstood it. However, I'm thinking that we should re-write the python interface of RTCounter to not use argument 0 (id) as instance number of the physical peripheral to use. But rather auto assign it. Or have a fixed max number of slots used for object creation. The number of c-module users like ticks_ms(), time.sleep_ms() could be known at compile time and be preregistered depending on configuration so that they are served before the dynamically created ones. To some extent the processing time for each of the c-module users can be measured and compensated for when setting the timeout value.

@hoihu

This comment has been minimized.

Copy link
Contributor Author

hoihu commented Dec 31, 2019

I might have misunderstood it.

You answered what I meant to ask ;) I'm sorry I'm not native english, so if there is some uncertainty please just ask.

However, I'm thinking that we should re-write the python interface of RTCounter to not use argument 0 (id) as instance number of the physical peripheral to use.

I kind of liked the link for id = hw module number. Maybe add a shim layer on top of those objects, e.g. a kind of an RTCManager that returns state of each RTCobject etc?

The number of c-module users like ticks_ms(), time.sleep_ms() could be known at compile time and be preregistered depending on configuration so that they are served before the dynamically created ones

Hmm, in the end we'll end up writing C code to initiate RTC1 for msec resolution, either by interfacting/reusing code in rtccounter.c or by using some hal functions from scratch. Both is possible.

But on the other hand - we do have support for the RTC's, in upy. Why bothering with writing C code? Can't we just create a RTC object at boot time (e.g. in boot.py) and then make the time module use the RTC counter? It would most likely also eliminate the need for an RTC manager, since it's pretty clear in uPy code which modules are used and which not.

I tried but unfortunately, you can't monkey patch time.ticks_ms().. possibly because it's a built in module (?). I guess thinking about it it would be nice to have a configurable time source for the time module than could even be changed at runtime.

@hoihu

This comment has been minimized.

Copy link
Contributor Author

hoihu commented Dec 31, 2019

actually writing the c code to initalize RTC1 in msec resolution was simpler than I thought. This worked for me and can be placed in mphalconfig.c:

nrfx_rtc_t rtc1 = NRFX_RTC_INSTANCE(1);

void rtc1_init_msec(void) {
    rtc1.p_reg->PRESCALER = 33;
    rtc1.p_reg->TASKS_START = 1;
}

then accessing the counter is as simple as

mp_uint_t mp_hal_ticks_ms(void) {
  return (mp_uint_t)rtc1.p_reg->COUNTER;
}

calling rtc1_init_msec is done within main.c. time.ticks_ms() then shows the expected increase in milliseconds. Would that be a way to go forward?

PS: I'm not sure if the above code will work on other nrf platforms (nrf51..)

@hoihu hoihu force-pushed the hoihu:feat-add-ticks-nrf branch from f80e8ba Dec 31, 2019
@hoihu

This comment has been minimized.

Copy link
Contributor Author

hoihu commented Dec 31, 2019

@glennrub : see force pushed update. It now uses rtc1 as milisecond timer. I checked the datasheet of nrf51 and it seems they have the same RTC interface structure, so I hope this will work there too (I haven't got HW to test with).

@hoihu hoihu force-pushed the hoihu:feat-add-ticks-nrf branch Dec 31, 2019
@dpgeorge dpgeorge added the port-nrf label Jan 6, 2020
@dpgeorge

This comment has been minimized.

Copy link
Member

dpgeorge commented Jan 6, 2020

@hoihu this looks ok to me but I don't have as much experience with the nRF as @glennrub . IMO it's more important to have a good/accurate ticks_ms() rather than retain a hardware counter for other use and have a poor ticks_ms(). I think it's also important to keep the low power WFI() instruction where it's currently used. You probably also want to include ticks_add() and ticks_diff() (easy, just copy how stm32 does it).

Is the rtc1 counter 24 bit? That's not a problem but you will need to configure MICROPY_PY_UTIME_TICKS_PERIOD appropriately.

@hoihu

This comment has been minimized.

Copy link
Contributor Author

hoihu commented Jan 14, 2020

IMO it's more important to have a good/accurate ticks_ms() rather than retain a hardware counter for other use and have a poor ticks_ms()

I think it's also important to keep the low power WFI() instruction where it's currently used

I agree on both.

It's not possible to reach both with the systick counter (because WFI switches off the CPU clock) and also not with the RTC counter (because it's not very precise for msec intervals).

So that leaves only the Timer module. This can be setup to use the (relative low power) 1MHz clock PCLK1M as input and has 32bit width.

But I'm not sure if WFI also switches off PCLK1M. this seems to imply that WFI is actually System ON: Low Power and maybe this also means that the 1MHz clock remains enabled. Perhaps @glennrub can share some insight on this? If so I can have a go at using a timer module for ticks.

Is the rtc1 counter 24 bit? That's not a problem but you will need to configure MICROPY_PY_UTIME_TICKS_PERIOD appropriately.

yes it's 24 bits. good point.

From a power saving point of view, the RTC can be used with really low power characteristics (< 0.5uA) but the 1MHz clock is not bad either with 5uA. A very power sensitive application may profit from the RTC.

by enabling rtc1 as milisecond timer
@hoihu hoihu force-pushed the hoihu:feat-add-ticks-nrf branch to adcf7b9 Jan 14, 2020
@hoihu hoihu force-pushed the hoihu:feat-add-ticks-nrf branch to 675ec0a Jan 14, 2020
@hoihu

This comment has been minimized.

Copy link
Contributor Author

hoihu commented Jan 20, 2020

So further testing report ;)

I did discover the ticker module which does excactly what I was looking for. I've used the ununsed CCR0 channel to setup a msec callback interrupt. That can be done in mphalport.c like:

volatile uint32_t tick_ms;

int32_t tick_cb(void) {
    tick_ms += 1;
    return 1000;
}

void ticker0_init_msec(void) {
    set_ticker_callback(0, tick_cb, 200);
}

mp_uint_t mp_hal_ticks_ms(void) {
    return tick_ms;
}

I tested this with the new asyncio module, and it works (even with WFI in the event_poll_hook). So I think this approach is the best compromise on power consumption (app. 5uA @ 1MHz) and usage (one timer that is used anyway for softpwm).

@hoihu

This comment has been minimized.

Copy link
Contributor Author

hoihu commented Jan 20, 2020

One thing to think about...

There is code in CCR3 where a tick variable is incremented too, but in 6msec steps (https://github.com/micropython/micropython/blob/master/ports/nrf/drivers/ticker.c#L117). I believe that was intended and cannot be easily changed? Otherwise, an option would be to just modify it to increase in 1msec steps.

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