Skip to content

Conversation

@gestrich
Copy link
Contributor

@gestrich gestrich commented Sep 4, 2021

See the associated workspace PR for a high level description. This will update carthage to point to the plugin. Assuming we will host the plugin in our workspace, this will need updated to the LoopKit organization before merge.

@gestrich gestrich marked this pull request as ready for review September 4, 2021 09:35
@4gra
Copy link

4gra commented Sep 4, 2021

Using this plugin on 2.2.5 with the pump simulator and it works well (subject to the usual limitations of Loop without any BLE wakeups).

I'd make one small comment about the settings, which is that it's not clear how it interacts with the existing Nightscout functionality, and the settings text that accompanies "Upload to remote service" doesn't really make it clearer. In fact that text doesn't really add much to the description and it might be useful to explain the implications (either way!), for example that it will duplicate records if "you" are following Loop on another phone, or already uploading BG via a third-party app; or if you're using the existing Nightscout uploader plugin.

@gestrich
Copy link
Contributor Author

@4gra Thanks for pointing this out. Maybe we should not have the "Upload to remote service" option at all (referencing screenshots in (workspace PR)? It seems odd that someone would want to pull in Nightscout CGM data to Loop then upload that data to another Service in Loop. Maybe there is a use case I'm not thinking of though.

cc @ps2 in case any thoughts on the upload option.

@gestrich
Copy link
Contributor Author

gestrich commented Sep 11, 2021

And assuming we remove that remote button, here is some descriptive text in the Settings view that maybe we can add explaining the feature. Let me know if it can be improved:

"The Nightscout CGM source will download CGM data from Nightscout. This is useful if you can't add your CGM device to Loop but your CGM data is available on Nightscout.

If you can instead add your actual CGM device to Loop, you may consider adding Nightscout as a Service in Settings, which will upload your CGM data to Nightscout."

@ps2
Copy link
Collaborator

ps2 commented Sep 13, 2021

@gestrich I think the descriptive text will be very useful to explain to users what the plugin is for. The recommendation is good too.

As for the "Upload to remote service" button, I think it would be nice to provide a way for users to prevent (re)uploading of CGM readings that originated in NS. I haven't looked at the code, but I think if the user had this service added, and also added the normal Nightscout service, that their CGM data would be re-uploaded, and that the button would not change that.

@4gra
Copy link

4gra commented Sep 13, 2021

I suppose one might want to chain Nightscout instances: read from one, write to another for some sort of comparison.

Outwith testing of services though I'm not sure I can imagine a use case; regardless, the more explicit the data flows, the better so the improved explanation is welcome.

@4gra
Copy link

4gra commented Sep 26, 2021

I've just noticed a slight limitation of this plugin: it doesn't (seem to) work with token-based authentication. Neither will the extant Nightscout uploader of course; but it has a field for the API_SECRET, so while the uploader plugin will work with a (recommended) default-deny setup, this plugin will not. I'll fork and attempt an enhancement in an idle moment.

@gestrich
Copy link
Contributor Author

gestrich commented Sep 26, 2021

@4gra Good catch.

Generally I've been wondering if maybe all this functionality belongs in the NightscoutService. I believe I saw some logic that allows services to offer their own pump and cgm source. Curious if @ps2 has any thoughts on including it in the service. We could just move the parts over that fetch the glucose data. The benefit seems to be reuse of the NightscoutService credentialing and possibly less confusing to users with an integrated UI.

@francescaneri
Copy link

@gestrich Are we ready to merge or do we need to make some other changes?

@gestrich gestrich force-pushed the nightscout-api-plugin-dev branch from 25bff09 to 1576560 Compare February 20, 2022 11:17
}
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sort CGM device list by name. They seemed to be in random order from what I could tell.

github "LoopKit/LogglyService" "dev"
github "LoopKit/MKRingProgressView" "appex-safe"
github "dexman/Minizip" "master"
github "instacart/TrueTime.swift"
Copy link
Contributor Author

@gestrich gestrich Feb 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pete - These Cartfile values in this PR and my other PRs probably will need your attention before merge. I'm never quite sure how to manage the files during the development cycle as they become quite cumbersome to keep up-to-date with the current versions (i.e. Each time I update a submodule, these are out-of-date). And it gets really tricky when you think that a bunch of submodules rely on LoopKit, all which need to be in sync with the same resolved version.

Do we even rely on these Cartfile versions or maybe these should not be updated until just before you merge?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And maybe I see my answer here. Looks like NightscoutService is way out of day. So I suppose we are not updating these.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct. Carthage is being dropped in favor of Workspace builds.

@gestrich gestrich force-pushed the nightscout-api-plugin-dev branch from 1576560 to fc9721a Compare February 20, 2022 12:04
@gestrich gestrich force-pushed the nightscout-api-plugin-dev branch from fc9721a to d7ee4ce Compare February 27, 2022 11:28
@ps2 ps2 merged commit f98e742 into LoopKit:dev Mar 1, 2022
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

Successfully merging this pull request may close these issues.

5 participants