fix(#529): give repeater CLI commands their own timeout budget
The CLI timeout used calculateTimeout, which mirrors the firmware's
calcDirectTimeoutMillisFor and estimates ONE-WAY delivery. A CLI command
is a request, an execution and a reply, so the budget was structurally
short.
Measured against rpt-01 on 910.525 MHz / SF7 / BW 62.5k / CR 4:5, where
the old window was 4074 ms: 27 commands sent, 12 replies matched, and 4
of those 12 arrived after the client had already given up, at 6.17 s,
8.28 s, 15.52 s and 20.33 s against a median of 2.75 s.
calculateCliTimeout sums four terms, each with a source rather than a
chosen value:
outbound leg calculateTimeout, which is what it actually models
cliReplyDelayMs 600, firmware CLI_REPLY_DELAY_MILLIS, unconditional
reply leg the reply is a second packet the ACK formula omits
retrieval budget replies are pull-based; the radio raises MSG_WAITING
and the app must ask, granting itself 5000 ms per
attempt across 3 retries
The retrieval term is derived from the sync constants rather than
restated, so the command timeout cannot drift below the layer it depends
on. That is the #530 invariant holding by construction, not by two
numbers being maintained in agreement.
Execution time is deliberately not modelled. The same verb, wifi on 30,
returned in both 2.31 s and 20.33 s, so it is not a per-command constant
that could be tabulated. The retrieval term carries that tail.
The reply leg uses physics only. The predictor is trained on
direct-message ACK latency, so asking it about a CLI reply leg would be
extrapolation; that is #534 and #535, not this change.
Tests cover the construction, the never-below-retrieval invariant, growth
with path length, coverage of the 20.33 s worst case actually observed,
and negatively that the direct-message ACK path is untouched.
Part of epic #473. Does not change the reported duration string (#531)
or the stale-prefix fallback (#532), and does nothing for commands that
draw no reply at all (#541).
release/1.5.0-beta.1
parent
9d690b2051
commit
83f6360f7e
@ -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),
|
||||
);
|
||||
});
|
||||
});
|
||||
}
|
||||
Loading…
Reference in new issue