From 6b989ec7aa55cb6e5173efde2186295b9e8f9696 Mon Sep 17 00:00:00 2001 From: Carlos Rafael Giani Date: Fri, 23 Dec 2022 17:02:33 +0100 Subject: [PATCH 1/3] combov2: Consider pump as uninitialized for a while after an error occurred Signed-off-by: Carlos Rafael Giani --- .../pump/combov2/ComboV2Fragment.kt | 1 + .../nightscout/pump/combov2/ComboV2Plugin.kt | 56 ++++++++++++++++++- pump/combov2/src/main/res/values/strings.xml | 2 + 3 files changed, 57 insertions(+), 2 deletions(-) diff --git a/pump/combov2/src/main/kotlin/info/nightscout/pump/combov2/ComboV2Fragment.kt b/pump/combov2/src/main/kotlin/info/nightscout/pump/combov2/ComboV2Fragment.kt index eac6eeacab..c8d9cd62e8 100644 --- a/pump/combov2/src/main/kotlin/info/nightscout/pump/combov2/ComboV2Fragment.kt +++ b/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) } diff --git a/pump/combov2/src/main/kotlin/info/nightscout/pump/combov2/ComboV2Plugin.kt b/pump/combov2/src/main/kotlin/info/nightscout/pump/combov2/ComboV2Plugin.kt index 6a71eef58a..5a5b3d3afc 100644 --- a/pump/combov2/src/main/kotlin/info/nightscout/pump/combov2/ComboV2Plugin.kt +++ b/pump/combov2/src/main/kotlin/info/nightscout/pump/combov2/ComboV2Plugin.kt @@ -95,6 +95,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, @@ -161,6 +163,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 +192,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 @@ -381,7 +390,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 +441,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, @@ -1398,6 +1417,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 @@ -1550,6 +1577,8 @@ class ComboV2Plugin @Inject constructor ( _serialNumberUIFlow.value = "" _bluetoothAddressUIFlow.value = "" + clearPumpErrorObservedFlag() + // The unpairing variable is set to false in // the PumpManager onPumpUnpaired callback. } @@ -1748,6 +1777,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 +2188,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)}", diff --git a/pump/combov2/src/main/res/values/strings.xml b/pump/combov2/src/main/res/values/strings.xml index ca0047ff78..73466f753f 100644 --- a/pump/combov2/src/main/res/values/strings.xml +++ b/pump/combov2/src/main/res/values/strings.xml @@ -135,4 +135,6 @@ buttons at the same time to cancel pairing)\n Date and/or time changed Daylight savings time (DST) started Daylight savings time (DST) ended + 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. + Refreshing pump status after the pump reported an error From e60e9a21ccdd7b4589c4d3993dc711567f4065cc Mon Sep 17 00:00:00 2001 From: Carlos Rafael Giani Date: Sun, 25 Dec 2022 16:13:28 +0100 Subject: [PATCH 2/3] combov2: Re-run Bluetooth connection attempts if permissions are missing Signed-off-by: Carlos Rafael Giani --- .../nightscout/pump/combov2/ComboV2Plugin.kt | 100 ++++++++++++------ .../info/nightscout/pump/combov2/Utility.kt | 43 +++++++- 2 files changed, 107 insertions(+), 36 deletions(-) diff --git a/pump/combov2/src/main/kotlin/info/nightscout/pump/combov2/ComboV2Plugin.kt b/pump/combov2/src/main/kotlin/info/nightscout/pump/combov2/ComboV2Plugin.kt index 5a5b3d3afc..6b38b859a9 100644 --- a/pump/combov2/src/main/kotlin/info/nightscout/pump/combov2/ComboV2Plugin.kt +++ b/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 @@ -109,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() @@ -286,32 +290,46 @@ class ComboV2Plugin @Inject constructor ( aapsLogger.debug(LTag.PUMP, "Creating bluetooth interface") bluetoothInterface = AndroidBluetoothInterface(context) - aapsLogger.debug(LTag.PUMP, "Setting up bluetooth interface") - bluetoothInterface!!.setup() + // 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() - aapsLogger.debug(LTag.PUMP, "Setting up pump manager") - pumpManager = ComboCtlPumpManager(bluetoothInterface!!, pumpStateStore) - pumpManager!!.setup { - _pairedStateUIFlow.value = false - unpairing = false + aapsLogger.debug(LTag.PUMP, "Setting up pump manager") + pumpManager = ComboCtlPumpManager(bluetoothInterface!!, pumpStateStore) + pumpManager!!.setup { + _pairedStateUIFlow.value = false + unpairing = false + } + + // UI flows that must have defined values right + // at start are initialized here. + + // The paired state UI flow is special in that it is also + // used as the backing store for the isPaired() function, + // so setting up that UI state flow equals updating that + // paired state. + val paired = pumpManager!!.getPairedPumpAddresses().isNotEmpty() + _pairedStateUIFlow.value = paired + + setDriverState(DriverState.Disconnected) + + // NOTE: EventInitializationChanged is sent in getPumpStatus() . + } } - - // UI flows that must have defined values right - // at start are initialized here. - - // The paired state UI flow is special in that it is also - // used as the backing store for the isPaired() function, - // so setting up that UI state flow equals updating that - // paired state. - val paired = pumpManager!!.getPairedPumpAddresses().isNotEmpty() - _pairedStateUIFlow.value = paired - - setDriverState(DriverState.Disconnected) - - // 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 { @@ -580,10 +598,15 @@ class ComboV2Plugin @Inject constructor ( var forciblyDisconnectDueToError = false try { - // 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) + 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. @@ -1486,15 +1509,22 @@ class ComboV2Plugin @Inject constructor ( pairingJob = pumpCoroutineScope.async { try { - val pairingResult = pumpManager?.pairWithNewPump(discoveryDuration) { newPumpAddress, previousAttemptFailed -> - aapsLogger.info( - LTag.PUMP, - "New pairing PIN request from Combo pump with Bluetooth " + - "address $newPumpAddress (previous attempt failed: $previousAttemptFailed)" - ) - _previousPairingAttemptFailedFlow.value = previousAttemptFailed - newPINChannel.receive() - } ?: throw IllegalStateException("Attempting to access uninitialized pump manager") + // 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 " + + "address $newPumpAddress (previous attempt failed: $previousAttemptFailed)" + ) + _previousPairingAttemptFailedFlow.value = previousAttemptFailed + newPINChannel.receive() + } ?: throw IllegalStateException("Attempting to access uninitialized pump manager") + } if (pairingResult !is ComboCtlPumpManager.PairingResult.Success) return@async diff --git a/pump/combov2/src/main/kotlin/info/nightscout/pump/combov2/Utility.kt b/pump/combov2/src/main/kotlin/info/nightscout/pump/combov2/Utility.kt index 971d88e900..03576e4401 100644 --- a/pump/combov2/src/main/kotlin/info/nightscout/pump/combov2/Utility.kt +++ b/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 @@ -22,4 +30,37 @@ fun AAPSProfile.toComboCtlBasalProfile(): BasalProfile { (this.getBasalTimeFromMidnight(hour * 60 * 60) * 1000.0).toInt() } return BasalProfile(factors) -} \ No newline at end of file +} + +suspend fun runWithPermissionCheck( + context: Context, + config: Config, + aapsLogger: AAPSLogger, + androidPermission: AndroidPermission, + permissionsToCheckFor: Collection, + 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 + } + } +} From 3887cc90ae040a59fa36676ab4a0e1e9cc9a73f3 Mon Sep 17 00:00:00 2001 From: Carlos Rafael Giani Date: Wed, 28 Dec 2022 22:52:40 +0100 Subject: [PATCH 3/3] combov2: Set enacted to true even if the basal profiles are identical Signed-off-by: Carlos Rafael Giani --- .../nightscout/pump/combov2/ComboV2Plugin.kt | 26 ++++++++++++------- 1 file changed, 17 insertions(+), 9 deletions(-) diff --git a/pump/combov2/src/main/kotlin/info/nightscout/pump/combov2/ComboV2Plugin.kt b/pump/combov2/src/main/kotlin/info/nightscout/pump/combov2/ComboV2Plugin.kt index 6b38b859a9..341ac5323e 100644 --- a/pump/combov2/src/main/kotlin/info/nightscout/pump/combov2/ComboV2Plugin.kt +++ b/pump/combov2/src/main/kotlin/info/nightscout/pump/combov2/ComboV2Plugin.kt @@ -786,17 +786,25 @@ class ComboV2Plugin @Inject constructor ( Notification.INFO, 60 ) - - 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 - } + // 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 } } } catch (e: CancellationException) {