diff --git a/lib/models/observer_config.dart b/lib/models/observer_config.dart index 2e7f493..70b824c 100644 --- a/lib/models/observer_config.dart +++ b/lib/models/observer_config.dart @@ -100,6 +100,23 @@ class SecretSet extends SecretField { final String value; } +/// Outcome of verifying that a broker enable/disable actually took effect, by +/// re-reading the slot after the SET (#89). A firmware build can ACK a toggle as +/// success without applying it (meshcore-firmware#179), so the UI never trusts +/// the ACK alone — it re-reads and reports what the device actually says. +enum BrokerApplyOutcome { + /// Re-read confirms the device is in the intended enabled state. + applied, + + /// The device ACKed success but the re-read shows the unchanged state — it did + /// not apply the toggle. Surfaced as a warning, never a false success. + notApplied, + + /// The re-read itself failed (timeout/disconnect — e.g. an enable that rebooted + /// the device), so the result can't be confirmed either way. + unverified, +} + /// One MQTT broker slot (0-9), as read back from the `OCFG_BROKERS` dump. /// Non-secret fields only — `password` is reported as presence ([passwordSet]), /// `jwt_token` is never a config key. @@ -155,6 +172,20 @@ class BrokerConfig { return null; } + /// Classify whether a toggle/save took, comparing the [intendedEnabled] state + /// to a re-read [actualEnabled] (null = the re-read failed). Pure and + /// device-agnostic: it reports only what the device reported back, never + /// assuming a change took that the device didn't confirm (#89). + static BrokerApplyOutcome classifyApply({ + required bool intendedEnabled, + required bool? actualEnabled, + }) { + if (actualEnabled == null) return BrokerApplyOutcome.unverified; + return actualEnabled == intendedEnabled + ? BrokerApplyOutcome.applied + : BrokerApplyOutcome.notApplied; + } + /// Build from one slot's decoded `key=value` lines (the OCFG_BROKER_KV bodies). /// Unknown keys are ignored; missing keys keep the defaults. factory BrokerConfig.fromWireFields(int slot, Map kv) { diff --git a/lib/screens/settings/broker_editor_screen.dart b/lib/screens/settings/broker_editor_screen.dart index 3bfb170..c80099b 100644 --- a/lib/screens/settings/broker_editor_screen.dart +++ b/lib/screens/settings/broker_editor_screen.dart @@ -140,6 +140,7 @@ class _BrokerEditorScreenState extends State { return; } } + final intended = _enabled; final svc = context.read(); final messenger = ScaffoldMessenger.of(context); final navigator = Navigator.of(context); @@ -148,17 +149,12 @@ class _BrokerEditorScreenState extends State { final result = await svc.saveBroker( _baseline.slot, fields: _changedFields(), - enable: _enabled, + enable: intended, wasLive: _baseline.enabled, ); if (!mounted) return; - setState(() => _busy = false); - if (result.ok) { - messenger.showSnackBar( - SnackBar(content: Text('Broker ${_baseline.slot} saved')), - ); - navigator.pop(true); - } else { + if (!result.ok) { + setState(() => _busy = false); messenger.showSnackBar( SnackBar( content: Text( @@ -168,6 +164,48 @@ class _BrokerEditorScreenState extends State { backgroundColor: errorColor, ), ); + return; + } + // The writes were ACKed — but a firmware build can ACK without applying the + // enabled state (meshcore-firmware#179). Settle, re-read, and only claim + // success if the device actually matches what we asked for. + await Future.delayed(ObserverConfigService.applySettleDelay); + if (!mounted) return; + final fresh = await svc.getBroker(_baseline.slot); + if (!mounted) return; + setState(() => _busy = false); + switch (BrokerConfig.classifyApply( + intendedEnabled: intended, + actualEnabled: fresh?.enabled, + )) { + case BrokerApplyOutcome.applied: + messenger.showSnackBar( + SnackBar(content: Text('Broker ${_baseline.slot} saved')), + ); + navigator.pop(true); + case BrokerApplyOutcome.notApplied: + // Stay on the editor, re-seed to the device's true state, and warn — the + // fields were written but the device didn't honor the enable/disable. + if (fresh != null) _seedFrom(fresh); + messenger.showSnackBar( + SnackBar( + content: Text( + 'Saved, but the device did not ${intended ? 'enable' : 'disable'} ' + 'broker ${_baseline.slot} — possible firmware issue', + ), + backgroundColor: errorColor, + ), + ); + case BrokerApplyOutcome.unverified: + messenger.showSnackBar( + SnackBar( + content: Text( + 'Broker ${_baseline.slot} saved — could not confirm enabled ' + 'state (device may be rebooting)', + ), + ), + ); + navigator.pop(true); } } diff --git a/lib/screens/settings/mqtt_brokers_screen.dart b/lib/screens/settings/mqtt_brokers_screen.dart index 1f9c0ba..ad316d7 100644 --- a/lib/screens/settings/mqtt_brokers_screen.dart +++ b/lib/screens/settings/mqtt_brokers_screen.dart @@ -74,16 +74,11 @@ class _MqttBrokersScreenState extends State { } } final svc = context.read(); + setState(() => _busy = true); final ok = await svc.setBrokerField(b.slot, 'enabled', enable ? '1' : '0'); if (!mounted) return; - if (ok) { - messenger.showSnackBar( - SnackBar( - content: Text('Broker ${b.slot} ${enable ? 'enabled' : 'disabled'}'), - ), - ); - await _reload(); - } else { + if (!ok) { + setState(() => _busy = false); messenger.showSnackBar( SnackBar( content: Text( @@ -93,6 +88,56 @@ class _MqttBrokersScreenState extends State { backgroundColor: errorColor, ), ); + return; + } + // The SET was ACKed — but a firmware build can ACK without applying it + // (meshcore-firmware#179). Settle, re-read the slot, and report what the + // device ACTUALLY did rather than trusting the ACK. + await Future.delayed(ObserverConfigService.applySettleDelay); + if (!mounted) return; + final fresh = await svc.getBroker(b.slot); + if (!mounted) return; + setState(() { + _busy = false; + if (fresh != null) { + _brokers = [ + for (final x in _brokers) + if (x.slot == b.slot) fresh else x, + ]; + } + }); + switch (BrokerConfig.classifyApply( + intendedEnabled: enable, + actualEnabled: fresh?.enabled, + )) { + case BrokerApplyOutcome.applied: + messenger.showSnackBar( + SnackBar( + content: Text( + 'Broker ${b.slot} ${enable ? 'enabled' : 'disabled'}', + ), + ), + ); + case BrokerApplyOutcome.notApplied: + messenger.showSnackBar( + SnackBar( + content: Text( + 'Device reported success but broker ${b.slot} is still ' + '${fresh!.enabled ? 'enabled' : 'disabled'} — possible firmware ' + 'issue', + ), + backgroundColor: errorColor, + ), + ); + case BrokerApplyOutcome.unverified: + messenger.showSnackBar( + SnackBar( + content: Text( + 'Broker ${b.slot} ${enable ? 'enable' : 'disable'} sent — could ' + 'not confirm (device may be rebooting). Tap Refresh to re-check.', + ), + ), + ); } } diff --git a/lib/services/observer_config_service.dart b/lib/services/observer_config_service.dart index 860c031..b17ad1d 100644 --- a/lib/services/observer_config_service.dart +++ b/lib/services/observer_config_service.dart @@ -24,6 +24,10 @@ class ObserverConfigService extends ChangeNotifier { /// an unsupported node or a dropped frame. Duration timeout = const Duration(seconds: 6); + /// After a broker enable/disable SET, wait this long before the verify re-read + /// so a device that applies the change asynchronously has settled (#89). + static const Duration applySettleDelay = Duration(milliseconds: 600); + ObserverConfig? _config; ObserverConfig? get config => _config; diff --git a/test/models/broker_apply_outcome_test.dart b/test/models/broker_apply_outcome_test.dart new file mode 100644 index 0000000..de51d69 --- /dev/null +++ b/test/models/broker_apply_outcome_test.dart @@ -0,0 +1,61 @@ +// Honest-apply classifier (#89). A firmware build can ACK a broker +// enable/disable as success but not apply it (HV4 desync, +// meshcore-firmware#179). After a toggle/save the UI re-reads the slot and +// compares to what it asked for; this classifier turns (intended, re-read) into +// the outcome the UI reports. It treats every device identically — it only ever +// reflects the re-read, never assuming the change took. + +import 'package:flutter_test/flutter_test.dart'; +import 'package:meshcore_open/models/observer_config.dart'; + +void main() { + group('BrokerConfig.classifyApply', () { + test('enable the device confirms -> applied', () { + expect( + BrokerConfig.classifyApply(intendedEnabled: true, actualEnabled: true), + BrokerApplyOutcome.applied, + ); + }); + + test('disable the device confirms -> applied', () { + expect( + BrokerConfig.classifyApply( + intendedEnabled: false, + actualEnabled: false, + ), + BrokerApplyOutcome.applied, + ); + }); + + test( + 'disable ACKed but re-read still enabled -> notApplied (the HV4 bug)', + () { + expect( + BrokerConfig.classifyApply( + intendedEnabled: false, + actualEnabled: true, + ), + BrokerApplyOutcome.notApplied, + ); + }, + ); + + test('enable ACKed but re-read still disabled -> notApplied', () { + expect( + BrokerConfig.classifyApply(intendedEnabled: true, actualEnabled: false), + BrokerApplyOutcome.notApplied, + ); + }); + + test('re-read failed (null) -> unverified, whatever the intent', () { + expect( + BrokerConfig.classifyApply(intendedEnabled: true, actualEnabled: null), + BrokerApplyOutcome.unverified, + ); + expect( + BrokerConfig.classifyApply(intendedEnabled: false, actualEnabled: null), + BrokerApplyOutcome.unverified, + ); + }); + }); +} diff --git a/test/screens/mqtt_brokers_screen_test.dart b/test/screens/mqtt_brokers_screen_test.dart index 9be7482..406fc3e 100644 --- a/test/screens/mqtt_brokers_screen_test.dart +++ b/test/screens/mqtt_brokers_screen_test.dart @@ -1,6 +1,7 @@ // Widget tests for the MQTT brokers screen (#80): a tile per broker, the + FAB // opens the editor on the next empty slot, and long-press exposes the -// Enable/Disable/Edit/Clear actions (Clear behind a confirm). +// Enable/Disable/Edit/Clear actions (Clear behind a confirm). A toggle/save now +// verifies the device actually applied the change before confirming (#89). import 'package:flutter/material.dart'; import 'package:flutter_test/flutter_test.dart'; @@ -21,6 +22,11 @@ class _FakeSvc extends ObserverConfigService { bool toggleOk = true; String? errorText; + /// When false, the device ACKs an enabled-SET but the verify re-read reports + /// the slot UNCHANGED — models the HV4 ack-but-doesn't-apply bug (#89). + bool deviceApplies = true; + final Map _enabledOverride = {}; + @override String? get lastError => errorText; @override @@ -30,9 +36,21 @@ class _FakeSvc extends ObserverConfigService { @override Future setBrokerField(int slot, String field, String value) async { setCalls.add('$slot.$field=$value'); + if (toggleOk && deviceApplies && field == 'enabled') { + _enabledOverride[slot] = value == '1'; + } return toggleOk; } + @override + Future getBroker(int slot) async { + final base = _brokers.firstWhere( + (b) => b.slot == slot, + orElse: () => BrokerConfig(slot: slot), + ); + return base.copyWith(enabled: _enabledOverride[slot] ?? base.enabled); + } + @override Future clearBroker(int slot) async { clearCalls.add(slot); @@ -102,7 +120,9 @@ void main() { expect(fake.clearCalls, contains(2)); }); - testWidgets('a successful quick Enable confirms', (tester) async { + testWidgets('a successful quick Enable verifies then confirms', ( + tester, + ) async { final fake = _FakeSvc(const [BrokerConfig(slot: 2, url: 'a', port: 1883)]); await _pump(tester, fake); @@ -110,9 +130,31 @@ void main() { await tester.pumpAndSettle(); await tester.tap(find.text('Enable')); await tester.pumpAndSettle(); + // Advance past the settle delay so the verify re-read runs. + await tester.pump(const Duration(milliseconds: 700)); + await tester.pumpAndSettle(); + + expect(fake.setCalls, contains('2.enabled=1')); + expect(find.textContaining('Broker 2 enabled'), findsOneWidget); + await tester.pump(const Duration(seconds: 5)); + }); + + testWidgets('an ACK-but-not-applied toggle warns of a firmware issue', ( + tester, + ) async { + final fake = _FakeSvc(const [BrokerConfig(slot: 2, url: 'a', port: 1883)]) + ..deviceApplies = false; // ACKs success but re-read stays disabled + await _pump(tester, fake); + + await tester.longPress(find.text('[2] a')); + await tester.pumpAndSettle(); + await tester.tap(find.text('Enable')); + await tester.pumpAndSettle(); + await tester.pump(const Duration(milliseconds: 700)); + await tester.pumpAndSettle(); expect(fake.setCalls, contains('2.enabled=1')); - expect(find.textContaining('enabled'), findsOneWidget); + expect(find.textContaining('possible firmware issue'), findsOneWidget); await tester.pump(const Duration(seconds: 5)); });