-
Notifications
You must be signed in to change notification settings - Fork 138
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
import change from #30 #36
Conversation
@sirKitKat volunteers these changes in issue #30.
Can this be integrated into a release so it will make it into the next docker build? I'm not getting carelink data at all I think because of this issue (it's getting the NaN error guessing the offset) |
Howdy @mattster98, thanks for the input! Which region are you located in? Are you saying @sirKitKat's fixes worked for you, or reporting that it does not currently work at all? I've been working on testing these changes for integration and release ASAP. |
No, I'm still trying to get a docker image built so I can try it out. I deployed the latest released build to my own kubernetes cluster and have not rolled my own before, so it's a bit of a struggle. |
OK - quite the journey to here, but I got my updated docker built that points to my custom npm for minimed-connect-to-nightscout that has this change in it, aaaaand it is still failing. It DOES now show the correctly guessed offset (I'm in US/Eastern and it says -0500). However I still get the RangeError: Invalid time value
|
I tried deploying a dev version so I could inspect as it was running to try and debug this, but that's beyond what's left of my capabilities today.. I'll keep trying tomorrow. I have the following env variables.. that may mean I'm not getting all of the fix based on my interpretation of the code. MMCONNECT_SERVER: US |
OK - I managed to get a local server running on windows so I could more easily debug via probably the hackiest way possible (more logging to console) but the only way I could seem to get the runtime values that were triggering the error. My sMedicalDeviceTime is 2023-01-06T12:56:01.582-00:00 That seems to be the problem since the non-EU fix in parsePumpTime looks to concatenate a timezone offset before date.parse instead of offsetting the resulting unix timestamp. I already have the offset, so it's generating an invalid input to date.parse. It might make more sense to just try the first method, and if that fails, try the second. By forcing the EU method on my server, I begin to successfully download from carelink. |
@@ -38,9 +38,9 @@ var CARELINK_TREND_TO_NIGHTSCOUT_TREND = { | |||
} | |||
}; | |||
|
|||
function parsePumpTime(pumpTimeString, offset, medicalDeviceFamily) { | |||
function parsePumpTime(pumpTimeString, offset, offsetMilliseconds, medicalDeviceFamily) { | |||
if (process.env['MMCONNECT_SERVER'] === 'EU' || medicalDeviceFamily === 'GUARDIAN') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The offset issues were affecting me (with MCONNECT_SERVER: US) so I added "or US" to this to get the same fix. I'm troubleshooting intermittent other issues but I'm guessing those are related to deploying on kubernetes, since it seems fine running in dev.
A more elegant solution (assuming appending the tz offset is still needed in some situations) might be to see if the first method works, and if so, go with it. If it doesn't, use the second method. I'm guessing medtronic changed how they're sending the device time out, but not sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think US and EU are doing the same thing, and the separation is a historical artifact we can ignore.
I've been meaning to more carefully to emulate what's happening in XDripCarelinkFollower: https://github.com/benceszasz/xDripCareLinkFollower/tree/master/app/src/main/java/com/eveningoutpost/dexdrip/cgm/carelinkfollow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also note, Sulka disabled some additional datetime munging just recently here: nightscout/cgm-remote-monitor@c1de8a5
@sirKitKat volunteers these changes in issue #30.