Skip to content

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

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 2 commits into from

Conversation

hoihu
Copy link
Contributor

@hoihu 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
Copy link

IveJ commented Dec 29, 2019 via email

@glennrub
Copy link
Contributor

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 :)

@hoihu
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
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
Copy link
Contributor

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
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
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
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
Copy link
Contributor

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
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
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 feat-add-ticks-nrf branch from f80e8ba to 3d8c81c Compare December 31, 2019 22:40
@hoihu
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).

@dpgeorge
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
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 feat-add-ticks-nrf branch from ad0b936 to adcf7b9 Compare January 14, 2020 21:04
@hoihu hoihu force-pushed the feat-add-ticks-nrf branch from cef8d98 to 675ec0a Compare January 14, 2020 21:32
@hoihu
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
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.

@hoihu
Copy link
Contributor Author

hoihu commented Feb 18, 2020

I'd like to proceed with this and would be glad for a feedback.

I have a working solution with the ticker's CCR0 module (which I believe has the least dependencies on other modules).

To summarize, the proposed approach is to use Timer1's CCR0 module. Timer1 is using the low 1MHz input clock and allows WFI support (important for asyncio). CCR0's IRQ would be calling a tick_handler function which would increment the global msec tick counter. My WIP is working with asyncio runs just fine.

My assumption, given the constraints above are that this the most viable way to go. Is this correct?

Then I'd like to rename the ticker's module compiler switch to something more general like MICROPY_PY_MACHINE_TICKER instead of MICROPY_PY_MACHINE_SOFT_PWM. Does that sound viable?

@hoihu
Copy link
Contributor Author

hoihu commented Feb 18, 2020

In addition, I'd like to support time.sleep_usec() as well.

currently, I'm using the Timer1's CCR1 module to do this like that:

mp_uint_t mp_hal_ticks_us() { 
    NRF_TIMER1->TASKS_CAPTURE[1] = 1;
    return NRF_TIMER1->CC[1];
}

this would eliminate the hard coded NOP instruction currently used. The code for the mp_hal_delay_us is then similar as on other port, e.g.

void mp_hal_delay_us(mp_uint_t us) {
    uint32_t now = mp_hal_ticks_us();
    if (us == 0) return;
    while (mp_hal_ticks_us() - now < us) {}
}

It does use CCR1, so I don't know if that's fine?

@glennrub
Copy link
Contributor

@hoihu , I'm so sorry about the late response. However, i've started to look into a timer replacement for nrf9160 now, and it is natural to start with this! :)

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).

A bit of history; The ticker is basically the backend for the music and display module originally for micro:bit (ported from the micro:bit repository). Instead of keeping this source in the boards/microbit, i had a wish to implement also a software machine.PWM for generic use in case the target needed more PWMs or did not have the HW peripheral. The music module was generic enough to be shared with other targets in the port.
The operations of displaying on a multiplexed array of LEDs with fixed intervals, and playing a music tone, needs a accurate timer. However, for a background timer, i'm still thinking RTC and not Timer to be able to have MicroPython on a battery for a decent period. It's not ideal as it is not accurate, but it will depend on the use case.

I have not updated myself to much on asyncio to really understand the requirements on how accurate the timer has to be and how it is integrated. I'll start looking at existing code and documentation to get up to speed on this module. If you have something you want to share, that would also be interesting.

In the case i need a similar timer for nrf9160 socket operations (and select) i believe the timer does not have to be that 100% accurate. Let's say you put a receive timeout on a socket for 3 seconds, i believe it does not really matter if this is 3.001 or 3.020 seconds, as there are natural network delays in any case. This is where i'm thinking RTC in this specific case, as it would make more sense to save the power, and compromise the accuracy.

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.

@dpgeorge , I agree. However, i guess the case is that you will have a 5mA draw just to keep that value accurate. What is better, accurate timers in application or overall low power? For me it is all use case dependent. Could it be an option add the possibility to boot up the chip in high accuracy (Timer), and on request switch to lower power (RTC)? By adding something like this: then add something like utime.mode(LOW_ACCURACY_LOW_POWER).

