Remote PR Set #2: Codable Models for Remote Notifications #768
Conversation
gestrich
force-pushed
the
feature/2023/02/bg/remote-command-service
branch
from
0940fb3
to
5842719
Compare
| @@ -0,0 +1,30 @@ | |||
| // | |||
There was a problem hiding this comment.
These are the Codable models used for Nightscout. I'm including these with the NightscoutUploadKit which I feel is the "client" of Nightscout APIs. It could be argued this is more part of the "NightscoutService" domain as these are only exposed in the context of the plugin. Nevertheless there will be client things for Remote 2.0 later that will go in here so I'm starting here to avoid fragmenting things unnecessarily.
There was a problem hiding this comment.
If they are things that any iOS app wanting to talk to Nightscout might use (upload, download), then it belongs in NightscoutKit. If it's specific to Loop, then it goes in NightscoutService.
There was a problem hiding this comment.
It sounds like all these Codable models should move to the NightscoutService as these are all Loop specific.
Do you see the role of NightscoutService as also serving other apps like Caregiver, associated with Loop, that needs to make Loop-specific Nightscout api calls?
If so, we would need to add something like a NightscoutLoopAPI to NightscoutService that handles those network things, similarly to NightscoutKit. I ask as the the notifications endpoint in NightscoutKit is currently doing some Loop domain things so probably doesn't belong there.
There was a problem hiding this comment.
Do you see the role of NightscoutService as also serving other apps like Caregiver, associated with Loop, that needs to make Loop-specific Nightscout api calls?
Hmm, I don't think so. NightscoutService is a Loop Plugin. It could vend onboarding screens, settings for controlling upload, all that kind of stuff.
I think CareGiver should probably have its own CareGiver <-> NightscoutKit conversion layer, or maybe CareGiver would interface with NightscoutKit directly.
The question is really about the commands themselves right? CareGiver creates commands, NightScout helps transfer them, and Loop consumes them. And I think there is a sense that some of the work of dealing with those commands or modeling them should be shared, right?
One of the questions I have is, does Nightscout have awareness of the contents of these commands, or are we assigning Nightscout the role of a dumb pipe? I think I'd argue against it being a dumb pipe, and would say that AID remote commands is a domain that Nightscout is privy to. Nightscout already does some remote command issuance, and I could see that being developed further. So I think that means that the Nightscout model includes the commands, and that any client (with proper authorizations and such) should be able query/create commands destined for the Looper's system. And possibly Non-Loop AID systems might want to use Nightscout and a even a different caregiver app in a similar way.
So I'm suggesting we treat these models as primarily Nightscout models, and part of the Nightscout API. Loop can consume Nightscout "Remote Commands", either via API, or by Nightscout pushing to Loop. And then they belong in NightscoutKit, which is the Swift front end to the Nightscout API.
There was a problem hiding this comment.
Nightscout is being used as a "dumb pipe" as you mentioned. I had considered a more domain-aware API as you mentioned but didn't want to get ahead of myself. I can work towards moving that direction when we get there.
On that matter, the NS reviewer sulkaharo had suggested on my NS Draft PR that this remote commands feature be coordinated with the wider community to potentially make it cross-compatible with other implementations (Like AndroidAPS). Your comments though also suggest a move in that direction.
| @@ -0,0 +1,59 @@ | |||
| // | |||
There was a problem hiding this comment.
The tests in the next few files just take some data samples and ensures I can still create notifications from it.
gestrich
changed the title
gestrich
changed the title
| public let otp: String | ||
|
|
||
| enum CodingKeys: String, CodingKey { | ||
| case remoteAddress = "remote-address" |
There was a problem hiding this comment.
Is this encoding (remote-address) the Nightscout api encoding? The json in the nightscout api typically uses camel case (remoteAddress).
There was a problem hiding this comment.
Yeah unfortunately the dash is used in the key. Here's the existing server and loop code that handles that
There was a problem hiding this comment.
I had Taylor Swift's Anti-Hero song pop into my head when looking at the git blame for this. :P
|
Closing this as these models have been moved to ps2/NightscoutService#33. All the models are Loop app specific so don't belong in the riley or NightscoutKit repos as suggested. |
This PR is part of the below set that should be merged together. This one will use "RemoteCommands", a protocol for interacting with the remote services that is the source of the RemoteCommand. This will lay the groundwork for remote 2.0 commands.