arduino-libraries / RTCZero

RTC Library for SAMD21 based boards
http://arduino.cc/en/Reference/RTC
GNU Lesser General Public License v2.1
77 stars 78 forks source link

Added support for Mode 0 and Mode 1 #58

Open amcmahon01 opened 4 years ago

amcmahon01 commented 4 years ago

Adds support for Mode 0 (32-bit counter) and Mode 1 (16-bit periodic counter). Mode 1 can operate with one or two compare values and optionally a set period. Adds options to reset on match and interrupt on overflow, as well as a helper function to get the source of RTC interrupts and enums for prescale values and interrupt sources. Includes examples for Mode 0 with reset on match, Mode 1 with two compares and a set period and Mode 1 with interrupt on overflow enabled (along with two compares).

msperl commented 4 years ago

Would it not make sense to separate things out into classes RTCZeroMode0, RTCZeroMode1, RTCZeroMode2 (= class RTCZero) to reduce the linked code and make it more explicit?

Maybe then eventually provide: RTCZeroMode0DateTime RTCZeroMode1DateTime with DateTime conversion functions from counters instead for those that need both - this is essentially a Timestamp to "struct tm" conversion.

msperl commented 4 years ago

actually I found out that it makes code bigger as now all the methods are now included regardless of whether they are used or not - my blinker increased from abut 11k to 22k in my simple tests...

msperl commented 4 years ago

one other comment: would it not make sense to use a different divider for gclk - so that one can get a higher precision timer instead of just a 1ms tick - at least for mode0/1? This could actually allow for a bigger range of prescaling values.

amcmahon01 commented 4 years ago

@msperl

actually I found out that it makes code bigger as now all the methods are now included regardless of whether they are used or not - my blinker increased from abut 11k to 22k in my simple tests...

Interesting, I had wondered if there was a more efficient way to structure things, especially since most of the register names for different modes are aliases of the same addresses. I wasn't sure how compiler optimizations would play out in that regard. To clarify, in your testing the version that separated mode by class is the one that ended up significantly larger?

one other comment: would it not make sense to use a different divider for gclk - so that one can get a higher precision timer instead of just a 1ms tick - at least for mode0/1? This could actually allow for a bigger range of prescaling values.

I think this depends on the application. For example, my initial use case will be long term, very slow, very low power devices installed in remote locations, so I've been more interested power consumption and sustainable run time. But I could see other applications benefiting from a higher tick resolution. Maybe that option could be added as a future enhancement?

msperl commented 4 years ago

Interesting, I had wondered if there was a more efficient way to structure things, especially since most of the register names for different modes are aliases of the same addresses. I wasn't sure how compiler optimizations would play out in that regard. To clarify, in your testing the version that separated mode by class is the one that ended up significantly larger?

Yes I found out about that as well...

I think this depends on the application. For example, my initial use case will be long term, very slow, very low power devices installed in remote locations, so I've been more interested power consumption and sustainable run time. But I could see other applications benefiting from a higher tick resolution. Maybe that option could be added as a future enhancement?

But then actually making the divider configurable would be even more important, because you could change it to longer intervals not shorter ones...

I believe I have sent you a pull request that implements it...

But there is one observation: my blinking test app is stopping to work after 1-3 days with the shortest possible divider in 32 bit mode - my guess is it happens on 32 bit overflow, which would be every 1.51 or 3.02 days.

One observation here is that while the sketch works it consumes 70uA with led off and 720uA with led on. But when this happens (and led is turned off) it consumes 120uA, so it may be in an interrupt loop.

msperl commented 4 years ago

Hi!

So as an update: I have taken your latest version with commit: 7256b4267f3815dba1c7c4e3a2451273ab76e9db

And then I ran the following sketch on a FeatherM0:

#define POWER_PIN 11

/* Create an rtc object */
#include <RTCZero.h>
RTCZero rtc;

void myRTC_handler() {
  digitalWrite(POWER_PIN, !digitalRead(POWER_PIN));
}

void setup()
{
  pinMode(LED_BUILTIN, OUTPUT);
  digitalWrite(LED_BUILTIN, HIGH);
  pinMode(POWER_PIN, OUTPUT);
  digitalWrite(POWER_PIN, HIGH);

  rtc.begin(true, 0, true, RTCZero::MODE0_DIV1);
  rtc.attachInterrupt(myRTC_handler);
  rtc.setCount(0ul); 
  rtc.enableCounter(4 * 1024ul);
}

void loop() {
  digitalWrite(LED_BUILTIN, LOW);
  rtc.standbyMode();  
  digitalWrite(LED_BUILTIN, HIGH);
  digitalWrite(POWER_PIN, HIGH);
  rtc.standbyMode();
}

The Feather is powered via a small LIPO battery connected to the Feather via a Current Ranger. So I can measure the current consumed.

So This sketch stopped blinking after 200493 seconds with 25057 positive pulses running at that time! (I have got a logic analyzer capture for the whole time at a 50MHz Sample rate)

It stopped with the LED disabled and at that time the current consumption is 181.1uA.

But note that during "normal" blinking operation: when the LED is off it consumes 87 uA when the LED is on it consumes 743 uA (this is measured after the reset button has been pressed after the blinking stopped)

So there is still something that is not working and the whole system is not reliable.

This also happens when using different clock dividers that are not 32 - see the patch for the arbitrary clock divider case, but for the sake of reporoducability I have used the original patch...

sslupsky commented 4 years ago

Glad to see someone jumping on this. I will offer some additional ideas of things you might want to consider with this PR.

