Merge pull request #1376 from PepperDash/client-id-issues

fix: handle subsequent join calls and clientid/websocket client mismatches

src/PepperDash.Essentials.Devices.Common/Displays/ScreenLiftController.cs

@@ -19,7 +19,7 @@ namespace PepperDash.Essentials.Devices.Common.Shades
     {
         None,
         Raise,
-        Lower,
+        Lower
     }
 
     /// <summary>
@@ -50,8 +50,7 @@ namespace PepperDash.Essentials.Devices.Common.Shades
             get { return _isInUpPosition; }
             set
             {
-                if (value == _isInUpPosition)
-                    return;
+                if (value == _isInUpPosition) return;
                 _isInUpPosition = value;
                 IsInUpPosition.FireUpdate();
                 PositionChanged?.Invoke(this, new EventArgs());
@@ -88,11 +87,7 @@ namespace PepperDash.Essentials.Devices.Common.Shades
         /// <summary>
         /// Constructor for ScreenLiftController
         /// </summary>
-        public ScreenLiftController(
-            string key,
-            string name,
-            ScreenLiftControllerConfigProperties config
-        )
+        public ScreenLiftController(string key, string name, ScreenLiftControllerConfigProperties config)
             : base(key, name)
         {
             Config = config;
@@ -110,60 +105,27 @@ namespace PepperDash.Essentials.Devices.Common.Shades
             switch (Mode)
             {
                 case eScreenLiftControlMode.momentary:
-                {
-                    RaiseRelayConfig = Config.Relays["raise"];
-                    LowerRelayConfig = Config.Relays["lower"];
-                    break;
-                }
+                    {
+                        RaiseRelayConfig = Config.Relays["raise"];
+                        LowerRelayConfig = Config.Relays["lower"];
+                        break;
+                    }
                 case eScreenLiftControlMode.latched:
-                {
-                    LatchedRelayConfig = Config.Relays["latched"];
-                    break;
-                }
+                    {
+                        LatchedRelayConfig = Config.Relays["latched"];
+                        break;
+                    }
             }
-
-            IsInUpPosition.OutputChange += (sender, args) =>
-            {
-                this.LogDebug(
-                    "ScreenLiftController '{name}' IsInUpPosition changed to {position}",
-                    Name,
-                    IsInUpPosition.BoolValue ? "Up" : "Down"
-                );
-
-                if (!Config.MuteOnScreenUp)
-                {
-                    return;
-                }
-
-                if (args.BoolValue)
-                {
-                    return;
-                }
-
-                if (DisplayDevice is IBasicVideoMuteWithFeedback videoMute)
-                {
-                    this.LogInformation("Unmuting video because screen is down");
-                    videoMute.VideoMuteOff();
-                }
-            };
-
-            IsInUpPosition.FireUpdate();
         }
 
         private void IsCoolingDownFeedback_OutputChange(object sender, FeedbackEventArgs e)
         {
-            if (
-                !DisplayDevice.IsCoolingDownFeedback.BoolValue
-                && Type == eScreenLiftControlType.lift
-            )
+            if (!DisplayDevice.IsCoolingDownFeedback.BoolValue && Type == eScreenLiftControlType.lift)
             {
                 Raise();
                 return;
             }
-            if (
-                DisplayDevice.IsCoolingDownFeedback.BoolValue
-                && Type == eScreenLiftControlType.screen
-            )
+            if (DisplayDevice.IsCoolingDownFeedback.BoolValue && Type == eScreenLiftControlType.screen)
             {
                 Raise();
                 return;
@@ -188,18 +150,18 @@ namespace PepperDash.Essentials.Devices.Common.Shades
             switch (Mode)
             {
                 case eScreenLiftControlMode.momentary:
-                {
-                    this.LogDebug("Getting relays for {mode}", Mode);
-                    RaiseRelay = GetSwitchedOutputFromDevice(RaiseRelayConfig.DeviceKey);
-                    LowerRelay = GetSwitchedOutputFromDevice(LowerRelayConfig.DeviceKey);
-                    break;
-                }
+                    {
+                        this.LogDebug("Getting relays for {mode}", Mode);
+                        RaiseRelay = GetSwitchedOutputFromDevice(RaiseRelayConfig.DeviceKey);
+                        LowerRelay = GetSwitchedOutputFromDevice(LowerRelayConfig.DeviceKey);
+                        break;
+                    }
                 case eScreenLiftControlMode.latched:
-                {
-                    this.LogDebug("Getting relays for {mode}", Mode);
-                    LatchedRelay = GetSwitchedOutputFromDevice(LatchedRelayConfig.DeviceKey);
-                    break;
-                }
+                    {
+                        this.LogDebug("Getting relays for {mode}", Mode);
+                        LatchedRelay = GetSwitchedOutputFromDevice(LatchedRelayConfig.DeviceKey);
+                        break;
+                    }
             }
 
             this.LogDebug("Getting display with key {displayKey}", DisplayDeviceKey);
@@ -210,8 +172,7 @@ namespace PepperDash.Essentials.Devices.Common.Shades
                 this.LogDebug("Subscribing to {displayKey} feedbacks", DisplayDeviceKey);
 
                 DisplayDevice.IsWarmingUpFeedback.OutputChange += IsWarmingUpFeedback_OutputChange;
-                DisplayDevice.IsCoolingDownFeedback.OutputChange +=
-                    IsCoolingDownFeedback_OutputChange;
+                DisplayDevice.IsCoolingDownFeedback.OutputChange += IsCoolingDownFeedback_OutputChange;
             }
 
             return base.CustomActivate();
@@ -222,17 +183,10 @@ namespace PepperDash.Essentials.Devices.Common.Shades
         /// </summary>
         public void Raise()
         {
-            if (RaiseRelay == null && LatchedRelay == null)
-                return;
+            if (RaiseRelay == null && LatchedRelay == null) return;
 
             this.LogDebug("Raise called for {type}", Type);
 
-            if (Config.MuteOnScreenUp && DisplayDevice is IBasicVideoMuteWithFeedback videoMute)
-            {
-                this.LogInformation("Muting video because screen is going up");
-                videoMute.VideoMuteOn();
-            }
-
             // If device is moving, bank the command
             if (_isMoving)
             {
@@ -246,33 +200,33 @@ namespace PepperDash.Essentials.Devices.Common.Shades
             switch (Mode)
             {
                 case eScreenLiftControlMode.momentary:
-                {
-                    PulseOutput(RaiseRelay, RaiseRelayConfig.PulseTimeInMs);
+                    {
+                        PulseOutput(RaiseRelay, RaiseRelayConfig.PulseTimeInMs);
 
-                    // Set moving flag and start timer if movement time is configured
-                    if (RaiseRelayConfig.MoveTimeInMs > 0)
-                    {
-                        _isMoving = true;
-                        _currentMovement = RequestedState.Raise;
-                        if (_movementTimer.Enabled)
+                        // Set moving flag and start timer if movement time is configured
+                        if (RaiseRelayConfig.MoveTimeInMs > 0)
                         {
-                            _movementTimer.Stop();
+                            _isMoving = true;
+                            _currentMovement = RequestedState.Raise;
+                            if (_movementTimer.Enabled)
+                            {
+                                _movementTimer.Stop();
+                            }
+                            _movementTimer.Interval = RaiseRelayConfig.MoveTimeInMs;
+                            _movementTimer.Start();
                         }
-                        _movementTimer.Interval = RaiseRelayConfig.MoveTimeInMs;
-                        _movementTimer.Start();
+                        else
+                        {
+                            InUpPosition = true;
+                        }
+                        break;
                     }
-                    else
-                    {
-                        InUpPosition = true;
-                    }
-                    break;
-                }
                 case eScreenLiftControlMode.latched:
-                {
-                    LatchedRelay.Off();
-                    InUpPosition = true;
-                    break;
-                }
+                    {
+                        LatchedRelay.Off();
+                        InUpPosition = true;
+                        break;
+                    }
             }
         }
 
@@ -281,8 +235,7 @@ namespace PepperDash.Essentials.Devices.Common.Shades
         /// </summary>
         public void Lower()
         {
-            if (LowerRelay == null && LatchedRelay == null)
-                return;
+            if (LowerRelay == null && LatchedRelay == null) return;
 
             this.LogDebug("Lower called for {type}", Type);
 
@@ -299,33 +252,33 @@ namespace PepperDash.Essentials.Devices.Common.Shades
             switch (Mode)
             {
                 case eScreenLiftControlMode.momentary:
-                {
-                    PulseOutput(LowerRelay, LowerRelayConfig.PulseTimeInMs);
+                    {
+                        PulseOutput(LowerRelay, LowerRelayConfig.PulseTimeInMs);
 
-                    // Set moving flag and start timer if movement time is configured
-                    if (LowerRelayConfig.MoveTimeInMs > 0)
-                    {
-                        _isMoving = true;
-                        _currentMovement = RequestedState.Lower;
-                        if (_movementTimer.Enabled)
+                        // Set moving flag and start timer if movement time is configured
+                        if (LowerRelayConfig.MoveTimeInMs > 0)
                         {
-                            _movementTimer.Stop();
+                            _isMoving = true;
+                            _currentMovement = RequestedState.Lower;
+                            if (_movementTimer.Enabled)
+                            {
+                                _movementTimer.Stop();
+                            }
+                            _movementTimer.Interval = LowerRelayConfig.MoveTimeInMs;
+                            _movementTimer.Start();
                         }
-                        _movementTimer.Interval = LowerRelayConfig.MoveTimeInMs;
-                        _movementTimer.Start();
+                        else
+                        {
+                            InUpPosition = false;
+                        }
+                        break;
                     }
-                    else
-                    {
-                        InUpPosition = false;
-                    }
-                    break;
-                }
                 case eScreenLiftControlMode.latched:
-                {
-                    LatchedRelay.On();
-                    InUpPosition = false;
-                    break;
-                }
+                    {
+                        LatchedRelay.On();
+                        InUpPosition = false;
+                        break;
+                    }
             }
         }
 
@@ -386,13 +339,16 @@ namespace PepperDash.Essentials.Devices.Common.Shades
         {
             output.On();
 
-            var timer = new Timer(pulseTime) { AutoReset = false };
+            var timer = new Timer(pulseTime)
+            {
+                AutoReset = false
+            };
 
             timer.Elapsed += (sender, e) =>
-            {
-                output.Off();
-                timer.Dispose();
-            };
+                  {
+                      output.Off();
+                      timer.Dispose();
+                  };
             timer.Start();
         }
 
@@ -405,10 +361,7 @@ namespace PepperDash.Essentials.Devices.Common.Shades
             }
             else
             {
-                this.LogWarning(
-                    "Error: Unable to get relay device with key '{relayKey}'",
-                    relayKey
-                );
+                this.LogWarning("Error: Unable to get relay device with key '{relayKey}'", relayKey);
                 return null;
             }
         }
@@ -422,13 +375,11 @@ namespace PepperDash.Essentials.Devices.Common.Shades
             }
             else
             {
-                this.LogWarning(
-                    "Error: Unable to get display device with key '{displayKey}'",
-                    displayKey
-                );
+                this.LogWarning("Error: Unable to get display device with key '{displayKey}'", displayKey);
                 return null;
             }
         }
+
     }
 
     /// <summary>
