diff --git a/lib/connector/meshcore_connector.dart b/lib/connector/meshcore_connector.dart index 369a6ca..9c1862a 100644 --- a/lib/connector/meshcore_connector.dart +++ b/lib/connector/meshcore_connector.dart @@ -5764,6 +5764,56 @@ class MeshCoreConnector extends ChangeNotifier { return physicsMax; } + /// Worst case the app allows *itself* to take fetching one message once the + /// radio already holds it: every `CMD_SYNC_NEXT_MESSAGE` attempt plus its + /// retries. Replies are pull-based, so this sits inside every CLI round trip. + /// + /// Derived from the retrieval constants rather than restated, so a command + /// timeout built on it cannot drift below the layer it depends on (#530). + int get messageRetrievalBudgetMs => + _queueSyncTimeoutMs * (_maxQueueSyncRetries + 1); + + /// Timeout for a repeater CLI command. + /// + /// [calculateTimeout] models **one-way delivery**: it mirrors the firmware's + /// `calcDirectTimeoutMillisFor`, whose job is to estimate when an outbound + /// packet should have been acknowledged. A CLI command is not that. It is a + /// request, an execution, and a reply, and the budget has to name all of it: + /// + /// 1. the outbound leg, which is what [calculateTimeout] is actually for; + /// 2. [cliReplyDelayMs], the fixed hold the repeater applies before it even + /// queues the reply; + /// 3. the reply's own transmission, a second packet the ACK formula never + /// modelled; + /// 4. [messageRetrievalBudgetMs], because the reply is not pushed to us. The + /// radio raises `MSG_WAITING` and we must ask for it. + /// + /// Command execution time is deliberately not modelled. Measured round trips + /// to a single repeater ranged from 1.65 s to 20.33 s, and the *same* verb + /// (`wifi on 30`) returned in both 2.31 s and 20.33 s, so execution cost is + /// not a per-verb constant that could be tabulated. The retrieval term is + /// what carries that tail. + /// + /// The reply leg uses physics only. The predictor is trained on direct-message + /// ACK latency, not on command round trips, so asking it about a reply leg + /// would be extrapolation (#534, #535). + int calculateCliTimeout({ + required int pathLength, + int messageBytes = maxFrameSize, + String? contactKey, + }) { + final outboundMs = calculateTimeout( + pathLength: pathLength, + messageBytes: messageBytes, + contactKey: contactKey, + ); + final replyLegMs = _physicsMaxTimeout( + pathLength, + _estimateAirtimeMs(messageBytes), + ); + return outboundMs + cliReplyDelayMs + replyLegMs + messageRetrievalBudgetMs; + } + /// Coalesces notifications during a bulk contact pull. /// /// Outside a pull this notifies immediately, preserving live-update diff --git a/lib/connector/meshcore_protocol.dart b/lib/connector/meshcore_protocol.dart index b8c6a7f..b43ba29 100644 --- a/lib/connector/meshcore_protocol.dart +++ b/lib/connector/meshcore_protocol.dart @@ -279,6 +279,13 @@ const int offbandFemLnaGet = 0x02; const int femLnaBypass = 0x00; const int femLnaEnabled = 0x01; +/// Fixed delay a repeater applies before queueing a CLI reply for transmit. +/// +/// Firmware `CLI_REPLY_DELAY_MILLIS` in `examples/simple_repeater/MyMesh.cpp`, +/// applied on both the direct and the flood reply path. It is unconditional, so +/// every CLI round trip pays it and any budget for one must include it. +const int cliReplyDelayMs = 600; + Uint8List buildOffbandFemLnaSetFrame(bool enabled) => Uint8List.fromList([ cmdOffbandFemLna, offbandFemLnaSet, diff --git a/lib/services/repeater_command_service.dart b/lib/services/repeater_command_service.dart index be074f8..f5b98e5 100644 --- a/lib/services/repeater_command_service.dart +++ b/lib/services/repeater_command_service.dart @@ -132,7 +132,7 @@ class RepeaterCommandService { final responseBytes = frame.length > maxFrameSize ? frame.length : maxFrameSize; - final timeoutMs = _connector.calculateTimeout( + final timeoutMs = _connector.calculateCliTimeout( pathLength: pathLengthValue, messageBytes: responseBytes, ); diff --git a/test/connector/cli_timeout_test.dart b/test/connector/cli_timeout_test.dart new file mode 100644 index 0000000..44f0052 --- /dev/null +++ b/test/connector/cli_timeout_test.dart @@ -0,0 +1,116 @@ +// #529 (epic #473): a repeater CLI command is a request-execute-reply exchange, +// but its timeout used calculateTimeout, which mirrors the firmware's +// calcDirectTimeoutMillisFor and models ONE-WAY delivery only. +// +// Measured against rpt-01: 27 commands, 12 replies, and 4 of those 12 arrived +// after the 4074 ms window had expired (6.17 s, 8.28 s, 15.52 s, 20.33 s; +// median 2.75 s). The same verb `wifi on 30` returned in both 2.31 s and +// 20.33 s, so the tail is not a per-command constant that could be tabulated. +// +// These tests pin the budget's construction and, just as importantly, pin that +// the direct-message ACK path was NOT changed. + +import 'package:flutter_test/flutter_test.dart'; +import 'package:meshcore_open/connector/meshcore_connector.dart'; +import 'package:meshcore_open/connector/meshcore_protocol.dart'; +import 'package:meshcore_open/storage/prefs_manager.dart'; +import 'package:shared_preferences/shared_preferences.dart'; + +void main() { + TestWidgetsFlutterBinding.ensureInitialized(); + + late MeshCoreConnector connector; + + setUp(() async { + SharedPreferences.setMockInitialValues({}); + PrefsManager.reset(); + await PrefsManager.initialize(); + connector = MeshCoreConnector(); + }); + + // With no SELF_INFO parsed there are no radio params, so _estimateAirtimeMs + // returns its 50 ms fallback and _physicsMaxTimeout(0, 50) is + // 500 + ((50 * 6) + 250) = 1050. Every expectation below is derived from + // that, not from a chosen number. + const fallbackPhysicsMax0Hop = 1050; + const retrievalBudget = 20000; // _queueSyncTimeoutMs 5000 * (3 retries + 1) + + group('retrieval budget', () { + test('is derived from the sync constants, not restated', () { + expect(connector.messageRetrievalBudgetMs, retrievalBudget); + }); + }); + + group('calculateCliTimeout', () { + test('sums outbound, repeater hold, reply leg and retrieval', () { + expect( + connector.calculateCliTimeout(pathLength: 0), + fallbackPhysicsMax0Hop + + cliReplyDelayMs + + fallbackPhysicsMax0Hop + + retrievalBudget, + ); + }); + + test('is never below the retrieval budget it depends on (#530)', () { + for (final path in [-1, 0, 1, 2, 5]) { + expect( + connector.calculateCliTimeout(pathLength: path), + greaterThanOrEqualTo(connector.messageRetrievalBudgetMs), + reason: 'path $path must not expire before the app can fetch a reply', + ); + } + }); + + test('is always larger than the one-way delivery estimate', () { + for (final path in [-1, 0, 1, 2, 5]) { + expect( + connector.calculateCliTimeout(pathLength: path), + greaterThan(connector.calculateTimeout(pathLength: path)), + reason: 'path $path', + ); + } + }); + + test('covers the slowest round trip actually measured (20.33 s)', () { + // The observed worst case on the owner's radio. The budget has to clear + // it even in this test's degraded no-radio-params state, where the + // physics terms are at their smallest. + expect(connector.calculateCliTimeout(pathLength: 0), greaterThan(20330)); + }); + + test('grows with path length', () { + final direct = connector.calculateCliTimeout(pathLength: 0); + final oneHop = connector.calculateCliTimeout(pathLength: 1); + final twoHop = connector.calculateCliTimeout(pathLength: 2); + expect(oneHop, greaterThan(direct)); + expect(twoHop, greaterThan(oneHop)); + }); + }); + + group('the direct-message ACK path is unchanged', () { + // Negative test. #529 must not move DM ACK behaviour, which legitimately + // uses the one-way formula. If this breaks, the fix has leaked. + test('calculateTimeout still returns the firmware formula', () { + expect(connector.calculateTimeout(pathLength: 0), fallbackPhysicsMax0Hop); + }); + + test('calculateTimeout does not include the CLI reply delay', () { + expect( + connector.calculateTimeout(pathLength: 0), + lessThan(cliReplyDelayMs + fallbackPhysicsMax0Hop), + ); + }); + + test('calculateTimeout stays well under the retrieval budget', () { + // Not a requirement, an observation that pins the #530 mismatch: the DM + // path is allowed to be shorter than retrieval because a DM ACK does not + // depend on fetching a queued message. If this ever flips, the invariant + // in calculateCliTimeout needs revisiting rather than silently holding. + expect( + connector.calculateTimeout(pathLength: 0), + lessThan(connector.messageRetrievalBudgetMs), + ); + }); + }); +}