One of the underlying issues with the SAMD21 RTC is the time it takes to "sync" the COUNT and COMP registers. I do not know if you have noticed this but it can take 6 GCLK cycles to do so. If you use a 32KHz GCLK to clock the RTC, this works out to about 180us to sync each register ... which is an enormous amount of time if you are trying to minimize power consumption. It is even worse if you use a 1KHz GCLK to clock the RTC ... the delay is on the order of 6ms!

And, if there are multiple accesses to the COUNT or COMP registers, you get hit with this delay each time. So, in an application where you use the RTC to trigger an interrupt to wake up and do something and go back to sleep, you need to access the COUNT and COMP register at least one time each which could lead to 12ms of delay. In some cases there are many more accesses of these registers required.

For this reason I strongly recommend you configure the GCLK that drives the rtc to 32 KHz. I also strongly recommend to ENABLE the RCONT (read continuously) feature. It is a bit complicated on how to use this feature and you need to be particularly careful when you wake from sleep.

I did some work on the rtc timer driver for zephyr to get this to work properly. When the zephyr rtc ISR fires, the COUNT and COMP registers could be accessed several times due to task scheduling and kernel timers. Here is a snippet that illustrates what you need to do to read the COUNT register properly to minimize the sync delay: https://github.com/sslupsky/zephyr/blob/46bbe17ce05cc8e373927036fe57042fe61b9909/drivers/timer/sam0_rtc_timer.c#L110-L127

The other thing I recommend is configuring the event system to generate periodic interrupts from the rtc as well. Here is some code I use to configure the event system:

    void begin() {
        RTCZero::begin();
        // Feed clock to RTC Event System Channel - Note, this register is not write synchronized so does not require SYNCBUSY wait
        GCLK->CLKCTRL.reg = GCLK_CLKCTRL_CLKEN | GCLK_CLKCTRL_GEN_GCLK2 | RTCZERO_GCLK_CLKCTRL_ID;
        //  EVSYS->CTRL.bit.GCLKREQ = 1;
        //  EVSYS->USER.reg = EVSYS_USER_CHANNEL(RTCZERO_EVSYS_CHANNEL + 1) | EVSYS_USER_USER(EVSYS_ID_USER_DMAC_CH_0);  // Set user event channel to n + 1
        //  turn on the event system peripheral
        PM->APBCMASK.reg |= PM_APBCMASK_EVSYS;
        EVSYS->CHANNEL.reg = EVSYS_CHANNEL_EDGSEL_RISING_EDGE |
                            EVSYS_CHANNEL_PATH_RESYNCHRONIZED |
                            EVSYS_CHANNEL_EVGEN(EVSYS_ID_GEN_RTC_PER_0 + kRTCEventGen) |    //  Period = 32 * 2^(4+3) / 32768 = 125ms
                            EVSYS_CHANNEL_CHANNEL(RTCZERO_EVSYS_CHANNEL);    //  Event Channel

        // enable EVSYS interrupt
        NVIC_EnableIRQ(EVSYS_IRQn);
        NVIC_SetPriority(EVSYS_IRQn, 0x00);

        //  enable interrupt for event channel
        EVSYS->INTENSET.reg |= RTCZERO_EVSYS_INTENSET;

        //  RTC->MODE2.EVCTRL.bit.PEREO7 = 1;

        while (RTC->MODE2.STATUS.bit.SYNCBUSY)
            ;

        voidCCallback callback = makeCCallback(&WITAP_RTC::periodicEvent, this);
        attachPeriodicInterrupt( callback );
        _configured = true;
        _periodicEvent = RTCZERO_EVCTL_PER;
        enablePeriodicEvent();
    }
amcmahon01 commented 4 years ago

Yes, I unfortunately haven't been able to work on this for several weeks, hopefully that will change more in the future.

Thanks @msperl for the detailed test info. If I'm understanding your configuration, you would expect to be calling the interrupt every 4 secs (1024Hz/(4*1024) = 1/4Hz), fully cycling the led every 8 seconds. You're also toggling pin 11, which should match the LED except in the first cycle where it will output 1-01-0-1... (where - represents a wait for interrupt) due to the additional digitalWrite in the main loop. So based on your data, you're "missing" ~5 cycles for the duration of the test? (((200493 secs - (25057 cycles*8=200456 secs))=37 secs)/8 = 4.625 cycles) Are seconds there as noted by your LA? (Thinking about the accuracy of the M0's 32kHz crystal, 200456/200493=0.9998, which seems within the expected accuracy with the specified operating range of 32.276-33.260khz at 25C.)

Regarding the lock ups, if the interrupt is actually being triggered every 4 secs, the RTC counter must be resetting or the interval would change, so that should avoid any overflow there, unless some other register/interrupt is involved. Could the first issue addressed in https://github.com/arduino-libraries/RTCZero/pull/46 be the cause of the lock ups?

@sslupsky Great point and info about the register synchronization especially for power efficiency purposes. I had considered implementing the event system but decided to start with only the simpler approach. Thanks for the example code.

msperl commented 4 years ago

@amcmahon01: my experience is that - contrary to original assumptions - it is not related to a rollover, because sometimes it takes just a few hours other times it takes more than a day (with a clock divider of 1). So it actually may be register sync that sometimes works but rarely fails... if I remember correctly the reset of the flags is not followed by a sync, so if the time until the next sleep is too short then we may reach this situation where we are in sleep again before we have synchronized the registers. There may be a need for this Waiting for sync in the interrupt handler or before we go to sleep...

The biggest problem is that it happens rarely so triggering the issue takes a long time to confirm that it is actually working...

CLAassistant commented 3 years ago

CLA assistant check
All committers have signed the CLA.