joeycastillo/Sensor-Watch

Leading-Zero 24hr Bugs

Closed this issue · 5 comments

CarpeNoctem commented

Branch: next
Most recent revision tested & present: c9cbb82
Board color: GREEN

Issue 1: When switching from 024hr mode to 12hr mode, there is a leftover leading zero in front of the hour. This goes away if the user goes back to the usual 24hr mode, then back to 12hr mode. Seems like it could be a stuck "pixel" due to the way simple_clock_face conserves power, except that this happens in both am/pm and an example of this happening includes going from 16:xx to 04:xx PM. (note that the first digit is 1, not 0.) This only appears to be happening in simple_time_face, and not in others such as set_time_face, alarm_face

Issue 2: 24hr Indicator missing from alarm_face when in 024h mode.

matheusmoreira commented

  1. Ah, I see what you mean. The clock faces try to conserve power by comparing timestamps and only drawing what has changed. They do not currently take into account whether the display mode has changed. I'll try to push a fix!

  2. Understood, I'll track it down and put it in.

CarpeNoctem commented
  1. Ah, I see what you mean. The clock faces try to conserve power by comparing timestamps and only drawing what has changed. They do not currently take into account whether the display mode has changed. I'll try to push a fix!

Well, just don't go down too much of a rabbit hole on that one. As I said, it's changing a 1 to a 0, so I'm not sure it's simply a matter of it not updating that digit. (Because it is indeed updating it.) I'm not sure how the leading-zero bit works, but maybe one of the display conditions just isn't quite right?

Thanks for looking into it!

matheusmoreira commented

The 024h mode works by ensuring there are always 2 digits in the hours display. So instead of 1:00 it displays 01:00.

The relevant display logic in the simple_clock_face is just this:

Sensor-Watch/movement/watch_faces/clock/simple_clock_face.c

Lines 140 to 141 in c9cbb82

if (set_leading_zero)
watch_display_string("0", 4);

I believe the problem happens because it does not unset the 0 pixels in LCD position 4 when the watch is not configured to display it. So if the watch was in 024h mode before and it is switched back to 24h or 12h, the pixels are not cleared, they remain there until the screen fully updates.

Simply adding an else clause to that if should at least partially solve the issue.

if (set_leading_zero) {
    watch_display_string("0", 4);
} else {
    watch_display_string(" ", 4);
}

In the refactored clock_face I already handled that case by using snprintf which has built in support for the leading zero format:

Sensor-Watch/movement/watch_faces/clock/clock_face.c

Lines 127 to 142 in c9cbb82

static void clock_display_all(watch_date_time date_time, bool leading_zero) {
char buf[10 + 1];
snprintf(
buf,
sizeof(buf),
leading_zero? "%s%02d%02d%02d%02d" : "%s%2d%2d%02d%02d",
watch_utility_get_weekday(date_time),
date_time.unit.day,
date_time.unit.hour,
date_time.unit.minute,
date_time.unit.second
);
watch_display_string(buf, 0);
}

A problem remains though: the above lines will only run when a full screen update is done, which occurs only when the current hour changes.

Sensor-Watch/movement/watch_faces/clock/simple_clock_face.c

Lines 102 to 129 in c9cbb82

bool set_leading_zero = false;
if ((date_time.reg >> 6) == (previous_date_time >> 6) && event.event_type != EVENT_LOW_ENERGY_UPDATE) {
// everything before seconds is the same, don't waste cycles setting those segments.
watch_display_character_lp_seconds('0' + date_time.unit.second / 10, 8);
watch_display_character_lp_seconds('0' + date_time.unit.second % 10, 9);
break;
} else if ((date_time.reg >> 12) == (previous_date_time >> 12) && event.event_type != EVENT_LOW_ENERGY_UPDATE) {
// everything before minutes is the same.
pos = 6;
sprintf(buf, "%02d%02d", date_time.unit.minute, date_time.unit.second);
} else {
// other stuff changed; let's do it all.
#ifndef CLOCK_FACE_24H_ONLY
if (!settings->bit.clock_mode_24h) {
// if we are in 12 hour mode, do some cleanup.
if (date_time.unit.hour < 12) {
watch_clear_indicator(WATCH_INDICATOR_PM);
} else {
watch_set_indicator(WATCH_INDICATOR_PM);
}
date_time.unit.hour %= 12;
if (date_time.unit.hour == 0) date_time.unit.hour = 12;
}
#endif
if (settings->bit.clock_24h_leading_zero && date_time.unit.hour < 10) {
set_leading_zero = true;
}

Sensor-Watch/movement/watch_faces/clock/clock_face.c

Lines 144 to 174 in c9cbb82

static bool clock_display_some(watch_date_time current, watch_date_time previous) {
if ((current.reg >> 6) == (previous.reg >> 6)) {
// everything before seconds is the same, don't waste cycles setting those segments.
watch_display_character_lp_seconds('0' + current.unit.second / 10, 8);
watch_display_character_lp_seconds('0' + current.unit.second % 10, 9);
return true;
} else if ((current.reg >> 12) == (previous.reg >> 12)) {
// everything before minutes is the same.
char buf[4 + 1];
snprintf(
buf,
sizeof(buf),
"%02d%02d",
current.unit.minute,
current.unit.second
);
watch_display_string(buf, 6);
return true;
} else {
// other stuff changed; let's do it all.
return false;
}
}

In order to fully resolve this issue, I will decompose those hours/minutes checks into actual bits of information in the clock's state structure: hours_dirty and minutes_dirty. The display code will simply check those bits instead of concerning itself with computing them. I'll add functions which compute whether the hours or minutes are dirty and set those bits. Finally, the problem will be fixed by simply setting hours_dirty = true when the user changes the display mode via the watch face iself, causing the face to redraw the hours in the new format.

Compared to all this I expect the alarm_face fix to be much simpler, if not a one liner.

matheusmoreira commented

OK issue should be fixed. Turned out to be much simpler than I initially thought it would be.

When I read the code again, I realized there was no way to change the display mode from inside the face, the user must go to preferences screen. That causes a face activation when the user returns to the clock face. Activating the clock face resets the previous time value's bits to all ones which ensures a full repainting. That means the bug actually had nothing to do with what I described above.

The real problem turned out to be the fact that there's a 24h_mode bit and a leading_0 bit, switching from 024h mode back to 12h mode cleared 24h_mode but not leading_0, and only leading_0 was being taken into account when the time came to decide whether to add a leading zero. Changing it to 24h_mode && leading_0 solved the problem, at least in the simulator.

I have also added the 24h indicator back to the alarm face. All issues should be resolved. Would appreciate further testing for confirmation.

  • ❤️1
CarpeNoctem commented

I've just run through all my testing again, and can confirm that these two issues are fixed. Good job and thanks!

and only leading_0 was being taken into account when the time came to decide whether to add a leading zero. Changing it to 24h_mode && leading_0 solved the problem

Doh. This is exactly what I meant when I said "but maybe one of the display conditions just isn't quite right?" in my comment above. Sorry for not being clearer; it was very late at night here. >__<

Thanks again!

  • 👍1