@@ -436,7 +387,7 @@ namespace PepperDash.Essentials.Devices.Common.Shades
     /// </summary>
     public class ScreenLiftControllerFactory : EssentialsDeviceFactory<RelayControlledShade>
     {
-        /// <summary>
+        /// <summary> 
         /// Constructor for ScreenLiftControllerFactory
         /// </summary>
         public ScreenLiftControllerFactory()
@@ -453,4 +404,4 @@ namespace PepperDash.Essentials.Devices.Common.Shades
             return new ScreenLiftController(dc.Key, dc.Name, props);
         }
     }
-}
+}

src/PepperDash.Essentials.Devices.Common/Displays/ScreenLiftControllerConfigProperties.cs

@@ -5,41 +5,37 @@ using PepperDash.Essentials.Core.DeviceTypeInterfaces;
 
 namespace PepperDash.Essentials.Devices.Common.Shades
 {
+  /// <summary>
+  /// Represents a ScreenLiftControllerConfigProperties
+  /// </summary>
+  public class ScreenLiftControllerConfigProperties
+  {
+
     /// <summary>
-    /// Represents a ScreenLiftControllerConfigProperties
+    /// Gets or sets the DisplayDeviceKey
     /// </summary>
-    public class ScreenLiftControllerConfigProperties
-    {
-        /// <summary>
-        /// Gets or sets the DisplayDeviceKey
-        /// </summary>
-        [JsonProperty("displayDeviceKey")]
-        public string DisplayDeviceKey { get; set; }
+    [JsonProperty("displayDeviceKey")]
+    public string DisplayDeviceKey { get; set; }
 
-        /// <summary>
-        /// Gets or sets the Type
-        /// </summary>
-        [JsonProperty("type")]
-        [JsonConverter(typeof(StringEnumConverter))]
-        public eScreenLiftControlType Type { get; set; }
+    /// <summary>
+    /// Gets or sets the Type
+    /// </summary>
+    [JsonProperty("type")]
+    [JsonConverter(typeof(StringEnumConverter))]
+    public eScreenLiftControlType Type { get; set; }
 
-        /// <summary>
-        /// Gets or sets the Mode
-        /// </summary>
-        [JsonProperty("mode")]
-        [JsonConverter(typeof(StringEnumConverter))]
-        public eScreenLiftControlMode Mode { get; set; }
+    /// <summary>
+    /// Gets or sets the Mode
+    /// </summary>
+    [JsonProperty("mode")]
+    [JsonConverter(typeof(StringEnumConverter))]
+    public eScreenLiftControlMode Mode { get; set; }
 
-        /// <summary>
-        /// Gets or sets the Relays
-        /// </summary>
-        [JsonProperty("relays")]
-        public Dictionary<string, ScreenLiftRelaysConfig> Relays { get; set; }
+    /// <summary>
+    /// Gets or sets the Relays
+    /// </summary>
+    [JsonProperty("relays")]
+    public Dictionary<string, ScreenLiftRelaysConfig> Relays { get; set; }
 
-        /// <summary>
-        /// Mutes the display when the screen is in the up position
-        /// </summary>
-        [JsonProperty("muteOnScreenUp")]
-        public bool MuteOnScreenUp { get; set; }
-    }
-}
+  }
+}

src/PepperDash.Essentials.MobileControl/WebSocketServer/MobileControlWebsocketServer.cs

@@ -69,10 +69,11 @@ namespace PepperDash.Essentials.WebSocketServer
         private readonly ConcurrentDictionary<string, string> pendingClientRegistrations = new ConcurrentDictionary<string, string>();
 
         /// <summary>
-        /// Stores queues of pending client IDs per token for legacy clients (FIFO)
-        /// This ensures thread-safety when multiple legacy clients use the same token
+        /// Stores pending client registrations with timestamp for legacy clients
+        /// Key is token, Value is list of (clientId, timestamp) tuples
+        /// Most recent registration is used to handle duplicate join requests
         /// </summary>
-        private readonly ConcurrentDictionary<string, ConcurrentQueue<string>> legacyClientIdQueues = new ConcurrentDictionary<string, ConcurrentQueue<string>>();
+        private readonly ConcurrentDictionary<string, ConcurrentBag<(string clientId, DateTime timestamp)>> legacyClientRegistrations = new ConcurrentDictionary<string, ConcurrentBag<(string, DateTime)>>();
 
         /// <summary>
         /// Gets the collection of UI clients
@@ -736,15 +737,23 @@ namespace PepperDash.Essentials.WebSocketServer
 
         private UiClient BuildUiClient(string roomKey, JoinToken token, string key)
         {
-            // Dequeue the next clientId for legacy client support (FIFO per token)
+            // Get the most recent unused clientId for this token (legacy support)
             // New clients will override this ID in OnOpen with the validated query parameter value
             var clientId = "pending";
-            if (legacyClientIdQueues.TryGetValue(key, out var queue) && queue.TryDequeue(out var dequeuedId))
+            if (legacyClientRegistrations.TryGetValue(key, out var registrations))
             {
-                clientId = dequeuedId;
-                this.LogVerbose("Dequeued legacy clientId {clientId} for token {token}", clientId, key);
+                // Get most recent registration
+                var sorted = registrations.OrderByDescending(r => r.timestamp).ToList();
+                if (sorted.Any())
+                {
+                    clientId = sorted.First().clientId;
+                    // Remove it from the bag
+                    var newBag = new ConcurrentBag<(string, DateTime)>(sorted.Skip(1));
+                    legacyClientRegistrations.TryUpdate(key, newBag, registrations);
+                    this.LogVerbose("Assigned most recent legacy clientId {clientId} for token {token}", clientId, key);
+                }
             }
-            
+
             var c = new UiClient($"uiclient-{key}-{roomKey}-{clientId}", clientId, token.Token, token.TouchpanelKey);
             this.LogInformation("Constructing UiClient with key {key} and temporary ID (will be set from query param)", key);
             c.Controller = _parent;
@@ -753,8 +762,8 @@ namespace PepperDash.Essentials.WebSocketServer
             c.Server = this; // Give UiClient access to server for ID registration
 
             // Don't add to uiClients yet - will be added in OnOpen after ID is set from query param
-            
-            c.ConnectionClosed += (o, a) => 
+
+            c.ConnectionClosed += (o, a) =>
             {
                 uiClients.TryRemove(a.ClientId, out _);
                 // Clean up any pending registrations for this token
@@ -765,11 +774,11 @@ namespace PepperDash.Essentials.WebSocketServer
                 {
                     pendingClientRegistrations.TryRemove(k, out _);
                 }
-                
-                // Clean up legacy queue if empty
-                if (legacyClientIdQueues.TryGetValue(key, out var legacyQueue) && legacyQueue.IsEmpty)
+
+                // Clean up legacy registrations if empty
+                if (legacyClientRegistrations.TryGetValue(key, out var legacyBag) && legacyBag.IsEmpty)
                 {
-                    legacyClientIdQueues.TryRemove(key, out _);
+                    legacyClientRegistrations.TryRemove(key, out _);
                 }
             };
             return c;
@@ -785,7 +794,7 @@ namespace PepperDash.Essentials.WebSocketServer
         public bool RegisterUiClient(UiClient client, string clientId, string tokenKey)
         {
             var registrationKey = $"{tokenKey}-{clientId}";
-            
+
             // Verify this clientId was generated during a join request for this token
             if (!pendingClientRegistrations.TryRemove(registrationKey, out _))
             {
@@ -799,11 +808,63 @@ namespace PepperDash.Essentials.WebSocketServer
                 this.LogWarning("Replacing existing client with duplicate id {id}", id);
                 return client;
             });
-            
+
             this.LogInformation("Successfully registered UiClient with ID {clientId} for token {token}", clientId, tokenKey);
             return true;
         }
 
