Partnered control functions are evicted by address-claim roll-calls, and restored CFs are immediately re-evicted

[Arjan-Woltjer]

Describe the bug

Two related defects in the address-claim roll-call path cause a PartneredControlFunction to be evicted from controlFunctionTable while a VT or TC client is actively connected to it, and then to be re-evicted on every subsequent roll-call. The practical result is that the client goes permanently deaf to a partner that never stopped transmitting.

Both are present on current main (checked today against isobus/src/can_network_manager.cpp).

This is adjacent to #584 but distinct. #584 covers the initial adoption of an already-active partner in update_new_partners() (I have a fix for that one in AgIsoStack-Arduino#16). Even with that fixed, the two defects below still bite, because they recur for the lifetime of the connection rather than only at bind time.

---

1. prune_inactive_control_functions() prunes Partnered CFs

Only Internal control functions are exempt:

auto controlFunction = tableEntry;
if (ControlFunction::Type::Internal != controlFunction->get_type())
{
    inactiveControlFunctions.push_back(controlFunction);
    LOG_INFO("[NM]: Control function with address %u and NAME %016llx is now offline on channel %u.", ...);
    tableEntry = nullptr;
    controlFunction->address = NULL_CAN_ADDRESS;
}

Any node broadcasting a PGN 60928 (Address Claimed) request clears claimedAddressSinceLastAddressClaimRequest on every tracked CF and starts the 755 ms MAX_ADDRESS_CLAIM_RESOLUTION_TIME timer. A partner that does not re-announce inside that window is evicted — out from under a live, working client session.

Whether it re-announces in time is largely luck. In our captures the same terminal survived one roll-call and not the next.

Why eviction is fatal rather than cosmetic. Once tableEntry is nulled, message.get_source_control_function() no longer resolves for that address, and process_can_message_for_global_and_partner_callbacks() gates global dispatch on exactly that:

if ((nullptr == messageDestination) &&
    ((nullptr != messageSource) || ...))

So every subsequent broadcast from that address is silently dropped before reaching any global PGN callback. VirtualTerminalClient::process_rx_message is registered as a global callback, so the VT client stops seeing VT Status messages and times out on VT_STATUS_TIMEOUT_MS. TaskControllerClient sits in WaitForServerStatusMessage, which has no timeout at all and therefore cannot recover under any circumstances.

The failure looks exactly like the terminal going quiet. It isn't — the frames are still on the wire.

2. update_address_table() restores a pruned CF without restoring its liveness flag

if (targetControlFunction != nullptr)
{
    targetControlFunction->claimedAddressSinceLastAddressClaimRequest = true;
}
else
{
    // Look through all inactive CFs, maybe one of them has freshly claimed the address
    ...
    if (result != inactiveControlFunctions.end())
    {
        targetControlFunction = *result;
        LOG_DEBUG("[NM]: %s CF '%016llx' is now active at address '%d' on channel '%d'.", ...);
        process_control_function_state_change_callback(targetControlFunction, ControlFunctionState::Online);
    }
}

The restore branch never sets claimedAddressSinceLastAddressClaimRequest, so a CF that has been pruned once returns to the table already eligible for pruning again on the next roll-call. That is what turns a transient eviction into a self-sustaining cycle: we observed unbroken is now offline → has claimed address → is now offline churn every 1–2 seconds, running for 300+ seconds, with the VT and TC dead throughout.

---

Supporting Documentation

Found on a real bus: New Holland tractor, Ag Leader InCommand 1200 as VT/TC, plus the tractor's own terminal and the usual ECU population. Our own guidance-PGN traffic counter climbed steadily throughout every failure, so this was not a receive stall.

Counting evictions per window, before and after exempting Partnered CFs:

Window	Evictions	Partner lost?
single VT, quiet bus	0	— no prunes ran
single VT, prunes running	23	no (survived on timing)
two VTs	62	yes
one VT, busy bus	6	yes — died ~3 s after connect
after the fix, same config, 800 s	235	no — zero losses

