Merge pull request #2311 from dv1/combov2-fixes-001

combov2: Fixes to Bluetooth permission checks, identical basal profile handling, pump errors

pump/combov2/src/main/kotlin/info/nightscout/pump/combov2/ComboV2Fragment.kt

@@ -41,6 +41,7 @@ class ComboV2Fragment : DaggerFragment() {
 
         binding.combov2RefreshButton.setOnClickListener {
             binding.combov2RefreshButton.isEnabled = false
+            combov2Plugin.clearPumpErrorObservedFlag()
             commandQueue.readStatus(rh.gs(info.nightscout.core.ui.R.string.user_request), null)
         }
 

pump/combov2/src/main/kotlin/info/nightscout/pump/combov2/ComboV2Plugin.kt

@@ -25,6 +25,8 @@ import info.nightscout.comboctl.parser.BatteryState
 import info.nightscout.comboctl.parser.ReservoirState
 import info.nightscout.core.ui.dialogs.OKDialog
 import info.nightscout.core.ui.toast.ToastUtils
+import info.nightscout.interfaces.AndroidPermission
+import info.nightscout.interfaces.Config
 import info.nightscout.interfaces.constraints.Constraint
 import info.nightscout.interfaces.constraints.Constraints
 import info.nightscout.interfaces.notifications.Notification
@@ -95,6 +97,8 @@ import info.nightscout.comboctl.base.Tbr as ComboCtlTbr
 import info.nightscout.comboctl.main.Pump as ComboCtlPump
 import info.nightscout.comboctl.main.PumpManager as ComboCtlPumpManager
 
+internal const val PUMP_ERROR_TIMEOUT_INTERVAL_MSECS = 1000L * 60 * 5
+
 @Singleton
 class ComboV2Plugin @Inject constructor (
     injector: HasAndroidInjector,
@@ -107,7 +111,9 @@ class ComboV2Plugin @Inject constructor (
     private val sp: SP,
     private val pumpSync: PumpSync,
     private val dateUtil: DateUtil,
-    private val uiInteraction: UiInteraction
+    private val uiInteraction: UiInteraction,
+    private val androidPermission: AndroidPermission,
+    private val config: Config
 ) :
     PumpPluginBase(
         PluginDescription()
@@ -161,6 +167,13 @@ class ComboV2Plugin @Inject constructor (
     private var lastConnectionTimestamp = 0L
     private var lastComboAlert: AlertScreenContent? = null
 
+    // States for when the pump reports an error. We then want isInitialized()
+    // to return false until either the user presses the Refresh button or the
+    // pumpErrorTimeoutJob expires. That way, the loop won't run until then,
+    // giving the user a chance to handle the error.
+    private var pumpErrorObserved = false
+    private var pumpErrorTimeoutJob: Job? = null
+
     // Set to true if a disconnect request came in while the driver
     // was in the Connecting, CheckingPump, or ExecutingCommand
     // state (in other words, while isBusy() was returning true).
@@ -183,7 +196,7 @@ class ComboV2Plugin @Inject constructor (
     private var activeBasalProfile: BasalProfile? = null
     // This is used for checking that the correct basal profile is
     // active in the Combo. If not, loop invocation is disallowed.
-    // This is _not_ reset by disconect(). That's on purpose; it
+    // This is _not_ reset by disconnect(). That's on purpose; it
     // is read by isLoopInvocationAllowed(), which is called even
     // if the pump is not connected.
     private var lastActiveBasalProfileNumber: Int? = null
@@ -277,6 +290,15 @@ class ComboV2Plugin @Inject constructor (
         aapsLogger.debug(LTag.PUMP, "Creating bluetooth interface")
         bluetoothInterface = AndroidBluetoothInterface(context)
 
+        // Continue initialization in a separate coroutine. This allows us to call
+        // runWithPermissionCheck(), which will keep trying to run the code block
+        // until either the necessary Bluetooth permissios are granted, or the
+        // coroutine is cancelled (see onStop() below).
+        pumpCoroutineScope.launch {
+            runWithPermissionCheck(
+                context, config, aapsLogger, androidPermission,
+                permissionsToCheckFor = listOf("android.permission.BLUETOOTH_CONNECT")
+            ) {
                 aapsLogger.debug(LTag.PUMP, "Setting up bluetooth interface")
                 bluetoothInterface!!.setup()
 
@@ -301,8 +323,13 @@ class ComboV2Plugin @Inject constructor (
 
                 // NOTE: EventInitializationChanged is sent in getPumpStatus() .
             }
+        }
+    }
 
     override fun onStop() {
+        // Cancel any ongoing background coroutines. This includes an ongoing
+        // unfinished initialization that still waits for the user to grant
+        // Bluetooth permissions.
         pumpCoroutineScope.cancel()
 
         runBlocking {
@@ -381,7 +408,7 @@ class ComboV2Plugin @Inject constructor (
     }
 
     override fun isInitialized(): Boolean =
-        isPaired() && (driverStateFlow.value != DriverState.NotInitialized)
+        isPaired() && (driverStateFlow.value != DriverState.NotInitialized) && !pumpErrorObserved
 
     override fun isSuspended(): Boolean =
         when (driverStateUIFlow.value) {
@@ -432,6 +459,16 @@ class ComboV2Plugin @Inject constructor (
             return
         }
 
+        if (pumpErrorObserved) {
+            aapsLogger.debug(LTag.PUMP, "Aborting connect attempt since the pumpErrorObserved flag is set")
+            uiInteraction.addNotification(
+                Notification.COMBO_PUMP_ALARM,
+                text = rh.gs(R.string.combov2_cannot_connect_pump_error_observed),
+                level = Notification.NORMAL
+            )
+            return
+        }
+
         when (driverStateFlow.value) {
             DriverState.Connecting,
             DriverState.CheckingPump,
@@ -561,10 +598,15 @@ class ComboV2Plugin @Inject constructor (
                 var forciblyDisconnectDueToError = false
 
                 try {
+                    runWithPermissionCheck(
+                        context, config, aapsLogger, androidPermission,
+                        permissionsToCheckFor = listOf("android.permission.BLUETOOTH_CONNECT")
+                    ) {
                         // Set maxNumAttempts to null to turn off the connection attempt limit inside the connect() call.
                         // The AAPS queue thread will anyway cause the connectionSetupJob to be canceled when its
                         // connection timeout expires, so the Pump class' own connection attempt limiter is redundant.
                         pump?.connect(maxNumAttempts = null)
+                    }
 
                     // No need to set the driver state here, since the pump's stateFlow will announce that.
 
@@ -744,18 +786,26 @@ class ComboV2Plugin @Inject constructor (
                             Notification.INFO,
                             60
                         )
+                    } else {
+                        aapsLogger.debug(LTag.PUMP, "Basal profiles are equal; did not have to set anything")
+                        // Treat this as if the command had been enacted. Setting a basal profile is
+                        // an idempotent operation, meaning that setting the exact same profile factors
+                        // twice in a row does not actually change anything. Therefore, we can just
+                        // completely skip such a redundant set basal profile operation and still get
+                        // the exact same result.
+                        // Furthermore, it is actually important to also set enacted to true in this case
+                        // because even though this _driver_ might know that the Combo uses this profile
+                        // already, _AAPS_ might not. A good example is when AAPS is set up the first time
+                        // and no profile has been activated. If in this case the profile happens to be
+                        // identical to what's already in the Combo, then enacted=false would cause errors,
+                        // because AAPS expects the driver to always enact the profile change in this case
+                        // (since it thinks that no profile is set yet).
+                    }
 
                     pumpEnactResult.apply {
                         success = true
                         enacted = true
                     }
-                    } else {
-                        aapsLogger.debug(LTag.PUMP, "Basal profiles are equal; did not have to set anything")
-                        pumpEnactResult.apply {
-                            success = true
-                            enacted = false
-                        }
-                    }
                 }
             } catch (e: CancellationException) {
                 // Cancellation is not an error, but it also means
@@ -1398,6 +1448,14 @@ class ComboV2Plugin @Inject constructor (
         commandQueue.readStatus(reason, null)
     }
 
+    fun clearPumpErrorObservedFlag() {
+        stopPumpErrorTimeout()
+        if (pumpErrorObserved) {
+            aapsLogger.info(LTag.PUMP, "Clearing pumpErrorObserved flag")
+            pumpErrorObserved = false
+        }
+    }
+
     /*** Loop constraints ***/
     // These restrict the function of the loop in case of an event
     // that makes running a loop too risky, for example because something
@@ -1459,7 +1517,13 @@ class ComboV2Plugin @Inject constructor (
 
         pairingJob = pumpCoroutineScope.async {
             try {
-                val pairingResult = pumpManager?.pairWithNewPump(discoveryDuration) { newPumpAddress, previousAttemptFailed ->
+                // Do the pairing attempt within runWithPermissionCheck()
+                // since pairing requires Bluetooth permissions.
+                val pairingResult = runWithPermissionCheck(
+                    context, config, aapsLogger, androidPermission,
+                    permissionsToCheckFor = listOf("android.permission.BLUETOOTH_CONNECT")
+                ) {
+                    pumpManager?.pairWithNewPump(discoveryDuration) { newPumpAddress, previousAttemptFailed ->
                         aapsLogger.info(
                             LTag.PUMP,
                             "New pairing PIN request from Combo pump with Bluetooth " +
@@ -1468,6 +1532,7 @@ class ComboV2Plugin @Inject constructor (
                         _previousPairingAttemptFailedFlow.value = previousAttemptFailed
                         newPINChannel.receive()
                     } ?: throw IllegalStateException("Attempting to access uninitialized pump manager")
+                }
 
                 if (pairingResult !is ComboCtlPumpManager.PairingResult.Success)
                     return@async
@@ -1550,6 +1615,8 @@ class ComboV2Plugin @Inject constructor (
         _serialNumberUIFlow.value = ""
         _bluetoothAddressUIFlow.value = ""
 
+        clearPumpErrorObservedFlag()
+
         // The unpairing variable is set to false in
         // the PumpManager onPumpUnpaired callback.
     }
@@ -1748,6 +1815,23 @@ class ComboV2Plugin @Inject constructor (
         }
     }
 
+    private fun startPumpErrorTimeout() {
+        if (pumpErrorTimeoutJob != null)
+            return
+
+        pumpErrorTimeoutJob = pumpCoroutineScope.launch {
+            delay(PUMP_ERROR_TIMEOUT_INTERVAL_MSECS)
+            aapsLogger.info(LTag.PUMP, "Clearing pumpErrorObserved flag after timeout was reached")
+            pumpErrorObserved = false
+            commandQueue.readStatus(rh.gs(R.string.combov2_refresh_pump_status_after_error), null)
+        }
+    }
+
+    private fun stopPumpErrorTimeout() {
+        pumpErrorTimeoutJob?.cancel()
+        pumpErrorTimeoutJob = null
+    }
+
     private fun updateBaseBasalRateUI() {
         val currentHour = DateTime().hourOfDay().get()
         // This sets value to null if no profile is set,
@@ -2142,6 +2226,12 @@ class ComboV2Plugin @Inject constructor (
         }
 
     private fun notifyAboutComboAlert(alert: AlertScreenContent) {
+        if (alert is AlertScreenContent.Error) {
+            aapsLogger.info(LTag.PUMP, "Error screen observed - setting pumpErrorObserved flag")
+            pumpErrorObserved = true
+            startPumpErrorTimeout()
+        }
+
         uiInteraction.addNotification(
             Notification.COMBO_PUMP_ALARM,
             text = "${rh.gs(R.string.combov2_combo_alert)}: ${getAlertDescription(alert)}",

pump/combov2/src/main/kotlin/info/nightscout/pump/combov2/Utility.kt

@@ -1,8 +1,16 @@
 package info.nightscout.pump.combov2
 
+import android.content.Context
+import android.os.Build
+import info.nightscout.comboctl.android.AndroidBluetoothPermissionException
 import info.nightscout.comboctl.main.BasalProfile
 import info.nightscout.comboctl.main.NUM_COMBO_BASAL_PROFILE_FACTORS
+import info.nightscout.interfaces.AndroidPermission
+import info.nightscout.interfaces.Config
 import info.nightscout.interfaces.profile.Profile as AAPSProfile
+import info.nightscout.rx.logging.AAPSLogger
+import info.nightscout.rx.logging.LTag
+import kotlinx.coroutines.delay
 
 // Utility extension functions for clearer conversion between
 // ComboCtl units and AAPS units. ComboCtl uses integer-encoded
@@ -23,3 +31,36 @@ fun AAPSProfile.toComboCtlBasalProfile(): BasalProfile {
     }
     return BasalProfile(factors)
 }
+
+suspend fun <T> runWithPermissionCheck(
+    context: Context,
+    config: Config,
+    aapsLogger: AAPSLogger,
+    androidPermission: AndroidPermission,
+    permissionsToCheckFor: Collection<String>,
+    block: suspend () -> T
+): T {
+    var permissions = permissionsToCheckFor
+    while (true) {
+        try {
+            if (config.PUMPDRIVERS && Build.VERSION.SDK_INT >= Build.VERSION_CODES.S) {
+                val notAllPermissionsGranted = permissions.fold(initial = false) { currentResult, permission ->
+                    return@fold if (androidPermission.permissionNotGranted(context, permission)) {
+                        aapsLogger.debug(LTag.PUMP, "permission $permission was not granted by the user")
+                        true
+                    } else
+                        currentResult
+                }
+
+                if (notAllPermissionsGranted) {
+                    delay(1000) // Wait a little bit before retrying to avoid 100% CPU usage
+                    continue
+                }
+            }
+
+            return block.invoke()
+        } catch (e: AndroidBluetoothPermissionException) {
+            permissions = permissionsToCheckFor union e.missingPermissions
+        }
+    }
+}

pump/combov2/src/main/res/values/strings.xml

@@ -135,4 +135,6 @@ buttons at the same time to cancel pairing)\n
     <string name="combov2_datetime_changed">Date and/or time changed</string>
     <string name="combov2_dst_started">Daylight savings time (DST) started</string>
     <string name="combov2_dst_ended">Daylight savings time (DST) ended</string>
+    <string name="combov2_cannot_connect_pump_error_observed">Cannot connect to pump because the pump reported an error. User must handle the error and then either wait 5 minutes or press the Refresh button in the driver tab.</string>
+    <string name="combov2_refresh_pump_status_after_error">Refreshing pump status after the pump reported an error</string>
 </resources>