From 83f7ef5061a209e30264d4b7ab475e7f36aafcec Mon Sep 17 00:00:00 2001 From: Dietmar Borgards <2646931+dborgards@users.noreply.github.com> Date: Sat, 3 Oct 2026 17:36:49 +0200 Subject: [PATCH 1/3] fix(canopen): restart the block-download request timer per confirmed segment, ignore heartbeats with the reserved bit 7 C12: the block-download client restarted its request timer only when the server answered, so a sub-block that took longer to send than SdoTimeout (127 segments at 10 kbit/s) timed out before its ACK could be asked for, and the healthy server's ACK was then discarded. The timer now restarts at every confirmed segment; after the last one it measures the wait for the ACK. #266: a heartbeat that sets the reserved bit 7 (a bystander's guarding reply, or a malformed frame) is no longer reported as HeartbeatReceived; it still counts as a sign of life. Co-Authored-By: Claude Sonnet 5.5 --- src/CanKit.Pro.CANopen/CanOpenEvents.cs | 2 +- .../CanOpenNode.SdoBlock.cs | 19 ++++++- src/CanKit.Pro.CANopen/CanOpenNode.cs | 21 ++++++-- src/CanKit.Pro.CANopen/CanOpenNodeOptions.cs | 4 +- .../CanOpenSdoClientSendFailureTests.cs | 52 +++++++++++++++++++ .../CANopen/CanOpenSdoCorrectnessTests.cs | 5 +- 6 files changed, 94 insertions(+), 9 deletions(-) diff --git a/src/CanKit.Pro.CANopen/CanOpenEvents.cs b/src/CanKit.Pro.CANopen/CanOpenEvents.cs index d88b9d8..1f8977c 100644 --- a/src/CanKit.Pro.CANopen/CanOpenEvents.cs +++ b/src/CanKit.Pro.CANopen/CanOpenEvents.cs @@ -15,7 +15,7 @@ public sealed class HeartbeatReceivedEventArgs : EventArgs /// Reported NMT state of the producer. is used /// for the CiA 301 §7.2.8.3.2 bootup frame (data[0] == 0x00). A frame whose state - /// byte is one CiA 301 reserves is not reported. + /// byte is one CiA 301 reserves, or that sets the reserved bit 7, is not reported. public NmtState State { get; } /// UTC timestamp captured when the frame was processed on the actor loop. diff --git a/src/CanKit.Pro.CANopen/CanOpenNode.SdoBlock.cs b/src/CanKit.Pro.CANopen/CanOpenNode.SdoBlock.cs index 682913e..a3d5123 100644 --- a/src/CanKit.Pro.CANopen/CanOpenNode.SdoBlock.cs +++ b/src/CanKit.Pro.CANopen/CanOpenNode.SdoBlock.cs @@ -587,8 +587,23 @@ private void SendNextBlockDownloadSubBlock(SdoBlockClientSession session) session.Phase = SdoBlockClientPhase.AwaitSubBlockAck; // Every way the transfer ends completes its task (abort, peer abort, cancel, timeout, // dispose), and that is readable from the sending task; the session tables are not. - _ = SendOrderedControlFrames(TrackSdoBlockClientSend(session), - () => session.Tcs.Task.IsCompleted, frames.ToArray()); + var completed = TrackSdoBlockClientSend(session); + var sendId = session.LatestSendId; + // The request timer measures how long the server has been silent, and while a sub-block + // is still going out the server has nothing to answer yet: it is restarted at every + // confirmed segment, so a long sub-block on a slow bus (127 segments at 10 kbit/s is + // over a second) does not use up the timer before the ACK it waits for can be sent (C12). + // After the last segment the timer runs for the ACK alone. + _ = SendOrderedControlFrames(completed, () => session.Tcs.Task.IsCompleted, + () => PostSdoClientSendOutcome(() => + { + if (!_sdoBlockClients.TryGetValue(session.ServerNodeId, out var live) + || !ReferenceEquals(live, session) + || sendId != session.LatestSendId + || session.TimedOut) + return; + RearmBlockClient(session, session.ServerNodeId); + }), frames.ToArray()); } private void SendBlockDownloadEnd(SdoBlockClientSession session) diff --git a/src/CanKit.Pro.CANopen/CanOpenNode.cs b/src/CanKit.Pro.CANopen/CanOpenNode.cs index 45cddc4..6738c3c 100644 --- a/src/CanKit.Pro.CANopen/CanOpenNode.cs +++ b/src/CanKit.Pro.CANopen/CanOpenNode.cs @@ -1361,9 +1361,11 @@ private void HandleHeartbeat(uint cobId, byte[] data) // A reserved state byte still shows the producer alive and is still recorded for the NMT // master, but it is not reported: as Initializing it would be indistinguishable from a // boot-up and read as a restart that did not happen (#255). - // The boot-up is the byte 0x00; 0x80 is state 0 with the guarding toggle set, which no - // producer sends as a heartbeat. - if (TryDecodeHeartbeatState(stateByte, out var state) && (stateByte != 0 || data[0] == 0)) + // Bit 7 is reserved and always 0 in a heartbeat (§7.2.8.3.2.2), so a frame that sets it is + // not one: a bystander's guarding reply (toggle) or a malformed frame. It is not reported, + // which also keeps 0x80 (state 0 with the toggle set) from reading as a boot-up (#266). + // It still counts as a sign of life below, as before. + if ((data[0] & 0x80) == 0 && TryDecodeHeartbeatState(stateByte, out var state)) RaiseHeartbeatReceived(producer, state, DateTime.UtcNow); NoteSlaveNmtState(producer, stateByte); _heartbeatConsumer.NoteReceived(producer); @@ -2427,6 +2429,15 @@ private Task SendOrderedControlFrames(Action? onSend /// private Task SendOrderedControlFrames(Action? onSendCompleted, Func? shouldStop, params (uint CobId, byte[] Payload)[] frames) + => SendOrderedControlFrames(onSendCompleted, shouldStop, onFrameConfirmed: null, frames); + + /// + /// As above. , when given, is called on the sending task + /// after each frame that was confirmed, so the owner of a long batch can tell progress from + /// silence. It must be cheap and thread-safe. + /// + private Task SendOrderedControlFrames(Action? onSendCompleted, + Func? shouldStop, Action? onFrameConfirmed, params (uint CobId, byte[] Payload)[] frames) { return Task.Run(async () => { @@ -2451,6 +2462,10 @@ private Task SendOrderedControlFrames(Action? onSend return; } } + else + { + onFrameConfirmed?.Invoke(); + } } catch (Exception ex) { diff --git a/src/CanKit.Pro.CANopen/CanOpenNodeOptions.cs b/src/CanKit.Pro.CANopen/CanOpenNodeOptions.cs index 0811fa8..3412f7c 100644 --- a/src/CanKit.Pro.CANopen/CanOpenNodeOptions.cs +++ b/src/CanKit.Pro.CANopen/CanOpenNodeOptions.cs @@ -15,7 +15,9 @@ namespace CanKit.Pro.CANopen; public sealed class CanOpenNodeOptions { /// Client-side SDO transfer timeout, applied to every request (initiate as well as - /// each segment ack). CiA 301 does not specify a fixed value; one second matches common + /// each segment ack). In a block download it also restarts at every confirmed segment of a + /// sub-block, so it measures the server's silence and not the time a sub-block takes to send. + /// CiA 301 does not specify a fixed value; one second matches common /// production tooling and is aggressive enough for tests on a virtual bus. public TimeSpan SdoTimeout { get; init; } = TimeSpan.FromSeconds(1); diff --git a/tests/CanKit.Pro.Tests/TestCases/CANopen/CanOpenSdoClientSendFailureTests.cs b/tests/CanKit.Pro.Tests/TestCases/CANopen/CanOpenSdoClientSendFailureTests.cs index 99d1122..10128a4 100644 --- a/tests/CanKit.Pro.Tests/TestCases/CANopen/CanOpenSdoClientSendFailureTests.cs +++ b/tests/CanKit.Pro.Tests/TestCases/CANopen/CanOpenSdoClientSendFailureTests.cs @@ -334,6 +334,58 @@ public async Task A_Lost_Echo_Of_An_Answered_Send_Does_Not_Fail_The_Upload() data.Should().Equal(1, 2, 3, 4); } + // C12: the request timer measures how long the server has been silent, and during a sub-block + // the server has nothing to answer yet. A sub-block that takes longer to send than SdoTimeout + // (127 segments at 10 kbit/s) used to use the timer up before the ACK could even be asked + // for, and the timeout that fired during the batch discarded the healthy server's ACK. On a + // clock the test drives: three segments, each confirmed 600 ms after the one before, against + // a 1 s timeout. No single gap reaches the timeout; the sum of them does. + [Fact] + public async Task A_SubBlock_Longer_Than_The_Sdo_Timeout_Does_Not_Time_Out_Before_Its_Ack() + { + using var bus = ControllableBus.DeferredEchoCapable($"canopen-sdo-block-long-{Guid.NewGuid():N}"); + var toServer = new List(); + bus.OnTransmitting = frame => + { + if ((uint)frame.ID == CanOpenCobId.SdoRx(0x11)) lock (toServer) toServer.Add(frame.Data.ToArray()); + }; + var clock = new ManualTimeSource(); + using var client = new CanOpenNode(new CanBusService(bus), 0x7F, + new CanOpenNodeOptions { SdoTimeout = TimeSpan.FromSeconds(1) }, ownsService: true, clock); + await bus.DeferredEchoes.WaitForEnqueuedAsync(1, ShortTimeout); // the boot-up + bus.DeferredEchoes.ReleaseAll(); + + var payload = Enumerable.Range(0, 21).Select(i => (byte)(0x30 + i)).ToArray(); // three segments + var download = client.SdoDownloadAsync(0x11, 0x1000, 0x00, payload, SdoTransferMode.Block); + await bus.DeferredEchoes.WaitForEnqueuedAsync(2, ShortTimeout); // the initiate + bus.DeferredEchoes.ReleaseNext(); + bus.RaiseObserved(CanFrame.Classic(unchecked((int)CanOpenCobId.SdoTx(0x11)), + SdoBlockFrames.BuildBlockDownloadInitResponse(0x1000, 0x00, serverCrcSupported: false, blockSize: 3)), + isEcho: false); + + for (var segment = 1; segment <= 3; segment++) + { + await bus.DeferredEchoes.WaitForEnqueuedAsync(2 + segment, ShortTimeout); + _ = client.State; // two actor round-trips: every timer the last step armed is armed + _ = client.State; + clock.Advance(TimeSpan.FromMilliseconds(600)); + _ = client.State; + _ = client.State; + bus.DeferredEchoes.ReleaseNext(); + } + + // 1.8 s of the virtual clock have passed since the initiate response, 1 s being the timeout. + bus.RaiseObserved(CanFrame.Classic(unchecked((int)CanOpenCobId.SdoTx(0x11)), + SdoBlockFrames.BuildSubBlockAck(SdoBlockFrames.ScsBlockDownloadSubBlockAck, lastAckedSeq: 3, nextBlockSize: 3)), + isEcho: false); + await bus.DeferredEchoes.WaitForEnqueuedAsync(6, ShortTimeout); // the end request + + download.IsCompleted.Should().BeFalse("the transfer is waiting for the end response, not timed out"); + byte[] last; + lock (toServer) last = toServer[^1]; + last[0].Should().Be(SdoBlockFrames.CcsBlockDownloadEndBase, "three full segments leave no unused byte"); + } + [Fact] public async Task A_Block_Timeout_With_Its_Send_Confirmed_Ends_In_The_Timeout_Abort() { diff --git a/tests/CanKit.Pro.Tests/TestCases/CANopen/CanOpenSdoCorrectnessTests.cs b/tests/CanKit.Pro.Tests/TestCases/CANopen/CanOpenSdoCorrectnessTests.cs index e1c1242..929f2c6 100644 --- a/tests/CanKit.Pro.Tests/TestCases/CANopen/CanOpenSdoCorrectnessTests.cs +++ b/tests/CanKit.Pro.Tests/TestCases/CANopen/CanOpenSdoCorrectnessTests.cs @@ -1425,7 +1425,8 @@ public async Task Sdo_Client_Upload_With_The_Announced_Length_Reports_Nothing() [InlineData(0x01)] [InlineData(0x03)] [InlineData(0x80)] // state 0 with the toggle bit set is not the boot-up byte - public async Task Heartbeat_With_A_Reserved_State_Byte_Is_Not_Reported_As_Bootup(byte reserved) + [InlineData(0x84)] // bit 7 is reserved in a heartbeat (#266) + public async Task Heartbeat_With_A_Reserved_State_Byte_Is_Not_Reported(byte reserved) { var session = NewSession(); using var busA = Open(session, 1); @@ -1444,7 +1445,7 @@ public async Task Heartbeat_With_A_Reserved_State_Byte_Is_Not_Reported_As_Bootup Send(rawBus, CanOpenCobId.HeartbeatBase + 0x11, new byte[] { 0x05 }); await last.Task.WithTimeoutAsync(ShortTimeout); - seen.Should().NotContain(NmtState.Initializing); + seen.ToArray().Should().Equal(new[] { NmtState.Operational }, "only the valid heartbeat is reported"); } [Theory] From 650957c2f2f1414eb51521a752594666b809be09 Mon Sep 17 00:00:00 2001 From: Dietmar Borgards <2646931+dborgards@users.noreply.github.com> Date: Sat, 3 Oct 2026 17:52:13 +0200 Subject: [PATCH 2/3] test(canopen): a late segment confirmation does not revive a timed-out block download; drop a redundant guard Co-Authored-By: Claude Sonnet 5.5 --- .../CanOpenNode.SdoBlock.cs | 3 +- .../CanOpenSdoClientSendFailureTests.cs | 37 +++++++++++++++++++ 2 files changed, 38 insertions(+), 2 deletions(-) diff --git a/src/CanKit.Pro.CANopen/CanOpenNode.SdoBlock.cs b/src/CanKit.Pro.CANopen/CanOpenNode.SdoBlock.cs index a3d5123..9dbf52f 100644 --- a/src/CanKit.Pro.CANopen/CanOpenNode.SdoBlock.cs +++ b/src/CanKit.Pro.CANopen/CanOpenNode.SdoBlock.cs @@ -599,8 +599,7 @@ private void SendNextBlockDownloadSubBlock(SdoBlockClientSession session) { if (!_sdoBlockClients.TryGetValue(session.ServerNodeId, out var live) || !ReferenceEquals(live, session) - || sendId != session.LatestSendId - || session.TimedOut) + || sendId != session.LatestSendId) return; RearmBlockClient(session, session.ServerNodeId); }), frames.ToArray()); diff --git a/tests/CanKit.Pro.Tests/TestCases/CANopen/CanOpenSdoClientSendFailureTests.cs b/tests/CanKit.Pro.Tests/TestCases/CANopen/CanOpenSdoClientSendFailureTests.cs index 10128a4..d18f43e 100644 --- a/tests/CanKit.Pro.Tests/TestCases/CANopen/CanOpenSdoClientSendFailureTests.cs +++ b/tests/CanKit.Pro.Tests/TestCases/CANopen/CanOpenSdoClientSendFailureTests.cs @@ -386,6 +386,43 @@ public async Task A_SubBlock_Longer_Than_The_Sdo_Timeout_Does_Not_Time_Out_Befor last[0].Should().Be(SdoBlockFrames.CcsBlockDownloadEndBase, "three full segments leave no unused byte"); } + // The other side of C12: a segment whose confirmation takes longer than the timeout alone is + // a send that is stuck, and the timeout stands. The confirmation arriving late does not + // restart a timer that has already decided; the transfer ends in the timeout abort once the + // batch is over. + [Fact] + public async Task A_Segment_Confirmed_After_The_Timeout_Does_Not_Revive_The_Block_Download() + { + using var bus = ControllableBus.DeferredEchoCapable($"canopen-sdo-block-late-{Guid.NewGuid():N}"); + var clock = new ManualTimeSource(); + using var client = new CanOpenNode(new CanBusService(bus), 0x7F, + new CanOpenNodeOptions { SdoTimeout = TimeSpan.FromSeconds(1) }, ownsService: true, clock); + await bus.DeferredEchoes.WaitForEnqueuedAsync(1, ShortTimeout); // the boot-up + bus.DeferredEchoes.ReleaseAll(); + + var payload = Enumerable.Range(0, 14).Select(i => (byte)(0x30 + i)).ToArray(); // two segments + var download = client.SdoDownloadAsync(0x11, 0x1000, 0x00, payload, SdoTransferMode.Block); + await bus.DeferredEchoes.WaitForEnqueuedAsync(2, ShortTimeout); // the initiate + bus.DeferredEchoes.ReleaseNext(); + bus.RaiseObserved(CanFrame.Classic(unchecked((int)CanOpenCobId.SdoTx(0x11)), + SdoBlockFrames.BuildBlockDownloadInitResponse(0x1000, 0x00, serverCrcSupported: false, blockSize: 2)), + isEcho: false); + + await bus.DeferredEchoes.WaitForEnqueuedAsync(3, ShortTimeout); // segment 1, unconfirmed + _ = client.State; + _ = client.State; + clock.Advance(TimeSpan.FromMilliseconds(1500)); + _ = client.State; + _ = client.State; + bus.DeferredEchoes.ReleaseNext(); + await bus.DeferredEchoes.WaitForEnqueuedAsync(4, ShortTimeout); // segment 2 + bus.DeferredEchoes.ReleaseNext(); + + var abort = (await FluentActions.Awaiting(() => download.WithTimeoutAsync(ShortTimeout)) + .Should().ThrowAsync()).Which; + abort.AbortCode.Should().Be((uint)SdoAbortCode.SdoProtocolTimedOut); + } + [Fact] public async Task A_Block_Timeout_With_Its_Send_Confirmed_Ends_In_The_Timeout_Abort() { From ba1f28d4cb58cf9abbff1828f8269e3d9450fc91 Mon Sep 17 00:00:00 2001 From: Dietmar Borgards <2646931+dborgards@users.noreply.github.com> Date: Sat, 3 Oct 2026 18:06:16 +0200 Subject: [PATCH 3/3] refactor(canopen): one liveness check for block-client send outcomes; document the approximated-confirmation limit Co-Authored-By: Claude Sonnet 5.5 --- src/CanKit.Pro.CANopen/CanOpenNode.SdoBlock.cs | 12 ++++++------ src/CanKit.Pro.CANopen/CanOpenNode.cs | 13 ++++++++++--- src/CanKit.Pro.CANopen/CanOpenNodeOptions.cs | 3 +++ 3 files changed, 19 insertions(+), 9 deletions(-) diff --git a/src/CanKit.Pro.CANopen/CanOpenNode.SdoBlock.cs b/src/CanKit.Pro.CANopen/CanOpenNode.SdoBlock.cs index 9dbf52f..03db432 100644 --- a/src/CanKit.Pro.CANopen/CanOpenNode.SdoBlock.cs +++ b/src/CanKit.Pro.CANopen/CanOpenNode.SdoBlock.cs @@ -593,15 +593,15 @@ private void SendNextBlockDownloadSubBlock(SdoBlockClientSession session) // is still going out the server has nothing to answer yet: it is restarted at every // confirmed segment, so a long sub-block on a slow bus (127 segments at 10 kbit/s is // over a second) does not use up the timer before the ACK it waits for can be sent (C12). - // After the last segment the timer runs for the ACK alone. + // After the last segment the timer runs for the ACK alone. A confirmation that is only the + // driver accepting the frame (no echo on the bus, TxConfirmation.IsApproximated) says + // nothing about when the frame reaches the wire, so a driver queue holding more than + // SdoTimeout of bus time still outlasts the timer; SdoTimeout is the remedy there. _ = SendOrderedControlFrames(completed, () => session.Tcs.Task.IsCompleted, () => PostSdoClientSendOutcome(() => { - if (!_sdoBlockClients.TryGetValue(session.ServerNodeId, out var live) - || !ReferenceEquals(live, session) - || sendId != session.LatestSendId) - return; - RearmBlockClient(session, session.ServerNodeId); + if (IsLiveBlockSend(session, sendId)) + RearmBlockClient(session, session.ServerNodeId); }), frames.ToArray()); } diff --git a/src/CanKit.Pro.CANopen/CanOpenNode.cs b/src/CanKit.Pro.CANopen/CanOpenNode.cs index 6738c3c..4c80bf6 100644 --- a/src/CanKit.Pro.CANopen/CanOpenNode.cs +++ b/src/CanKit.Pro.CANopen/CanOpenNode.cs @@ -2369,9 +2369,7 @@ private void SendSdoBlockClientRequest(SdoBlockClientSession session, byte[] pay session.LatestSendPending = true; return failure => PostSdoClientSendOutcome(() => { - if (!_sdoBlockClients.TryGetValue(session.ServerNodeId, out var live) || !ReferenceEquals(live, session)) - return; - if (sendId != session.LatestSendId) return; // answered since: it reached the server + if (!IsLiveBlockSend(session, sendId)) return; // ended, or answered since: it reached the server session.LatestSendPending = false; if (failure is not null) { @@ -2385,6 +2383,15 @@ private void SendSdoBlockClientRequest(SdoBlockClientSession session, byte[] pay }); } + /// + /// Whether is still the latest send of a block-client session that + /// is still open. Actor-only: it reads the session table. + /// + private bool IsLiveBlockSend(SdoBlockClientSession session, int sendId) + => _sdoBlockClients.TryGetValue(session.ServerNodeId, out var live) + && ReferenceEquals(live, session) + && sendId == session.LatestSendId; + /// /// Runs a send-outcome reaction on the actor, where the session tables live. After disposal /// there is nothing left to report to: disposal has already completed every open transfer. diff --git a/src/CanKit.Pro.CANopen/CanOpenNodeOptions.cs b/src/CanKit.Pro.CANopen/CanOpenNodeOptions.cs index 3412f7c..d5f4507 100644 --- a/src/CanKit.Pro.CANopen/CanOpenNodeOptions.cs +++ b/src/CanKit.Pro.CANopen/CanOpenNodeOptions.cs @@ -17,6 +17,9 @@ public sealed class CanOpenNodeOptions /// Client-side SDO transfer timeout, applied to every request (initiate as well as /// each segment ack). In a block download it also restarts at every confirmed segment of a /// sub-block, so it measures the server's silence and not the time a sub-block takes to send. + /// On a bus without echo that confirmation is the driver accepting the frame, not the frame + /// reaching the wire: a driver queue holding more than this much bus time still outlasts + /// the timer, and raising it is the remedy. /// CiA 301 does not specify a fixed value; one second matches common /// production tooling and is aggressive enough for tests on a virtual bus. public TimeSpan SdoTimeout { get; init; } = TimeSpan.FromSeconds(1);