From 8af66cc2980c17c8ff598bf4d7d5472e552f7000 Mon Sep 17 00:00:00 2001 From: Dietmar Borgards <2646931+dborgards@users.noreply.github.com> Date: Sat, 3 Oct 2026 10:35:34 +0200 Subject: [PATCH 1/5] fix(canopen): report a short classic upload, refuse a long one, stop reporting reserved heartbeat states as boot-up #253: the classic upload client now aborts with 0607 0012h when the server sends more than it announced (as the server's receive path and the block-upload client do), and reports through BackgroundExceptionOccurred when it sends fewer; the shorter data is still returned. #255: a heartbeat or guarding state byte that CiA 301 reserves is no longer raised as HeartbeatReceived / NodeGuardingReceived with State = Initializing, which read as a boot-up. It still counts as a sign of life and is still recorded for the NMT master. #254: SendSyncAsync is documented as the raw send it is (not gated by NMT state or bit 30 of 1005h); the periodic producer is the gated one. Co-Authored-By: Claude Sonnet 5.5 --- src/CanKit.Pro.CANopen/CanOpenEvents.cs | 3 +- .../CanOpenNode.NodeGuarding.cs | 12 +- src/CanKit.Pro.CANopen/CanOpenNode.cs | 50 +++++-- src/CanKit.Pro.CANopen/ICanOpenNode.cs | 4 + src/CanKit.Pro.CANopen/README.md | 2 +- .../CANopen/CanOpenSdoCorrectnessTests.cs | 139 ++++++++++++++++++ 6 files changed, 189 insertions(+), 21 deletions(-) diff --git a/src/CanKit.Pro.CANopen/CanOpenEvents.cs b/src/CanKit.Pro.CANopen/CanOpenEvents.cs index dab32a70..d88b9d84 100644 --- a/src/CanKit.Pro.CANopen/CanOpenEvents.cs +++ b/src/CanKit.Pro.CANopen/CanOpenEvents.cs @@ -14,7 +14,8 @@ public sealed class HeartbeatReceivedEventArgs : EventArgs public byte ProducerNodeId { get; } /// Reported NMT state of the producer. is used - /// for the CiA 301 §7.2.8.3.2 bootup frame (data[0] == 0x00). + /// 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. public NmtState State { get; } /// UTC timestamp captured when the frame was processed on the actor loop. diff --git a/src/CanKit.Pro.CANopen/CanOpenNode.NodeGuarding.cs b/src/CanKit.Pro.CANopen/CanOpenNode.NodeGuarding.cs index b61bf040..c16c699e 100644 --- a/src/CanKit.Pro.CANopen/CanOpenNode.NodeGuarding.cs +++ b/src/CanKit.Pro.CANopen/CanOpenNode.NodeGuarding.cs @@ -206,14 +206,8 @@ private void HandleNodeGuardingResponse(byte producerNodeId, byte[] data) // (Bugbot, plus two adjacent findings from Codex on the same mechanism). bool toggle = (b & 0x80) != 0; byte stateByte = (byte)(b & 0x7F); - NmtState state = stateByte switch - { - 0x00 => NmtState.Initializing, // Bootup / freshly reset. - 0x04 => NmtState.Stopped, - 0x05 => NmtState.Operational, - 0x7F => NmtState.PreOperational, - _ => NmtState.Initializing, - }; + // A reserved state byte is a reply all the same, but it is not reported (#255). + bool reportable = TryDecodeHeartbeatState(stateByte, out var state); // CiA 301 §7.2.8.3.3: a reply that does not alternate the toggle bit is invalid for // resetting the life-time window (stale/repeated frames must not keep the consumer @@ -235,7 +229,7 @@ private void HandleNodeGuardingResponse(byte producerNodeId, byte[] data) () => OnNodeGuardingTimeout(producerNodeId)); } - RaiseNodeGuardingReceived(producerNodeId, state, toggle, DateTime.UtcNow); + if (reportable) RaiseNodeGuardingReceived(producerNodeId, state, toggle, DateTime.UtcNow); } // ========================================================================================= diff --git a/src/CanKit.Pro.CANopen/CanOpenNode.cs b/src/CanKit.Pro.CANopen/CanOpenNode.cs index 282b15a0..a9d72268 100644 --- a/src/CanKit.Pro.CANopen/CanOpenNode.cs +++ b/src/CanKit.Pro.CANopen/CanOpenNode.cs @@ -1335,6 +1335,21 @@ private void HandleEmcy(uint cobId, byte[] data) // ========================================================================================= // Heartbeat (FR-CO-008) // ========================================================================================= + + /// The NMT state a heartbeat or guarding state byte (bit 7 already masked) reports; + /// false for the values CiA 301 §7.2.8.3.2 reserves. + internal static bool TryDecodeHeartbeatState(byte stateByte, out NmtState state) + { + switch (stateByte) + { + case 0x00: state = NmtState.Initializing; return true; // Bootup frame. + case 0x04: state = NmtState.Stopped; return true; + case 0x05: state = NmtState.Operational; return true; + case 0x7F: state = NmtState.PreOperational; return true; + default: state = default; return false; + } + } + private void HandleHeartbeat(uint cobId, byte[] data) { if (data.Length < 1) return; @@ -1343,15 +1358,11 @@ private void HandleHeartbeat(uint cobId, byte[] data) // reserved (always 0)"), and a guarding reply is routed to HandleNodeGuardingResponse // before it gets here, so the bit is masked rather than interpreted. byte stateByte = (byte)(data[0] & 0x7F); - NmtState state = stateByte switch - { - 0x00 => NmtState.Initializing, // Bootup frame. - 0x04 => NmtState.Stopped, - 0x05 => NmtState.Operational, - 0x7F => NmtState.PreOperational, - _ => NmtState.Initializing, - }; - RaiseHeartbeatReceived(producer, state, DateTime.UtcNow); + // 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). + if (TryDecodeHeartbeatState(stateByte, out var state)) + RaiseHeartbeatReceived(producer, state, DateTime.UtcNow); NoteSlaveNmtState(producer, stateByte); _heartbeatConsumer.NoteReceived(producer); } @@ -2067,6 +2078,7 @@ private void HandleSdoClientResponse(byte serverNodeId, byte[] data) RearmSdoClientDeadline(session); session.InSegmentPhase = true; session.Payload = declared > 0 ? new byte[declared] : Array.Empty(); + session.DeclaredTotalSize = declared; session.Offset = 0; session.Toggle = false; SendNextClientUploadSegmentRequest(session); @@ -2125,6 +2137,14 @@ private void HandleSdoClientResponse(byte serverNodeId, byte[] data) AbortClient(session, SdoAbortCode.OutOfMemory); return; } + // More than the server announced is a protocol error, as it is for the server's own + // receive path and for the block-upload client (#253). Without a declared size + // (0) the buffer grows, which is what that case is for. + if (session.DeclaredTotalSize > 0 && needed > session.DeclaredTotalSize) + { + AbortClient(session, SdoAbortCode.LengthTooHigh); + return; + } if (session.Payload!.Length < needed) { var grown = new byte[GrowCapacity(session.Payload.Length, needed, _options.MaxSdoTransferBytes)]; @@ -2139,13 +2159,22 @@ private void HandleSdoClientResponse(byte serverNodeId, byte[] data) var final = session.Payload; if (session.Offset != final.Length) { - // Server declared a size but sent less. Trim. + // The buffer is larger than the data: slack from geometric growth when no + // size was announced, or a server that announced more than it sent. var trimmed = new byte[session.Offset]; Buffer.BlockCopy(final, 0, trimmed, 0, session.Offset); final = trimmed; } session.Deadline?.Dispose(); _sdoClients.Remove(serverNodeId); + // Fewer bytes than announced (a device that announces the maximum length of a + // VISIBLE_STRING and sends what it holds) is accepted, but no longer silently: + // the caller gets the data, and the shortfall is reported (#253). + if (session.DeclaredTotalSize > 0 && session.Offset < session.DeclaredTotalSize) + { + RaiseBackgroundException(new CanOpenTransportException( + $"SDO upload of 0x{session.Index:X4}:{session.Subindex:X2} from node {serverNodeId} announced {session.DeclaredTotalSize} byte(s) and delivered {session.Offset}; the shorter data was returned.")); + } session.Tcs.TrySetResult(final); return; } @@ -2577,6 +2606,7 @@ public SdoClientSession(byte serverNodeId, ushort index, byte subindex, bool isD /// transfer. public byte[]? Payload { get; set; } public int Offset { get; set; } + public uint DeclaredTotalSize { get; set; } public bool Toggle { get; set; } /// Numbers this transfer's sends; only the latest can still decide it (#197). diff --git a/src/CanKit.Pro.CANopen/ICanOpenNode.cs b/src/CanKit.Pro.CANopen/ICanOpenNode.cs index 32f3df6d..91a7a3b0 100644 --- a/src/CanKit.Pro.CANopen/ICanOpenNode.cs +++ b/src/CanKit.Pro.CANopen/ICanOpenNode.cs @@ -251,6 +251,10 @@ Task SendNmtCommandAsync(NmtCommand command, byte targetNodeId, /// Transmits a single SYNC frame (payload-less) on the SYNC COB-ID configured in /// 1005h. + /// This is a raw send, not the producer: it is not gated by the NMT state and not by + /// bit 30 of 1005h ("device generates SYNC"), so a node in Stopped, or one that is not + /// configured to produce SYNC, transmits when asked. The periodic producer + /// () is the one that follows CiA 301 Table 37. Task SendSyncAsync(CancellationToken cancellationToken = default); // ----------------------------------------------------------------------------------------- diff --git a/src/CanKit.Pro.CANopen/README.md b/src/CanKit.Pro.CANopen/README.md index ab7dd668..83327cfe 100644 --- a/src/CanKit.Pro.CANopen/README.md +++ b/src/CanKit.Pro.CANopen/README.md @@ -445,7 +445,7 @@ Table 37 of CiA 301, as the node applies it: | --- | --- | --- | --- | | PDO | — | transmitted and received | — | | SDO | served | served | requests ignored; an open server session is aborted with `0800 0022h` | -| SYNC | consumed and produced | consumed and produced | neither; the producer keeps its cycle and resumes afterwards | +| SYNC | consumed and produced | consumed and produced | neither; the producer keeps its cycle and resumes afterwards (`SendSyncAsync` is a raw send and transmits regardless of the state and of bit 30 of `1005h`) | | EMCY | transmitted | transmitted | held; the most recent one is transmitted when the node leaves Stopped | | Heartbeat, guarding | active | active | active | diff --git a/tests/CanKit.Pro.Tests/TestCases/CANopen/CanOpenSdoCorrectnessTests.cs b/tests/CanKit.Pro.Tests/TestCases/CANopen/CanOpenSdoCorrectnessTests.cs index c42fabd4..1f22cac9 100644 --- a/tests/CanKit.Pro.Tests/TestCases/CANopen/CanOpenSdoCorrectnessTests.cs +++ b/tests/CanKit.Pro.Tests/TestCases/CANopen/CanOpenSdoCorrectnessTests.cs @@ -13,6 +13,7 @@ using CanKit.Abstractions.API.Common.Definitions; using CanKit.Core; using CanKit.Pro.CANopen; +using CanKit.Pro.CANopen.Nmt; using CanKit.Pro.CANopen.Sdo; using CanKit.Pro.RawCan; using CanKit.Pro.Tests.Infrastructure; @@ -1274,6 +1275,144 @@ public async Task Sdo_BlockUpload_Client_Sizeless_Grows_Geometrically() #endif + // ----------------------------------------------------------------------------------------- + // #253: the classic upload client against the length the server announced. More than that is + // refused as the server's receive path and the block-upload client already do; fewer is + // accepted (a device that announces the OD maximum of a VISIBLE_STRING and sends what it + // holds) but reported through BackgroundExceptionOccurred instead of passing silently. + // ----------------------------------------------------------------------------------------- + private static byte[] AnnouncedUploadInit(ushort index, byte subindex, uint length) => new byte[] + { + SdoFrames.ScsUploadInitSegmented, + (byte)(index & 0xFF), (byte)((index >> 8) & 0xFF), subindex, + (byte)length, (byte)(length >> 8), (byte)(length >> 16), (byte)(length >> 24), + }; + + [Fact] + public async Task Sdo_Client_Upload_Aborts_When_The_Server_Sends_More_Than_It_Announced() + { + var session = NewSession(); + using var busA = Open(session, 1); + using var rawBus = Open(session, 2); + using var client = CanOpen.OpenNode(busA, nodeId: 0x01); + PeerSdoLaboratory.Bind(client, 0x02); + using var tap = new FrameTap(rawBus, CanOpenCobId.SdoRx(0x02)); + + var upload = client.SdoUploadAsync(0x02, 0x2100, 0x00); + tap.Next(ShortTimeout)[0].Should().Be(SdoFrames.CcsUploadInit); + Send(rawBus, CanOpenCobId.SdoTx(0x02), AnnouncedUploadInit(0x2100, 0x00, 10)); + tap.Next(ShortTimeout)[0].Should().Be(SdoFrames.CcsUploadSegmentBase); + Send(rawBus, CanOpenCobId.SdoTx(0x02), SdoFrames.BuildSegment( + SdoFrames.ScsUploadSegmentBase, toggle: false, lastSegment: false, new byte[] { 1, 2, 3, 4, 5, 6, 7 })); + tap.Next(ShortTimeout)[0].Should().Be((byte)(SdoFrames.CcsUploadSegmentBase | SdoFrames.ToggleBit)); + // Seven bytes were delivered and ten announced; seven more make fourteen. + Send(rawBus, CanOpenCobId.SdoTx(0x02), SdoFrames.BuildSegment( + SdoFrames.ScsUploadSegmentBase, toggle: true, lastSegment: true, new byte[] { 8, 9, 10, 11, 12, 13, 14 })); + + var abort = tap.Next(ShortTimeout); + abort[0].Should().Be(SdoFrames.CsAbort); + SdoFrames.ReadAbortCode(abort).Should().Be((uint)SdoAbortCode.LengthTooHigh); + var ex = await Assert.ThrowsAsync(() => upload.WithTimeoutAsync(ShortTimeout)); + ex.AbortCode.Should().Be((uint)SdoAbortCode.LengthTooHigh); + } + + [Fact] + public async Task Sdo_Client_Upload_Returns_Short_Data_And_Reports_The_Shortfall() + { + var session = NewSession(); + using var busA = Open(session, 1); + using var rawBus = Open(session, 2); + using var client = CanOpen.OpenNode(busA, nodeId: 0x01); + PeerSdoLaboratory.Bind(client, 0x02); + using var tap = new FrameTap(rawBus, CanOpenCobId.SdoRx(0x02)); + var reports = new System.Collections.Concurrent.ConcurrentQueue(); + client.BackgroundExceptionOccurred += (_, e) => reports.Enqueue(e); + + var upload = client.SdoUploadAsync(0x02, 0x2100, 0x00); + tap.Next(ShortTimeout)[0].Should().Be(SdoFrames.CcsUploadInit); + Send(rawBus, CanOpenCobId.SdoTx(0x02), AnnouncedUploadInit(0x2100, 0x00, 20)); + tap.Next(ShortTimeout)[0].Should().Be(SdoFrames.CcsUploadSegmentBase); + Send(rawBus, CanOpenCobId.SdoTx(0x02), SdoFrames.BuildSegment( + SdoFrames.ScsUploadSegmentBase, toggle: false, lastSegment: true, new byte[] { 1, 2, 3 })); + + (await upload.WithTimeoutAsync(ShortTimeout)).Should().Equal(1, 2, 3); + reports.Should().ContainSingle().Which.Should().BeOfType() + .Which.Message.Should().Contain("announced 20").And.Contain("delivered 3"); + } + + [Fact] + public async Task Sdo_Client_Upload_With_The_Announced_Length_Reports_Nothing() + { + var session = NewSession(); + using var busA = Open(session, 1); + using var rawBus = Open(session, 2); + using var client = CanOpen.OpenNode(busA, nodeId: 0x01); + PeerSdoLaboratory.Bind(client, 0x02); + using var tap = new FrameTap(rawBus, CanOpenCobId.SdoRx(0x02)); + var reports = new System.Collections.Concurrent.ConcurrentQueue(); + client.BackgroundExceptionOccurred += (_, e) => reports.Enqueue(e); + + var upload = client.SdoUploadAsync(0x02, 0x2100, 0x00); + tap.Next(ShortTimeout); + Send(rawBus, CanOpenCobId.SdoTx(0x02), AnnouncedUploadInit(0x2100, 0x00, 3)); + tap.Next(ShortTimeout); + Send(rawBus, CanOpenCobId.SdoTx(0x02), SdoFrames.BuildSegment( + SdoFrames.ScsUploadSegmentBase, toggle: false, lastSegment: true, new byte[] { 1, 2, 3 })); + + (await upload.WithTimeoutAsync(ShortTimeout)).Should().Equal(1, 2, 3); + reports.Should().BeEmpty(); + } + + // ----------------------------------------------------------------------------------------- + // #255: a heartbeat or guarding reply whose state byte CiA 301 reserves is no heartbeat. It + // used to be reported as Initializing, the value of a boot-up. The valid frame sent after the + // reserved ones is what tells them apart: events arrive in order, so the first one seen is + // the one that decides. + // ----------------------------------------------------------------------------------------- + [Fact] + public async Task Heartbeat_With_A_Reserved_State_Byte_Is_Not_Reported_As_Bootup() + { + var session = NewSession(); + using var busA = Open(session, 1); + using var rawBus = Open(session, 2); + using var node = CanOpen.OpenNode(busA, nodeId: 0x01); + var first = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + node.HeartbeatReceived += (_, e) => + { + if (e.ProducerNodeId == 0x11) first.TrySetResult(e.State); + }; + + Send(rawBus, CanOpenCobId.HeartbeatBase + 0x11, new byte[] { 0x01 }); + Send(rawBus, CanOpenCobId.HeartbeatBase + 0x11, new byte[] { 0x03 }); + Send(rawBus, CanOpenCobId.HeartbeatBase + 0x11, new byte[] { 0x05 }); + + (await first.Task.WithTimeoutAsync(ShortTimeout)).Should().Be(NmtState.Operational); + } + + [Fact] + public async Task Guarding_Reply_With_A_Reserved_State_Byte_Is_Not_Reported() + { + var session = NewSession(); + using var busA = Open(session, 1); + using var rawBus = Open(session, 2); + using var node = CanOpen.OpenNode(busA, nodeId: 0x01); + var first = new TaskCompletionSource<(NmtState State, bool Toggle)>(TaskCreationOptions.RunContinuationsAsynchronously); + node.NodeGuardingReceived += (_, e) => + { + if (e.ProducerNodeId == 0x11) first.TrySetResult((e.State, e.Toggle)); + }; + node.StartNodeGuardingConsumer(producerNodeId: 0x11, guardTime: TimeSpan.FromSeconds(30), lifeTimeFactor: 3); + + // The reserved reply is still a reply (toggle false), so it is not reported but it counts; + // the next one flips the toggle. + Send(rawBus, CanOpenCobId.HeartbeatBase + 0x11, new byte[] { 0x01 }); + Send(rawBus, CanOpenCobId.HeartbeatBase + 0x11, new byte[] { 0xFF }); + + var seen = await first.Task.WithTimeoutAsync(ShortTimeout); + seen.State.Should().Be(NmtState.PreOperational); + seen.Toggle.Should().BeTrue(); + } + [Fact] public void Sdo_Server_Sizeless_Download_Aborts_OutOfMemory_Before_Passing_The_Cap() { From 124130a79b3d435048b37d0a28476b91b2b387a8 Mon Sep 17 00:00:00 2001 From: Dietmar Borgards <2646931+dborgards@users.noreply.github.com> Date: Sat, 3 Oct 2026 10:50:16 +0200 Subject: [PATCH 2/5] fix(canopen): an announced upload length of zero is a length Co-Authored-By: Claude Sonnet 5.5 --- src/CanKit.Pro.CANopen/CanOpenNode.cs | 11 +++++---- .../CANopen/CanOpenSdoCorrectnessTests.cs | 24 +++++++++++++++++++ 2 files changed, 31 insertions(+), 4 deletions(-) diff --git a/src/CanKit.Pro.CANopen/CanOpenNode.cs b/src/CanKit.Pro.CANopen/CanOpenNode.cs index a9d72268..111d18cd 100644 --- a/src/CanKit.Pro.CANopen/CanOpenNode.cs +++ b/src/CanKit.Pro.CANopen/CanOpenNode.cs @@ -2079,6 +2079,7 @@ private void HandleSdoClientResponse(byte serverNodeId, byte[] data) session.InSegmentPhase = true; session.Payload = declared > 0 ? new byte[declared] : Array.Empty(); session.DeclaredTotalSize = declared; + session.SizeIndicated = (cs & 0x01) != 0; session.Offset = 0; session.Toggle = false; SendNextClientUploadSegmentRequest(session); @@ -2138,9 +2139,10 @@ private void HandleSdoClientResponse(byte serverNodeId, byte[] data) return; } // More than the server announced is a protocol error, as it is for the server's own - // receive path and for the block-upload client (#253). Without a declared size - // (0) the buffer grows, which is what that case is for. - if (session.DeclaredTotalSize > 0 && needed > session.DeclaredTotalSize) + // receive path and for the block-upload client (#253). Without an + // indicated size the buffer grows, which is what that case is for. An indicated size + // of zero is a size. + if (session.SizeIndicated && needed > session.DeclaredTotalSize) { AbortClient(session, SdoAbortCode.LengthTooHigh); return; @@ -2170,7 +2172,7 @@ private void HandleSdoClientResponse(byte serverNodeId, byte[] data) // Fewer bytes than announced (a device that announces the maximum length of a // VISIBLE_STRING and sends what it holds) is accepted, but no longer silently: // the caller gets the data, and the shortfall is reported (#253). - if (session.DeclaredTotalSize > 0 && session.Offset < session.DeclaredTotalSize) + if (session.SizeIndicated && session.Offset < session.DeclaredTotalSize) { RaiseBackgroundException(new CanOpenTransportException( $"SDO upload of 0x{session.Index:X4}:{session.Subindex:X2} from node {serverNodeId} announced {session.DeclaredTotalSize} byte(s) and delivered {session.Offset}; the shorter data was returned.")); @@ -2607,6 +2609,7 @@ public SdoClientSession(byte serverNodeId, ushort index, byte subindex, bool isD public byte[]? Payload { get; set; } public int Offset { get; set; } public uint DeclaredTotalSize { get; set; } + public bool SizeIndicated { get; set; } public bool Toggle { get; set; } /// Numbers this transfer's sends; only the latest can still decide it (#197). diff --git a/tests/CanKit.Pro.Tests/TestCases/CANopen/CanOpenSdoCorrectnessTests.cs b/tests/CanKit.Pro.Tests/TestCases/CANopen/CanOpenSdoCorrectnessTests.cs index 1f22cac9..e9e3e9d0 100644 --- a/tests/CanKit.Pro.Tests/TestCases/CANopen/CanOpenSdoCorrectnessTests.cs +++ b/tests/CanKit.Pro.Tests/TestCases/CANopen/CanOpenSdoCorrectnessTests.cs @@ -1316,6 +1316,30 @@ public async Task Sdo_Client_Upload_Aborts_When_The_Server_Sends_More_Than_It_An ex.AbortCode.Should().Be((uint)SdoAbortCode.LengthTooHigh); } + [Fact] + public async Task Sdo_Client_Upload_Aborts_When_The_Server_Announces_Zero_And_Sends_Data() + { + var session = NewSession(); + using var busA = Open(session, 1); + using var rawBus = Open(session, 2); + using var client = CanOpen.OpenNode(busA, nodeId: 0x01); + PeerSdoLaboratory.Bind(client, 0x02); + using var tap = new FrameTap(rawBus, CanOpenCobId.SdoRx(0x02)); + + var upload = client.SdoUploadAsync(0x02, 0x2100, 0x00); + tap.Next(ShortTimeout)[0].Should().Be(SdoFrames.CcsUploadInit); + // The size indicator is set and the size is zero: that is an announced length of nothing, + // not a missing one. + Send(rawBus, CanOpenCobId.SdoTx(0x02), AnnouncedUploadInit(0x2100, 0x00, 0)); + tap.Next(ShortTimeout)[0].Should().Be(SdoFrames.CcsUploadSegmentBase); + Send(rawBus, CanOpenCobId.SdoTx(0x02), SdoFrames.BuildSegment( + SdoFrames.ScsUploadSegmentBase, toggle: false, lastSegment: true, new byte[] { 1, 2, 3 })); + + var abort = tap.Next(ShortTimeout); + SdoFrames.ReadAbortCode(abort).Should().Be((uint)SdoAbortCode.LengthTooHigh); + await Assert.ThrowsAsync(() => upload.WithTimeoutAsync(ShortTimeout)); + } + [Fact] public async Task Sdo_Client_Upload_Returns_Short_Data_And_Reports_The_Shortfall() { From 43851c7ad2278e7838c5dbd2f9f98760b1149b3c Mon Sep 17 00:00:00 2001 From: Dietmar Borgards <2646931+dborgards@users.noreply.github.com> Date: Sat, 3 Oct 2026 11:05:21 +0200 Subject: [PATCH 3/5] fix(canopen): a guarding reply of state 0 is not a boot-up either; measure the reserved-state tests on all events Co-Authored-By: Claude Sonnet 5.5 --- .../CanOpenNode.NodeGuarding.cs | 6 ++- .../CANopen/CanOpenSdoCorrectnessTests.cs | 53 +++++++++++-------- 2 files changed, 35 insertions(+), 24 deletions(-) diff --git a/src/CanKit.Pro.CANopen/CanOpenNode.NodeGuarding.cs b/src/CanKit.Pro.CANopen/CanOpenNode.NodeGuarding.cs index c16c699e..8922aa80 100644 --- a/src/CanKit.Pro.CANopen/CanOpenNode.NodeGuarding.cs +++ b/src/CanKit.Pro.CANopen/CanOpenNode.NodeGuarding.cs @@ -206,8 +206,10 @@ private void HandleNodeGuardingResponse(byte producerNodeId, byte[] data) // (Bugbot, plus two adjacent findings from Codex on the same mechanism). bool toggle = (b & 0x80) != 0; byte stateByte = (byte)(b & 0x7F); - // A reserved state byte is a reply all the same, but it is not reported (#255). - bool reportable = TryDecodeHeartbeatState(stateByte, out var state); + // A reserved state byte is a reply all the same, but it is not reported (#255). Neither is + // state 0 with the toggle bit set: a node that answers guarding is never Initializing + // (see the boot-up branch above), so 0x80 would read as a boot-up that is not one. + bool reportable = TryDecodeHeartbeatState(stateByte, out var state) && stateByte != 0; // CiA 301 §7.2.8.3.3: a reply that does not alternate the toggle bit is invalid for // resetting the life-time window (stale/repeated frames must not keep the consumer diff --git a/tests/CanKit.Pro.Tests/TestCases/CANopen/CanOpenSdoCorrectnessTests.cs b/tests/CanKit.Pro.Tests/TestCases/CANopen/CanOpenSdoCorrectnessTests.cs index e9e3e9d0..05edff57 100644 --- a/tests/CanKit.Pro.Tests/TestCases/CANopen/CanOpenSdoCorrectnessTests.cs +++ b/tests/CanKit.Pro.Tests/TestCases/CANopen/CanOpenSdoCorrectnessTests.cs @@ -1388,53 +1388,62 @@ public async Task Sdo_Client_Upload_With_The_Announced_Length_Reports_Nothing() } // ----------------------------------------------------------------------------------------- - // #255: a heartbeat or guarding reply whose state byte CiA 301 reserves is no heartbeat. It - // used to be reported as Initializing, the value of a boot-up. The valid frame sent after the - // reserved ones is what tells them apart: events arrive in order, so the first one seen is - // the one that decides. + // #255: a heartbeat or guarding state byte that CiA 301 reserves is not reported. It used to be + // reported as Initializing, the value of a boot-up. Events of one producer may be coalesced + // by the queue, so "the first event" is not a measuring point; the valid frame sent last is + // always delivered, and nothing delivered by then may have been Initializing. // ----------------------------------------------------------------------------------------- - [Fact] - public async Task Heartbeat_With_A_Reserved_State_Byte_Is_Not_Reported_As_Bootup() + [Theory] + [InlineData(0x01)] + [InlineData(0x03)] + public async Task Heartbeat_With_A_Reserved_State_Byte_Is_Not_Reported_As_Bootup(byte reserved) { var session = NewSession(); using var busA = Open(session, 1); using var rawBus = Open(session, 2); using var node = CanOpen.OpenNode(busA, nodeId: 0x01); - var first = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var seen = new System.Collections.Concurrent.ConcurrentQueue(); + var last = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); node.HeartbeatReceived += (_, e) => { - if (e.ProducerNodeId == 0x11) first.TrySetResult(e.State); + if (e.ProducerNodeId != 0x11) return; + seen.Enqueue(e.State); + if (e.State == NmtState.Operational) last.TrySetResult(true); }; - Send(rawBus, CanOpenCobId.HeartbeatBase + 0x11, new byte[] { 0x01 }); - Send(rawBus, CanOpenCobId.HeartbeatBase + 0x11, new byte[] { 0x03 }); + Send(rawBus, CanOpenCobId.HeartbeatBase + 0x11, new byte[] { reserved }); Send(rawBus, CanOpenCobId.HeartbeatBase + 0x11, new byte[] { 0x05 }); + await last.Task.WithTimeoutAsync(ShortTimeout); - (await first.Task.WithTimeoutAsync(ShortTimeout)).Should().Be(NmtState.Operational); + seen.Should().NotContain(NmtState.Initializing); } - [Fact] - public async Task Guarding_Reply_With_A_Reserved_State_Byte_Is_Not_Reported() + [Theory] + [InlineData(0x01, 0xFF)] // a reserved state, toggle clear; the valid reply flips the toggle + [InlineData(0x80, 0x7F)] // state 0 with the toggle set: no guarding producer is Initializing + public async Task Guarding_Reply_With_A_Reserved_State_Byte_Is_Not_Reported(byte reserved, byte valid) { var session = NewSession(); using var busA = Open(session, 1); using var rawBus = Open(session, 2); using var node = CanOpen.OpenNode(busA, nodeId: 0x01); - var first = new TaskCompletionSource<(NmtState State, bool Toggle)>(TaskCreationOptions.RunContinuationsAsynchronously); + var seen = new System.Collections.Concurrent.ConcurrentQueue(); + var last = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); node.NodeGuardingReceived += (_, e) => { - if (e.ProducerNodeId == 0x11) first.TrySetResult((e.State, e.Toggle)); + if (e.ProducerNodeId != 0x11) return; + seen.Enqueue(e.State); + if (e.State == NmtState.PreOperational) last.TrySetResult(true); }; node.StartNodeGuardingConsumer(producerNodeId: 0x11, guardTime: TimeSpan.FromSeconds(30), lifeTimeFactor: 3); - // The reserved reply is still a reply (toggle false), so it is not reported but it counts; - // the next one flips the toggle. - Send(rawBus, CanOpenCobId.HeartbeatBase + 0x11, new byte[] { 0x01 }); - Send(rawBus, CanOpenCobId.HeartbeatBase + 0x11, new byte[] { 0xFF }); + // The reserved reply is still a reply and takes the toggle baseline, so the valid one + // carries the opposite toggle. + Send(rawBus, CanOpenCobId.HeartbeatBase + 0x11, new byte[] { reserved }); + Send(rawBus, CanOpenCobId.HeartbeatBase + 0x11, new byte[] { valid }); + await last.Task.WithTimeoutAsync(ShortTimeout); - var seen = await first.Task.WithTimeoutAsync(ShortTimeout); - seen.State.Should().Be(NmtState.PreOperational); - seen.Toggle.Should().BeTrue(); + seen.Should().NotContain(NmtState.Initializing); } [Fact] From 8ff03eb5c64c8e45294949c52dd697dc4adcda86 Mon Sep 17 00:00:00 2001 From: Dietmar Borgards <2646931+dborgards@users.noreply.github.com> Date: Sat, 3 Oct 2026 11:20:09 +0200 Subject: [PATCH 4/5] fix(canopen): judge an overrun of the announced upload length before the transfer cap Co-Authored-By: Claude Sonnet 5.5 --- src/CanKit.Pro.CANopen/CanOpenNode.cs | 17 +++++------ .../CANopen/CanOpenSdoCorrectnessTests.cs | 28 +++++++++++++++++++ 2 files changed, 37 insertions(+), 8 deletions(-) diff --git a/src/CanKit.Pro.CANopen/CanOpenNode.cs b/src/CanKit.Pro.CANopen/CanOpenNode.cs index 111d18cd..5e41a7eb 100644 --- a/src/CanKit.Pro.CANopen/CanOpenNode.cs +++ b/src/CanKit.Pro.CANopen/CanOpenNode.cs @@ -2133,20 +2133,21 @@ private void HandleSdoClientResponse(byte serverNodeId, byte[] data) // enforce MaxSdoTransferBytes so a zero-size init cannot bypass the cap by // streaming unbounded segments. int needed = session.Offset + payload.Length; - if (needed > _options.MaxSdoTransferBytes) - { - AbortClient(session, SdoAbortCode.OutOfMemory); - return; - } // More than the server announced is a protocol error, as it is for the server's own - // receive path and for the block-upload client (#253). Without an - // indicated size the buffer grows, which is what that case is for. An indicated size - // of zero is a size. + // receive path and for the block-upload client (#253). It is judged before the cap, + // so that a server announcing exactly the cap and sending more is told what it did + // wrong. Without an indicated size the buffer grows, which is what that case is for; + // an indicated size of zero is a size. if (session.SizeIndicated && needed > session.DeclaredTotalSize) { AbortClient(session, SdoAbortCode.LengthTooHigh); return; } + if (needed > _options.MaxSdoTransferBytes) + { + AbortClient(session, SdoAbortCode.OutOfMemory); + return; + } if (session.Payload!.Length < needed) { var grown = new byte[GrowCapacity(session.Payload.Length, needed, _options.MaxSdoTransferBytes)]; diff --git a/tests/CanKit.Pro.Tests/TestCases/CANopen/CanOpenSdoCorrectnessTests.cs b/tests/CanKit.Pro.Tests/TestCases/CANopen/CanOpenSdoCorrectnessTests.cs index 05edff57..df8276b7 100644 --- a/tests/CanKit.Pro.Tests/TestCases/CANopen/CanOpenSdoCorrectnessTests.cs +++ b/tests/CanKit.Pro.Tests/TestCases/CANopen/CanOpenSdoCorrectnessTests.cs @@ -1316,6 +1316,34 @@ public async Task Sdo_Client_Upload_Aborts_When_The_Server_Sends_More_Than_It_An ex.AbortCode.Should().Be((uint)SdoAbortCode.LengthTooHigh); } + [Fact] + public async Task Sdo_Client_Upload_Over_An_Announced_Cap_Sized_Length_Is_Length_Too_High_Not_Out_Of_Memory() + { + var session = NewSession(); + using var busA = Open(session, 1); + using var rawBus = Open(session, 2); + using var client = CanOpen.OpenNode(busA, nodeId: 0x01, + new CanOpenNodeOptions().With(maxSdoTransferBytes: 10)); + PeerSdoLaboratory.Bind(client, 0x02); + using var tap = new FrameTap(rawBus, CanOpenCobId.SdoRx(0x02)); + + var upload = client.SdoUploadAsync(0x02, 0x2100, 0x00); + tap.Next(ShortTimeout)[0].Should().Be(SdoFrames.CcsUploadInit); + Send(rawBus, CanOpenCobId.SdoTx(0x02), AnnouncedUploadInit(0x2100, 0x00, 10)); + tap.Next(ShortTimeout)[0].Should().Be(SdoFrames.CcsUploadSegmentBase); + Send(rawBus, CanOpenCobId.SdoTx(0x02), SdoFrames.BuildSegment( + SdoFrames.ScsUploadSegmentBase, toggle: false, lastSegment: false, new byte[] { 1, 2, 3, 4, 5, 6, 7 })); + tap.Next(ShortTimeout); + // Fourteen bytes against ten announced, and ten is also the cap: the announcement is what + // was broken, so that is what the abort says. + Send(rawBus, CanOpenCobId.SdoTx(0x02), SdoFrames.BuildSegment( + SdoFrames.ScsUploadSegmentBase, toggle: true, lastSegment: true, new byte[] { 8, 9, 10, 11, 12, 13, 14 })); + + SdoFrames.ReadAbortCode(tap.Next(ShortTimeout)).Should().Be((uint)SdoAbortCode.LengthTooHigh); + var ex = await Assert.ThrowsAsync(() => upload.WithTimeoutAsync(ShortTimeout)); + ex.AbortCode.Should().Be((uint)SdoAbortCode.LengthTooHigh); + } + [Fact] public async Task Sdo_Client_Upload_Aborts_When_The_Server_Announces_Zero_And_Sends_Data() { From 48c6cbe0794cc90be4ee5b1a781a86e3101a960b Mon Sep 17 00:00:00 2001 From: Dietmar Borgards <2646931+dborgards@users.noreply.github.com> Date: Sat, 3 Oct 2026 11:35:36 +0200 Subject: [PATCH 5/5] fix(canopen): a heartbeat of 0x80 is not a boot-up Co-Authored-By: Claude Sonnet 5.5 --- src/CanKit.Pro.CANopen/CanOpenNode.cs | 4 +++- .../TestCases/CANopen/CanOpenSdoCorrectnessTests.cs | 1 + 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/src/CanKit.Pro.CANopen/CanOpenNode.cs b/src/CanKit.Pro.CANopen/CanOpenNode.cs index 5e41a7eb..45cddc47 100644 --- a/src/CanKit.Pro.CANopen/CanOpenNode.cs +++ b/src/CanKit.Pro.CANopen/CanOpenNode.cs @@ -1361,7 +1361,9 @@ 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). - if (TryDecodeHeartbeatState(stateByte, out var state)) + // 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)) RaiseHeartbeatReceived(producer, state, DateTime.UtcNow); NoteSlaveNmtState(producer, stateByte); _heartbeatConsumer.NoteReceived(producer); diff --git a/tests/CanKit.Pro.Tests/TestCases/CANopen/CanOpenSdoCorrectnessTests.cs b/tests/CanKit.Pro.Tests/TestCases/CANopen/CanOpenSdoCorrectnessTests.cs index df8276b7..e1c12428 100644 --- a/tests/CanKit.Pro.Tests/TestCases/CANopen/CanOpenSdoCorrectnessTests.cs +++ b/tests/CanKit.Pro.Tests/TestCases/CANopen/CanOpenSdoCorrectnessTests.cs @@ -1424,6 +1424,7 @@ public async Task Sdo_Client_Upload_With_The_Announced_Length_Reports_Nothing() [Theory] [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) { var session = NewSession();