From f56a6ac58f55f47d62be075d2cfe2ae064b67b9c Mon Sep 17 00:00:00 2001 From: Strycher Date: Mon, 22 Jun 2026 11:47:45 -0400 Subject: [PATCH] fix(#79): degrade gracefully when the broker dump fails (G2 finding) The G2 BLE log showed the config protocol works (GETs return values; the broker dump starts) but the firmware's broker dump stalls without BROKERS_END, so getBrokers() times out. The client was treating that as a TOTAL failure -- discarding the 7 flat settings it read cleanly and blanking the pane. Now the broker pool loads independently: refresh() builds + shows the flat config even when getBrokers fails, flags brokersUnavailable, and the pane shows the broker section as "unavailable" instead of going stale. Flat settings stay usable. TDD red->green; full observer suite (39) green. Co-Authored-By: Claude Opus 4.8 --- .../settings/observer_settings_view.dart | 8 ++++- lib/services/observer_config_service.dart | 30 +++++++++++-------- test/screens/observer_settings_view_test.dart | 23 ++++++++++++-- .../observer_config_service_test.dart | 28 ++++++++++++++++- 4 files changed, 72 insertions(+), 17 deletions(-) diff --git a/lib/screens/settings/observer_settings_view.dart b/lib/screens/settings/observer_settings_view.dart index 1676b12..1e1cc39 100644 --- a/lib/screens/settings/observer_settings_view.dart +++ b/lib/screens/settings/observer_settings_view.dart @@ -236,7 +236,13 @@ class _ObserverSettingsViewState extends State { ), const Divider(height: 32), _sectionTitle('MQTT brokers'), - ..._brokerTiles(svc.config?.brokers ?? const []), + if (svc.brokersUnavailable) + _banner( + Icons.cloud_off, + 'Broker pool unavailable โ€” the device did not finish sending it.', + ) + else + ..._brokerTiles(svc.config?.brokers ?? const []), ], ); } diff --git a/lib/services/observer_config_service.dart b/lib/services/observer_config_service.dart index 15c13b8..e696d4a 100644 --- a/lib/services/observer_config_service.dart +++ b/lib/services/observer_config_service.dart @@ -37,6 +37,11 @@ class ObserverConfigService extends ChangeNotifier { String? _lastError; String? get lastError => _lastError; + /// True when the broker-pool dump failed but the flat settings read OK โ€” the + /// UI shows the broker section as unavailable instead of blanking the pane. + bool _brokersUnavailable = false; + bool get brokersUnavailable => _brokersUnavailable; + /// Whether the connected device supports the config command โ€” the version /// gate AND the capability bit, read live from the connector's device-info /// parse. The settings UI watches the connector, so the Observer category @@ -164,28 +169,27 @@ class ObserverConfigService extends ChangeNotifier { final rotation = await getFlat('display.rotation'); final brokers = await getBrokers(); - if (brokers == null) { - _stale = true; - notifyListeners(); - return; - } - + // The broker pool loads independently of the flat settings: a broker-dump + // failure (e.g. the firmware never sends BROKERS_END) must NOT blank + // settings that read cleanly. Surface it as broker-only "unavailable". _config = ObserverConfig( wifi: _parseWifi(ssid, wifiEnabled, wifiStatus), mqtt: MqttGlobalConfig( iata: iata ?? '', statusInterval: int.tryParse(statusInterval ?? '') ?? 60, ), - brokers: brokers, + brokers: brokers ?? const [], display: DisplayConfig( alwaysOn: alwaysOn == '1', rotation: int.tryParse(rotation ?? '') ?? 0, ), ); - // A null from any getFlat is a failed read (GET returns the value, null - // on ERR/timeout). Don't present defaults as a complete snapshot or wipe - // the error a failed field already surfaced (SAFELANE error-visibility). - final allRead = + _brokersUnavailable = brokers == null; + + // A null from any flat getFlat is a failed read (GET returns the value, + // null on ERR/timeout). Stale reflects the FLAT read only; a broker miss + // is shown in its own section, not as a stale snapshot (SAFELANE ยง6). + final allFlatRead = ssid != null && wifiEnabled != null && wifiStatus != null && @@ -193,8 +197,8 @@ class ObserverConfigService extends ChangeNotifier { statusInterval != null && alwaysOn != null && rotation != null; - _stale = !allRead; - if (allRead) _lastError = null; + _stale = !allFlatRead; + if (allFlatRead) _lastError = null; notifyListeners(); } catch (e) { _stale = true; diff --git a/test/screens/observer_settings_view_test.dart b/test/screens/observer_settings_view_test.dart index 1ea7dae..a32a8ae 100644 --- a/test/screens/observer_settings_view_test.dart +++ b/test/screens/observer_settings_view_test.dart @@ -17,14 +17,21 @@ import 'package:meshcore_open/services/observer_config_service.dart'; class _DummyConn extends MeshCoreConnector {} class _FakeSvc extends ObserverConfigService { - _FakeSvc(this._cfg, {this.staleFlag = false, this.errorText}) - : super(_DummyConn()); + _FakeSvc( + this._cfg, { + this.staleFlag = false, + this.errorText, + this.brokersDown = false, + }) : super(_DummyConn()); final ObserverConfig _cfg; final bool staleFlag; final String? errorText; + final bool brokersDown; final List> sets = []; + @override + bool get brokersUnavailable => brokersDown; @override bool get supported => true; @override @@ -171,4 +178,16 @@ void main() { expect(iv, hasLength(1)); expect(iv.single.value, '120'); }); + + testWidgets('broker pool shows unavailable when the dump failed (#79)', ( + tester, + ) async { + final fake = _FakeSvc(_cfg(), brokersDown: true); + await _pump(tester, fake); + + // The broker section is at the bottom of the scroll view; scroll it in. + await tester.drag(find.byType(ListView), const Offset(0, -2000)); + await tester.pumpAndSettle(); + expect(find.textContaining('Broker pool unavailable'), findsOneWidget); + }); } diff --git a/test/services/observer_config_service_test.dart b/test/services/observer_config_service_test.dart index 0035d8e..0faca77 100644 --- a/test/services/observer_config_service_test.dart +++ b/test/services/observer_config_service_test.dart @@ -52,8 +52,9 @@ class _FakeConnector extends MeshCoreConnector { /// manual interleaving. [failKeys] answer ERR; others answer VALUE; the broker /// dump is an empty pool (START -> END). class _AutoConnector extends MeshCoreConnector { - _AutoConnector({this.failKeys = const {}}); + _AutoConnector({this.failKeys = const {}, this.failBrokers = false}); final Set failKeys; + final bool failBrokers; final StreamController _frames = StreamController.broadcast(); @@ -78,6 +79,7 @@ class _AutoConnector extends MeshCoreConnector { : _resp(ObserverConfigClient.rValue, '$key = ${_valueFor(key)}'); Future.microtask(() => _frames.add(frame)); } else if (op == ObserverConfigClient.opBrokers) { + if (failBrokers) return; // no response -> getBrokers times out Future.microtask(() { _frames.add( Uint8List.fromList([ @@ -222,4 +224,28 @@ void main() { expect(s.lastError, isNull); auto.closeStream(); }); + + // ---- #79: a broker-dump failure must not blank the flat settings ---- + test( + 'broker-dump failure keeps flat settings + flags brokers unavailable', + () async { + final auto = _AutoConnector(failBrokers: true); + final s = ObserverConfigService(auto); + s.timeout = const Duration(milliseconds: 50); + await s.refresh(); + expect( + s.config, + isNotNull, + reason: 'settings that read cleanly must still show', + ); + expect(s.config!.wifi.ssid, 'x'); + expect(s.brokersUnavailable, isTrue); + expect( + s.stale, + isFalse, + reason: 'a broker-dump miss is not a stale snapshot', + ); + auto.closeStream(); + }, + ); }