@dpgeorge , @hoihu , Despite my new ideas and questions, i'm thinking we should proceed with what is working (ticker/Timer1). It is a bit tricky putting power and accuracy up against each other, but choosing Timer1 for now will make things move. Or should we investigate more options?

@dpgeorge
Copy link
Member

However, i guess the case is that you will have a 5mA draw just to keep that value accurate.

I think it's only 5uA (micro-amp), which is not much.

What is better, accurate timers in application or overall low power? For me it is all use case dependent. Could it be an option add the possibility to boot up the chip in high accuracy (Timer), and on request switch to lower power (RTC)?

I think to start with it'd make sense to have just a compile-time option to select the ticks ms/us implementation, either Timer or RTC. The default could be accurate Timer and a user could rebuild with RTC if needed.

Despite my new ideas and questions, i'm thinking we should proceed with what is working (ticker/Timer1). It is a bit tricky putting power and accuracy up against each other, but choosing Timer1 for now will make things move.

Yes I agree. Let's just get something working.

@hoihu I see that this PR is still using RTC1, which is 24-bit, so wouldn't work with wrap around as-is. Are you able to change it to use the Timer1 CCR that you've tested?

@hoihu
Copy link
Contributor Author

hoihu commented Jun 14, 2020

I'm so sorry about the late response

No problem at all. Glad we can continue on this.

Are you able to change it to use the Timer1 CCR that you've tested?

I'll look into it and hope that I can provide something this week.

@hoihu
Copy link
Contributor Author

hoihu commented Jun 17, 2020

@glennrub can you confirm that #define MICROSECONDS_PER_TICK 16 is actually wrong? As it is, the timer1 is configured for 1usec / tick, thus MICROSECONDS_PER_TICK should be 1 IMO. Perhaps timer1 was set to another frequency in the past?

I tried this and it's working with 1. I would include that change in the PR.

So far I can configure the time base to be fed by either ticker/timer1 or RTC1(see https://github.com/hoihu/micropython/tree/feat-nrf-ticks-support).

I'll cleanup and do some more testing. Probably post it tomorrow.

@dpgeorge
Copy link
Member

can you confirm that #define MICROSECONDS_PER_TICK 16 is actually wrong? As it is, the timer1 is configured for 1usec / tick, thus MICROSECONDS_PER_TICK should be 1 IMO

I'm pretty sure it's correct as-is. TIMER1 counts at 1MHz but it has 4 channels which trigger at different times and call callbacks. Channels 0, 1 and 2 are "fast" and use units of MICROSECONDS_PER_TICK in setting their callback period (minimum period is 16us). Channel 3 is "slow" and uses MICROSECONDS_PER_MACRO_TICK for its period (6ms). This is all original from the microbit.

@hoihu
Copy link
Contributor Author

hoihu commented Jun 18, 2020

I'm pretty sure it's correct as-is.

you are of course right, I was slightly confused about TICKS but as you said it is meant to be the timebase for the ticker module, not the actual timer's HW ticks. Sorry about the noise.

However it's not ideal as per now because 1000 (us) is not a multiple of 16.. So the CCR0 irq cannot be fired in precise 1msec intervals.

Could we possibly redefine the CCR0's timebase to be scaled in 1usec (line https://github.com/micropython/micropython/blob/master/ports/nrf/drivers/ticker.c#L107)?

@dpgeorge
Copy link
Member

Could we possibly redefine the CCR0's timebase to be scaled in 1usec

Yeah that should be ok. If anything relies on it being 16us then they can most likely be adjusted to work with 1us. I'd say go for it.

@hoihu hoihu mentioned this pull request Jun 18, 2020
@hoihu
Copy link
Contributor Author

hoihu commented Jun 18, 2020

PR ready to review: #6171

@dpgeorge
Copy link
Member

Superseded by the implementation in 15574cd

@dpgeorge dpgeorge closed this Jul 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants