Fixing the high resolution version of SoftwarePwmChannel (#1075)

* Fixing the high resolution version of SoftwarePwmChannel

* Fixing an xml comment

* Resolving build errors

* Updating SoftwarePwm to use DelayHelper and adding a DelayHelper that takes TimeSpan

src/devices/Common/System/Device/DelayHelper.cs

@@ -12,23 +12,32 @@ namespace System.Device
     /// </summary>
     internal static class DelayHelper
     {
-        // GetTimestamp() currently can take ~300ns. We hope to improve this to get better
-        // fidelity for very tight spins.
-        //
-        // SpinWait currently spins to approximately 1μs before it will yield the thread.
+        /* GetTimestamp() currently can take ~300ns. We hope to improve this to get better
+         * fidelity for very tight spins.
+         *
+         * SpinWait currently spins to approximately 1μs before it will yield the thread.
+         */
+
+        private const long TicksPerSecond = TimeSpan.TicksPerSecond;
+        private const long TicksPerMillisecond = TimeSpan.TicksPerMillisecond;
+        private const long TicksPerMicrosecond = TimeSpan.TicksPerMillisecond / 1000;
+
+        /// <summary>A scale that normalizes the hardware ticks to <see cref="TimeSpan" /> ticks which are 100ns in length.</summary>
+        private static readonly double s_tickFrequency = (double)TicksPerSecond / Stopwatch.Frequency;
 
         /// <summary>
-        /// Delay for at least the specified <paramref name="microseconds"/>.
+        /// Delay for at least the specified <paramref name="time" />.
         /// </summary>
-        /// <param name="microseconds">The number of microseconds to delay.</param>
+        /// <param name="time">The amount of time to delay.</param>
         /// <param name="allowThreadYield">
         /// True to allow yielding the thread. If this is set to false, on single-proc systems
         /// this will prevent all other code from running.
         /// </param>
-        public static void DelayMicroseconds(int microseconds, bool allowThreadYield)
+        public static void Delay(TimeSpan time, bool allowThreadYield)
         {
             long start = Stopwatch.GetTimestamp();
-            ulong minimumTicks = (ulong)(microseconds * Stopwatch.Frequency / 1_000_000);
+            long delta = (long)(time.Ticks / s_tickFrequency);
+            long target = start + delta;
 
             if (!allowThreadYield)
             {
@@ -36,7 +45,7 @@ namespace System.Device
                 {
                     Thread.SpinWait(1);
                 }
-                while ((ulong)(Stopwatch.GetTimestamp() - start) < minimumTicks);
+                while (Stopwatch.GetTimestamp() < target);
             }
             else
             {
@@ -45,10 +54,24 @@ namespace System.Device
                 {
                     spinWait.SpinOnce();
                 }
-                while ((ulong)(Stopwatch.GetTimestamp() - start) < minimumTicks);
+                while (Stopwatch.GetTimestamp() < target);
             }
         }
 
+        /// <summary>
+        /// Delay for at least the specified <paramref name="microseconds"/>.
+        /// </summary>
+        /// <param name="microseconds">The number of microseconds to delay.</param>
+        /// <param name="allowThreadYield">
+        /// True to allow yielding the thread. If this is set to false, on single-proc systems
+        /// this will prevent all other code from running.
+        /// </param>
+        public static void DelayMicroseconds(int microseconds, bool allowThreadYield)
+        {
+            var time = TimeSpan.FromTicks(microseconds * TicksPerMicrosecond);
+            Delay(time, allowThreadYield);
+        }
+
         /// <summary>
         /// Delay for at least the specified <paramref name="milliseconds"/>
         /// </summary>
@@ -59,11 +82,14 @@ namespace System.Device
         /// </param>
         public static void DelayMilliseconds(int milliseconds, bool allowThreadYield)
         {
-            // We have this as a separate method for now to make calling code clearer
-            // and to allow us to add additional logic to the millisecond wait in the
-            // future. If waiting only 1 millisecond we still have ample room for more
-            // complicated logic. For 1 microsecond that isn't the case.
-            DelayMicroseconds(milliseconds * 1000, allowThreadYield);
+            /* We have this as a separate method for now to make calling code clearer
+             * and to allow us to add additional logic to the millisecond wait in the
+             * future. If waiting only 1 millisecond we still have ample room for more
+             * complicated logic. For 1 microsecond that isn't the case.
+             */
+
+            var time = TimeSpan.FromTicks(milliseconds * TicksPerMillisecond);
+            Delay(time, allowThreadYield);
         }
     }
 }

src/devices/SoftPwm/SoftwarePwm.csproj

@@ -8,6 +8,7 @@
 
   <ItemGroup>
     <Compile Include="SoftwarePwmChannel.cs" />
+    <Compile Include="..\Common\System\Device\DelayHelper.cs" Link="DelayHelper.cs" />
   </ItemGroup>
 
   <ItemGroup>

src/devices/SoftPwm/SoftwarePwmChannel.cs

@@ -2,51 +2,52 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 // See the LICENSE file in the project root for more information.
 
+using System;
 using System.Device.Gpio;
+using System.Device.Pwm;
 using System.Diagnostics;
 using System.Threading;
-using System.Threading.Tasks;
 
 namespace System.Device.Pwm.Drivers
 {
-    /// <summary>
-    /// Software PWM channel implementation
-    /// </summary>
+    /// <summary>Software PWM channel implementation</summary>
     public class SoftwarePwmChannel : PwmChannel
     {
+        private readonly int _pin;
+
         private readonly bool _shouldDispose;
-        // how long the signal is high in its period
-        private double _pulseWidthInMilliseconds;
-        private double _periodInMilliseconds;
+        private readonly bool _usePrecisionTimer;
+
+        /* Frequency represents the number of times the pin should "pulse" (go from low to high and back) per second
+         * DutyCycle represents the percentage of time the pin should be in the high state
+         *
+         * So, if the Frequency is 1 and the Duty Cycle is 0.5, the pin will go High once per second and stay on for 0.5 seconds
+         * While if the Frequency is 400 and the Duty Cycle is 0.5, the pin will go High 400 times per second staying on for 0.00125 seconds each time, for a total of 0.5 seconds
+         */
+
         private int _frequency;
-        // Use to determine the length of the pulse
-        // 100% = 1.0 = full output. 0% = 0.0 = nothing as output
         private double _dutyCycle;
 
-        // Determines if a high precision timer should be used.
-        private bool _usePrecisionTimer;
+        private GpioController _controller;
 
-        private bool _isRunning;
-        private bool _isStopped = true;
-        private int _pin;
+        private TimeSpan _pinHighTime;
+        private TimeSpan _pinLowTime;
 
-        private Stopwatch _stopwatch = Stopwatch.StartNew();
+        private Thread _thread;
 
-        private Thread _runningThread;
-        private GpioController _controller;
-        private bool _runThread = true;
+        private bool _isRunning;
+        private bool _isTerminating;
 
-        /// <summary>
-        /// The frequency in hertz.
-        /// </summary>
+        /// <summary>The frequency in hertz.</summary>
         public override int Frequency
         {
             get => _frequency;
+
             set
             {
                 if (value <= 0)
                 {
-                    throw new ArgumentOutOfRangeException(nameof(value), "Frequency must be a positive value.");
+                    throw new ArgumentOutOfRangeException(nameof(value), "Frequency must be a positive non-zero value.");
                 }
 
                 _frequency = value;
@@ -54,17 +55,16 @@ namespace System.Device.Pwm.Drivers
             }
         }
 
-        /// <summary>
-        /// The duty cycle percentage represented as a value between 0.0 and 1.0.
-        /// </summary>
+        /// <summary>The duty cycle percentage represented as a value between 0.0 and 1.0.</summary>
         public override double DutyCycle
         {
             get => _dutyCycle;
+
             set
             {
                 if (value < 0.0 || value > 1.0)
                 {
-                    throw new ArgumentOutOfRangeException(nameof(value), value, "Value must be between 0.0 and 1.0.");
+                    throw new ArgumentOutOfRangeException(nameof(value), value, "DutyCycle must be between 0.0 and 1.0 (inclusive).");
                 }
 
                 _dutyCycle = value;
@@ -72,16 +72,13 @@ namespace System.Device.Pwm.Drivers
             }
         }
 
-        /// <summary>
-        /// Initializes a new instance of the <see cref="SoftwarePwmChannel"/> class.
-        /// </summary>
+        /// <summary>Initializes a new instance of the <see cref="SoftwarePwmChannel"/> class.</summary>
         /// <param name="pinNumber">The GPIO pin number to be used</param>
         /// <param name="frequency">The frequency in hertz. Defaults to 400</param>
         /// <param name="dutyCycle">The duty cycle percentage represented as a value between 0.0 and 1.0</param>
         /// <param name="usePrecisionTimer">Determines if a high precision timer should be used.</param>
-        /// <param name="controller">The <see cref="GpioController"/> to which <paramref name="pinNumber"/> belongs to. Null defaults to board GpioController</param>
-        /// <param name="shouldDispose">True to automatically dispose the controller when this class is disposed, false otherwise.
-        /// This parameter is ignored if <paramref name="controller"/> is null.</param>
+        /// <param name="controller">The <see cref="GpioController"/> to which <paramref name="pinNumber"/> belongs to. <c>null</c> defaults to board GpioController</param>
+        /// <param name="shouldDispose"><c>true</c> to automatically dispose the controller when this class is disposed, <c>false</c> otherwise. This parameter is ignored if <paramref name="controller"/> is <c>null</c>.</param>
         public SoftwarePwmChannel(int pinNumber, int frequency = 400, double dutyCycle = 0.5, bool usePrecisionTimer = false, GpioController controller = null, bool shouldDispose = true)
         {
             if (pinNumber == -1)
@@ -91,10 +88,20 @@ namespace System.Device.Pwm.Drivers
 
             if (frequency <= 0)
             {
-                throw new ArgumentOutOfRangeException(nameof(frequency), "Frequency must be a positive value.");
+                throw new ArgumentOutOfRangeException(nameof(frequency), frequency, "Frequency must be a positive non-zero value.");
+            }
+
+            if (dutyCycle < 0.0 || dutyCycle > 1.0)
+            {
+                throw new ArgumentOutOfRangeException(nameof(dutyCycle), dutyCycle, "DutyCycle must be between 0.0 and 1.0 (inclusive).");
             }
 
-            if (controller == null)
+            _pin = pinNumber;
+
+            _frequency = frequency;
+            _dutyCycle = dutyCycle;
+
+            if (controller is null)
             {
                 _controller = new GpioController();
                 _shouldDispose = true;
@@ -105,106 +112,67 @@ namespace System.Device.Pwm.Drivers
                 _shouldDispose = shouldDispose;
             }
 
-            _pin = pinNumber;
-            _controller.OpenPin(_pin, PinMode.Output);
-            _usePrecisionTimer = usePrecisionTimer;
-            _isRunning = false;
+            UpdatePulseWidthParameters();
 
-            _frequency = frequency;
-            _dutyCycle = dutyCycle;
+            _thread = new Thread(Run);
 
-            UpdatePulseWidthParameters();
+            if (usePrecisionTimer)
+            {
+                _usePrecisionTimer = true;
+                _thread.Priority = ThreadPriority.Highest;
+            }
+
+            _controller.OpenPin(_pin, PinMode.Output);
 
-            _runningThread = new Thread(RunSoftPWM);
-            _runningThread.Start();
+            _thread.Start();
         }
 
         private void UpdatePulseWidthParameters()
         {
-            _periodInMilliseconds = 1000.0 / _frequency;
-            _pulseWidthInMilliseconds = _dutyCycle * _periodInMilliseconds;
+            double cycleTicks = TimeSpan.TicksPerSecond / (double)_frequency;
+
+            double pinHighTicks = cycleTicks * _dutyCycle;
+            _pinHighTime = TimeSpan.FromTicks((long)pinHighTicks);
+
+            double pinLowTicks = cycleTicks - pinHighTicks;
+            _pinLowTime = TimeSpan.FromTicks((long)pinLowTicks);
         }
 
-        private void RunSoftPWM()
+        private void Run()
         {
-            if (_usePrecisionTimer)
-            {
-                Thread.CurrentThread.Priority = ThreadPriority.Highest;
-            }
+            bool allowThreadYield = !_usePrecisionTimer;
 
-            while (_runThread)
+            while (!_isTerminating)
             {
-                // Write the pin high for the appropriate length of time
-                if (_isRunning)
+                if (!_isRunning)
                 {
-                    if (_pulseWidthInMilliseconds != 0)
-                    {
-                        _controller.Write(_pin, PinValue.High);
-                        _isStopped = false;
-                    }
-
-                    // Use the wait helper method to wait for the length of the pulse
-                    if (_usePrecisionTimer)
-                    {
-                        Wait(_pulseWidthInMilliseconds);
-                    }
-                    else
-                    {
-                        Task.Delay(TimeSpan.FromMilliseconds(_pulseWidthInMilliseconds)).Wait();
-                    }
-
-                    // The pulse if over and so set the pin to low and then wait until it's time for the next pulse
-                    _controller.Write(_pin, PinValue.Low);
+                    Thread.Yield();
+                    continue;
+                }
 
-                    if (_usePrecisionTimer)
-                    {
-                        Wait(_periodInMilliseconds - _pulseWidthInMilliseconds);
-                    }
-                    else
-                    {
-                        Task.Delay(TimeSpan.FromMilliseconds(_periodInMilliseconds - _pulseWidthInMilliseconds)).Wait();
-                    }
+                if (_pinHighTime != TimeSpan.Zero)
+                {
+                    _controller.Write(_pin, PinValue.High);
+                    DelayHelper.Delay(_pinHighTime, allowThreadYield);
                 }
-                else
+
+                if (_pinLowTime != TimeSpan.Zero)
                 {
-                    if (!_isStopped)
-                    {
-                        _controller.Write(_pin, PinValue.Low);
-                        _isStopped = true;
-                    }
+                    _controller.Write(_pin, PinValue.Low);
+                    DelayHelper.Delay(_pinLowTime, allowThreadYield);
                 }
             }
-        }
 
-        /// <summary>
-        /// A synchronous wait is used to avoid yielding the thread
-        /// This method calculates the number of CPU ticks will elapse in the specified time and spins
-        /// in a loop until that threshold is hit. This allows for very precise timing.
-        /// </summary>
-        /// <param name="milliseconds">The milliseconds to wait for</param>
-        private void Wait(double milliseconds)
-        {
-            long initialTick = _stopwatch.ElapsedTicks;
-            long initialElapsed = _stopwatch.ElapsedMilliseconds;
-            double desiredTicks = milliseconds / 1000.0 * Stopwatch.Frequency;
-            double finalTick = initialTick + desiredTicks;
-            while (_stopwatch.ElapsedTicks < finalTick)
-            {
-                // nothing than waiting
-            }
+            _controller.Write(_pin, PinValue.Low);
         }
 
-        /// <summary>
-        /// Starts the PWM channel.
-        /// </summary>
+        /// <summary>Starts the PWM channel.</summary>
         public override void Start()
         {
             _isRunning = true;
         }
 
-        /// <summary>
-        /// Stops the PWM channel.
-        /// </summary>
+        /// <summary>Stops the PWM channel.</summary>
         public override void Stop()
         {
             _isRunning = false;
@@ -213,10 +181,10 @@ namespace System.Device.Pwm.Drivers
         /// <inheritdoc/>
         protected override void Dispose(bool disposing)
         {
-            _isRunning = false;
-            _runThread = false;
-            _runningThread?.Join();
-            _runningThread = null;
+            _isTerminating = true;
+
+            _thread?.Join();
+            _thread = null;
 
             if (_shouldDispose)
             {