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

Fixed offset timezones #4901

Closed
ps2 opened this issue Aug 17, 2019 · 17 comments
Closed

Fixed offset timezones #4901

ps2 opened this issue Aug 17, 2019 · 17 comments

Comments

@ps2
Copy link
Contributor

ps2 commented Aug 17, 2019

Is your feature request related to a problem? Please describe.
When Loop uploads fixed offset timezones, like "GMT-0500" they don't show up in the timezone picker for the profile editor. The schedule rates themselves seem to be displaying properly in the main screen, so it seems like there is some level of support for them. Fixed offset timezones are important for Loop, as they are more representative of how time is tracked in a pump than timezones that have special rules for daylight savings time or other switches.

Describe the solution you'd like
The offset that the schedule is stored in show be displayed in the picker.

@sulkaharo
Copy link
Member

I'm not sure I follow what the desired end goal is here? Where do you see the profile time zone being used? Is Loop reading it in? Is some data being displayed wrong somewhere?

Nightscout's current date implementation works so that when data is sent to the REST API, the API expects the dates to use the ISO-8601 format. The date is then normalized to UTC in the database for the purpose of REST queries to work as expected (given the dates are treated as Strings in the database layer and if the system allowed mixed format non-normalized dates, the API would give incorrect query results). The API then returns the normalized strings, unless the instance is configured to de-normalize strings back to format that has the time zone. Internally in the runtime the dates are deserialized to Date objects and logic either uses the API in Date() or "UTC milliseconds from January 1, 1970 00:00:00 UTC" as per Date.now() representation for comparisons. Nightscout clients are expected to basically use whatever native date implementation is in the platform they've been implemented in, and deserialize the ISO-8601 dates to the native date objects and not make assumptions on the string representation other than the date being in the ISO format.

@ps2
Copy link
Contributor Author

ps2 commented Aug 21, 2019

The profile timezone should be shown in the profile editor, and it isn't for fixed offset timezones.

@ps2
Copy link
Contributor Author

ps2 commented Aug 21, 2019

