diff --git a/src/Daqifi.Core.Tests/Device/Internal/SessionCommandInterpreterTests.cs b/src/Daqifi.Core.Tests/Device/Internal/SessionCommandInterpreterTests.cs new file mode 100644 index 0000000..65c8061 --- /dev/null +++ b/src/Daqifi.Core.Tests/Device/Internal/SessionCommandInterpreterTests.cs @@ -0,0 +1,179 @@ +using Daqifi.Core.Communication.Producers; +using Daqifi.Core.Device.Internal; +using Xunit; + +namespace Daqifi.Core.Tests.Device.Internal; + +/// +/// Unit tests for , which reads a command that has already +/// been sent and decides what it means for the streaming session (issue #379). These pin the +/// decision itself; DeviceReconnectTests pins the effects the device applies from it. +/// +public class SessionCommandInterpreterTests +{ + private const int MaxSamplingRate = 1000; + + [Theory] + [InlineData(null)] + [InlineData("")] + [InlineData(" ")] + public void ABlankCommand_MeansNothing(string? command) + { + var effect = SessionCommandInterpreter.Interpret(command, MaxSamplingRate); + + Assert.Equal(SessionCommandEffectKind.None, effect.Kind); + } + + [Theory] + [InlineData("SYSTem:REboot")] + [InlineData("DIO:PORt:ENAble 1")] + [InlineData("SYSTem:SYSInfoPB?")] + public void ACommandThatSaysNothingAboutTheSession_PassesThrough(string command) + { + // The global DIO enable is deliberately in this group: it is one switch for the whole port + // rather than a per-channel mask, so it carries no information about which digital channels + // were wanted and none is inferred. + var effect = SessionCommandInterpreter.Interpret(command, MaxSamplingRate); + + Assert.Equal(SessionCommandEffectKind.None, effect.Kind); + } + + [Fact] + public void AStopCommand_EndsTheSession() + { + var effect = SessionCommandInterpreter.Interpret("SYSTem:StopStreamData", MaxSamplingRate); + + Assert.Equal(SessionCommandEffectKind.StopStreaming, effect.Kind); + } + + [Fact] + public void TheProducersOwnCommands_AreTheOnesRecognized() + { + // The constants are duplicated from the producer's output rather than derived from it, so + // this is the guard that keeps the two from drifting apart silently. + var start = SessionCommandInterpreter.Interpret( + ScpiMessageProducer.StartStreaming(250).Data, MaxSamplingRate); + var stop = SessionCommandInterpreter.Interpret( + ScpiMessageProducer.StopStreaming.Data, MaxSamplingRate); + var enable = SessionCommandInterpreter.Interpret( + ScpiMessageProducer.EnableAdcChannels("10").Data, MaxSamplingRate); + + Assert.Equal(SessionCommandEffectKind.StartStreaming, start.Kind); + Assert.Equal(250, start.StreamingFrequency); + Assert.Equal(SessionCommandEffectKind.StopStreaming, stop.Kind); + Assert.Equal(SessionCommandEffectKind.SetAdcEnableMask, enable.Kind); + Assert.Equal(10u, enable.AdcEnableMask); + } + + [Theory] + [InlineData("systEM:startstreamdata 100")] + [InlineData("SYSTEM:STARTSTREAMDATA 100")] + public void CommandMatchingIsCaseInsensitive(string command) + { + // SCPI's short/long forms differ only in case, so a caller writing the long form must be + // recognized exactly like the producer's mixed-case output. + var effect = SessionCommandInterpreter.Interpret(command, MaxSamplingRate); + + Assert.Equal(SessionCommandEffectKind.StartStreaming, effect.Kind); + Assert.Equal(100, effect.StreamingFrequency); + } + + [Fact] + public void SurroundingWhitespace_DoesNotHideACommand() + { + var effect = SessionCommandInterpreter.Interpret(" SYSTem:StartStreamData 100 ", MaxSamplingRate); + + Assert.Equal(SessionCommandEffectKind.StartStreaming, effect.Kind); + Assert.Equal(100, effect.StreamingFrequency); + } + + [Theory] + [InlineData("SYSTem:StartStreamData")] // no argument at all + [InlineData("SYSTem:StartStreamData ")] // argument present but empty + [InlineData("SYSTem:StartStreamData abc")] // not a number + [InlineData("SYSTem:StartStreamData 100 extra")] // trailing junk + [InlineData("SYSTem:StartStreamData 0")] // below the usable range + [InlineData("SYSTem:StartStreamData -5")] // negative + [InlineData("SYSTem:StartStreamData 99999999")] // beyond the device's sampling rate + public void AStartCommandWithAnUnusableRate_IsNotAStart(string command) + { + // Reported as its own kind rather than as None so the caller can trace the rejection, but + // carrying no rate: marking a session started here would leave the streaming frequency + // holding a rate from an earlier session, which a reconnect would then faithfully restore. + var effect = SessionCommandInterpreter.Interpret(command, MaxSamplingRate); + + Assert.Equal(SessionCommandEffectKind.UnusableStreamingStart, effect.Kind); + Assert.Equal(0, effect.StreamingFrequency); + } + + [Fact] + public void ARejectedRate_IsReportedAsItWasWritten() + { + var effect = SessionCommandInterpreter.Interpret("SYSTem:StartStreamData not-a-rate ", MaxSamplingRate); + + Assert.Equal("not-a-rate", effect.RejectedRate); + } + + [Fact] + public void TheCeilingIsInclusive() + { + var atCeiling = SessionCommandInterpreter.Interpret("SYSTem:StartStreamData 1000", MaxSamplingRate); + var pastCeiling = SessionCommandInterpreter.Interpret("SYSTem:StartStreamData 1001", MaxSamplingRate); + + Assert.Equal(SessionCommandEffectKind.StartStreaming, atCeiling.Kind); + Assert.Equal(1000, atCeiling.StreamingFrequency); + Assert.Equal(SessionCommandEffectKind.UnusableStreamingStart, pastCeiling.Kind); + } + + [Theory] + [InlineData(0)] + [InlineData(-1)] + public void AnUnusableCeiling_FallsBackToOneRatherThanRejectingEverything(int maxSamplingRate) + { + // MaxSamplingRate is a mutable, unvalidated public property. An uninitialized value must not + // produce an impossible range like "1..0" that turns every rate into a rejection. + var atFloor = SessionCommandInterpreter.Interpret("SYSTem:StartStreamData 1", maxSamplingRate); + var aboveFloor = SessionCommandInterpreter.Interpret("SYSTem:StartStreamData 2", maxSamplingRate); + + Assert.Equal(SessionCommandEffectKind.StartStreaming, atFloor.Kind); + Assert.Equal(1, atFloor.StreamingFrequency); + Assert.Equal(SessionCommandEffectKind.UnusableStreamingStart, aboveFloor.Kind); + } + + [Fact] + public void StopIsNotMistakenForStart() + { + // Both commands share the "SYSTem:St" prefix, so the order the two are tested in is load + // bearing rather than incidental. + var effect = SessionCommandInterpreter.Interpret("SYSTem:StopStreamData", MaxSamplingRate); + + Assert.Equal(SessionCommandEffectKind.StopStreaming, effect.Kind); + } + + [Theory] + [InlineData("ENAble:VOLTage:DC 0", 0u)] + [InlineData("ENAble:VOLTage:DC 5", 5u)] + [InlineData("ENAble:VOLTage:DC 65535 ", 65535u)] + [InlineData("ENAble:VOLTage:DC 4294967295", uint.MaxValue)] + public void AnAdcEnableCommand_CarriesItsMask(string command, uint expected) + { + var effect = SessionCommandInterpreter.Interpret(command, MaxSamplingRate); + + Assert.Equal(SessionCommandEffectKind.SetAdcEnableMask, effect.Kind); + Assert.Equal(expected, effect.AdcEnableMask); + } + + [Theory] + [InlineData("ENAble:VOLTage:DC")] // no argument + [InlineData("ENAble:VOLTage:DC abc")] // not a number + [InlineData("ENAble:VOLTage:DC -1")] // negative + [InlineData("ENAble:VOLTage:DC 4294967296")] // past uint + public void AnUnparseableMask_ChangesNothing(string command) + { + // A mask that cannot be read is not evidence that no channels are enabled — clearing the + // set from it would be inventing a state the device never reported. + var effect = SessionCommandInterpreter.Interpret(command, MaxSamplingRate); + + Assert.Equal(SessionCommandEffectKind.None, effect.Kind); + } +} diff --git a/src/Daqifi.Core/Device/DaqifiStreamingDevice.cs b/src/Daqifi.Core/Device/DaqifiStreamingDevice.cs index 4d77a4b..cc73557 100644 --- a/src/Daqifi.Core/Device/DaqifiStreamingDevice.cs +++ b/src/Daqifi.Core/Device/DaqifiStreamingDevice.cs @@ -12,7 +12,6 @@ using System; using System.Collections.Generic; using System.Diagnostics; -using System.Globalization; using System.IO; using System.Linq; using System.Net; @@ -361,15 +360,6 @@ public void StopStreaming() #region Session state tracking for commands sent directly (issue #379) - /// The command emits. - private const string StartStreamingCommand = "SYSTem:StartStreamData"; - - /// The command emits. - private const string StopStreamingCommand = "SYSTem:StopStreamData"; - - /// The command emits. - private const string EnableAdcChannelsCommand = "ENAble:VOLTage:DC"; - /// /// Sends a command, and keeps this device's view of the streaming session in step with it. /// @@ -379,21 +369,13 @@ public void StopStreaming() /// example CLI does the whole job that way — but a session driven through it used to be /// completely invisible to this class: stayed false while /// data poured in, and the enabled-channel set stayed empty. That mattered once reconnect - /// arrived (issue #379). A physical cable pull on the bench recovered the link and then - /// reported StreamingResumed: false, because as far as Core was concerned nothing had - /// ever been streaming — while re-initialization had in fact just stopped the stream that - /// was running. The session looked restored and was not. + /// arrived (issue #379). reads the commands that + /// define a session; this method applies what they imply, so the same state is updated that + /// / and + /// would have set. /// /// - /// So the two commands that define a streaming session are recognized here regardless of - /// which API produced them, and the same state is updated that - /// / and - /// would have set. This is the same principle as #409, where - /// analog IsEnabled is resynced from the device's own reported mask: what Core - /// believes about a session has to track what is actually true of it. - /// - /// - /// Only these commands are interpreted, and only after the send itself has succeeded. + /// Only those commands are interpreted, and only after the send itself has succeeded. /// Everything else passes through untouched. /// /// @@ -421,13 +403,6 @@ public void StopStreaming() /// /// /// - /// Deliberately outside it. The global DIO enable is one switch for the whole port - /// rather than a per-channel mask, so a raw DIO:PORt:ENAble carries no information - /// about which digital channels were wanted and none is inferred. Argument validation - /// is not replayed either: by the time a command is seen here it has already gone to the - /// device, and the device is the authority on whether it accepted it. - /// - /// /// Running after the send is safe rather than merely convenient. A string command is handed /// to the background producer, so it has not reached the wire when this runs, and the device /// cannot answer a command it has not received; on the producer-less path that writes @@ -452,100 +427,55 @@ public override void Send(IOutboundMessage message) /// /// Updates the streaming-session view from a command that has just been sent. /// + /// + /// The sampling ceiling is read exactly once and handed to the interpreter, which validates + /// the rate against it; the value that comes back is then assigned to the backing field + /// rather than through the validating setter. That setter + /// re-reads , which is a mutable public + /// property — so validating against one read and assigning through a setter that takes + /// another would let a concurrent capabilities update throw out of a + /// whose command has already gone to the device. Tracking a command must never be able to + /// fail the send that carried it. + /// private void TrackSessionCommand(string? command) { - if (string.IsNullOrWhiteSpace(command)) - { - return; - } + var effect = SessionCommandInterpreter.Interpret(command, Metadata.Capabilities.MaxSamplingRate); - var trimmed = command.Trim(); - - if (trimmed.StartsWith(StopStreamingCommand, StringComparison.OrdinalIgnoreCase)) - { - IsStreaming = false; - return; - } - - if (trimmed.StartsWith(StartStreamingCommand, StringComparison.OrdinalIgnoreCase)) - { - TrackStreamingStart(trimmed.AsSpan(StartStreamingCommand.Length)); - return; - } - - if (trimmed.StartsWith(EnableAdcChannelsCommand, StringComparison.OrdinalIgnoreCase)) + switch (effect.Kind) { - TrackAdcEnableMask(trimmed.AsSpan(EnableAdcChannelsCommand.Length)); - } - } + case SessionCommandEffectKind.StopStreaming: + IsStreaming = false; + break; + + case SessionCommandEffectKind.StartStreaming: + // Frequency first: anything observing IsStreaming must never catch it true next + // to a rate belonging to a previous session. + _streamingFrequency = effect.StreamingFrequency; + + // A restart while already streaming is not a session boundary — the typed API + // cannot even express it (StartStreaming returns early) — so there is nothing to + // re-anchor and recording the new rate is all that is warranted. + if (!IsStreaming) + { + // A session is beginning, so it gets exactly the preparation StartStreaming + // would have given it. Ordering matches too: the state is ready before the + // flag flips. + BeginStreamingSession(); + IsStreaming = true; + } - /// - /// Records a start-streaming command, but only one carrying a rate this device can model. - /// - /// - /// - /// A command whose argument is missing, unparseable, or outside the device's sampling range - /// is not treated as the start of a session. Marking one as streaming anyway would be - /// wrong three times over: the firmware rejects such a command and does not start streaming, - /// so the flag would not describe the device; would be left - /// holding a rate from some earlier session, which a reconnect would then faithfully restore - /// — resuming at a rate nobody asked for is the silent-wrong-data failure this whole feature - /// exists to prevent; and a stale makes the next legitimate - /// a silent no-op, which is the same stale-flag trap that - /// issue #118 and the defensive stops scattered through the SD paths already guard against. - /// - /// - /// The existing state is left alone rather than cleared. A device already streaming at a - /// good rate goes on doing exactly that when the firmware rejects a malformed start, so - /// and both remain true of it; - /// forcing them off would swap one inaccuracy for another. - /// - /// - /// Because of this, is never true alongside a rate that was - /// not validated — so session restore has no "streaming at an unknown rate" case to decide - /// what to do about. The state it replays is always one that was really commanded. - /// - /// - private void TrackStreamingStart(ReadOnlySpan argument) - { - var rate = argument.Trim(); - - // One read of the ceiling, used for both the check and the assignment below. The public - // StreamingFrequency setter re-reads it and throws when it does not like the value, and - // MaxSamplingRate is a mutable public property — so validating against one read and - // then assigning through a setter that takes another would let a concurrent - // capabilities update throw out of a Send whose command has already gone to the device. - // Tracking a command must never be able to fail the send that carried it. - var maxSamplingRate = Math.Max(1, Metadata.Capabilities.MaxSamplingRate); - - if (!int.TryParse(rate, NumberStyles.Integer, CultureInfo.InvariantCulture, out var frequency) - || frequency < 1 - || frequency > maxSamplingRate) - { - SafeTrace( - $"[{nameof(TrackStreamingStart)}] Ignoring a start-streaming command with an unusable rate " - + $"('{rate.ToString()}'); the session state is unchanged."); - return; - } + break; - // Frequency first: anything observing IsStreaming must never catch it true next to a - // rate belonging to a previous session. Assigned to the backing field, not through the - // validating setter, for the reason above — the value has just been validated against - // the same rule. - _streamingFrequency = frequency; + case SessionCommandEffectKind.UnusableStreamingStart: + SafeTrace( + $"[{nameof(TrackSessionCommand)}] Ignoring a start-streaming command with an unusable rate " + + $"('{effect.RejectedRate}'); the session state is unchanged."); + break; - if (IsStreaming) - { - // A restart while already streaming. The typed API cannot even express this - // (StartStreaming returns early), so there is no session boundary to re-anchor at - // and no equivalence to preserve; recording the new rate is all that is warranted. - return; + case SessionCommandEffectKind.SetAdcEnableMask: + ApplyAdcEnableMask(effect.AdcEnableMask); + break; } - - // A session is beginning, so it gets exactly the preparation StartStreaming would have - // given it. Ordering matches too: the state is ready before the flag flips. - BeginStreamingSession(); - IsStreaming = true; } /// @@ -559,13 +489,8 @@ private void TrackStreamingStart(ReadOnlySpan argument) /// next status frame (#409) — that is the device's own view, and it outranks what was asked /// for. /// - private void TrackAdcEnableMask(ReadOnlySpan argument) + private void ApplyAdcEnableMask(uint mask) { - if (!uint.TryParse(argument.Trim(), NumberStyles.Integer, CultureInfo.InvariantCulture, out var mask)) - { - return; - } - WithChannelsLock(() => { foreach (var channel in SnapshotChannels()) diff --git a/src/Daqifi.Core/Device/Internal/SessionCommandInterpreter.cs b/src/Daqifi.Core/Device/Internal/SessionCommandInterpreter.cs new file mode 100644 index 0000000..5850bb2 --- /dev/null +++ b/src/Daqifi.Core/Device/Internal/SessionCommandInterpreter.cs @@ -0,0 +1,227 @@ +using System; +using System.Globalization; + +#nullable enable + +namespace Daqifi.Core.Device.Internal +{ + /// + /// What a command that has just been sent means for the device's view of its streaming session. + /// + internal enum SessionCommandEffectKind + { + /// The command says nothing about the session; the device state is untouched. + None, + + /// The session has ended. + StopStreaming, + + /// + /// A session is starting, or a running one is changing rate, at + /// . + /// + StartStreaming, + + /// + /// A start-streaming command carrying a rate this device cannot model. Reported rather than + /// folded into so the caller can still trace it — see + /// for why it changes no state. + /// + UnusableStreamingStart, + + /// + /// An ADC enable bitmask was sent; carries it. + /// + SetAdcEnableMask, + } + + /// + /// The decision reached about one sent command, together + /// with the value it carries. + /// + internal readonly struct SessionCommandEffect + { + private SessionCommandEffect( + SessionCommandEffectKind kind, + int streamingFrequency, + uint adcEnableMask, + string? rejectedRate) + { + Kind = kind; + StreamingFrequency = streamingFrequency; + AdcEnableMask = adcEnableMask; + RejectedRate = rejectedRate; + } + + /// Gets what the command means for the session. + public SessionCommandEffectKind Kind { get; } + + /// + /// Gets the validated rate, in Hz, when is + /// . Zero otherwise. + /// + public int StreamingFrequency { get; } + + /// + /// Gets the bitmask when is + /// . Zero otherwise. + /// + public uint AdcEnableMask { get; } + + /// + /// Gets the rate argument as it was written, when is + /// . Null otherwise. Present + /// only so the rejection can be traced with the text that caused it. + /// + public string? RejectedRate { get; } + + /// The command carries nothing this device tracks. + public static SessionCommandEffect None { get; } = new(SessionCommandEffectKind.None, 0, 0, null); + + /// The session has ended. + public static SessionCommandEffect StopStreaming { get; } = new(SessionCommandEffectKind.StopStreaming, 0, 0, null); + + /// A session is running at Hz. + public static SessionCommandEffect StartStreaming(int frequency) => + new(SessionCommandEffectKind.StartStreaming, frequency, 0, null); + + /// A start command whose rate argument, , is unusable. + public static SessionCommandEffect UnusableStreamingStart(string rejectedRate) => + new(SessionCommandEffectKind.UnusableStreamingStart, 0, 0, rejectedRate); + + /// An ADC enable bitmask of was sent. + public static SessionCommandEffect SetAdcEnableMask(uint mask) => + new(SessionCommandEffectKind.SetAdcEnableMask, 0, mask, null); + } + + /// + /// Reads a SCPI command that has just been sent and decides what it means for the streaming + /// session, so a session driven through the raw path stays as + /// visible to Core as one driven through the typed API (issue #379). + /// + /// + /// + /// is public and is a perfectly ordinary way to drive a device + /// — the example CLI does the whole job that way — but a session driven through it used to be + /// completely invisible: stayed false + /// while data poured in, and the enabled-channel set stayed empty. That mattered once reconnect + /// arrived. A physical cable pull on the bench recovered the link and then reported + /// StreamingResumed: false, because as far as Core was concerned nothing had ever been + /// streaming — while re-initialization had in fact just stopped the stream that was running. + /// The session looked restored and was not. + /// + /// + /// So the commands that define a streaming session are recognized here regardless of which API + /// produced them, and the caller applies the same state the typed methods would have set. This + /// is the same principle as #409, where analog IsEnabled is resynced from the device's own + /// reported mask: what Core believes about a session has to track what is actually true of it. + /// + /// + /// Deciding is separated from applying on purpose. The decision is pure text-and-arithmetic and + /// is pinned directly by SessionCommandInterpreterTests; the effects it implies — + /// re-anchoring the session, flipping the flag, assigning channel state under the device's lock — + /// stay on the device, which is the only thing that owns them. + /// + /// + /// Deliberately outside the scope. The global DIO enable is one switch for the whole port + /// rather than a per-channel mask, so a raw DIO:PORt:ENAble carries no information about + /// which digital channels were wanted and none is inferred. Argument validation is not + /// replayed either: by the time a command is read here it has already gone to the device, and + /// the device is the authority on whether it accepted it. + /// + /// + internal static class SessionCommandInterpreter + { + /// The command emits. + internal const string StartStreamingCommand = "SYSTem:StartStreamData"; + + /// The command emits. + internal const string StopStreamingCommand = "SYSTem:StopStreamData"; + + /// The command emits. + internal const string EnableAdcChannelsCommand = "ENAble:VOLTage:DC"; + + /// + /// Decides what means for the streaming session. + /// + /// + /// + /// A start command whose argument is missing, unparseable, or outside the device's sampling + /// range comes back as rather + /// than as the start of a session. Treating one as a start would be wrong three times over: + /// the firmware rejects such a command and does not start streaming, so the flag would not + /// describe the device; the streaming frequency would be left holding a rate from some + /// earlier session, which a reconnect would then faithfully restore — resuming at a rate + /// nobody asked for is the silent-wrong-data failure this whole feature exists to prevent; + /// and a stale streaming flag makes the next legitimate + /// a silent no-op, which is the same trap + /// issue #118 and the defensive stops scattered through the SD paths already guard against. + /// + /// + /// The existing state is therefore left alone rather than cleared. A device already streaming + /// at a good rate goes on doing exactly that when the firmware rejects a malformed start, so + /// both its flag and its rate remain true of it; forcing them off would swap one inaccuracy + /// for another. Because of this, the device is never marked streaming alongside a rate that + /// was not validated — so session restore has no "streaming at an unknown rate" case to + /// decide what to do about. + /// + /// + /// The command text that was just sent; null or blank yields . + /// + /// The device's advertised maximum sampling rate. Passed in as a single read by the caller + /// rather than read here twice: it is a mutable public property, so validating against one + /// read and applying against another would let a concurrent capabilities update reject a + /// command that has already reached the device. Sanitized with a floor of 1 so an + /// uninitialized or invalid value (0 or negative) cannot produce an impossible range like + /// "1..0" that rejects every rate. + /// + public static SessionCommandEffect Interpret(string? command, int maxSamplingRate) + { + if (string.IsNullOrWhiteSpace(command)) + { + return SessionCommandEffect.None; + } + + var trimmed = command!.Trim(); + + if (trimmed.StartsWith(StopStreamingCommand, StringComparison.OrdinalIgnoreCase)) + { + return SessionCommandEffect.StopStreaming; + } + + if (trimmed.StartsWith(StartStreamingCommand, StringComparison.OrdinalIgnoreCase)) + { + return InterpretStreamingStart(trimmed.AsSpan(StartStreamingCommand.Length), maxSamplingRate); + } + + if (trimmed.StartsWith(EnableAdcChannelsCommand, StringComparison.OrdinalIgnoreCase)) + { + return InterpretAdcEnableMask(trimmed.AsSpan(EnableAdcChannelsCommand.Length)); + } + + return SessionCommandEffect.None; + } + + private static SessionCommandEffect InterpretStreamingStart(ReadOnlySpan argument, int maxSamplingRate) + { + var rate = argument.Trim(); + var ceiling = Math.Max(1, maxSamplingRate); + + if (!int.TryParse(rate, NumberStyles.Integer, CultureInfo.InvariantCulture, out var frequency) + || frequency < 1 + || frequency > ceiling) + { + return SessionCommandEffect.UnusableStreamingStart(rate.ToString()); + } + + return SessionCommandEffect.StartStreaming(frequency); + } + + private static SessionCommandEffect InterpretAdcEnableMask(ReadOnlySpan argument) + { + return uint.TryParse(argument.Trim(), NumberStyles.Integer, CultureInfo.InvariantCulture, out var mask) + ? SessionCommandEffect.SetAdcEnableMask(mask) + : SessionCommandEffect.None; + } + } +}