Before the fix, the record for a held VT connection on that bus was roughly three seconds. After it, VT and TC stayed up together for 21+ minutes, including through a second terminal being powered on mid-session and enumerating the bus — an event that had previously killed a healthy session within about four seconds.

Environment

• OS: bare metal, Teensy 4.1 (i.MX RT1062)
• Compiler: arm-none-eabi-gcc (PlatformIO teensy platform), C++17
• CAN driver: FlexCAN_T4 via FlexCANT4Plugin
• Library: AgIsoStack-Arduino 0.1.5; both defects re-verified against AgIsoStack-plus-plus main before filing

Suggested fix

For (1), exempt Partnered alongside Internal. A partner is a CF the application explicitly bound to and is actively conversing with; a bus roll-call should not be able to evict it out from under a live session, and genuine partner loss is already detected at the right layer by each client's own status timeout. For (2), set the flag when restoring — by the time a CF is being restored it has just announced, so the flag is true by definition.

Happy to open a PR with both, plus a regression test if you can point me at where you would want it. I have deliberately kept these two changes separate from #584's so each stays attributable.

[Arjan-Woltjer]

Correcting part of my own report above, having since read your test suite properly.

CoreTest.InvalidatingControlFunctions already covers defect (1) explicitly, and asserts the current behaviour for a partnered control function:

auto testPartner = test_helpers::force_claim_partnered_control_function(0x79, 0);
...
// Request the address claim PGN again
...
// Now, if we wait a while, that partner didn't claim again, so it should be invalid.
time_source.update_for_ms(2000);
CANNetworkManager::CANNetwork.update();

EXPECT_FALSE(testPartner->get_address_valid());
EXPECT_TRUE(wasTestStateCallbackHit);
EXPECT_EQ(testControlFunctionState, ControlFunctionState::Offline);

That is a named, deliberate test, not an accident, so I was wrong to present defect (1) flatly as a bug. It is a design decision, and it's yours to make. I've left it out of #718, which now contains only defect (2) — the restore path, which that test never exercises and which I think is unambiguous.

For what it's worth, the design as tested is defensible: ISO 11783-5 requires a CF to answer a Request for Address Claimed, so a partner that stays silent through one arguably is gone. What I'd offer against it is not the principle but the margin and the consequence:

• The window is tight against real-world bus behaviour. MAX_ADDRESS_CLAIM_RESOLUTION_TIME is 755 ms, and every node on the bus is answering the same broadcast with contention. Our captures show the same terminal winning that race on one roll-call and losing it on the next, with bus load as the apparent variable — 23 evictions in one healthy window with no partner loss, then loss after 6 in a busier one.
• Roll-calls can be far more frequent than the design seems to assume. That bus issued them roughly every 3 seconds — 235 prune cycles in 800 seconds. At that rate the question stops being "will we ever lose the race" and becomes "how soon".
• The consequence is disproportionate and, in practice, terminal. Losing one race nulls the table entry, which silently stops all global-callback dispatch from that address. VirtualTerminalClient then trips VT_STATUS_TIMEOUT_MS, and TaskControllerClient sits in WaitForServerStatusMessage, which has no timeout at all and so cannot self-recover under any circumstance. A single missed answer became a dead session needing a physical power cycle.

Defect (2) alone may substantially address that, which is another reason to take it separately: with the restore path crediting the flag, a partner that re-announces gets durably restored rather than immediately re-pruned, so the failure should degrade from permanent to transient. If you'd rather stop there and leave the pruning policy untouched, that's a perfectly coherent outcome from my side.

If you would consider changing the policy, the options I can see are exempting Partnered while a client is actively bound; giving partners a second window or a targeted Request for Address Claimed before eviction; or leaving eviction alone but making the clients recover from it. I'm happy to prepare whichever you prefer, including updating InvalidatingControlFunctions to match — but I didn't want to rewrite an intentional test to suit my own diagnosis without asking first.