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

world_clock2_face Broken in next branch #475

Closed
CarpeNoctem opened this issue Sep 10, 2024 · 6 comments · Fixed by #469
Closed

world_clock2_face Broken in next branch #475

CarpeNoctem opened this issue Sep 10, 2024 · 6 comments · Fixed by #469
Assignees
Labels
bug Something isn't working next This feature or pull request is present in the next branch

Comments

@CarpeNoctem
Copy link
Contributor

CarpeNoctem commented Sep 10, 2024

Branch: next
Tested Branch/Revision: next / be969c4 (11 Sep 2024)
Last working branch / revision: main / b69cd11 (5 Sep 2024)
Board color: GREEN

When setting offsets to enable, the offset/hour never changes, but instead stays stuck at the system's offset.
When viewing, the time never updates, but only shows the current system time as also shown by the simple clock.

@CarpeNoctem
Copy link
Contributor Author

CarpeNoctem commented Sep 14, 2024

Looking at the diff between the branches, this face was updated in commit bae8c65

Haven't determined yet whether this is certainly the cause. I see it also touched the sunrise and moon phase faces that I use, but I haven't seen any issue with either of those. That said, the world clock face likely relies on TZ data differently (less statically) than those two.

UPDATE: This does seem to be the culprit. The world clock face relies on grabbing/calculating timezone data on the fly, and this commit removes that. I'll revert this commit and test.

@CarpeNoctem
Copy link
Contributor Author

Need to look into this further... Reverting that commit fixed the timezones updating in the settings, but in display mode, the time stays locked to current local time. So there must be some other additional culprit there. Looking further...

@voloved
Copy link
Contributor

voloved commented Sep 14, 2024

I found the issue.
There is a variable called current_zone that I confused for the time zone that the preferences hold.

Two lines in that code need to be modified. Unfortunately, I'm AFK until Tuesday, but it's lines 188 and 293 that need changing.

I would prefer adding an extra int16 in world_clock2_state that's called current_tz, and change that variable only when you change the viewed location. That way, there's fewer calculations for finding the same time zone info each time.

Reviewing the other 18 complications the auto DST logic touched, it's only this face and the original world clock that had lines changed that didn't original use settings.bit.time_zone, so only those 2 faces need changing.

@CarpeNoctem
Copy link
Contributor Author

Yep, I see it now, thanks! I had run out of time earlier today before getting to look at the other commits that are new in this branch and had affected this file. That new DST functionality indeed overwrote the current_zone bit.

I'll try testing this again - instead of reverting that other commit, I'll try just correcting these two lines.

@CarpeNoctem
Copy link
Contributor Author

CarpeNoctem commented Sep 15, 2024

Made the fixes as @voloved suggested, and it works. Two things to note, though:

  1. The call on line 297 could possibly be optimized to run less frequently, but I'm not sure where to put it, given how the settings UI works. Maybe in that if-statement just above?
  2. I think DST may be messing with the settings. When DST=true, each TZ viewed is shown as being an hour ahead of where it should be. E.g. UTC shows as +1:00 instead of 0:00. It is probably a simple matter of accounting for the DST setting in the settings view method.
  3. I've only looked at world_clock2, not world_clock.

Here is the git diff of the fix:

user@persona:~/src/github.com/joeycastillo/Sensor-Watch ((be969c4...) *)
$ git diff movement/watch_faces/clock/world_clock2_face.h
diff --git a/movement/watch_faces/clock/world_clock2_face.h b/movement/watch_faces/clock/world_clock2_face.h
index efc107e..72dd1ca 100644
--- a/movement/watch_faces/clock/world_clock2_face.h
+++ b/movement/watch_faces/clock/world_clock2_face.h
@@ -105,6 +105,7 @@ typedef struct {
     uint8_t current_zone;
     uint32_t previous_date_time;
     int16_t tz;
+    int16_t current_tz;
 } world_clock2_state_t;
 
 void world_clock2_face_setup(movement_settings_t *settings, uint8_t watch_face_index, void **context_ptr);

