fix(canopen): restart the block-download request timer per confirmed segment, ignore heartbeats with the reserved bit 7

src/CanKit.Pro.CANopen/CanOpenEvents.cs

@@ -15,7 +15,7 @@ public sealed class HeartbeatReceivedEventArgs : EventArgs
 
     /// <summary>Reported NMT state of the producer. <see cref="NmtState.Initializing"/> is used
     /// for the CiA 301 §7.2.8.3.2 bootup frame (<c>data[0] == 0x00</c>). A frame whose state
-    /// byte is one CiA 301 reserves is not reported.</summary>
+    /// byte is one CiA 301 reserves, or that sets the reserved bit 7, is not reported.</summary>
     public NmtState State { get; }
 
     /// <summary>UTC timestamp captured when the frame was processed on the actor loop.</summary>

src/CanKit.Pro.CANopen/CanOpenNode.SdoBlock.cs

@@ -587,8 +587,22 @@ 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. 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 (IsLiveBlockSend(session, sendId))
+                    RearmBlockClient(session, session.ServerNodeId);
+            }), frames.ToArray());
     }
 
     private void SendBlockDownloadEnd(SdoBlockClientSession session)

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);
@@ -2367,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)
             {
@@ -2383,6 +2383,15 @@ private void SendSdoBlockClientRequest(SdoBlockClientSession session, byte[] pay
         });
     }
 
+    /// <summary>
+    /// Whether <paramref name="sendId"/> is still the latest send of a block-client session that
+    /// is still open. Actor-only: it reads the session table.
+    /// </summary>
+    private bool IsLiveBlockSend(SdoBlockClientSession session, int sendId)
+        => _sdoBlockClients.TryGetValue(session.ServerNodeId, out var live)
+           && ReferenceEquals(live, session)
+           && sendId == session.LatestSendId;
+
     /// <summary>
     /// 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.
@@ -2427,6 +2436,15 @@ private Task SendOrderedControlFrames(Action<CanOpenTransportException?>? onSend
     /// </summary>
     private Task SendOrderedControlFrames(Action<CanOpenTransportException?>? onSendCompleted,
         Func<bool>? shouldStop, params (uint CobId, byte[] Payload)[] frames)
+        => SendOrderedControlFrames(onSendCompleted, shouldStop, onFrameConfirmed: null, frames);
+
+    /// <summary>
+    /// As above. <paramref name="onFrameConfirmed"/>, 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.
+    /// </summary>
+    private Task SendOrderedControlFrames(Action<CanOpenTransportException?>? onSendCompleted,
+        Func<bool>? shouldStop, Action? onFrameConfirmed, params (uint CobId, byte[] Payload)[] frames)
     {
         return Task.Run(async () =>
         {
@@ -2451,6 +2469,10 @@ private Task SendOrderedControlFrames(Action<CanOpenTransportException?>? onSend
                                 return;
                             }
                         }
+                        else
+                        {
+                            onFrameConfirmed?.Invoke();
+                        }
                     }
                     catch (Exception ex)
                     {

src/CanKit.Pro.CANopen/CanOpenNodeOptions.cs

@@ -15,7 +15,12 @@ namespace CanKit.Pro.CANopen;
 public sealed class CanOpenNodeOptions
 {
     /// <summary>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.
+    /// 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.</summary>
     public TimeSpan SdoTimeout { get; init; } = TimeSpan.FromSeconds(1);
 

tests/CanKit.Pro.Tests/TestCases/CANopen/CanOpenSdoClientSendFailureTests.cs

@@ -334,6 +334,95 @@ 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<byte[]>();
+        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");
+    }
+
+    // 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<SdoAbortException>()).Which;
+        abort.AbortCode.Should().Be((uint)SdoAbortCode.SdoProtocolTimedOut);
+    }
+
     [Fact]
     public async Task A_Block_Timeout_With_Its_Send_Confirmed_Ends_In_The_Timeout_Abort()
     {

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]