Skip to content
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

Refactor override management #238

Draft
wants to merge 31 commits into
base: dev
Choose a base branch
from

Conversation

avouspierre
Copy link
Contributor

The PR includes a large refactoring of the swift part of override/profile functions :

  • Override is stored in override core data, including history
  • Override preset is stored in overridepreset core data
  • Add the display of the override in main graph
  • allows to select a override preset (or a temp target) in AW
  • allows to cancel a current override preset or specific override (idem for temp target) in AW
  • add a color profil for override profil in AW
  • Fix some minor code in AW
  • add the upload of override as a exercice in Nightscout - Fix [feature request] Display profiles/overrides in Nightscout  #145
  • improve the management of indefinate override / stop of indefinate override
  • modify the code to respect the Ivan’s patterns of the app :
  • Use of swiftInject (dependency injection) with the use of protocol class in the code
  • Use of MVP principes, in particular not use of direct coredata in view class
  • Use of a proxy model class between coredata and the app to manage changes of core data
  • Use of the pattern of observe to refresh data/view/uploads
  • add a core data unit tests allowing to add tests for coredata with a in-memory datastore for tests.
  • test for overrideStorage available

This PR do NOT change the logic with oref and the interface of override informations in oref. This PR do NOT require a update of trio-oref code.

This PR is a part of #219 (split required by admin for review).

override display override in NS
Simulator Screenshot - iPhone 15 - 2024-05-07 at 21 43 34 CleanShot 2024-05-07 at 21 44 16

avouspierre and others added 10 commits May 13, 2024 19:57
Refactoring the shortcuts with adding :
- same organisation of files
- remove junk code
- add internationalization of shortcuts
- add AddCarbs shortcuts
- add createAndApply temp target
- Add override preset selection, application and cancelation
- Add Carbs preset list and display all carbs, fat and proteins associated
- fix carb list shortcut issue
- fix carb added with fat/protein  shortcut
- add bolus shortcuts with guardrails
- harmonise temporary target labels
- add in OIAPS a shortcut config allowing to authorize bolus enact and guardrails.
- Add localisation for the UI shortcuts config view and shortcuts
- minor improvements to avoid warning in build
md/dL instead of mg/L
for instead of during

# Conflicts:
#	FreeAPS/Sources/Localizations/ShortcutsDetail/ShortcutsDetail.xcstrings
however the BolusIntent does not popup the amount dialog
• Mark bolusQuantity as non-optional
• Remove associated guard/else {...} since this is not optional
@avouspierre
Copy link
Contributor Author

This version is now updated with the dev version including the new edit of override. Could be review

@mountrcg
Copy link
Contributor

mountrcg commented Jun 1, 2024

What happens if someone enacts at temp target on top of an override with a target and an insulin change? It used to be that this temp would override the override target (pun intended) and change sens to whatever the settings dictate.

What should be the expected behaviour in your view?

  • prevent TT while override
  • Auto cancel override when doing TT
  • Only replace override settings that the TT changes
  • Disregatd any sens change a tt would do (which could be done changing the overrides target now)

  • ??

@avouspierre
Copy link
Contributor Author

Very good question @mountrcg because there's nothing to stop you running both at the same time. The logic of override is different (change ISF before sending to oref) to temp target (change the BG Target). So the conflict is probably (not sure) if you change the BG Target in override...I don't go in the oref code to analyse the result.

Perhaps, we could manage this case in a new PR after merging this ?

@dsnallfot
Copy link
Contributor

dsnallfot commented Jun 1, 2024

@avouspierre I just noticed a (small) thing that I messed up in my previous PR 235. I always use dark mode in the app, and didn't look at this particular thing in light mode. The edit sheet that I thought looked good in dark mode doesn't look as good in light mode due to the .padding() when opening the sheet in OverrideProfilesRootView.

