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..03db432 100644
--- a/src/CanKit.Pro.CANopen/CanOpenNode.SdoBlock.cs
+++ b/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)
diff --git a/src/CanKit.Pro.CANopen/CanOpenNode.cs b/src/CanKit.Pro.CANopen/CanOpenNode.cs
index 45cddc4..4c80bf6 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);
@@ -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
});
}
+ ///
+ /// 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.
@@ -2427,6 +2436,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 +2469,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..d5f4507 100644
--- a/src/CanKit.Pro.CANopen/CanOpenNodeOptions.cs
+++ b/src/CanKit.Pro.CANopen/CanOpenNodeOptions.cs
@@ -15,7 +15,12 @@ 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.
+ /// 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);
diff --git a/tests/CanKit.Pro.Tests/TestCases/CANopen/CanOpenSdoClientSendFailureTests.cs b/tests/CanKit.Pro.Tests/TestCases/CANopen/CanOpenSdoClientSendFailureTests.cs
index 99d1122..d18f43e 100644
--- a/tests/CanKit.Pro.Tests/TestCases/CANopen/CanOpenSdoClientSendFailureTests.cs
+++ b/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();
+ 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()).Which;
+ abort.AbortCode.Should().Be((uint)SdoAbortCode.SdoProtocolTimedOut);
+ }
+
[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]