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

WIP switching RTC to count32 (vs clock) #345

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

wryun
Copy link
Contributor

@wryun wryun commented Jan 1, 2024

The RTC has three modes, COUNT16, COUNT32 and CLOCK.

CLOCK is nice, in that it:

  • gives us year/month/day/hours/mins/secs straight up
  • allows one to set a number of alarms

However, it doesn't support more than 1 sec granularity. This makes things like a stopwatch (or time systems that don't use seconds) annoying, and also makes it harder for us to set it appropriately (see hackwatch).

If we use COUNT32 instead, we get up to 1024Hz granularity (with the 1024Hz oscillator configured). This commit has it set to 256Hz.

Downsides:

  • extra code complexity
  • we have to maintain an offset (and know about the TZ at a lower level if we want compatibility)
  • we lose the in-built periodic alarms. At the moment, we're only using these for the 1minute wake from sleep operation, which could be replaced by a COMP check (TODO!)
  • potentially, power consumption (I suspect not, since the internals have to deal with the 1024Hz oscillator and the events based on this anyway, but ...)

TODO:

  • simulator support
  • consider moving to 64-bit signed time_t
  • consider defining _times (perhaps also CLOCKS_PER_SEC and using tzset?) so we can just use newlib/time.h; potentially leave in some faster/lower power functions, though.
  • comments on new public functions and warn than BKUP7 is used
  • public function to get just the sub-second count (maybe?) or create a non-compatible watch_get_date_time function which returns all the relevant info (and the real year?)
  • add COMP check to replicate wake from sleep behaviour
  • power consumption implications (if none, switch to 1024Hz counter?)

Clock is nice, in that it:
- gives us year/month/day/hours/mins/secs straight up
- allows one to set a number of alarms

However, it doesn't support more than 1 sec granularity.
This makes things like a stopwatch (or time systems that don't use seconds) annoying,
and also makes it harder for us to set it appropriately (see hackwatch).

If we use COUNT32 instead, we get up to 1024Hz granularity (with the 1024Hz oscillator configured).
This commit has it set to 256Hz.

Downsides:
- extra code complexity
- we have to maintain an offset (and know about the TZ at a lower level if we want compatibility)
- we lose the in-built periodic alarms. At the moment, we're only using these for the 1minute wake from sleep
  operation, which could be replaced by a COMP check (TODO!)

TODO:
- simulator support
- comments on new public functions
- public function to get just the sub-second count (maybe?)
  or create a non-compatible watch_get_date_time function
  which returns all the relevant info (and the real year?)
- add COMP check to replicate wake from sleep behaviour
- power consumption implications (if none, switch to 1024Hz counter?)
@wryun
Copy link
Contributor Author

wryun commented Jan 1, 2024

Interested in any thoughts on the approach, and whether it's ok to drop CLOCK support entirely (as this PR does).

@matheusmoreira
Copy link
Collaborator

This looks really promising.

CLOCK is nice, in that it:

  • allows one to set a number of alarms

So how big a deal is this?

Movement could have a queue of alarms sorted by delta to current time. When an alarm fires, a scheduler overwrites the RTC comparison register with the time of the next alarm in the queue after the current alarm has been processed.

There's something I can't figure out from the documentation. The RTC section only ever talks about COMP0 and ALARM0, suggesting there's only one register for both. The block diagrams though say ALARMn and COMPn, suggesting there are many. I can't figure out which one is true, seems to be ambiguous. If there are many COMPn registers, it should be possible to set multiple hardware alarms in COUNT32 mode as well.

The CLOCK mode does provide masking registers which make it efficient to match seconds, minutes, hours. This would have to be replicated in software.

we lose the in-built periodic alarms. At the moment, we're only using these for the 1minute wake from sleep operation, which could be replaced by a COMP check (TODO!)

potentially, power consumption (I suspect not, since the internals have to deal with the 1024Hz oscillator and the events based on this anyway, but ...)

Can you elaborate on this?

The data sheet says system ticks are supposed to be implemented using the prescaler interrupts which are independent of RTC counter resolution:

The RTC prescaler can generate interrupts and events at periodic intervals, allowing flexible system tick creation.

Periodic events are independent of the prescaler setting used by the RTC counter

Does movement implement it differently? I haven't read that part of the code base yet.

@joeycastillo
Copy link
Owner

joeycastillo commented Mar 3, 2024

This is a case where I feel pretty strongly that Movement shouldn't go this direction. CLOCK mode is baked pretty deeply into the assumptions not just around how Movement should work, but around how the lower-level watch library was designed. One important note is that the idea of an “alarm” to the RTC is different from the user-facing concept of “setting an alarm”: in the context of the RTC peripheral, an alarm is simply something that can wake the device from a sleep mode, via an interrupt. The RTC peripheral offers us three ways to wake up: the periodic tick, the single alarm/comparison, and the external wake via button press.

When imagining how this device would be used, I specifically wanted to be able to wake on periodic ticks when in wake mode, and to be able to wake either once per minute or via a button press when in low energy mode. Configuring the RTC in CLOCK mode lets us set a recurring interrupt at the top of the minute, all with a simple bit mask that's set once at boot and never needs to change. Moving instead to COUNT32 would mean that we can only match on an absolute number of seconds or ticks, which means that as soon as the interrupt fires, we'd have to reset it to match 60 seconds later, or risk not having the device wake at all.

To be clear I have no objection to some alternate firmware concept using the RTC peripheral in this way; I just don't think it makes sense for the main Movement firmware. A consistent, set-and-forget wake at the top of each minute in CLOCK mode grants us a great deal of versatility: in addition to enabling the once-per-minute update in low energy mode, it also allows all watch faces to configure any number of alarms or interactions that happen at the top of the minute. Moving to COUNT32 would require a lot of complexity to replicate this functionality, and I'm reluctant to risk a glitch that breaks the main function of the watch in exchange for sub-second granularity. It would be nice. But it's not a tradeoff I want to make in Movement.

@matheusmoreira
Copy link
Collaborator

matheusmoreira commented Mar 3, 2024

I understand your concerns. I saw that the CLOCK mode provides built-in masking but hadn't considered its impact on the reliability of the watch.

I suppose the prescaler interrupts can't be used because they are always powers of two.

@wryun
Copy link
Contributor Author

wryun commented Mar 3, 2024

Agree with Joey's concerns, but I'd take minor issue with:

Moving to COUNT32 would require a lot of complexity to replicate this functionality...

I think it's not going to be that much code to do the COMP check and replicate the 1-minute notification to all faces. I say, having not actually written the code :)

The other thing to note is that - at least when I last looked - I don't think a single other face was using the alarm configurability, just relying on the background wake. Sounds cool, but in practice no-one has found a use for this.

@joeycastillo
Copy link
Owner

I don't think a single other face was using the alarm configurability, just relying on the background wake.

I think the point here is that every face does rely pretty deeply on the one-minute wake, currently enabled by the alarm masked for an interrupt at :00. It's the way that Movement enables background tasks (which the temperature log uses), and the way it updates the display when in low energy mode, which gets to the heart of the watch being useful at a glance. Also, as a side note, I am unsure how this faster tick would impact battery life, and even if the change could be made without any risk to the user experience, I would want to do fairly significant power profiling before being comfortable with it.

Again, all I'll say is that I am very wary of merging a change like this to Movement and the low-level watch library, whereas if it were a different app that managed the RTC directly, without making these kinds of deep changes that in my mind introduce risk, I would be open to it. Maybe consider forking Movement and a couple of watch faces into a separate app in the apps folder, just to have a place for interested folks to test this out without pushing such a profound change to everyone?

@matheusmoreira
Copy link
Collaborator

matheusmoreira commented Mar 14, 2024

Related discussion:

@wryun and @zzm634 should combine efforts!

RTC/COUNT32 could have its hardware timer comparator register multiplexed between multiple software timers, all we need is a way to dequeue the timer nearest to completion and @theAlexes have suggested radix timers as a possible solution. Every time a timer is started, stopped or fired, the next one would be scheduled for interruption in the RTC. Then faces would be able to flexibly schedule functions for execution.

We must keep in mind @joeycastillo's concerns over reliability of the watch. I assume the frequency/resolution of the RTC counter is irrelevant for power consumption, and that only the frequency of interrupts is irrelevant since they wake up the CPU. Correct me if I'm wrong.

If we can make this facility reliable, then the watch face's minutely low power screen updates will be just one timer among many. Perhaps we could somehow make that time keeping function of the watch a high priority timer or interrupt.

union time_backup tb;
tb.reg = watch_get_backup_data(TB_BKUP_REG);
_sync_rtc();
return watch_utility_date_time_from_unix_time((tb.bit.offset << 8) + RTC->MODE0.COUNT.reg / CLK_RTC_CNT_HZ, tb.bit.tz_offset * TZ_OFFSET_DIVIDER);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am concerned about the effect this is going to have on battery life. Doing the unix time -> datetime conversion is a lot of math, and it's going to be called frequently by a lot of faces that need access to the current time. Whenever possible we should err on the side of dedicated hardware doing the math.

Copy link
Collaborator

@matheusmoreira matheusmoreira Mar 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand your concern and I agree. I think I have a solution though: let's make the current time independent of the RTC. We can make it part of movement's state and have a high priority 1 Hz task keep the current time every second.

That way we can increase movement's flexibility by giving it freedom over how it stores the time, enabling perpetual timekeeping. We should be able to make the epoch configurable too. I speculate it would also reduce power usage: when the watch doesn't have any background tasks scheduled, this will be the only scheduled task running, and provided we can make the timer facility efficient, it should be much faster than all the loops in the current implementation. And who knows, maybe one day someone will want to compile a firmware without the clock face. Movement can allow any face to act as the low energy watch face.


As an aside, I'm also worried about how widely used the % modulo operator is in the sensor watch code base. It's famous for being an inefficient operation and it probably uses a lot of power. For example, hash tables typically require the number of buckets to be a power of 2 in order to make modular arithmetic over them efficient.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some thoughts:

  • there are a lot of faces call watch_utility_date_time_from_unix_time, which this approach makes much less math-y. It might be interesting to do some kind of survey of what current watch faces need (e.g. some need intervals, some need local time, some just need UTC time (i.e. unix time)). For me the attraction of this approach is that we have a single path to optimise where we might be able to provide all of these use-cases with a high-precision time rather than making trade-offs.
  • might be worth experimenting with some kind of caching to optimise the basic clock face time stuff (incrementing only seems scary)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the caching approach. It would be easy to clear on a 1hz tick interval.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still think most of the time, the watch is going to be displaying a regular clock face, in which case it would be best to take advantage of the RTC's calendar functions, and to synchronize the RTC with the local time zone, not UTC. That would let us save the most power by taking advantage of the hardware and avoiding a bunch of timestamp/timezone math.

The best solution would probably be to leave it up to the user. We could move this change behind a preprocessor flag in movement_config.h that lets them switch between "RTC in local time" and "RTC in unix timestamp". For people who want a regular clock face on their watch most of the time, they'd leave the RTC as is, but if their default face is something that makes extensive use of different time zones or unix time, they could switch it to this mode instead.

@matheusmoreira
Copy link
Collaborator

I believe this is the critical issue we need to solve:

as soon as the interrupt fires, we'd have to reset it to match 60 seconds later, or risk not having the device wake at all

In other words, we must ensure that movement's fundamental time keeping function is maintained at all times. I believe this could be accomplished by disabling interrupts when scheduling the timers so that this critical functionality cannot be preempted.

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.

4 participants