Skip to content

Commit f22dee4

Browse files
rafael2kclaude
andcommitted
arq/fsm: anchor the CALL retry to PTT-OFF, not to enqueue
fsm_calling had no ARQ_EV_TX_COMPLETE handler, so the 8 s retry deadline was set when the CALL was QUEUED. A CALL spends ~3.8 s modulating (DATAC16), and the peer cannot even start its ACCEPT until our PTT drops — its own ACCEPT then takes another ~3.8 s. The retransmission therefore fired at t~8.0 s while the ACCEPT was still arriving at t~8.1 s: a collision on essentially every connect, costing a fourth transmission on a channel that had lost nothing. fsm_accepting already anchors ACCEPT exactly this way (its TX_COMPLETE resets the RX window from PTT-OFF); CALLING never got the same treatment. Measured with tests/sim/connect_bench, 24 seeds per point: PER connects frames censored time 0.00 24/24 -> 24/24 4.0 -> 3.0 13.1 s -> 13.1 s 0.10 24/24 -> 24/24 4.6 -> 3.9 19.5 s -> 22.2 s 0.20 23/24 -> 24/24 5.4 -> 4.6 32.3 s -> 28.7 s 0.30 21/24 -> 22/24 5.9 -> 5.3 49.4 s -> 48.2 s 0.50 10/24 -> 15/24 6.5 -> 6.9 121.8 s -> 101.5 s A clean connect now costs three transmissions for a three-frame handshake, which is the point. Reliability improves where it matters most: at PER 0.5 the success rate goes 10/24 to 15/24 and censored time drops 20 s. Reported honestly: PER 0.10 censored time is ~2.7 s worse. The buggy early retransmission was accidentally acting as a fast retry, so a genuinely lost CALL now waits the full interval. I checked whether the interval should shrink to compensate and it should not — at 6.5 s and 5.5 s the success rate FALLS (12/12 -> 11/12 at PER 0.2, and worse at 0.5), because the four retry slots are consumed sooner. 8 s stays. The censored column counts a failure as the 180 s cap. Means over successes alone are survivor-biased: when a change makes previously-hopeless connects succeed, those slow successes inflate the success-only mean while the link has actually improved. An earlier 12-seed run showed a PER 0.20 "regression" that reversed at 24 seeds — small-sample means on this metric are not trustworthy. Test verified to fail without the fix. Full C suite green (36 FSM tests). Integration: one failure observed when the suite was started immediately after a full parallel rebuild, then four consecutive passes (three at 203.6 s identical); treated as load-related flake, not reproducible. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 parent e4e6da5 commit f22dee4

3 files changed

Lines changed: 65 additions & 7 deletions

File tree

datalink_arq/arq_fsm.c

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -839,6 +839,23 @@ static void fsm_calling(arq_session_t *sess, const arq_event_t *ev)
839839
}
840840
break;
841841

842+
case ARQ_EV_TX_COMPLETE:
843+
/* Re-anchor the retry clock to PTT-OFF, not to when the frame was
844+
* queued.
845+
*
846+
* A CALL spends ~3.8 s modulating (DATAC16), and the retry deadline
847+
* was set when the frame was ENQUEUED — so that airtime came straight
848+
* out of the 8 s retry interval. The peer cannot even begin its ACCEPT
849+
* until our PTT drops, and its own ACCEPT takes another ~3.8 s, so the
850+
* retransmission fired at t~8.0 s while the ACCEPT was still arriving
851+
* at t~8.1 s: a collision on essentially every connect, costing a
852+
* fourth transmission on a channel that lost nothing.
853+
*
854+
* fsm_accepting already anchors ACCEPT this way (see its TX_COMPLETE);
855+
* CALLING never got the same treatment. */
856+
sess->deadline_ms = deadline_from_s(arq_protocol_call_interval_s());
857+
break;
858+
842859
case ARQ_EV_TIMER_RETRY:
843860
if (sess->tx_retries_left > 0)
844861
{

tests/datalink_arq/test_arq_fsm.c

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -932,6 +932,39 @@ void test_irs_mirror_resets_toward_floor_on_silence(void)
932932
TEST_ASSERT_EQUAL_INT(MERCURY_MODE_MFSK, sess.peer_tx_mode); /* stepped back to the floor */
933933
}
934934