+        /// <summary>
+        /// Updates a client's ID when a mismatch is detected between stored ID and message ID
+        /// </summary>
+        /// <param name="oldClientId">The current/old client ID</param>
+        /// <param name="newClientId">The new client ID from the message</param>
+        /// <param name="tokenKey">The token key for validation</param>
+        /// <returns>True if update successful, false otherwise</returns>
+        public bool UpdateClientId(string oldClientId, string newClientId, string tokenKey)
+        {
+            if (string.IsNullOrEmpty(oldClientId) || string.IsNullOrEmpty(newClientId))
+            {
+                this.LogWarning("Cannot update client ID with null or empty values");
+                return false;
+            }
+
+            if (oldClientId == newClientId)
+            {
+                return true; // No update needed
+            }
+
+            // Verify the new clientId was registered for this token
+            var registrationKey = $"{tokenKey}-{newClientId}";
+            if (!pendingClientRegistrations.TryRemove(registrationKey, out _))
+            {
+                this.LogWarning("Cannot update to unregistered clientId {newClientId} for token {token}", newClientId, tokenKey);
+                return false;
+            }
+
+            // Get the existing client
+            if (!uiClients.TryRemove(oldClientId, out var client))
+            {
+                this.LogWarning("Cannot find client with old ID {oldClientId}", oldClientId);
+                return false;
+            }
+
+            // Update the client's ID
+            client.UpdateId(newClientId);
+
+            // Re-add with new ID
+            if (!uiClients.TryAdd(newClientId, client))
+            {
+                // If add fails, try to restore old entry
+                uiClients.TryAdd(oldClientId, client);
+                client.UpdateId(oldClientId);
+                this.LogError("Failed to update client ID from {oldClientId} to {newClientId}", oldClientId, newClientId);
+                return false;
+            }
+
+            this.LogInformation("Successfully updated client ID from {oldClientId} to {newClientId}", oldClientId, newClientId);
+            return true;
+        }
+
         /// <summary>
         /// Registers a UiClient using legacy flow (for backwards compatibility with older clients)
         /// </summary>