I don't want to make a new PR with the fix for this until after this PR is merged, but it would be great if you could change this (just delete the .padding() line) in this open PR ?

            .sheet(isPresented: $isEditSheetPresented) {
                editPresetPopover
                    .padding()
Dark mode edit sheet ok Light mode edit sheet ugly borders
IMG_3896 IMG_3897

And @mountrcg, the same light mode ugliness as above is also present in the edit sheet for temp targets in AddTempTargetRootView after PR 236 and needs the same delete .padding()

(On a related note: I'm looking into your PR 241 but didn't immediately see any quick fixes to the outstanding questions - will get back to yours as soon as I have some ideas)

Copy link
Contributor

@polscm32 polscm32 left a comment

Choose a reason for hiding this comment

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

Before this pull request can be merged, some changes need to be made in my opinion:

  1. The current Core Data stack needs to be expanded to include methods like persistent history tracking, to merge changes from background contexts into the view context.
  2. For this, background contexts need to be introduced. For example, the save function in the stack currently uses the view context. Everything in the OverrideStorage does so as well. However, we should try to keep the main thread as free as possible. The UI is already buggy in the app and the existing Core Data Setup is pretty bad. We should first improve the basic setup before adding more features in my opinion.
  3. Error handling: We should avoid using try?. If it fails, it is currently not handled at all. We should definitely handle it. Also, we should not use fatalError in a shipping application, as it causes the app to crash if a Core Data save operation fails.
  4. It would be better to create an entity for the overrides with a flag (e.g., "isPreset") to distinguish between presets and overrides.
  5. The Core Data code should be thread-safe, meaning the app should build with the Core Data concurrency debug flag enabled. Otherwise, it can lead to incorrect access to the managed object context and the potential for data races.
  6. We should be more cautious with performAndWait, as it always blocks the calling thread and can therefore cause performance issues.

Copy link
Contributor

@dnzxy dnzxy left a comment

Choose a reason for hiding this comment

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

First of all, thank you for tackling this important feature. Your introduction of the NS upload is crucial for caregivers and family/partners of Trio users, and it is appreciated.

I conducted a code review and performed a quick test in a simulator. While the initial implementation looks promising, there have been reports of some glitches and issues on Discord. Unfortunately, I couldn't test the upload part as my second NS instance is currently down.

That being said, we need to have a broader discussion about how significant backend rewrites, such as this, should be approached, given the current state of our codebase. As you know, Trio (and iAPS) are quite leaky, non-thread-safe, and performance-hungry in their current form. I concur with @polscm32 that before diving into a rewrite or enhancement that heavily relies on CoreData, we need to address several foundational issues.

I'm not sure how we can best tackle this in a timely manner before the release of Trio 1.0.0. My instinct is that we should aim to change as little as possible around Overrides for now and ensure they are uploaded to Nightscout using the Exercise data type. Larger rewrites can be addressed once we've made more of the backend improvements, particularly making it thread-safe.

Thank you again for your efforts on this. Your work is valuable, and I don't want to make it seem like I am diminishing your effort or rejecting your contributions. I'm certain, with some additional adjustments, it will significantly enhance our project. Let's continue the discussion to find the best path forward.

Feel free to add your own thoughts and suggestions.

FreeAPS/Sources/APS/APSManager.swift Outdated Show resolved Hide resolved
FreeAPS/Sources/APS/OpenAPS/OpenAPS.swift Outdated Show resolved Hide resolved
FreeAPS/Sources/APS/OpenAPS/OpenAPS.swift Outdated Show resolved Hide resolved
FreeAPS/Sources/APS/OpenAPS/OpenAPS.swift Outdated Show resolved Hide resolved
FreeAPS/Sources/Models/OverrideProfil.swift Outdated Show resolved Hide resolved
FreeAPS/Sources/Models/OverrideProfil.swift Outdated Show resolved Hide resolved
FreeAPS/Sources/Modules/Home/View/HomeRootView.swift Outdated Show resolved Hide resolved
Trio.xcworkspace/xcshareddata/swiftpm/Package.resolved Outdated Show resolved Hide resolved
@avouspierre
Copy link
Contributor Author

avouspierre commented Jun 1, 2024

@polscm32 @dnzxy I analysed quickly your implementation of CoreData and it is a amazing job... more important, I think, than the current PR. So I tried to answer to your proposal :

  1. The current Core Data stack needs to be expanded to include methods like persistent history tracking, to merge changes from background contexts into the view context.

The core Data is used in different part of the code of Trio and this proposal seems to be a specific PR to create the structure to manage the core data. One subject is the purge of old data but could be pushed in a specific PR.

  1. For this, background contexts need to be introduced. For example, the save function in the stack currently uses the view context. Everything in the OverrideStorage does so as well. However, we should try to keep the main thread as free as possible. The UI is already buggy in the app and the existing Core Data Setup is pretty bad. We should first improve the basic setup before adding more features in my opinion.

My objective is only to have a correct behavior of override and a maintenable code. That's why there is now only one function to save overrides in core Data... A new PR seems adequate to modify all save functions in Trio as background.

  1. Error handling: We should avoid using try?. If it fails, it is currently not handled at all. We should definitely handle it. Also, we should not use fatalError in a shipping application, as it causes the app to crash if a Core Data save operation fails.

Understood. I modify the code with a do/catch

  1. It would be better to create an entity for the overrides with a flag (e.g., "isPreset") to distinguish between presets and overrides.

I could but I'm not sure the interest of the attribute. The override table/entity stores all historical overrides without any linked with the preset entity. Probably the more interesting thing to do will be to store the id of the preset used for the override but should be in a more global remastering data in core data.

  1. The Core Data code should be thread-safe, meaning the app should build with the Core Data concurrency debug flag enabled. Otherwise, it can lead to incorrect access to the managed object context and the potential for data races.

Here too, a new PR because the flag changes all core data access in the code of Trio.

  1. We should be more cautious with performAndWait, as it always blocks the calling thread and can therefore cause performance issues.

OK but the interest is the fact thet performandwait is safe-thread and without to manage publishers strategy. 😌.

Thank you again for your efforts on this. Your work is valuable, and I don't want to make it seem like I am diminishing your effort or rejecting your contributions. I'm certain, with some additional adjustments, it will significantly enhance our project. Let's continue the discussion to find the best path forward.

I don't have any problem if you think the PR is not appropriate. I learn lot of swift techniques with your proposals 🙏. But I push two arguments 😊😊to review this PR : First, you asked me to split in more simple PR and the previous propositions are largely more than the objective of the PR. Second this PR cleaned the code of override in regard of the maintenance - one class manages the core data interface ! - and it brings some improvements (NS uploads and display the override in the graph in particular).

A last thing (but not a argument) : I used this PR in IRL for 2 months without issue.

I will integrate your different updates in the next commit !

@aug0211
Copy link
Contributor

aug0211 commented Jun 1, 2024

I think scope control is our biggest challenge here. If (and only if!) getting this merged is going to take more than 1 week, maybe we pair this into a couple PRs, focused on:

  1. Nightscout and main graph visualizations (blocking issues: safety consideration)
  2. Deletion and handling of indefinite overrides (I'm unclear whether this is a safety consideration or optimization)
  3. Watch capabilities (feature)
  4. Refactor for Core data storage (optimization)

I list in this prioritized order based on what I think is most urgent, with item 1 being high criticality and items 3 and 4 being things we absolutely should do, but I would not personally recommend blocking a release for. Item 2 I am not 100% clear on.

Again, I would only break this out if we think this PR is large enough that it'll take a week or more to work through and merge - so that we can get the top priority items merged in 1-2 days each (in serial, not parallel), hopefully.

@dnzxy
Copy link
Contributor

dnzxy commented Jun 1, 2024

I agree that the smaller the PR scope, the easier to work on it as a dev and the easier to test it for testers. I also think the NS upload of overrides (as they are) is the top priority, the other features are reaching further.

@avouspierre
Copy link
Contributor Author

I updated with some proposals from @dnzxy and @polscm32 (minor improvements)

Thanks to @aug0211 to take the action to split in different PRs because it's a little bit frustrated (for me) to do this in regard of the work done (but I understand the logic and the difficulty to review by others).

@aug0211
Copy link
Contributor

aug0211 commented Jun 2, 2024

Sent ya a DM on Discord. It's out of reach for me to be a lead dev on any of the components (I've tried on my own the past week with no success 😂) but am motivated to help here however I can.

@bjornoleh
Copy link
Contributor

The complexity of this PR makes a review quite far beyond my skillset, so I will have to give a pass on actual code review here. I could build and check that it works, but I am sure that it does so already, so there is limited value in that.

From what I read, a bigger rewrite of core data is planned after v1.0, or already in the works. Would it be possible to have working overrides and upload to NS with little or no changes to core data? The original idea for v1.0 was to make minimal changes, then do more involved changes later. Hopefully that would still be possible :-) I can't really participate much in the detailed technical discussions about this, I just wanted to ask this as a more general question (as others already have asked, sorry for repeating it :-) )