I also realized that the console log was filling with error related to the fixed offset timezones:
211_-8_→
Based on those errors, I patched Loop to upload fixed offset timezones with identifiers like ETC/GMT+6 instead of "GMT-0600" (ps2/rileylink_ios#535), which does fix the console errors. But, unfortunately, the dropdown in the profile editor is still blank, so I'll keep this ticket open.

@Nightfoxy
Copy link

Would this also cause the issue where the basal, IOB and COB data is missing in the day to day report when your timezone isn't the same as the person who is uploading? I tried setting the profile editor timezone manually and re-running the report but setting the profile timezone didn't fix it.
image

@marionbarker
Copy link

I posted a question on Loop zulipchat and was told by @ps2 that he believes this ticket was closed prematurely.

I'll repeat the zulipchat post here and then add follow-up information in next comment.

This has been a long standing issue for mentors who help Loopers via viewing their Nightscout sites.

  • I do not know if this is strictly a NS problem or Loop + NS problem.
  • I will state the problem and then I will list the two open issues related to the topic that I found.
  • I will be happy to open a new issue or add to an existing one, but I thought I'd start with a conversation here first.

Problem:

  • If I view a Looper's site that is in a different time zone
    • day-to-day charts have some issues:
      • basal plots start at the Looper's "default" midnight
      • IOB/COB plots start at the Looper's "default" midnight
      • the reported date is wrong
  • If I then change my computer time to the Looper's time zone, every thing plots correctly
  • The NS profile (must be authenticated to view) does not include the Looper's time zone but if you look at the NS report for profile, that does include the time zone

Figure below shows the same day plotted with my computer set to 3 different time zones

  • top is my actual time zone PST - reports correctly as Thursday 1/12/2023
  • middle is Atlanta EST (3 hours in my future) - reports as Friday 1/13/2023
  • bottom is Honolulu (2 hours in my past) - reports as Thursday 1/11/2023
  • note basal / IOB / COB / Prediction plots begin at "Default" line, not included before
  • overrides, glucose (uploaded from Loop) and prediction lines are correct

nighscout-time-zone

List of open issues on time zone, Nightscout and Loop.

cgm-remote-monitor (NS):

  • 2021-02-08: #6853: Wrong dates in day to day report
    • sulkaharo says: "sounds like a Loop issue, where it's uploading an incorrect profile. To get that fixed, you'd need to get in touch with the Loop developers."

Loop:

@marionbarker
Copy link

The response from @ps2 on Loop zulipchat is as follows:

Pete Schwamb: My guess is it's related to Nightscout not handling fixed offset timezones properly. This ticket was closed: #4901, but I'm not sure it was actually fixed. I think the issues you're seeing might be a better use case to drive proper handling of these timezones in Nightscout.

Pete Schwamb: There has been a widespread misunderstanding/mishandling of pump timezones by Nightscout, OpenAPS, etc. These systems will often take the user's current timezone, with daylight savings time effects, and use it as the timezone of record for basal schedules, etc. The issue is that pumps don't really use those timezones, as they don't account for DST effects. They just use a dumb, fixed offset timezone. If the pump runs by itself through the night during a DST change, the pump events on it should be interpreted with the fixed offset timezone. Loop is doing the right thing by uploading a fixed offset timezone to represent how schedules are being used by Loop, and the pump, at a particular moment in time.

@bewest
Copy link
Member

bewest commented Jan 15, 2023

Thanks for digging this up, @marionbarker. I've been meaning to spend some time on this issue.

Looks like the issue is mainly the profile records.
Here's what Loop provides:

{"_id":"63bc9a9274a6c7619984d57d","store":{"Default":{"timezone":"ETC/GMT+8","dia":6.166666666666667,"basal":[{"time":"00:00","timeAsSeconds":0,"value":1.8},{"time":"05:30","timeAsSeconds":19800,"value":1.7},{"timeAsSeconds":81000,"value":1.8,"time":"22:30"}],"carbs_hr":"0","carbratio":[{"time":"00:00","value":10,"timeAsSeconds":0}],"delay":"0","target_high":[{"value":102,"timeAsSeconds":0,"time":"00:00"}],"sens":[{"timeAsSeconds":0,"time":"00:00","value":40}],"target_low":[{"timeAsSeconds":0,"value":97,"time":"00:00"}]}},"startDate":"2023-01-09T22:52:01Z","enteredBy":"Loop","defaultProfile":"Default","units":"mg/dL","mills":"1673304721904","loopSettings":{"minimumBGGuard":55,"dosingEnabled":true,"bundleIdentifier":"com.5A8TKU9HN9.loopkit.Loop","preMealTargetRange":[64,65],"overridePresets":[{"duration":3600,"insulinNeedsScaleFactor":0.5,"targetRange":[120,125],"name":"sleepin","symbol":"🤸‍♀️"},{"symbol":"🚵‍♂️","duration":10800,"targetRange":[135,136],"insulinNeedsScaleFactor":1.5,"name":"horse"},{"targetRange":[165,180],"insulinNeedsScaleFactor":0.7,"symbol":"⛹️‍♂️","duration":5400,"name":"basketball"},{"symbol":"⚽️","insulinNeedsScaleFactor":0.8,"name":"soccer","targetRange":[155,170],"duration":8100},{"duration":7200,"name":"tennis","targetRange":[160,165],"symbol":"🎾"},{"targetRange":[110,115],"symbol":"🏹","insulinNeedsScaleFactor":1.2,"name":"medicine","duration":0},{"name":"swim","duration":6300,"targetRange":[150,180],"symbol":"🏊‍♂️🏊‍♂️","insulinNeedsScaleFactor":0.8},{"symbol":"💃","name":"extra","insulinNeedsScaleFactor":1.5,"duration":3600},{"duration":7200,"targetRange":[108,112],"symbol":"➖","name":"less","insulinNeedsScaleFactor":0.9}],"deviceToken":"b37bba40759e47496f0663025af5ece63feb439c87ec595e75b9d378f72da323","maximumBasalRatePerHour":6,"scheduleOverride":{"insulinNeedsScaleFactor":1.5,"name":"extra","duration":10800,"symbol":"💃"},"maximumBolus":9.9}}

As Pete mentions, the timezone field is set to timezone":"ETC/GMT+8".

openaps will produce one of the names in the list of official timezone names.

Here's the List. https://en.wikipedia.org/wiki/List_of_tz_database_time_zones

The format Pete mentions is on the list. It would be great to take a look at AndroidAPS/openaps or similar and see if theirs differ. It looks like FreeAPSXNG or similar has the same issue.

@bewest
Copy link
Member

bewest commented Jan 15, 2023

I'm not sure the issue are the way timezones are handled at-large, it looks like everything that is a valid timezone name and is 8601 compliant is working as expected. I think the issue might be that the daytoday reports need to have the day start at midnight in the timezone of the profile. The daytoday report in particular is always in the profile's timezone. Currently the day starts for the chart in the browser's timezone.

@marionbarker
Copy link

Thanks much Ben.

I looked and there are a lot of day to day report issues open. I think this one is appropriate for me to add to.

#6853

@sulkaharo
Copy link
Member

There's several things to do here: 1) Document exactly the list of supported time zones and add validation that those are used in the code, 2) make complete refactor of the graphing code, which has bugs in the time zones but location of those is unknown. The Moment errors above are probably related and caused by Nightscout not uploading the full Moment time zone library to the client, which last I checked was several megabytes of data that significantly slowed down the client. There's been attempts at getting rid of Moment altogether, but I haven't been able to find a replacement that actually works (proposed libraries like Day.js have terrible bugs with... time zone support). Right now the "easy fix" is for Loop to upload recognised time zones - ones from the list you can see in the profile editor.

@sulkaharo
Copy link
Member

Ah right, assuming people are using a version of Loop that uploads a zone string from this list, UI in profile editor is probably failing with the time zone due to simple string case mismatch. This is nontrivial to fix though as the UI case right now is strongly tied to the persisted data and we probably don't want the non-cased string comparison to leak into the persisted data

@sulkaharo
Copy link
Member

#7833 should change the profile editor behaviour so it shows the time zones uploaded by Loop. And maybe changes the reports to work as well. I think what's happening is, the ISO standard is case sensitive with the time zone identifiers (the identifiers are "not just strings") and the time zone code uploaded by Loop doesn't comply with ISO as it's using the wrong case. Loop uploads the zone as ETC/GMT+6, when the correct form is Etc/GMT+6. The workaround in the PR is not guaranteed to stay in the codebase forever. If someone could test that branch with data from Loop, that'd be great.

@bewest
Copy link
Member

bewest commented Jan 15, 2023

@marionbarker, I haven't tested, or run this properly, but reading through additional code today, I suspect this change might help line up the daytoday charts so that midnight is always relative to the profile's timezone. #7836

@marionbarker
Copy link

I heard that dev won't build on Google Cloud, which is what I'm using for my NS sites.
Here's the information: Discussion 2618: NightScout on Google Cloud - update to DEV (15.0.0) version not possible.
I tried it anyway and it didn't work; but subsequently heard from Navid how to update my VM to build dev. But that will be another day. If you think I'll get a different result when applying patches to dev, I'll be happy to repeat the test.

Since I couldn't build dev on Google Cloud, I patched the PR into master in my branch: marionbarker:day-to-day-test

There was no discernable difference with or without #7833. The change came from #7836 in terms of where midnight line shows up and where basal, IOB, COB plots are included or cut off. Days are still confused when not in the same time zone. Basal, IOB, COB plots are now complete for -2 hrs wrt my time zone, but date is wrong. Basal, IOB, COB plots completely missing now for +3 hr wrt my time zone (Default line all the way to the right).

To be clear: my time zone = PST, +3 hrs is EST, -2 hrs is Honolulu (don't know the code)

Patch seen here:
marionbarker/cgm-remote-monitor@master.../day-to-day-test

Graphic below - left side is using master (14.2.6) and right side is using day-to-day-test (14.2.6-day-to-day-test.7833.7836). The actual day is Thursday Jan 12, 2023. The glucose, prediction, bolus and carbs are correct for each plot - just labeled with wrong day/date if time zone doesn't match.

nightscout-time-zone-test-master-7833-7836

@sulkaharo
Copy link
Member

SO wait, am I correct in understanding the only bug is the date in on top of the graph? Looks like additionally the profile switch is rendered at the wrong position.

@paddingtonil
Copy link

Although my phone and computer are set to GMT +2, my profile shows GMT -2. However, the reports do not show a time offset.

Screen Shot 2023-02-20 at 12 04 24

@sulkaharo
Copy link
Member

@paddingtonil the profile editor shows whatever is saved as the time zone in the profile. If your profiles are saved by Loop, it means the profile sent by Loop to Nightscout says the time zone is GMT-2. The value does not automatically reflect any device, it's intended to be manually set to the time zone of the PWD who's profile this is.

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

No branches or pull requests

7 participants