935+
/* The CALL retry clock must start when the burst leaves the air, not when it
936+
* was queued. A CALL spends ~3.8 s modulating, so anchoring at enqueue spent
937+
* that airtime out of the 8 s retry interval and fired the retransmission at
938+
* t~8.0 s — while the peer's ACCEPT was still arriving at t~8.1 s. That cost
939+
* a fourth transmission on a channel that had lost nothing (measured: 4.0
940+
* frames for a 3-frame handshake on a clean link). */
941+
void test_calling_reanchors_retry_on_tx_complete(void)
942+
{
943+
arq_event_t ev = make_event(ARQ_EV_APP_LISTEN);
944+
arq_fsm_dispatch(&sess, &ev);
945+
946+
mock_set_uptime_ms(1000);
947+
ev = make_event(ARQ_EV_APP_CONNECT);
948+
strncpy(ev.remote_call, "DST1", CALLSIGN_MAX_SIZE);
949+
arq_fsm_dispatch(&sess, &ev);
950+
TEST_ASSERT_EQUAL_INT(ARQ_CONN_CALLING, sess.conn_state);
951+
uint64_t deadline_at_enqueue = sess.deadline_ms;
952+
953+
/* The burst occupies the channel, then PTT drops. */
954+
mock_set_uptime_ms(1000 + 3840);
955+
ev = make_event(ARQ_EV_TX_COMPLETE);
956+
arq_fsm_dispatch(&sess, &ev);
957+
958+
TEST_ASSERT_EQUAL_INT(ARQ_CONN_CALLING, sess.conn_state);
959+
TEST_ASSERT_TRUE_MESSAGE(sess.deadline_ms > deadline_at_enqueue,
960+
"CALL retry still anchored at enqueue: it will collide with the ACCEPT");
961+
/* A full retry interval measured from PTT-OFF. */
962+
TEST_ASSERT_UINT64_WITHIN(50,
963+
(uint64_t)(1000 + 3840) +
964+
(uint64_t)(arq_protocol_call_interval_s() * 1000.0f),
965+
sess.deadline_ms);
966+
}
967+
935968
int main(void)
936969
{
937970
UNITY_BEGIN();
@@ -972,5 +1005,6 @@ int main(void)
9721005
RUN_TEST(test_irs_mirror_climbs_with_peer);
9731006
RUN_TEST(test_irs_mirror_steps_down_on_duplicate);
9741007
RUN_TEST(test_irs_mirror_resets_toward_floor_on_silence);
1008+
RUN_TEST(test_calling_reanchors_retry_on_tx_complete);
9751009
return UNITY_END();
9761010
}

tests/sim/connect_bench.c

Lines changed: 14 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -76,31 +76,38 @@ int main(int argc, char **argv)
7676
int trials = (argc > 1) ? atoi(argv[1]) : 8;
7777

7878
printf("Connect latency (virtual time, %d seeds per point)\n\n", trials);
79-
printf("%8s %9s %12s %12s %10s\n",
80-
"PER", "connects", "caller(ms)", "callee(ms)", "frames");
79+
printf("%8s %9s %12s %12s %10s %12s\n",
80+
"PER", "connects", "caller(ms)", "callee(ms)", "frames", "censored");
8181

8282
const double pers[] = { 0.0, 0.1, 0.2, 0.3, 0.5 };
8383
for (unsigned p = 0; p < sizeof pers / sizeof pers[0]; p++)
8484
{
8585
unsigned ok = 0, fr = 0;
86-
uint64_t sum_caller = 0, sum_callee = 0;
86+
uint64_t sum_caller = 0, sum_callee = 0, sum_censored = 0;
8787
for (int t = 0; t < trials; t++)
8888
{
8989
run_t r = one_connect(pers[p], 1000u + (uint64_t)t * 7919u);
90+
/* Censored mean: a failure counts as the cap, so improving the
91+
* success rate cannot masquerade as a latency regression. Means
92+
* over successes ALONE are survivor-biased — when a change makes
93+
* previously-hopeless connects succeed, those slow successes pull
94+
* the success-only mean up while the link actually got better. */
95+
sum_censored += r.ok ? r.callee_ms : CONNECT_CAP_MS;
9096
if (!r.ok) continue;
9197
ok++;
9298
sum_caller += r.caller_ms;
9399
sum_callee += r.callee_ms;
94100
fr += r.frames_a + r.frames_b;
95101
}
96102
if (ok)
97-
printf("%8.2f %6u/%-3d %12.0f %12.0f %10.1f\n",
103+
printf("%8.2f %6u/%-3d %12.0f %12.0f %10.1f %12.0f\n",
98104
pers[p], ok, trials,
99105
(double)sum_caller / ok, (double)sum_callee / ok,
100-
(double)fr / ok);
106+
(double)fr / ok, (double)sum_censored / trials);
101107
else
102-
printf("%8.2f %6u/%-3d %12s %12s %10s\n",
103-
pers[p], ok, trials, "-", "-", "-");
108+
printf("%8.2f %6u/%-3d %12s %12s %10s %12.0f\n",
109+
pers[p], ok, trials, "-", "-", "-",
110+
(double)sum_censored / trials);
104111
fflush(stdout);
105112
}
106113
return 0;

0 commit comments

Comments
 (0)