diff --git a/movement/watch_faces/clock/world_clock2_face.c b/movement/watch_faces/clock/world_clock2_face.c
index 0681096..dfef13d 100644
--- a/movement/watch_faces/clock/world_clock2_face.c
+++ b/movement/watch_faces/clock/world_clock2_face.c
@@ -155,6 +155,7 @@ void world_clock2_face_activate(movement_settings_t *settings, void *context)
             break;
     }
     state->tz = get_timezone_offset(settings->bit.time_zone, watch_rtc_get_date_time());
+    state->current_tz = movement_timezone_offsets[state->current_zone];
     refresh_face = true;
 }
 
@@ -185,7 +186,7 @@ static bool mode_display(movement_event_t event, movement_settings_t *settings,
             /* Determine current time at time zone and store date/time */
            date_time = watch_rtc_get_date_time();
            timestamp = watch_utility_date_time_to_unix_time(date_time, state->tz * 60);
-           date_time = watch_utility_date_time_from_unix_time(timestamp, state->tz * 60);
+           date_time = watch_utility_date_time_from_unix_time(timestamp, state->current_tz * 60);
            previous_date_time = state->previous_date_time;
            state->previous_date_time = date_time.reg;
 
@@ -239,14 +240,16 @@ static bool mode_display(movement_event_t event, movement_settings_t *settings,
            break;
        case EVENT_ALARM_BUTTON_UP:
            state->current_zone = find_selected_zone(state, FORWARD);
-            state->previous_date_time = REFRESH_TIME;
+        state->current_tz = movement_timezone_offsets[state->current_zone];
+        state->previous_date_time = REFRESH_TIME;
            break;
        case EVENT_LIGHT_BUTTON_DOWN:
            /* Do nothing. */
            break;
        case EVENT_LIGHT_BUTTON_UP:
            state->current_zone = find_selected_zone(state, BACKWARD);
-            state->previous_date_time = REFRESH_TIME;
+        state->current_tz = movement_timezone_offsets[state->current_zone];
+        state->previous_date_time = REFRESH_TIME;
            break;
        case EVENT_LIGHT_LONG_PRESS:
            movement_illuminate_led();
@@ -290,7 +293,8 @@ static bool mode_settings(movement_event_t event, movement_settings_t *settings,
                 watch_clear_indicator(WATCH_INDICATOR_PM);
                 refresh_face = false;
             }
-           result = div(state->tz, 60);
+           // result = div(state->current_tz, 60);
+           result = div(get_timezone_offset(state->current_zone, watch_rtc_get_date_time()), 60);
            hours = result.quot;
            minutes = result.rem;
 
@@ -329,7 +333,8 @@ static bool mode_settings(movement_event_t event, movement_settings_t *settings,
        case EVENT_ALARM_LONG_PRESS:
            /* Find next selected zone */
            if (!state->zones[state->current_zone].selected)
-               state->current_zone = find_selected_zone(state, FORWARD);
+                   state->current_zone = find_selected_zone(state, FORWARD);
+        state->current_tz = movement_timezone_offsets[state->current_zone];
 
            /* Switch to display mode */
            state->current_mode = WORLD_CLOCK2_MODE_DISPLAY;

matheusmoreira added a commit that referenced this issue Sep 15, 2024
The new DST changes caused problems in one specific face - world_clock2.
An incorrect variable was used due to a confusing name.
It has been revised to fix the problems that were caused.

Closes #475.

Reported-by: CarpeNoctem <[email protected]>
Fixed-by: David Volovskiy <[email protected]>
Tested-by: CarpeNoctem <[email protected]>
Tested-on-hardware-by: CarpeNoctem <[email protected]>
GitHub-Pull-Request: #470
GitHub-Issue: #475
@matheusmoreira matheusmoreira self-assigned this Sep 15, 2024
@matheusmoreira matheusmoreira added bug Something isn't working next This feature or pull request is present in the next branch labels Sep 15, 2024
@matheusmoreira matheusmoreira linked a pull request Sep 16, 2024 that will close this issue
34 tasks
@CarpeNoctem
Copy link
Contributor Author

Can confirm that world_clock2_face and world_clock_face are now both working again as of 3c86a42.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working next This feature or pull request is present in the next branch
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants