Fix TelescopeSimulator IsSlewing/AtPark wedging (slew-state thread-safety + GEM hour-angle-limit crossing)

TelescopeSimulator/TelescopeHardware.cs

@@ -270,6 +270,31 @@ public static class TelescopeHardware
         private static TrackingMode trackingMode;
         private static bool slewing;
 
+        // Slew no-progress guard. Tracks per-tick slew progress so a slew that
+        // can make no headway — e.g. CheckAxisLimits
+        // keeps undoing a move that drives the primary axis into the hour-angle
+        // limit — is stopped instead of leaving IsSlewing stuck true forever.
+        private static double lastSlewProgressX;
+        private static double lastSlewProgressY;
+        private static int slewStallTicks;
+        // 5 * TIMER_INTERVAL(0.1 s) = 0.5 s of no real progress => give up.
+        // Tracking drift (~0.0004 deg/tick) is far below slewSpeedSlow/2, so a
+        // healthy slew (>= slewSpeedSlow/tick) never trips this.
+        private const int SLEW_STALL_TICKS = 5;
+
+        /// <summary>
+        /// Synchronises access to the slew-engine state shared between the Kestrel
+        /// HTTP request threads (StartSlewAxes / SyncTo* / AbortSlew / Slewing / RA /
+        /// Dec) and the s_wTimer tick thread (MoveAxes -> DoSlew). These previously
+        /// used plain unsynchronised static fields (mountAxes, targetAxes, slewing,
+        /// SlewState), so StartSlewAxes' writes were not ordered against the timer
+        /// thread's reads: the tick can observe SlewState == SlewRaDec while still
+        /// seeing slewing == false, bail out of DoSlew, and never advance the slew, so
+        /// IsSlewing stays true indefinitely. Taking this lock on both sides restores
+        /// that ordering.
+        /// </summary>
+        private static readonly object hardwareLock = new object();
+
         private static DateTime lastUpdateTime;
 
         #endregion Private variables
