add subsecond precision to set_time_face#203
Conversation
| // Like a casio watch we reset the seconds to 0 to the nearest minute | ||
| bool to_next_minute = date_time.unit.second >= 30; | ||
|
|
||
| if (subseconds >= half_freq) { |
There was a problem hiding this comment.
can we get some comments explaining what's going on here? we've got the feel for what it does, but can't quite put it into words.
There was a problem hiding this comment.
ah! the words came to us: as we understand it, this is doing fixed-point rounding to the subseconds field.
| case EVENT_ALARM_BUTTON_UP: | ||
| _abort_quick_ticks(); |
There was a problem hiding this comment.
we get changing it to act on down, but it looks like we should probably abort_quick_ticks too.
There was a problem hiding this comment.
when I followed the existing logic it didn't seem that it was needed:
- quick ticks are started on longpress
- quick ticks are stopped on longpress up
- quick ticks are also stopped on any tick event when the button pin is detected as up
So I can put it back in, but I don't think it does anything
| bool to_next_minute = date_time.unit.second >= 30; | ||
|
|
||
| if (subseconds >= half_freq) { | ||
| delta = (subseconds - half_freq) * 8; |
There was a problem hiding this comment.
This has to do with the table in
right? So it should tick when the counter is at half frequency = 64 ticks?So I went through some test cases:
current ticks above 64: 66, 100, 126
66 means we want to wait 128 - 66 + 64, so that's 126 ticks worth of delta
100 -> 128 - 100 + 64 = 92
126 -> 128 - 126 + 64 = 66
So shouldn't this be
delta = (freq - subseconds + half_freq) * 8
because subseconds - half_freq would be
66 - 64 = 2
100 - 64 = 36
126 - 64 = 62
so would wait for shorter time when it should wait for longer time
I also went through the * 8 conversion from ticks to ms part, I assume that's because it's an approximation of 1000/128? How about using *1000/128, or * 125 >> 4? For above values that would be
| delta | * 8 | * 1000 / 128 | * 125 >> 4 | difference (in ms) |
|---|---|---|---|---|
| 126 | 1008 | 984.4 | 984 | 24 |
| 92 | 736 | 718.8 | 718 | 18 |
| 66 | 528 | 515.6 | 515 | 13 |
Another thing, would it make sense to call it subsecond_ticks? because subseconds sounds to me like the milliseconds part of the full second going on ...
There was a problem hiding this comment.
You're correct that the reason for this math is due to the 1Hz periodic callback happening when counter % 128 == 64.
I don't remember how I came up with the math above, but should probably trust that is correct, since I've used it for month and has always behaved as expected.
Let's work out some examples, so I can maybe re-discover how/why I did it this way:
First example, when we press the button to zero the seconds, and the counter happens to be at counter % 128 = 66.
It means that the rtc counter is 2 ticks ahead of the actual time, and we need to stop it 2 ticks, which is exactly subsecond - half_freq as in the code.
Now let's take the other side, when we press the button, the counter happens to be at counter % 128 = 62.
It means that the actual time is 2 ticks ahead of the rtc time. Clearly we cannot stop time, so we stop the rtc clock for 126 ticks instead, which is exactly half_freq + subsecond as in the code.
Finally, you are correct that I'm doing an approximation where I say 128 * 8 = 1024 ~= 1000.
I think 0.024s in the worst case scenario is irrelevant, and well more precise than the accuracy of a person pressing the button to zero the watch.
There was a problem hiding this comment.
Makes sense, I misunderstood, thanks for the explanation how to read this.
| if (subseconds >= half_freq) { | ||
| delta = (subseconds - half_freq) * 8; | ||
| } else { | ||
| delta = (half_freq + subseconds) * 8; |
There was a problem hiding this comment.
I think this one might also be the wrong way round, say we're at tick 2, we want to wait 62 ticks, so that's 64 - 2 or half_freq - subseconds.
| date_time = watch_utility_date_time_from_unix_time(timestamp, 0); | ||
| } | ||
|
|
||
| watch_rtc_enable(false); |
There was a problem hiding this comment.
Maybe overkill or pointless, or bad idea for other reasons, not sure really, but would it make sense to __disable_irq(); while between reading the counter, and until RTC is stopped so that the calculation can't take longer than expected (which would make delta wrong)?
Make the set time behave like a standard casio watch:
Apparently there is a slight chance of freezing the watch when we enable/disable the RTC, but that's something we also do in finetune and nanosec.