@@ -821,7 +882,7 @@ namespace PepperDash.Essentials.WebSocketServer
                 this.LogWarning("Replacing existing client with duplicate id {id} (legacy flow)", id);
                 return client;
             });
-            
+
             this.LogInformation("Successfully registered UiClient with ID {clientId} using legacy flow", client.Id);
         }
 
@@ -1133,16 +1194,17 @@ namespace PepperDash.Essentials.WebSocketServer
 
             // Generate a client ID for this join request
             var clientId = $"{Utilities.GetNextClientId()}";
-            
+            var now = DateTime.UtcNow;
+
             // Store in pending registrations for new clients that send clientId via query param
             var registrationKey = $"{token}-{clientId}";
             pendingClientRegistrations.TryAdd(registrationKey, clientId);
-            
-            // Also enqueue for legacy clients (thread-safe FIFO per token)
-            var queue = legacyClientIdQueues.GetOrAdd(token, _ => new ConcurrentQueue<string>());
-            queue.Enqueue(clientId);
 
-            this.LogVerbose("Assigning ClientId: {clientId} for token: {token}", clientId, token);
+            // For legacy clients, store with timestamp instead of FIFO queue
+            var legacyBag = legacyClientRegistrations.GetOrAdd(token, _ => new ConcurrentBag<(string, DateTime)>());
+            legacyBag.Add((clientId, now));
+
+            this.LogVerbose("Assigning ClientId: {clientId} for token: {token} at {timestamp}", clientId, token, now);
 
             // Construct WebSocket URL with clientId query parameter
             var wsProtocol = "ws";

src/PepperDash.Essentials.MobileControl/WebSocketServer/UiClient.cs

@@ -26,6 +26,15 @@ namespace PepperDash.Essentials.WebSocketServer
         /// </summary>
         public string Id { get; private set; }
 
+        /// <summary>
+        /// Updates the client ID - only accessible from within the assembly (e.g., by the server)
+        /// </summary>
+        /// <param name="newId">The new client ID</param>
+        internal void UpdateId(string newId)
+        {
+            Id = newId;
+        }
+
         /// <summary>
         /// Token associated with this client
         /// </summary>