@@ -357,6 +382,21 @@ public static void Init()
         {
             try
             {
+                // Dispose the previously-created timer before allocating a new
+                // one. Init() runs on every Telescope (re)construction — including
+                // a "restart to a clean state" reset (e.g. issued between test or
+                // conformance runs). Without this the prior AutoReset timer is
+                // never stopped/unsubscribed; the runtime keeps it alive and it
+                // keeps firing M_wTimer_Tick on the shared static slew state, so
+                // every reset leaked another tick source racing the single slew
+                // engine.
+                if (s_wTimer != null)
+                {
+                    s_wTimer.Stop();
+                    s_wTimer.Elapsed -= M_wTimer_Tick;
+                    s_wTimer.Dispose();
+                }
+
                 s_wTimer = new System.Timers.Timer();
                 s_wTimer.Interval = (int)(SharedResources.TIMER_INTERVAL * 1000);
                 s_wTimer.Elapsed += M_wTimer_Tick;
@@ -630,11 +670,21 @@ public static void Init()
                 SlewSettleTime = 0;
                 ChangePark(AtPark);
 
-                // invalid target position
-                targetRaDec = new Vector(double.NaN, double.NaN);
-                SlewState = SlewType.SlewNone;
+                // Reset the slew-engine state under hardwareLock so these writes
+                // are ordered against any still-in-flight M_wTimer_Tick. `slewing`
+                // in particular was never cleared on reset, so a slew left in
+                // flight by a prior Telescope instance could keep IsSlewing stuck
+                // true after a "restart to clean state".
+                lock (hardwareLock)
+                {
+                    // invalid target position
+                    targetRaDec = new Vector(double.NaN, double.NaN);
+                    SlewState = SlewType.SlewNone;
+                    slewing = false;
+                    rateMoveAxes = new Vector();
 
-                mountAxes = MountFunctions.ConvertAltAzmToAxes(altAzm); // Convert the start position AltAz coordinates into the current axes representation and set this as the simulator start position
+                    mountAxes = MountFunctions.ConvertAltAzmToAxes(altAzm); // Convert the start position AltAz coordinates into the current axes representation and set this as the simulator start position
+                }
                 LogMessage("TelescopeHardware New", string.Format("Startup mode: {0}, Azimuth: {1}, Altitude: {2}", startupMode, altAzm.X.ToString(CultureInfo.InvariantCulture), altAzm.Y.ToString(CultureInfo.InvariantCulture)));
 
                 LogMessage("TelescopeHardware New", "Successfully initialised hardware");
@@ -680,7 +730,16 @@ public static void Start()
         //Update the Telescope Based on Timed Events
         private static void M_wTimer_Tick(object sender, EventArgs e)
         {
-            MoveAxes();
+            // Hold hardwareLock for the entire tick so MoveAxes / DoSlew see a
+            // consistent, ordered view of the slew state that HTTP request threads
+            // mutate (StartSlewAxes / AbortSlew / SyncTo*). MoveAxes is only ever
+            // called from here, so this also serialises any reentrant tick that
+            // System.Timers.Timer (AutoReset = true) fires on a second ThreadPool
+            // thread when a tick overruns TIMER_INTERVAL.
+            lock (hardwareLock)
+            {
+                MoveAxes();
+            }
         }
 
         /// <summary>
@@ -828,7 +887,7 @@ private static void MoveAxes()
             // update the displayed values
             UpdatePositions();
 
-            // check and update slew state 
+            // check and update slew state
             switch (SlewState)
             {
                 case SlewType.SlewSettle:
@@ -840,6 +899,35 @@ private static void MoveAxes()
                     break;
             }
 
+            // No-progress guard. If DoSlew did not finish the
+            // slew this tick (slewing still true) yet the axes did not actually
+            // advance, the slew is wedged — typically CheckAxisLimits is undoing a
+            // move into the hour-angle limit. A real mount stops at its limit; stop
+            // too, rather than reporting IsSlewing == true forever. Tracking drift
+            // (~0.0004 deg/tick) is far below slewSpeedSlow/2, so a healthy slew
+            // (which advances >= slewSpeedSlow/tick) never trips this.
+            if (slewing)
+            {
+                double progressed = Math.Abs(mountAxes.X - lastSlewProgressX)
+                                  + Math.Abs(mountAxes.Y - lastSlewProgressY);
+                if (progressed < slewSpeedSlow / 2.0)
+                {
+                    if (++slewStallTicks >= SLEW_STALL_TICKS)
+                    {
+                        slewing = false;
+                        SlewState = SlewType.SlewNone;
+                        slewStallTicks = 0;
+                        LogMessage("DoSlew", "Slew stopped: axis limit reached, target unreachable in this pier state");
+                    }
+                }
+                else
+                {
+                    slewStallTicks = 0;
+                }
+                lastSlewProgressX = mountAxes.X;
+                lastSlewProgressY = mountAxes.Y;
+            }
+
             // List changes this cycle
             TL.LogMessage(LogLevel.Verbose, $"MoveAxes (Final)", $"RA: {currentRaDec.X.ToHMS()}, Dec: {currentRaDec.Y.ToDMS()}, Hour angle: {Utilities.ConditionHA(SiderealTime - currentRaDec.X).ToHMS()}, Sidereal Time: {SiderealTime.ToHMS()}");
             TL.LogMessage(LogLevel.Verbose, $"MoveAxes (Final)", $"Azimuth: {altAzm.X.ToDMS()}, Altitude: {altAzm.Y.ToDMS()}, Pointing state: {SideOfPier}");
@@ -1278,7 +1366,24 @@ public static double Altitude
             set { altAzm.Y = value; }
         }
 
-        public static bool AtPark { get; private set; }
+        private static bool atPark;
+
+        /// <summary>
+        /// `true` once a park slew has completed. Written by <see cref="ChangePark"/>
+        /// — from the timer-tick completion path and from StartSlewAxes, both of
+        /// which hold <c>hardwareLock</c> — and read by the Alpaca `AtPark` poller
+        /// on a Kestrel request thread. The lock orders the tick's write against the
+        /// poller's read; without it a client polling `AtPark` right after `Park()`
+        /// can miss the completion on weak memory (AArch64) / under JIT register
+        /// caching and spin until its own deadline. Same race class as the
+        /// IsSlewing / RightAscension / Declination accessors; this closes the
+        /// park-side gap.
+        /// </summary>
+        public static bool AtPark
+        {
+            get { lock (hardwareLock) { return atPark; } }
+            private set { lock (hardwareLock) { atPark = value; } }
+        }
 
         public static double Azimuth
         {
@@ -1427,17 +1532,31 @@ public static double DeclinationRate
 
         public static double Declination
         {
-            get { return currentRaDec.Y; }
-            set { currentRaDec.Y = value; }
+            get { lock (hardwareLock) { return currentRaDec.Y; } }
+            set { lock (hardwareLock) { currentRaDec.Y = value; } }
         }
 
         public static double RightAscension
         {
-            get { return currentRaDec.X; }
-            set { currentRaDec.X = value; }
+            get { lock (hardwareLock) { return currentRaDec.X; } }
+            set { lock (hardwareLock) { currentRaDec.X = value; } }
         }
 
-        public static SlewType SlewState { get; private set; }
+        private static SlewType slewStateField;
+
+        /// <summary>
+        /// The slew-engine state. Written under <c>hardwareLock</c> (timer tick,
+        /// StartSlewAxes, AbortSlew) and also read off the hardware thread — e.g.
+        /// `Telescope.cs` traffic logging reads it directly — so the accessor takes
+        /// the same lock for a consistent cross-thread view. (`private set` keeps
+        /// the writer internal; the lock is reentrant, so the tick writing it while
+        /// already holding the lock is fine.)
+        /// </summary>
+        public static SlewType SlewState
+        {
+            get { lock (hardwareLock) { return slewStateField; } }
+            private set { lock (hardwareLock) { slewStateField = value; } }
+        }
 
         public static SlewSpeed SlewSpeed { get; set; }
 
@@ -1591,36 +1710,48 @@ public static bool IsSlewing
         {
             get
             {
-                if (SlewState != SlewType.SlewNone)
-                    return true;
-                if (slewing)
-                    return true;
-                if (rateMoveAxes.LengthSquared != 0)
-                    return true;
-                //if (rateRaDec.LengthSquared != 0) // Commented out by Peter 4th August 2018 because the Telescope specification says that RightAscensionRate and DeclinationRate do not affect the Slewing state
-                //    return true;
-                return slewing && rateMoveAxes.Y != 0 && rateMoveAxes.X != 0;
+                lock (hardwareLock)
+                {
+                    if (SlewState != SlewType.SlewNone)
+                        return true;
+                    if (slewing)
+                        return true;
+                    if (rateMoveAxes.LengthSquared != 0)
+                        return true;
+                    //if (rateRaDec.LengthSquared != 0) // Commented out by Peter 4th August 2018 because the Telescope specification says that RightAscensionRate and DeclinationRate do not affect the Slewing state
+                    //    return true;
+                    return slewing && rateMoveAxes.Y != 0 && rateMoveAxes.X != 0;
+                }
             }
         }
 
         public static void AbortSlew()
         {
-            slewing = false;
-            rateMoveAxes = new Vector();
-            rateRaDecOffsetInternal = new Vector();
-            SlewState = SlewType.SlewNone;
+            lock (hardwareLock)
+            {
+                slewing = false;
+                rateMoveAxes = new Vector();
+                rateRaDecOffsetInternal = new Vector();
+                SlewState = SlewType.SlewNone;
+            }
         }
 
         public static void SyncToTarget()
         {
-            mountAxes = MountFunctions.ConvertRaDecToAxes(targetRaDec, true);
-            UpdatePositions();
+            lock (hardwareLock)
+            {
+                mountAxes = MountFunctions.ConvertRaDecToAxes(targetRaDec, true);
+                UpdatePositions();
+            }
         }
 
         public static void SyncToAltAzm(double targetAzimuth, double targetAltitude)
         {
-            mountAxes = MountFunctions.ConvertAltAzmToAxes(new Vector(targetAzimuth, targetAltitude));
-            UpdatePositions();
+            lock (hardwareLock)
+            {
+                mountAxes = MountFunctions.ConvertAltAzmToAxes(new Vector(targetAzimuth, targetAltitude));
+                UpdatePositions();
+            }
         }
 
         public static void StartSlewRaDec(double rightAscension, double declination, bool doSideOfPier)
@@ -1661,10 +1792,20 @@ public static void StartSlewAxes(double primaryAxis, double secondaryAxis, SlewT
         /// <param name="targetPosition">The position.</param>
         public static void StartSlewAxes(Vector targetPosition, SlewType slewState)
         {
-            targetAxes = targetPosition;
-            SlewState = slewState;
-            slewing = true;
-            ChangePark(false);
+            // Order these writes against the timer thread's reads in DoSlew. Without
+            // the lock the tick can see slewing == false (stale) after SlewState has
+            // already become SlewRaDec, bail out of DoSlew, and wedge IsSlewing at
+            // true forever.
+            lock (hardwareLock)
+            {
+                targetAxes = targetPosition;
+                SlewState = slewState;
+                slewing = true;
+                slewStallTicks = 0;
+                lastSlewProgressX = mountAxes.X;
+                lastSlewProgressY = mountAxes.Y;
+                ChangePark(false);
+            }
         }
 
         public static void Park()
@@ -1828,6 +1969,28 @@ private static Vector DoSlew()
                 if (delta < -180) delta += 360;
                 if (delta > 180) delta -= 360;
             }
+
+            // The shortest-path rotation above can drive the primary axis through
+            // the GEM hour-angle limit (CheckAxisLimits then
+            // undoes every step, so the slew never finishes and IsSlewing wedges
+            // forever). When that happens but the SAME target is reachable the
+            // other way round, take the longer rotation instead. This reaches the
+            // identical target (so the pier side is unchanged — no conformance
+            // impact) and is a no-op for any slew whose short path stays in range.
+            if (alignmentMode == AlignmentMode.GermanPolar)
+            {
+                double endShort = mountAxes.X + delta;
+                if (endShort < -hourAngleLimit || endShort > 180.0 + hourAngleLimit)
+                {
+                    double deltaLong = delta + (delta > 0 ? -360.0 : 360.0);
+                    double endLong = mountAxes.X + deltaLong;
+                    if (endLong >= -hourAngleLimit && endLong <= 180.0 + hourAngleLimit)
+                    {
+                        delta = deltaLong;
+                    }
+                }
+            }
+
             int signDelta = delta < 0 ? -1 : +1;
             delta = Math.Abs(delta);