-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Loop status upload #110
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
Loop status upload #110
Conversation
Loop/Managers/LoopDataManager.swift
Outdated
| failureReason = nil | ||
| } | ||
|
|
||
| let loopName = NSBundle.mainBundle().bundleDisplayName |
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.
Can we move lines like this out of the LoopDataManager and into the receiver's method?
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 ideally all the NightscoutUploadKit models would be created somewhere else, like DiagnosticLogger, or an extension of NightscoutUploader?
|
🎉👍👏😂😍 So glad you did this, @ps2. This will make integrating with Urchin so much easier - I've been running a much simpler/hackier version of these changes to show the prediction graph and eventual BG: |
Some suggested changes based on my comments
|
|
||
| if insulinOnBoard == nil { | ||
| dispatch_group_enter(updateGroup) | ||
| deviceDataManager.doseStore.insulinOnBoardAtDate(NSDate()) { (value, error) in |
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.
Is this the right timestamp to use?
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.
from a dosing perspective, yeah, you'd want it to be as close-as-possible to startDate on line 331, which it will be here, since these data retrievals should be pretty fast. If it needs to be exact, you could set startDate on line 126 and pass it in as an argument to self.updatePredictedGlucoseAndRecommendedBasal
|
|
||
| let loopStatus = LoopStatus(name: loopName, version: loopVersion, timestamp: statusTime, iob: iob, suggested: loopSuggested, enacted: loopEnacted, failureReason: lastLoopError) | ||
|
|
||
| deviceDataManager.remoteDataManager.uploadDeviceStatus(nil, loopStatus: loopStatus) |
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.
Maybe this implementation can move into here, now, to keep all the NS models together?
Also, would be good to punt at the top of this method if there's no nightscout uploader
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.
Sure, sounds good.
…ting suggested attributes when last basal is successful
|
@mddub I've been using urchin, and love it. Forecasted bg would be amazing! I'm just about done with this, I think. |
|
Was trying to get COB and IOB NS attributes populated to wrap this up, but it looks like they might require more work, so I'll tackle that as a separate PR. I'll cut a RL release, and then point this to it, and the PR should be ready for review again. |
Loop/Managers/LoopDataManager.swift
Outdated
| - parameter resultsHandler: A closure called once the values have been retrieved. The closure takes the following arguments: | ||
| - predictedGlucose: The calculated timeline of predicted glucose values | ||
| - insulinEffect: The predicted effect of insulin | ||
| - carbEffect: The predicted effect of carbohydrates |
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.
still need to add these?
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'm going to soon; I just need to figure out how to turn them into BG lines. carbEffect -> predicted BG if only carbs were considered, and insulinEffect -> predicted BG if only insulin were considered.
But I can remove them for this PR, if you'd like.
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.
Not sure I follow with the "only [x] were considered"?
I'm good with merging this PR if you are, which would mean reconciling this docstring and reverting the import line at the top. It sounds like there's still some munging of the carb/insulin effect lines for Nightscout, though ideally you can sort that processing out in the cloud. Just let me know what you'd like to do.
| let predBGs: PredictedBG? | ||
|
|
||
| if let predicted = predictedGlucose { | ||
| let values = predicted.map({ (value) -> HKQuantity in |
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.
You can condense this to predicted.map { $0.quantity } if you like.
Loop/Managers/LoopDataManager.swift
Outdated
|
|
||
| - parameter resultsHandler: A closure called once the values have been retrieved. The closure takes the following arguments: | ||
| - predictedGlucose: The calculated timeline of predicted glucose values | ||
| - insulinOnBoard Current insulin on board |
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 order here doesn't match the arguments below (which is OK for Swift 2, but not for Swift 3)
|
LGTM – cut a final build of RileyLink frameworks? |
|
Huzzah! Thanks for all the work you put into this. |
- https://tidepool.atlassian.net/browse/LOOP-1361 - Add wasUserEntered property to various glucose data structures
Uploads Loop status to Nightscout, with BG forecasts.