@Sjoerd-Bo3 Sjoerd-Bo3 marked this pull request as draft June 8, 2024 23:04
@Sjoerd-Bo3 Sjoerd-Bo3 added enhancement New feature or request Pre 1.0 labels Jun 8, 2024
@Sjoerd-Bo3 Sjoerd-Bo3 added this to the Trio 1.0 release milestone Jun 8, 2024
Sjoerd-Bo3 and others added 19 commits June 13, 2024 22:28
Signed-off-by: Sjoerd-Bo3 <sjoerd.bozon@gmail.com>
Signed-off-by: Sjoerd-Bo3 <sjoerd.bozon@gmail.com>
Signed-off-by: Sjoerd-Bo3 <sjoerd.bozon@gmail.com>
Signed-off-by: Sjoerd-Bo3 <sjoerd.bozon@gmail.com>
Signed-off-by: Sjoerd-Bo3 <sjoerd.bozon@gmail.com>
Signed-off-by: Sjoerd-Bo3 <sjoerd.bozon@gmail.com>
Signed-off-by: Sjoerd-Bo3 <sjoerd.bozon@gmail.com>
Signed-off-by: Sjoerd-Bo3 <sjoerd.bozon@gmail.com>
Signed-off-by: Sjoerd-Bo3 <sjoerd.bozon@gmail.com>
updates all localizations shortcuts
fix issue for the shortcut create a temp target” where the unit should be stored only in mgdL
@dnzxy dnzxy added Post 1.0 and removed Pre 1.0 labels Jul 4, 2024
@aug0211
Copy link
Contributor

