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

Libre Conversion Notification Bug #40

Open
kylmcw opened this issue Mar 31, 2024 · 21 comments
Open

Libre Conversion Notification Bug #40

kylmcw opened this issue Mar 31, 2024 · 21 comments
Assignees
Labels
bug Something isn't working

Comments

@kylmcw
Copy link
Contributor

kylmcw commented Mar 31, 2024

image

Dev branch with live activities and loop_dev_submodules

Notification shows mg/dL when mmol/L selected as glucose value.

Transmitter also shows N/A.

@kylmcw kylmcw added the bug Something isn't working label Mar 31, 2024
@bjornoleh
Copy link
Contributor

Is this exclusively related to Libre CGM?

@kylmcw
Copy link
Contributor Author

kylmcw commented Apr 16, 2024

I believe so - here

@bjornoleh
Copy link
Contributor

Is this still an issue for Libre CGM?

@avouspierre
Copy link
Contributor

Like #167 the notification is published by Libre submodules "itself" and the update should be done by the module...

@JeremyStorring JeremyStorring added this to the Trio 1.0 release milestone May 18, 2024
@Sjoerd-Bo3 Sjoerd-Bo3 self-assigned this Jun 28, 2024
@pepijnjoost
Copy link

I have noticed the same, but as soon as I disconnected Trio with FSL2 through bluetooth and reconnected it through LibreLink the notifications were in mmol instead of mg/dl.

@drrree
Copy link

drrree commented Jul 25, 2024

I'm receiving notifications about my blood sugar in mg/dL when I have it set to mmol/L. I even tried to turn off the notification but still get it.

@dnzxy
Copy link
Contributor

dnzxy commented Jul 26, 2024

This is an issue with the Libre module and cannot be solved by the Trio team as of now. Apologies.

@bjornoleh
Copy link
Contributor

Tagging @dabear, perhaps something to consider fixing in https://github.com/LoopKit/LibreTransmitter?

@dabear
Copy link

dabear commented Oct 9, 2024

LibreTransmitterManagerV3 defaults to the mgdl unit, but will change the glucose value formatter according to events received following the Loopkit DisplayGlucoseUnitObserver protocol unitDidChange method. This is how Loop does that:

private var displayGlucoseUnitObservers = WeakSynchronizedSet<DisplayGlucoseUnitObserver>()
//(...)
//updates all observers when there are actual changes to 
func notifyObserversOfDisplayGlucoseUnitChange(to displayGlucoseUnit: HKUnit) {
        self.displayGlucoseUnitObservers.forEach {
            $0.unitDidChange(to: displayGlucoseUnit)
        }
    }

//updates all observers (cgmManagers) immediately when they are created
func addDisplayGlucoseUnitObserver(_ observer: DisplayGlucoseUnitObserver) {
        let queue = DispatchQueue.main
        displayGlucoseUnitObservers.insert(observer, queue: queue)
        queue.async {
            observer.unitDidChange(to: self.displayGlucosePreference.unit)
        }
 }
 
 //(...)
 private extension DeviceDataManager {
    func setupCGM() {
        //(...)

        if let cgmManagerUI = cgmManager as? CGMManagerUI {
            addDisplayGlucoseUnitObserver(cgmManagerUI)
        }
        
final class DeviceDataManager {
 init() {   
 NotificationCenter.default.addObserver(forName: .HealthStorePreferredGlucoseUnitDidChange, object: healthStore, queue: nil) { [weak self] _ in
            guard let self else {
                return
            }

            Task { @MainActor in
                if let unit = await self.healthStore.cachedPreferredUnits(for: .bloodGlucose) {
                    self.displayGlucosePreference.unitDidChange(to: unit)
                    self.notifyObserversOfDisplayGlucoseUnitChange(to: unit)
                }
            }
        }

Additionally, I did not find any calls to unitDidChange inside of Trio.
So this seems like Trio's fault really, not updating the cgmManagers when there is glucose unit changes

@dabear
Copy link

dabear commented Oct 9, 2024

Trios code:

extension CGM {
    struct CGMSettingsView: UIViewControllerRepresentable {
        let cgmManager: CGMManagerUI?
        let bluetoothManager: BluetoothStateManager
        let unit: GlucoseUnits
        weak var completionDelegate: CompletionDelegate?

        func makeUIViewController(context _: UIViewControllerRepresentableContext<CGMSettingsView>) -> UIViewController {
            let displayGlucosePreference: DisplayGlucosePreference
            switch unit {
            case .mgdL:
                displayGlucosePreference = DisplayGlucosePreference(displayGlucoseUnit: .milligramsPerDeciliter)
            case .mmolL:
                displayGlucosePreference = DisplayGlucosePreference(displayGlucoseUnit: .millimolesPerLiter)
            }

            guard let cgmManager = cgmManager else { return UIViewController() }

            var vc = cgmManager.settingsViewController(
                bluetoothProvider: bluetoothManager,
                displayGlucosePreference: displayGlucosePreference,
                colorPalette: .default,
                allowDebugFeatures: false
            )
            // vc.cgmManagerOnboardingDelegate =
            // vc.completionDelegate = self
            vc.completionDelegate = completionDelegate

            return vc
        }

It explicitly calls the CGMManager's settings view with displayglucose units derived from Trio's current settings. So that's why it works in the ui

@dnzxy dnzxy changed the title Conversion Notification Bug Libre Conversion Notification Bug Oct 9, 2024
@dnzxy
Copy link
Contributor

dnzxy commented Oct 9, 2024

@dabear We need to ensure that all values that are propagated to Trio from LT are mg/dL and are only parsed for display.

  1. Does invoking the unit observer like suggested above change to way values are sent to Trio?
  2. Would be open to looking into this and PRing a fix? I'll re-open the issue and assign you to it, if so 😊

@dabear
Copy link

dabear commented Oct 9, 2024

Recommendation: On these two events

  • CGMManager is created / updated
  • Glucose Preferences is changed in Trio

then retrieve the CGMManager as CGMManagerUI , GlucoseUnits from trio settings and apply

theCgmManager.unitDidChange(to: unit == .mgdL ? HKUnit.milligramsPerDeciliter ? .millimolesPerLiter)

@dabear
Copy link

dabear commented Oct 10, 2024

You can re-open the ticket and assign it to me. I already fixed it yesterday locally.
During startup:
image

In the preferences:
image
image

@dabear
Copy link

dabear commented Oct 10, 2024

what's the target I should make a PR against? Trio Dev branch?
Patch:

diff --git a/FreeAPS/Sources/APS/FetchGlucoseManager.swift b/FreeAPS/Sources/APS/FetchGlucoseManager.swift
index 333b4ef7..7b6f44c7 100644
--- a/FreeAPS/Sources/APS/FetchGlucoseManager.swift
+++ b/FreeAPS/Sources/APS/FetchGlucoseManager.swift
@@ -1,5 +1,6 @@
 import Combine
 import Foundation
+import HealthKit
 import LoopKit
 import LoopKitUI
 import SwiftDate
@@ -98,6 +99,14 @@ final class BaseFetchGlucoseManager: FetchGlucoseManager, Injectable {
         settingsManager.settings.uploadGlucose = cgmM.shouldSyncToRemoteService
     }
 
+    private func updateManagerUnits(_ manager: CGMManagerUI?) {
+        let units = settingsManager.settings.units
+        let managerName = cgmManager.map { "\(type(of: $0))" } ?? "nil"
+        let loopkitUnits: HKUnit = units == .mgdL ? .milligramsPerDeciliter : .millimolesPerLiter
+        print("manager: \(managerName) is changing units to: \(loopkitUnits.description) ")
+        manager?.unitDidChange(to: loopkitUnits)
+    }
+
     func updateGlucoseSource(cgmGlucoseSourceType: CGMType, cgmGlucosePluginId: String, newManager: CGMManagerUI?) {
         // if changed, remove all calibrations
         if self.cgmGlucoseSourceType != cgmGlucoseSourceType || self.cgmGlucosePluginId != cgmGlucosePluginId {
@@ -120,6 +129,8 @@ final class BaseFetchGlucoseManager: FetchGlucoseManager, Injectable {
             removeCalibrations()
         } else if self.cgmGlucoseSourceType == .plugin, cgmManager == nil, let rawCGMManager = rawCGMManager {
             cgmManager = cgmManagerFromRawValue(rawCGMManager)
+            updateManagerUnits(cgmManager)
+
         } else {
             saveConfigManager()
         }
@@ -151,7 +162,6 @@ final class BaseFetchGlucoseManager: FetchGlucoseManager, Injectable {
         else {
             return nil
         }
-
         return Manager.init(rawState: rawState)
     }
 
diff --git a/FreeAPS/Sources/Modules/PreferencesEditor/PreferencesEditorDataFlow.swift b/FreeAPS/Sources/Modules/PreferencesEditor/PreferencesEditorDataFlow.swift
index 1bbd54d6..0915350f 100644
--- a/FreeAPS/Sources/Modules/PreferencesEditor/PreferencesEditorDataFlow.swift
+++ b/FreeAPS/Sources/Modules/PreferencesEditor/PreferencesEditorDataFlow.swift
@@ -109,6 +109,7 @@ protocol PreferencesEditorProvider: Provider {
     var preferences: Preferences { get }
     func savePreferences(_ preferences: Preferences)
     func migrateUnits()
+    func updateManagerUnits()
 }
 
 protocol PreferencesSettable: AnyObject {
diff --git a/FreeAPS/Sources/Modules/PreferencesEditor/PreferencesEditorProvider.swift b/FreeAPS/Sources/Modules/PreferencesEditor/PreferencesEditorProvider.swift
index 7e8c63a0..f2a2765e 100644
--- a/FreeAPS/Sources/Modules/PreferencesEditor/PreferencesEditorProvider.swift
+++ b/FreeAPS/Sources/Modules/PreferencesEditor/PreferencesEditorProvider.swift
@@ -1,8 +1,10 @@
 import Foundation
+import HealthKit
 
 extension PreferencesEditor {
     final class Provider: BaseProvider, PreferencesEditorProvider {
         @Injected() private var settingsManager: SettingsManager!
+        @Injected() var fetchGlucoseManager: FetchGlucoseManager!
         private let processQueue = DispatchQueue(label: "PreferencesEditorProvider.processQueue")
 
         var preferences: Preferences {
@@ -17,6 +19,15 @@ extension PreferencesEditor {
             }
         }
 
+        func updateManagerUnits() {
+            var manager = fetchGlucoseManager.cgmManager
+            let managerName = manager.map { "\(type(of: $0))" } ?? "nil"
+            let units = settingsManager.settings.units
+            let loopkitUnits: HKUnit = units == .mgdL ? .milligramsPerDeciliter : .millimolesPerLiter
+            print("manager: \(managerName) is changing units to: \(loopkitUnits.description) ")
+            manager?.unitDidChange(to: loopkitUnits)
+        }
+
         func migrateUnits() {
             migrateTargets()
             migrateISF()
diff --git a/FreeAPS/Sources/Modules/PreferencesEditor/PreferencesEditorStateModel.swift b/FreeAPS/Sources/Modules/PreferencesEditor/PreferencesEditorStateModel.swift
index d28869fb..01175ffb 100644
--- a/FreeAPS/Sources/Modules/PreferencesEditor/PreferencesEditorStateModel.swift
+++ b/FreeAPS/Sources/Modules/PreferencesEditor/PreferencesEditorStateModel.swift
@@ -16,6 +16,7 @@ extension PreferencesEditor {
             subscribeSetting(\.units, on: $unitsIndex.map { $0 == 0 ? GlucoseUnits.mgdL : .mmolL }) {
                 unitsIndex = $0 == .mgdL ? 0 : 1
             } didSet: { [weak self] _ in
+                self?.provider.updateManagerUnits()
                 self?.provider.migrateUnits()
             }
 

dabear pushed a commit to dabear/Trio that referenced this issue Oct 10, 2024
dabear pushed a commit to dabear/Trio that referenced this issue Oct 10, 2024
@dabear
Copy link

dabear commented Oct 10, 2024

see #429

@Sjoerd-Bo3
Copy link
Contributor

@dabear Indeed against Dev, and I reopened it and assigned it to you

@dnzxy
Copy link
Contributor

dnzxy commented Oct 10, 2024

@dabear can you please answer this?

Does invoking the unit observer like suggested above change to way values are sent to Trio?

Will the change you have now PR'd also prompt propagation of mmol/L glucose values, or will they stay (expected behavior on my end) mg/dL?

@dabear
Copy link

dabear commented Oct 10, 2024

Shouldn't change anything. But since all cgmmanagers get this change you can even test it with dexcom G7 sensors

@dnzxy
Copy link
Contributor

dnzxy commented Oct 10, 2024

Great, thanks ☺️
I use G6 + Anubis, no G7s here 😅 but we have plenty testers.

@dabear
Copy link

dabear commented Oct 10, 2024

Any cgmmanagers including G6..

@dabear
Copy link

dabear commented Oct 11, 2024

Let me know if anything is needed to get that PR merged :) no hurry from my side though

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: Done
Development

No branches or pull requests

9 participants