Improve performance for large SPI buffer transfers (#1841)

- Support waiting for SPI write commands. When using fast data rates, a buffer overrun
will otherwise occur
- Write SPI data in 7-bit encoding format (requires firmata update)
- Make flow control parameter controllable from outside
- Various stability fixes

src/devices/Arduino/ArduinoBoard.cs

@@ -63,12 +63,27 @@ public class ArduinoBoard : Board.Board, IDisposable
         /// The device is initialized when the first command is sent. The constructor always succeeds.
         /// </remarks>
         /// <param name="serialPortStream">A stream to an Arduino/Firmata device</param>
-        public ArduinoBoard(Stream serialPortStream)
+        /// <param name="usesHardwareFlowControl">True to indicate that the stream supports hardware flow control (can be a serial port
+        /// with RTS/CTS handshake or a network stream where the protocol already supports flow control)</param>
+        public ArduinoBoard(Stream serialPortStream, bool usesHardwareFlowControl)
         {
             _dataStream = serialPortStream ?? throw new ArgumentNullException(nameof(serialPortStream));
+            StreamUsesHardwareFlowControl = usesHardwareFlowControl;
             _logger = this.GetCurrentClassLogger();
         }
 
+        /// <summary>
+        /// Creates an instance of an Ardino board connection using the given stream (typically from a serial port)
+        /// </summary>
+        /// <remarks>
+        /// The device is initialized when the first command is sent. The constructor always succeeds.
+        /// </remarks>
+        /// <param name="serialPortStream">A stream to an Arduino/Firmata device</param>
+        public ArduinoBoard(Stream serialPortStream)
+        : this(serialPortStream, false)
+        {
+        }
+
         /// <summary>
         /// Creates an instance of the Arduino board connection connected to a serial port
         /// </summary>
@@ -80,6 +95,7 @@ public ArduinoBoard(string portName, int baudRate)
         {
             _dataStream = null;
             _serialPort = new SerialPort(portName, baudRate);
+            StreamUsesHardwareFlowControl = false; // Would need to configure the serial port externally for this to work
             _logger = this.GetCurrentClassLogger();
         }
 
@@ -88,6 +104,16 @@ public ArduinoBoard(string portName, int baudRate)
         /// </summary>
         protected ILogger Logger => _logger;
 
+        /// <summary>
+        /// Set this to true if the underlying stream uses some kind of hardware or low-level flow control (RTS/CTS for
+        /// a serial port, or a TCP socket). Setting this to true may improve performance on bulk transfers (such as
+        /// large SPI blocks) but can result in buffer overflows if flow control is not working. Default: false
+        /// </summary>
+        public bool StreamUsesHardwareFlowControl
+        {
+            get;
+        }
+
         /// <summary>
         /// The list of supported pin modes.
         /// This list can be extended by adding special modes using <see cref="AddCommandHandler{T}"/>.
@@ -162,14 +188,47 @@ public IReadOnlyList<SupportedMode> KnownModes
 #endif
             out ArduinoBoard? board)
         {
-            board = null;
+            return TryConnectToNetworkedBoard(boardAddress, port, true, out board);
+        }
+
+        /// <summary>
+        /// Tries to connect to an arduino over network.
+        /// This requires an arduino with an ethernet shield or an ESP32 with enabled WIFI support.
+        /// </summary>
+        /// <param name="boardAddress">The IP address of the board</param>
+        /// <param name="port">The network port to use. The default port is 27016</param>
+        /// <param name="useAutoReconnect">True to use an auto-reconnecting stream. Helpful when using an unreliable connection.</param>
+        /// <param name="board">Returns the board if successful</param>
+        /// <returns>True on success, false otherwise</returns>
+        public static bool TryConnectToNetworkedBoard(IPAddress boardAddress, int port, bool useAutoReconnect,
+#if NET5_0_OR_GREATER
+            [NotNullWhen(true)]
+#endif
+            out ArduinoBoard? board)
+        {
             try
             {
-                var socket = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp);
-                socket.Connect(boardAddress, port);
-                socket.NoDelay = true;
-                var networkStream = new NetworkStream(socket, true);
-                board = new ArduinoBoard(networkStream);
+                Stream networkStream;
+                if (useAutoReconnect)
+                {
+                    networkStream = new ReconnectingNetworkStream(() =>
+                    {
+                        var socket = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp);
+                        socket.Connect(boardAddress, port);
+                        socket.NoDelay = true;
+                        Stream socketStream = new NetworkStream(socket, true);
+                        return socketStream;
+                    });
+                }
+                else
+                {
+                    var socket = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp);
+                    socket.Connect(boardAddress, port);
+                    socket.NoDelay = true;
+                    networkStream = new NetworkStream(socket, true);
+                }
+
+                board = new ArduinoBoard(networkStream, true);
                 if (!(board.FirmataVersion > new Version(1, 0)))
                 {
                     // Actually not expecting to get here (but the above will throw a SocketException if the remote end is not there)
@@ -180,6 +239,7 @@ public IReadOnlyList<SupportedMode> KnownModes
             }
             catch (SocketException)
             {
+                board = null;
                 return false;
             }
         }
