search

nightscout / Trio

MIT License
45 stars 125 forks source link

Remove unused Import Settings in JSON that cause errors decoding JSON #249

Closed kskandis closed 1 month ago

kskandis commented 1 month ago

Resolves Nightscout CGM Import Settings fails with error by:

  1. Removing unused fields from the JSON struct which do not get saved or written to settings/pump. These unused fields used Types that were inconsistent with Loop JSON.
  2. Checks if JSON Profile was entered by Loop to ensure proper Default record is loaded.
bjornoleh commented 1 month ago

Thanks for your contribution. The changes look good, but I am not overly familiar with these details, and don’t have other looping data to test from.

Hoping someone with fresh profile data uploaded to NS from both Loop and AAPS will have a chance to test. Import of manual NS profile entries would also be good to test here.

Sjoerd-Bo3 commented 1 month ago

And maybe we need to make a separate PR for this?

My only suggestion to consider is maybe setting target to the middle of the target range from Loop rather than the bottom target, since Trio only uses a single target.

@kskandis are you also up for that?

bjornoleh commented 1 month ago

So this looks good according to current reviews. Could someone also quickly check that NS-generated profiles are also imported correctly?

bjornoleh commented 1 month ago

And possibly AAPS-generated profiles? @t1dude ?

bjornoleh commented 1 month ago

Could someone also quickly check that NS-generated profiles are also imported correctly?

I got to test this now, and can confirm that changes made from the NS profile editor are correctly imported. For target, only the lower bound is imported, the upper bound is ignored, as noted by others above.

For me this is LGTM. Would be interesting to know if AAPS profiles are supported too, but that’s a little out of scope for this PR.

dnzxy commented 1 month ago

I will merge this with 4 approvals and 4 testers.

@t1dude if you could please test this with AAPS-uploaded profiles after the fact; if a fix for AAPS-profiles is necessary, we will record this as a new issue ticket.

@MikePlante1 comment regarding not using just the lower bound of Loop's correction range can be handled via #252 .

Thank you everyone for testing and reviewing. Thank you @kskandis for your contribution to Trio.

kskandis commented 1 month ago

And maybe we need to make a separate PR for this?

My only suggestion to consider is maybe setting target to the middle of the target range from Loop rather than the bottom target, since Trio only uses a single target.

@kskandis are you also up for that?

That makes sense to me. Yes, I'll look into it.