aug0211 commented Jul 7, 2024

I made some sort of mistake here. I'm sorry @avouspierre, this is in your fork and on your branch.

I was working on creating a branch in my own repo, off of @avouspierre's dev+overrides and dev+shortcuts branches, to do some work with getting overrides working in Nightscout.

I created a new branch "overrides" in my repo, intending to pull in first Pierre's shortcuts branch and then his overrides branch over top of this. Somehow I messed this up, and made a mess of Pierre's dev+overrides branch in his fork (again, I'm sorry Pierre!).

It almost seems as if I merged things into Pierre's branch by accident instead of my fresh branch in my repo, but I suspect something tricker like Github upstreams/remotes pointing going to the wrong places on my end (?), and then creating this spiderweb.

I did not expect (or intend) to see the following commits in Pierre's branch:
0994ae3 - was able to revert this one, but then stopped because I don't understand this situation and do not want to make it worse
1b41386 - unable to revert this one, and it seems the weirdest to me of the lot
848b5dc
0e7e8d4

image

mountrcg pushed a commit to mountrcg/Trio that referenced this pull request Aug 1, 2024
Import Basal settings, carb ratios, sensitivities and glucose targets from Nightscout manually when tapping "Import Settings from Nightcsout"in the iAPS Nightscout settings. 

co-author @dnzxy
@flyingpie101
Copy link

@avouspierre Would you be able to do a PR that just updates NS with Override info as a NS Exercise ?

@Sjoerd-Bo3
Copy link
Contributor

@avouspierre Would you be able to do a PR that just updates NS with Override info as a NS Exercise ?

Is it not wise to wait for the core data rework and revisit this PR as a whole again? Then we have a good structure and we can get this setup the right way? Would hate to see @avouspierre his work get halted again by something like this.

@flyingpie101
Copy link

Honestly, I have no idea if it's wise or not ;)
As a caregiver, being able to see if my daughter has put on an Override, or forgot to turn one off, has been super helpful.

And, so after reading this Issue log, it seemed displaying an Override in NS using the an NS Exercise function, which seemed important to a few of us ;), looked like it got tied up with a core date re-work, when I am not sure it should have.

Since it has been a little more than a month, and rc1 is happening, I thought I would refresh the topic.

@Sjoerd-Bo3
Copy link
Contributor

I feel you, but this PR has core data tangled into it. The PR (and shortcuts) also have core data rewrites in them. Making it a mess in combination with the whole core data rework. When 0.2 is released and core data is in test swing, it would be good to stream this in its wake. But if @avouspierre wants to fix this one. Ofc take a swing! But I’m just worrisome about lost efforts and this getting a dragging topic.. Just cautious for that, understand the need a lot!

@flyingpie101
Copy link

Absolutely. I have no idea of the effort. I wasn't sure who did the version in iAPS, so I kinda thought one of the experts here, might be able to leverage that fairly quickly.

@Sjoerd-Bo3 Sjoerd-Bo3 removed this from the Trio 0.2.0 release milestone Aug 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Post 1.0
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

[feature request] Display profiles/overrides in Nightscout
10 participants