@@ -508,7 +568,21 @@ private void FirmataOnError(string message, Exception? exception)
             }
             else
             {
-                Logger.LogInformation(message);
+                // If the message contains a line feed, strip that
+                Logger.LogInformation(message.TrimEnd(new char[] { '\r', '\n' }));
+            }
+
+            _commandHandlersLock.EnterReadLock();
+            try
+            {
+                foreach (var handler in _extendedCommandHandlers)
+                {
+                    handler.OnErrorMessage(message, exception);
+                }
+            }
+            finally
+            {
+                _commandHandlersLock.ExitReadLock();
             }
         }
 
@@ -561,7 +635,12 @@ public override GpioController CreateGpioController()
         {
             Initialize();
 
-            return new GpioController(PinNumberingScheme.Logical, new ArduinoGpioControllerDriver(this, _supportedPinConfigurations));
+            if (_firmata == null)
+            {
+                throw new ObjectDisposedException(nameof(_firmata));
+            }
+
+            return new GpioController(PinNumberingScheme.Logical, new ArduinoGpioControllerDriver(_firmata, _supportedPinConfigurations));
         }
 
         /// <inheritdoc />
@@ -691,6 +770,16 @@ public void SetAnalogPinSamplingInterval(TimeSpan timeSpan)
             Firmata.SetAnalogInputSamplingInterval(timeSpan);
         }
 
+        /// <summary>
+        /// Performs a software reset of the Arduino firmware
+        /// </summary>
+        public void SoftwareReset()
+        {
+            Initialize();
+            Firmata.SendSoftwareReset();
+            Firmata.QueryCapabilities();
+        }
+
         /// <summary>
         /// Standard dispose pattern
         /// </summary>
@@ -708,6 +797,12 @@ protected override void Dispose(bool disposing)
                 }
             }
 
+            if (_firmata != null)
+            {
+                // Can end the next possible moment (otherwise might just throw a bunch of warnings before actually terminating anyway)
+                _firmata.InputThreadShouldExit = true;
+            }
+
             _isDisposed = true;
             // Do this first, to force any blocking read operations to end
             if (_dataStream != null)
@@ -754,5 +849,43 @@ internal void DisableSpi()
                 Firmata.DisableSpi();
             }
         }
+
+        /// <summary>
+        /// Pings the device, to get an estimate about the round-trip time.
+        /// With some Wifi setups, the round trip time may be significantly higher than desired.
+        /// </summary>
+        /// <param name="number">The number of pings to send</param>
+        /// <returns>The list of reply times. Contains a negative value for lost packets</returns>
+        public List<TimeSpan> Ping(int number)
+        {
+            Initialize();
+            if (_firmata == null)
+            {
+                throw new ObjectDisposedException("Not connected");
+            }
+
+            List<TimeSpan> ret = new();
+            Stopwatch sw = new Stopwatch();
+            for (int i = 0; i < number; i++)
+            {
+                sw.Restart();
+                try
+                {
+                    _firmata.QueryFirmwareVersion(out _);
+                    var elapsed = sw.Elapsed;
+                    ret.Add(elapsed);
+                    _logger.LogInformation($"Round trip time: {elapsed.TotalMilliseconds}ms");
+                }
+                catch (TimeoutException x)
+                {
+                    _logger.LogError(x, $"Timeout: {x.Message}");
+                    ret.Add(TimeSpan.FromMinutes(-1));
+                }
+
+                sw.Stop();
+            }
+
+            return ret;
+        }
     }
 }

src/devices/Arduino/ArduinoGpioControllerDriver.cs

@@ -14,26 +14,28 @@ namespace Iot.Device.Arduino
 {
     internal class ArduinoGpioControllerDriver : GpioDriver
     {
-        private readonly ArduinoBoard _arduinoBoard;
+        private readonly FirmataDevice _device;
         private readonly IReadOnlyCollection<SupportedPinConfiguration> _supportedPinConfigurations;
         private readonly Dictionary<int, CallbackContainer> _callbackContainers;
         private readonly ConcurrentDictionary<int, PinMode> _pinModes;
         private readonly object _callbackContainersLock;
         private readonly AutoResetEvent _waitForEventResetEvent;
         private readonly ILogger _logger;
+        private readonly ConcurrentDictionary<int, PinValue?> _outputPinValues;
 
-        internal ArduinoGpioControllerDriver(ArduinoBoard arduinoBoard, IReadOnlyCollection<SupportedPinConfiguration> supportedPinConfigurations)
+        internal ArduinoGpioControllerDriver(FirmataDevice device, IReadOnlyCollection<SupportedPinConfiguration> supportedPinConfigurations)
         {
-            _arduinoBoard = arduinoBoard ?? throw new ArgumentNullException(nameof(arduinoBoard));
+            _device = device ?? throw new ArgumentNullException(nameof(device));
             _supportedPinConfigurations = supportedPinConfigurations ?? throw new ArgumentNullException(nameof(supportedPinConfigurations));
             _callbackContainers = new Dictionary<int, CallbackContainer>();
             _waitForEventResetEvent = new AutoResetEvent(false);
             _callbackContainersLock = new object();
             _pinModes = new ConcurrentDictionary<int, PinMode>();
+            _outputPinValues = new ConcurrentDictionary<int, PinValue?>();
             _logger = this.GetCurrentClassLogger();
 
             PinCount = _supportedPinConfigurations.Count;
-            _arduinoBoard.Firmata.DigitalPortValueUpdated += FirmataOnDigitalPortValueUpdated;
+            _device.DigitalPortValueUpdated += FirmataOnDigitalPortValueUpdated;
         }
 
         protected override int PinCount { get; }
@@ -48,6 +50,10 @@ protected override int ConvertPinNumberToLogicalNumberingScheme(int pinNumber)
 
         protected override void OpenPin(int pinNumber)
         {
+            if (pinNumber < 0 || pinNumber >= PinCount)
+            {
+                throw new ArgumentOutOfRangeException(nameof(pinNumber), $"Pin {pinNumber} is not valid");
+            }
         }
 
         protected override void ClosePin(int pinNumber)
@@ -65,9 +71,11 @@ protected override void SetPinMode(int pinNumber, PinMode mode)
                     break;
                 case PinMode.InputPullUp:
                     firmataMode = SupportedMode.InputPullup;
+                    _outputPinValues.TryRemove(pinNumber, out _);
                     break;
                 case PinMode.Input:
                     firmataMode = SupportedMode.DigitalInput;
+                    _outputPinValues.TryRemove(pinNumber, out _);
                     break;
                 default:
                     _logger.LogError($"Mode {mode} is not supported as argument to {nameof(SetPinMode)}");
@@ -81,7 +89,7 @@ protected override void SetPinMode(int pinNumber, PinMode mode)
                 throw new NotSupportedException($"Mode {mode} is not supported on Pin {pinNumber}.");
             }
 
-            _arduinoBoard.Firmata.SetPinMode(pinNumber, firmataMode);
+            _device.SetPinMode(pinNumber, firmataMode);
 
             // Cache this value. Since the GpioController calls GetPinMode when trying to write a pin (to verify whether it is set to output),
             // that would be very expensive here. And setting output pins should be cheap.
@@ -95,7 +103,7 @@ protected override PinMode GetPinMode(int pinNumber)
                 return existingValue;
             }
 
-            var mode = _arduinoBoard.Firmata.GetPinMode(pinNumber);
+            var mode = _device.GetPinMode(pinNumber);
 
             PinMode ret;
             if (mode == SupportedMode.DigitalOutput.Value)
