From 31c06743300bf555dd7930f234cc4703a16ee4fd Mon Sep 17 00:00:00 2001 From: Strycher Date: Mon, 22 Jun 2026 20:06:14 -0400 Subject: [PATCH] fix(#81): stop the observer config feature from slowing the channel sync Three observer-delta fixes (per the #80 sync diagnosis / G2): - A: settings screen gates the Observer category via context.select on the supported flag, not a connector-wide context.watch -- the watch rebuilt the whole settings screen on every sync frame, thrashing the UI thread. - B: a flat Save re-reads only the flat settings (refresh includeBrokers:false), never the 84-frame broker pool -- no BLE re-flood for a display toggle. - C: the initial observer read is deferred until the device's channel/contact sync settles, so it never competes with that sync for BLE. TDD: B (no OCFG_BROKERS on a flat refresh) + C (refresh deferred while syncing, fires once idle). Full observer suite green. Co-Authored-By: Claude Opus 4.8 --- .../settings/observer_settings_view.dart | 53 +++++++++++++++++-- lib/screens/settings_screen.dart | 15 ++++-- lib/services/observer_config_service.dart | 11 ++-- test/screens/observer_settings_view_test.dart | 52 ++++++++++++++++-- .../observer_config_service_test.dart | 24 +++++++++ 5 files changed, 138 insertions(+), 17 deletions(-) diff --git a/lib/screens/settings/observer_settings_view.dart b/lib/screens/settings/observer_settings_view.dart index 1e1cc39..8a62cfc 100644 --- a/lib/screens/settings/observer_settings_view.dart +++ b/lib/screens/settings/observer_settings_view.dart @@ -2,6 +2,7 @@ import 'package:flutter/material.dart'; import 'package:flutter/services.dart'; import 'package:provider/provider.dart'; +import '../../connector/meshcore_connector.dart'; import '../../models/observer_config.dart'; import '../../services/observer_config_service.dart'; @@ -14,9 +15,8 @@ import '../../services/observer_config_service.dart'; /// the source of truth — Refresh re-reads it. /// /// TODO(l10n): strings are English-only pending ARB keys. -/// TODO(#64 / firmware F-task): broker enable/disable/clear + the broker editor -/// are blocked on the firmware wire path (no `enabled` SET; VIEW allowlist is -/// read-only), so the broker pool is read-only here for now. +/// The broker pool is read-only HERE; the editor (tap -> edit, long-press -> +/// Enable/Disable/Edit/Clear) is tracked as epic #80 (firmware supports it). class ObserverSettingsView extends StatefulWidget { const ObserverSettingsView({super.key}); @@ -36,15 +36,46 @@ class _ObserverSettingsViewState extends State { bool _loading = false; bool _saving = false; bool _seeded = false; + bool _waitingForSync = false; + MeshCoreConnector? _syncWaitConn; @override void initState() { super.initState(); - WidgetsBinding.instance.addPostFrameCallback((_) => _refresh()); + WidgetsBinding.instance.addPostFrameCallback((_) => _refreshWhenIdle()); } + /// Defer the initial read until the device's channel/contact sync settles — + /// observer config traffic must not compete with (and slow) that sync (#81). + void _refreshWhenIdle() { + final conn = context.read(); + if (!_syncing(conn)) { + _refresh(); + return; + } + setState(() => _waitingForSync = true); + _syncWaitConn = conn; + conn.addListener(_onSyncProgress); + } + + void _onSyncProgress() { + final conn = _syncWaitConn; + if (conn == null || _syncing(conn)) return; + conn.removeListener(_onSyncProgress); + _syncWaitConn = null; + if (!mounted) return; + setState(() => _waitingForSync = false); + _refresh(); + } + + bool _syncing(MeshCoreConnector c) => + c.isLoadingContacts || + c.isSyncingChannels || + c.isShowingQueuedMessageSyncProgress; + @override void dispose() { + _syncWaitConn?.removeListener(_onSyncProgress); _ssid.dispose(); _pwd.dispose(); _iata.dispose(); @@ -125,7 +156,8 @@ class _ObserverSettingsViewState extends State { ); _pwd.clear(); - await svc.refresh(); + // Flat save: don't re-pull the 84-frame broker pool, just the flats (#81). + await svc.refresh(includeBrokers: false); _seedFromConfig(svc.config); if (!mounted) return; setState(() => _saving = false); @@ -147,6 +179,17 @@ class _ObserverSettingsViewState extends State { @override Widget build(BuildContext context) { final svc = context.watch(); + if (_waitingForSync && !_seeded) { + return const Center( + child: Padding( + padding: EdgeInsets.all(24), + child: Text( + 'Waiting for the device sync to finish before reading Observer settings…', + textAlign: TextAlign.center, + ), + ), + ); + } if (!_seeded && _loading) { return const Center(child: CircularProgressIndicator()); } diff --git a/lib/screens/settings_screen.dart b/lib/screens/settings_screen.dart index 6236a7a..943b0c9 100644 --- a/lib/screens/settings_screen.dart +++ b/lib/screens/settings_screen.dart @@ -10,7 +10,7 @@ import '../connector/meshcore_protocol.dart'; import '../l10n/l10n.dart'; import '../models/radio_settings.dart'; import '../services/app_debug_log_service.dart'; -import '../services/observer_config_service.dart'; +import '../connector/observer_config_client.dart'; import '../helpers/snack_bar_builder.dart'; import 'settings/settings_shell.dart'; import 'settings/app_settings_view.dart'; @@ -73,10 +73,15 @@ class _SettingsScreenState extends State { List _categories(BuildContext context) { final l10n = context.l10n; - // Watch the connector so the list re-evaluates when the observer capability - // (device-info offband_caps) flips on connect/disconnect. - context.watch(); - final showObserver = context.read().supported; + // Rebuild ONLY when the observer gate flips — NOT on every connector update. + // A blanket context.watch here rebuilt the whole settings screen on every + // sync frame, thrashing the UI thread and slowing the channel sync (#81). + final showObserver = context.select( + (c) => ObserverConfigClient.supportsConfig( + firmwareVerCode: c.firmwareVerCode ?? 0, + offbandCaps: c.offbandCaps ?? 0, + ), + ); return [ SettingsCategory( icon: Icons.settings_input_antenna, diff --git a/lib/services/observer_config_service.dart b/lib/services/observer_config_service.dart index e696d4a..928c23f 100644 --- a/lib/services/observer_config_service.dart +++ b/lib/services/observer_config_service.dart @@ -158,7 +158,7 @@ class ObserverConfigService extends ChangeNotifier { /// Read the whole observer config from the device into [config]. On any /// failure sets [stale] so the UI never presents a half-read snapshot as live. - Future refresh() async { + Future refresh({bool includeBrokers = true}) async { try { final ssid = await getFlat('wifi.ssid'); final wifiEnabled = await getFlat('wifi.enabled'); @@ -167,7 +167,10 @@ class ObserverConfigService extends ChangeNotifier { final statusInterval = await getFlat('mqtt.status_interval'); final alwaysOn = await getFlat('display.always_on'); final rotation = await getFlat('display.rotation'); - final brokers = await getBrokers(); + // Skip the heavy broker dump (84 frames) when only flat settings changed + // (e.g. after a flat Save) — it must not re-flood BLE for a toggle (#81). + final brokers = includeBrokers ? await getBrokers() : null; + final keptBrokers = _config?.brokers ?? const []; // The broker pool loads independently of the flat settings: a broker-dump // failure (e.g. the firmware never sends BROKERS_END) must NOT blank @@ -178,13 +181,13 @@ class ObserverConfigService extends ChangeNotifier { iata: iata ?? '', statusInterval: int.tryParse(statusInterval ?? '') ?? 60, ), - brokers: brokers ?? const [], + brokers: includeBrokers ? (brokers ?? const []) : keptBrokers, display: DisplayConfig( alwaysOn: alwaysOn == '1', rotation: int.tryParse(rotation ?? '') ?? 0, ), ); - _brokersUnavailable = brokers == null; + if (includeBrokers) _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 diff --git a/test/screens/observer_settings_view_test.dart b/test/screens/observer_settings_view_test.dart index a32a8ae..132f8b8 100644 --- a/test/screens/observer_settings_view_test.dart +++ b/test/screens/observer_settings_view_test.dart @@ -16,6 +16,21 @@ import 'package:meshcore_open/services/observer_config_service.dart'; class _DummyConn extends MeshCoreConnector {} +class _SyncConn extends MeshCoreConnector { + _SyncConn({this.syncing = false}); + bool syncing; + @override + bool get isSyncingChannels => syncing; + @override + bool get isLoadingContacts => false; + @override + bool get isShowingQueuedMessageSyncProgress => false; + void finishSync() { + syncing = false; + notifyListeners(); + } +} + class _FakeSvc extends ObserverConfigService { _FakeSvc( this._cfg, { @@ -29,6 +44,7 @@ class _FakeSvc extends ObserverConfigService { final String? errorText; final bool brokersDown; final List> sets = []; + int refreshCalls = 0; @override bool get brokersUnavailable => brokersDown; @@ -41,7 +57,10 @@ class _FakeSvc extends ObserverConfigService { @override String? get lastError => errorText; @override - Future refresh() async {} + Future refresh({bool includeBrokers = true}) async { + refreshCalls++; + } + @override Future setFlat(String key, String value) async { sets.add(MapEntry(key, value)); @@ -64,8 +83,11 @@ ObserverConfig _cfg({String ssid = 'MyNet', int rotation = 0}) => Future _pump(WidgetTester tester, _FakeSvc fake) async { await tester.pumpWidget( - ChangeNotifierProvider.value( - value: fake, + MultiProvider( + providers: [ + ChangeNotifierProvider.value(value: _SyncConn()), + ChangeNotifierProvider.value(value: fake), + ], child: const MaterialApp(home: Scaffold(body: ObserverSettingsView())), ), ); @@ -190,4 +212,28 @@ void main() { await tester.pumpAndSettle(); expect(find.textContaining('Broker pool unavailable'), findsOneWidget); }); + + testWidgets('observer refresh waits for the device sync to finish (#81)', ( + tester, + ) async { + final conn = _SyncConn(syncing: true); + final fake = _FakeSvc(_cfg()); + await tester.pumpWidget( + MultiProvider( + providers: [ + ChangeNotifierProvider.value(value: conn), + ChangeNotifierProvider.value(value: fake), + ], + child: const MaterialApp(home: Scaffold(body: ObserverSettingsView())), + ), + ); + await tester.pumpAndSettle(); + + expect(fake.refreshCalls, 0, reason: 'must not refresh while syncing'); + expect(find.textContaining('Waiting for the device sync'), findsOneWidget); + + conn.finishSync(); + await tester.pumpAndSettle(); + expect(fake.refreshCalls, 1, reason: 'refresh once the sync settles'); + }); } diff --git a/test/services/observer_config_service_test.dart b/test/services/observer_config_service_test.dart index 0faca77..f299498 100644 --- a/test/services/observer_config_service_test.dart +++ b/test/services/observer_config_service_test.dart @@ -55,6 +55,7 @@ class _AutoConnector extends MeshCoreConnector { _AutoConnector({this.failKeys = const {}, this.failBrokers = false}); final Set failKeys; final bool failBrokers; + final List sent = []; final StreamController _frames = StreamController.broadcast(); @@ -71,6 +72,7 @@ class _AutoConnector extends MeshCoreConnector { String? channelSendQueueId, bool expectsGenericAck = false, }) async { + sent.add(data); final op = data[1]; if (op == ObserverConfigClient.opGet) { final key = utf8.decode(data.sublist(2, data.length - 1)); @@ -248,4 +250,26 @@ void main() { auto.closeStream(); }, ); + + // ---- #81: a flat refresh must not re-pull the broker dump ---- + test( + 'refresh(includeBrokers:false) skips the broker dump, keeps brokers', + () async { + final auto = _AutoConnector(); + final s = ObserverConfigService(auto); + int brokerReqs() => auto.sent + .where((f) => f.length > 1 && f[1] == ObserverConfigClient.opBrokers) + .length; + await s.refresh(); // full read sends one OCFG_BROKERS + expect(brokerReqs(), 1, reason: 'full refresh dumps the pool'); + await s.refresh(includeBrokers: false); // flat-only + expect( + brokerReqs(), + 1, + reason: 'includeBrokers:false must NOT send another OCFG_BROKERS', + ); + expect(s.config, isNotNull); + auto.closeStream(); + }, + ); }