@@ -144,12 +152,22 @@ protected override bool IsPinModeSupported(int pinNumber, PinMode mode)
 
         protected override PinValue Read(int pinNumber)
         {
-            return _arduinoBoard.Firmata.ReadDigitalPin(pinNumber);
+            return _device.ReadDigitalPin(pinNumber);
         }
 
         protected override void Write(int pinNumber, PinValue value)
         {
-            _arduinoBoard.Firmata.WriteDigitalPin(pinNumber, value);
+            if (_outputPinValues.TryGetValue(pinNumber, out PinValue? existingValue) && existingValue != null)
+            {
+                // If this output value is already present, don't send a message.
+                if (value == existingValue.Value)
+                {
+                    return;
+                }
+            }
+
+            _device.WriteDigitalPin(pinNumber, value);
+            _outputPinValues.AddOrUpdate(pinNumber, x => value, (y, z) => value);
         }
 
         protected override WaitForEventResult WaitForEvent(int pinNumber, PinEventTypes eventTypes, CancellationToken cancellationToken)
@@ -173,7 +191,7 @@ void WaitForEventPortValueUpdated(object sender, PinValueChangedEventArgs e)
                 }
             }
 
-            _arduinoBoard.Firmata.DigitalPortValueUpdated += WaitForEventPortValueUpdated;
+            _device.DigitalPortValueUpdated += WaitForEventPortValueUpdated;
             try
             {
                 WaitHandle.WaitAny(new[] { cancellationToken.WaitHandle, _waitForEventResetEvent });
@@ -188,7 +206,7 @@ void WaitForEventPortValueUpdated(object sender, PinValueChangedEventArgs e)
             }
             finally
             {
-                _arduinoBoard.Firmata.DigitalPortValueUpdated -= WaitForEventPortValueUpdated;
+                _device.DigitalPortValueUpdated -= WaitForEventPortValueUpdated;
             }
 
             return new WaitForEventResult()
@@ -265,7 +283,8 @@ protected override void Dispose(bool disposing)
                     _callbackContainers.Clear();
                 }
 
-                _arduinoBoard.Firmata.DigitalPortValueUpdated -= FirmataOnDigitalPortValueUpdated;
+                _outputPinValues.Clear();
+                _device.DigitalPortValueUpdated -= FirmataOnDigitalPortValueUpdated;
             }
 
             base.Dispose(disposing);

src/devices/Arduino/ArduinoI2cBus.cs

@@ -15,6 +15,7 @@ internal class ArduinoI2cBus : I2cBus
         private readonly ArduinoBoard _board;
         private readonly int _busId;
         private readonly HashSet<int> _usedAddresses;
+        private bool _busInitialized;
 
         public ArduinoI2cBus(ArduinoBoard board, int busId)
         {
@@ -31,6 +32,18 @@ public override I2cDevice CreateDevice(int deviceAddress)
                 throw new InvalidOperationException($"Device number {deviceAddress} is already in use");
             }
 
+            if (!_busInitialized)
+            {
+                // Ensure the corresponding pins are set to I2C (not strictly necessary, but consistent)
+                foreach (SupportedPinConfiguration supportedPinConfiguration in _board.SupportedPinConfigurations.Where(x => x.PinModes.Contains(SupportedMode.I2c)))
+                {
+                    _board.Firmata.SetPinMode(supportedPinConfiguration.Pin, SupportedMode.I2c);
+                }
+
+                _board.Firmata.SendI2cConfigCommand();
+                _busInitialized = true;
+            }
+
             var device = new ArduinoI2cDevice(_board, this, new I2cConnectionSettings(_busId, deviceAddress));
             _usedAddresses.Add(deviceAddress);
             return device;

src/devices/Arduino/ArduinoSpiDevice.cs

@@ -50,7 +50,7 @@ public override void WriteByte(byte value)
 
         public override void Write(ReadOnlySpan<byte> buffer)
         {
-            Board.Firmata.SpiWrite(ConnectionSettings.ChipSelectLine, buffer);
+            Board.Firmata.SpiWrite(ConnectionSettings.ChipSelectLine, buffer, !Board.StreamUsesHardwareFlowControl);
         }
 
         public override void TransferFullDuplex(ReadOnlySpan<byte> writeBuffer, Span<byte> readBuffer)