From 99ab83dfab0cad357bfd3858d33d06900f766073 Mon Sep 17 00:00:00 2001 From: Neil Dorin Date: Tue, 29 Sep 2026 12:20:59 -0600 Subject: [PATCH 1/2] fix: track press-and-hold per device action and make it thread-safe Holds were keyed by device only in an unlocked static dictionary, so two simultaneous holds on one device (e.g. pan + tilt for a diagonal PTZ move) collided: a duplicate-key exception, a second action that never started, and an axis that stopped ~1s late. Holds are now keyed by device and action path, and start/reset/stop/expiry run under a lock so each action starts and stops exactly once. The device-only overload is kept for plugins. Fixes #1489 Co-Authored-By: Claude Opus 5.5 --- .../DeviceTypeExtensions/IChannelMessenger.cs | 12 +- .../DeviceTypeExtensions/IColorMessenger.cs | 8 +- .../DeviceTypeExtensions/IDPadMessenger.cs | 14 +- .../DeviceTypeExtensions/IDvrMessenger.cs | 4 +- .../DeviceTypeExtensions/INumericMessenger.cs | 24 +-- .../ISetTopBoxControlsMessenger.cs | 4 +- .../ITransportMessenger.cs | 16 +- .../Messengers/CameraControlMessenger.cs | 17 ++- .../IBasicVolumeControlsMessenger.cs | 4 +- .../Messengers/ILevelControlsMessenger.cs | 4 +- .../Messengers/PressAndHoldHandler.cs | 138 ++++++++++++------ .../MobileControlEssentialsRoomBridge.cs | 4 +- 12 files changed, 152 insertions(+), 97 deletions(-) diff --git a/src/PepperDash.Essentials.MobileControl.Messengers/DeviceTypeExtensions/IChannelMessenger.cs b/src/PepperDash.Essentials.MobileControl.Messengers/DeviceTypeExtensions/IChannelMessenger.cs index 8d1858046..61549f4f1 100644 --- a/src/PepperDash.Essentials.MobileControl.Messengers/DeviceTypeExtensions/IChannelMessenger.cs +++ b/src/PepperDash.Essentials.MobileControl.Messengers/DeviceTypeExtensions/IChannelMessenger.cs @@ -27,13 +27,13 @@ protected override void RegisterActions() { base.RegisterActions(); - AddAction("/chanUp", (id, content) => PressAndHoldHandler.HandlePressAndHold(DeviceKey, content, (b) => channelDevice?.ChannelUp(b))); + AddAction("/chanUp", (id, content) => PressAndHoldHandler.HandlePressAndHold(DeviceKey, "/chanUp", content, (b) => channelDevice?.ChannelUp(b))); - AddAction("/chanDown", (id, content) => PressAndHoldHandler.HandlePressAndHold(DeviceKey, content, (b) => channelDevice?.ChannelDown(b))); - AddAction("/lastChan", (id, content) => PressAndHoldHandler.HandlePressAndHold(DeviceKey, content, (b) => channelDevice?.LastChannel(b))); - AddAction("/guide", (id, content) => PressAndHoldHandler.HandlePressAndHold(DeviceKey, content, (b) => channelDevice?.Guide(b))); - AddAction("/info", (id, content) => PressAndHoldHandler.HandlePressAndHold(DeviceKey, content, (b) => channelDevice?.Info(b))); - AddAction("/exit", (id, content) => PressAndHoldHandler.HandlePressAndHold(DeviceKey, content, (b) => channelDevice?.Exit(b))); + AddAction("/chanDown", (id, content) => PressAndHoldHandler.HandlePressAndHold(DeviceKey, "/chanDown", content, (b) => channelDevice?.ChannelDown(b))); + AddAction("/lastChan", (id, content) => PressAndHoldHandler.HandlePressAndHold(DeviceKey, "/lastChan", content, (b) => channelDevice?.LastChannel(b))); + AddAction("/guide", (id, content) => PressAndHoldHandler.HandlePressAndHold(DeviceKey, "/guide", content, (b) => channelDevice?.Guide(b))); + AddAction("/info", (id, content) => PressAndHoldHandler.HandlePressAndHold(DeviceKey, "/info", content, (b) => channelDevice?.Info(b))); + AddAction("/exit", (id, content) => PressAndHoldHandler.HandlePressAndHold(DeviceKey, "/exit", content, (b) => channelDevice?.Exit(b))); } } } \ No newline at end of file diff --git a/src/PepperDash.Essentials.MobileControl.Messengers/DeviceTypeExtensions/IColorMessenger.cs b/src/PepperDash.Essentials.MobileControl.Messengers/DeviceTypeExtensions/IColorMessenger.cs index 2a50c4a80..24e92d038 100644 --- a/src/PepperDash.Essentials.MobileControl.Messengers/DeviceTypeExtensions/IColorMessenger.cs +++ b/src/PepperDash.Essentials.MobileControl.Messengers/DeviceTypeExtensions/IColorMessenger.cs @@ -27,10 +27,10 @@ protected override void RegisterActions() { base.RegisterActions(); - AddAction("/red", (id, content) => PressAndHoldHandler.HandlePressAndHold(DeviceKey, content, (b) => colorDevice?.Red(b))); - AddAction("/green", (id, content) => PressAndHoldHandler.HandlePressAndHold(DeviceKey, content, (b) => colorDevice?.Green(b))); - AddAction("/yellow", (id, content) => PressAndHoldHandler.HandlePressAndHold(DeviceKey, content, (b) => colorDevice?.Yellow(b))); - AddAction("/blue", (id, content) => PressAndHoldHandler.HandlePressAndHold(DeviceKey, content, (b) => colorDevice?.Blue(b))); + AddAction("/red", (id, content) => PressAndHoldHandler.HandlePressAndHold(DeviceKey, "/red", content, (b) => colorDevice?.Red(b))); + AddAction("/green", (id, content) => PressAndHoldHandler.HandlePressAndHold(DeviceKey, "/green", content, (b) => colorDevice?.Green(b))); + AddAction("/yellow", (id, content) => PressAndHoldHandler.HandlePressAndHold(DeviceKey, "/yellow", content, (b) => colorDevice?.Yellow(b))); + AddAction("/blue", (id, content) => PressAndHoldHandler.HandlePressAndHold(DeviceKey, "/blue", content, (b) => colorDevice?.Blue(b))); } } } \ No newline at end of file diff --git a/src/PepperDash.Essentials.MobileControl.Messengers/DeviceTypeExtensions/IDPadMessenger.cs b/src/PepperDash.Essentials.MobileControl.Messengers/DeviceTypeExtensions/IDPadMessenger.cs index d7207e09a..05ffe9480 100644 --- a/src/PepperDash.Essentials.MobileControl.Messengers/DeviceTypeExtensions/IDPadMessenger.cs +++ b/src/PepperDash.Essentials.MobileControl.Messengers/DeviceTypeExtensions/IDPadMessenger.cs @@ -27,13 +27,13 @@ protected override void RegisterActions() { base.RegisterActions(); - AddAction("/up", (id, content) => PressAndHoldHandler.HandlePressAndHold(DeviceKey, content, (b) => dpadDevice?.Up(b))); - AddAction("/down", (id, content) => PressAndHoldHandler.HandlePressAndHold(DeviceKey, content, (b) => dpadDevice?.Down(b))); - AddAction("/left", (id, content) => PressAndHoldHandler.HandlePressAndHold(DeviceKey, content, (b) => dpadDevice?.Left(b))); - AddAction("/right", (id, content) => PressAndHoldHandler.HandlePressAndHold(DeviceKey, content, (b) => dpadDevice?.Right(b))); - AddAction("/select", (id, content) => PressAndHoldHandler.HandlePressAndHold(DeviceKey, content, (b) => dpadDevice?.Select(b))); - AddAction("/menu", (id, content) => PressAndHoldHandler.HandlePressAndHold(DeviceKey, content, (b) => dpadDevice?.Menu(b))); - AddAction("/exit", (id, content) => PressAndHoldHandler.HandlePressAndHold(DeviceKey, content, (b) => dpadDevice?.Exit(b))); + AddAction("/up", (id, content) => PressAndHoldHandler.HandlePressAndHold(DeviceKey, "/up", content, (b) => dpadDevice?.Up(b))); + AddAction("/down", (id, content) => PressAndHoldHandler.HandlePressAndHold(DeviceKey, "/down", content, (b) => dpadDevice?.Down(b))); + AddAction("/left", (id, content) => PressAndHoldHandler.HandlePressAndHold(DeviceKey, "/left", content, (b) => dpadDevice?.Left(b))); + AddAction("/right", (id, content) => PressAndHoldHandler.HandlePressAndHold(DeviceKey, "/right", content, (b) => dpadDevice?.Right(b))); + AddAction("/select", (id, content) => PressAndHoldHandler.HandlePressAndHold(DeviceKey, "/select", content, (b) => dpadDevice?.Select(b))); + AddAction("/menu", (id, content) => PressAndHoldHandler.HandlePressAndHold(DeviceKey, "/menu", content, (b) => dpadDevice?.Menu(b))); + AddAction("/exit", (id, content) => PressAndHoldHandler.HandlePressAndHold(DeviceKey, "/exit", content, (b) => dpadDevice?.Exit(b))); } } } \ No newline at end of file diff --git a/src/PepperDash.Essentials.MobileControl.Messengers/DeviceTypeExtensions/IDvrMessenger.cs b/src/PepperDash.Essentials.MobileControl.Messengers/DeviceTypeExtensions/IDvrMessenger.cs index 08b1d1002..43b63df6f 100644 --- a/src/PepperDash.Essentials.MobileControl.Messengers/DeviceTypeExtensions/IDvrMessenger.cs +++ b/src/PepperDash.Essentials.MobileControl.Messengers/DeviceTypeExtensions/IDvrMessenger.cs @@ -29,8 +29,8 @@ protected override void RegisterActions() { base.RegisterActions(); - AddAction("/dvrlist", (id, content) => PressAndHoldHandler.HandlePressAndHold(DeviceKey, content, (b) => dvrDevice?.DvrList(b))); - AddAction("/record", (id, content) => PressAndHoldHandler.HandlePressAndHold(DeviceKey, content, (b) => dvrDevice?.Record(b))); + AddAction("/dvrlist", (id, content) => PressAndHoldHandler.HandlePressAndHold(DeviceKey, "/dvrlist", content, (b) => dvrDevice?.DvrList(b))); + AddAction("/record", (id, content) => PressAndHoldHandler.HandlePressAndHold(DeviceKey, "/record", content, (b) => dvrDevice?.Record(b))); } } diff --git a/src/PepperDash.Essentials.MobileControl.Messengers/DeviceTypeExtensions/INumericMessenger.cs b/src/PepperDash.Essentials.MobileControl.Messengers/DeviceTypeExtensions/INumericMessenger.cs index 746caf188..aac647d79 100644 --- a/src/PepperDash.Essentials.MobileControl.Messengers/DeviceTypeExtensions/INumericMessenger.cs +++ b/src/PepperDash.Essentials.MobileControl.Messengers/DeviceTypeExtensions/INumericMessenger.cs @@ -28,18 +28,18 @@ protected override void RegisterActions() { base.RegisterActions(); - AddAction("/num0", (id, content) => PressAndHoldHandler.HandlePressAndHold(DeviceKey, content, (b) => keypadDevice?.Digit0(b))); - AddAction("/num1", (id, content) => PressAndHoldHandler.HandlePressAndHold(DeviceKey, content, (b) => keypadDevice?.Digit1(b))); - AddAction("/num2", (id, content) => PressAndHoldHandler.HandlePressAndHold(DeviceKey, content, (b) => keypadDevice?.Digit2(b))); - AddAction("/num3", (id, content) => PressAndHoldHandler.HandlePressAndHold(DeviceKey, content, (b) => keypadDevice?.Digit3(b))); - AddAction("/num4", (id, content) => PressAndHoldHandler.HandlePressAndHold(DeviceKey, content, (b) => keypadDevice?.Digit4(b))); - AddAction("/num5", (id, content) => PressAndHoldHandler.HandlePressAndHold(DeviceKey, content, (b) => keypadDevice?.Digit5(b))); - AddAction("/num6", (id, content) => PressAndHoldHandler.HandlePressAndHold(DeviceKey, content, (b) => keypadDevice?.Digit6(b))); - AddAction("/num7", (id, content) => PressAndHoldHandler.HandlePressAndHold(DeviceKey, content, (b) => keypadDevice?.Digit7(b))); - AddAction("/num8", (id, content) => PressAndHoldHandler.HandlePressAndHold(DeviceKey, content, (b) => keypadDevice?.Digit8(b))); - AddAction("/num9", (id, content) => PressAndHoldHandler.HandlePressAndHold(DeviceKey, content, (b) => keypadDevice?.Digit9(b))); - AddAction("/numDash", (id, content) => PressAndHoldHandler.HandlePressAndHold(DeviceKey, content, (b) => keypadDevice?.KeypadAccessoryButton1(b))); - AddAction("/numEnter", (id, content) => PressAndHoldHandler.HandlePressAndHold(DeviceKey, content, (b) => keypadDevice?.KeypadAccessoryButton2(b))); + AddAction("/num0", (id, content) => PressAndHoldHandler.HandlePressAndHold(DeviceKey, "/num0", content, (b) => keypadDevice?.Digit0(b))); + AddAction("/num1", (id, content) => PressAndHoldHandler.HandlePressAndHold(DeviceKey, "/num1", content, (b) => keypadDevice?.Digit1(b))); + AddAction("/num2", (id, content) => PressAndHoldHandler.HandlePressAndHold(DeviceKey, "/num2", content, (b) => keypadDevice?.Digit2(b))); + AddAction("/num3", (id, content) => PressAndHoldHandler.HandlePressAndHold(DeviceKey, "/num3", content, (b) => keypadDevice?.Digit3(b))); + AddAction("/num4", (id, content) => PressAndHoldHandler.HandlePressAndHold(DeviceKey, "/num4", content, (b) => keypadDevice?.Digit4(b))); + AddAction("/num5", (id, content) => PressAndHoldHandler.HandlePressAndHold(DeviceKey, "/num5", content, (b) => keypadDevice?.Digit5(b))); + AddAction("/num6", (id, content) => PressAndHoldHandler.HandlePressAndHold(DeviceKey, "/num6", content, (b) => keypadDevice?.Digit6(b))); + AddAction("/num7", (id, content) => PressAndHoldHandler.HandlePressAndHold(DeviceKey, "/num7", content, (b) => keypadDevice?.Digit7(b))); + AddAction("/num8", (id, content) => PressAndHoldHandler.HandlePressAndHold(DeviceKey, "/num8", content, (b) => keypadDevice?.Digit8(b))); + AddAction("/num9", (id, content) => PressAndHoldHandler.HandlePressAndHold(DeviceKey, "/num9", content, (b) => keypadDevice?.Digit9(b))); + AddAction("/numDash", (id, content) => PressAndHoldHandler.HandlePressAndHold(DeviceKey, "/numDash", content, (b) => keypadDevice?.KeypadAccessoryButton1(b))); + AddAction("/numEnter", (id, content) => PressAndHoldHandler.HandlePressAndHold(DeviceKey, "/numEnter", content, (b) => keypadDevice?.KeypadAccessoryButton2(b))); // Deal with the Accessory functions on the numpad later } } diff --git a/src/PepperDash.Essentials.MobileControl.Messengers/DeviceTypeExtensions/ISetTopBoxControlsMessenger.cs b/src/PepperDash.Essentials.MobileControl.Messengers/DeviceTypeExtensions/ISetTopBoxControlsMessenger.cs index 5e5a286d7..ef4bc4387 100644 --- a/src/PepperDash.Essentials.MobileControl.Messengers/DeviceTypeExtensions/ISetTopBoxControlsMessenger.cs +++ b/src/PepperDash.Essentials.MobileControl.Messengers/DeviceTypeExtensions/ISetTopBoxControlsMessenger.cs @@ -29,8 +29,8 @@ protected override void RegisterActions() { base.RegisterActions(); AddAction("/fullStatus", (id, content) => SendISetTopBoxControlsFullMessageObject()); - AddAction("/dvrList", (id, content) => PressAndHoldHandler.HandlePressAndHold(DeviceKey, content, (b) => stbDevice?.DvrList(b))); - AddAction("/replay", (id, content) => PressAndHoldHandler.HandlePressAndHold(DeviceKey, content, (b) => stbDevice?.Replay(b))); + AddAction("/dvrList", (id, content) => PressAndHoldHandler.HandlePressAndHold(DeviceKey, "/dvrList", content, (b) => stbDevice?.DvrList(b))); + AddAction("/replay", (id, content) => PressAndHoldHandler.HandlePressAndHold(DeviceKey, "/replay", content, (b) => stbDevice?.Replay(b))); } /// /// Helper method to build call status for vtc diff --git a/src/PepperDash.Essentials.MobileControl.Messengers/DeviceTypeExtensions/ITransportMessenger.cs b/src/PepperDash.Essentials.MobileControl.Messengers/DeviceTypeExtensions/ITransportMessenger.cs index 9eba08489..60493a362 100644 --- a/src/PepperDash.Essentials.MobileControl.Messengers/DeviceTypeExtensions/ITransportMessenger.cs +++ b/src/PepperDash.Essentials.MobileControl.Messengers/DeviceTypeExtensions/ITransportMessenger.cs @@ -28,14 +28,14 @@ protected override void RegisterActions() { base.RegisterActions(); - AddAction("/play", (id, content) => PressAndHoldHandler.HandlePressAndHold(DeviceKey, content, (b) => transportDevice?.Play(b))); - AddAction("/pause", (id, content) => PressAndHoldHandler.HandlePressAndHold(DeviceKey, content, (b) => transportDevice?.Pause(b))); - AddAction("/stop", (id, content) => PressAndHoldHandler.HandlePressAndHold(DeviceKey, content, (b) => transportDevice?.Stop(b))); - AddAction("/prevTrack", (id, content) => PressAndHoldHandler.HandlePressAndHold(DeviceKey, content, (b) => transportDevice?.ChapPlus(b))); - AddAction("/nextTrack", (id, content) => PressAndHoldHandler.HandlePressAndHold(DeviceKey, content, (b) => transportDevice?.ChapMinus(b))); - AddAction("/rewind", (id, content) => PressAndHoldHandler.HandlePressAndHold(DeviceKey, content, (b) => transportDevice?.Rewind(b))); - AddAction("/ffwd", (id, content) => PressAndHoldHandler.HandlePressAndHold(DeviceKey, content, (b) => transportDevice?.FFwd(b))); - AddAction("/record", (id, content) => PressAndHoldHandler.HandlePressAndHold(DeviceKey, content, (b) => transportDevice?.Record(b))); + AddAction("/play", (id, content) => PressAndHoldHandler.HandlePressAndHold(DeviceKey, "/play", content, (b) => transportDevice?.Play(b))); + AddAction("/pause", (id, content) => PressAndHoldHandler.HandlePressAndHold(DeviceKey, "/pause", content, (b) => transportDevice?.Pause(b))); + AddAction("/stop", (id, content) => PressAndHoldHandler.HandlePressAndHold(DeviceKey, "/stop", content, (b) => transportDevice?.Stop(b))); + AddAction("/prevTrack", (id, content) => PressAndHoldHandler.HandlePressAndHold(DeviceKey, "/prevTrack", content, (b) => transportDevice?.ChapPlus(b))); + AddAction("/nextTrack", (id, content) => PressAndHoldHandler.HandlePressAndHold(DeviceKey, "/nextTrack", content, (b) => transportDevice?.ChapMinus(b))); + AddAction("/rewind", (id, content) => PressAndHoldHandler.HandlePressAndHold(DeviceKey, "/rewind", content, (b) => transportDevice?.Rewind(b))); + AddAction("/ffwd", (id, content) => PressAndHoldHandler.HandlePressAndHold(DeviceKey, "/ffwd", content, (b) => transportDevice?.FFwd(b))); + AddAction("/record", (id, content) => PressAndHoldHandler.HandlePressAndHold(DeviceKey, "/record", content, (b) => transportDevice?.Record(b))); } } diff --git a/src/PepperDash.Essentials.MobileControl.Messengers/Messengers/CameraControlMessenger.cs b/src/PepperDash.Essentials.MobileControl.Messengers/Messengers/CameraControlMessenger.cs index 3c02ee591..c6e3445e3 100644 --- a/src/PepperDash.Essentials.MobileControl.Messengers/Messengers/CameraControlMessenger.cs +++ b/src/PepperDash.Essentials.MobileControl.Messengers/Messengers/CameraControlMessenger.cs @@ -68,7 +68,7 @@ protected override void RegisterActions() if (Camera is IHasCameraPtzControl ptzCamera) { // Need to evaluate how to pass through these P&H actions. Need a method that takes a bool maybe? - AddAction("/cameraUp", (id, content) => HandleCameraPressAndHold(content, (b) => + AddAction("/cameraUp", (id, content) => HandleCameraPressAndHold("/cameraUp", content, (b) => { if (b) { @@ -78,7 +78,7 @@ protected override void RegisterActions() ptzCamera.TiltStop(); })); - AddAction("/cameraDown", (id, content) => HandleCameraPressAndHold(content, (b) => + AddAction("/cameraDown", (id, content) => HandleCameraPressAndHold("/cameraDown", content, (b) => { if (b) { @@ -88,7 +88,7 @@ protected override void RegisterActions() ptzCamera.TiltStop(); })); - AddAction("/cameraLeft", (id, content) => HandleCameraPressAndHold(content, (b) => + AddAction("/cameraLeft", (id, content) => HandleCameraPressAndHold("/cameraLeft", content, (b) => { if (b) { @@ -98,7 +98,7 @@ protected override void RegisterActions() ptzCamera.PanStop(); })); - AddAction("/cameraRight", (id, content) => HandleCameraPressAndHold(content, (b) => + AddAction("/cameraRight", (id, content) => HandleCameraPressAndHold("/cameraRight", content, (b) => { if (b) { @@ -108,7 +108,7 @@ protected override void RegisterActions() ptzCamera.PanStop(); })); - AddAction("/cameraZoomIn", (id, content) => HandleCameraPressAndHold(content, (b) => + AddAction("/cameraZoomIn", (id, content) => HandleCameraPressAndHold("/cameraZoomIn", content, (b) => { if (b) { @@ -118,7 +118,7 @@ protected override void RegisterActions() ptzCamera.ZoomStop(); })); - AddAction("/cameraZoomOut", (id, content) => HandleCameraPressAndHold(content, (b) => + AddAction("/cameraZoomOut", (id, content) => HandleCameraPressAndHold("/cameraZoomOut", content, (b) => { if (b) { @@ -163,7 +163,7 @@ protected override void RegisterActions() } } - private void HandleCameraPressAndHold(JToken content, Action cameraAction) + private void HandleCameraPressAndHold(string actionPath, JToken content, Action cameraAction) { var state = content.ToObject>(); @@ -173,7 +173,8 @@ private void HandleCameraPressAndHold(JToken content, Action cameraAction) return; } - timerHandler(Camera.Key, cameraAction); + // Keyed per action, so holds on different axes (e.g. pan and tilt for a diagonal move) are independent. + timerHandler($"{Camera.Key}{actionPath}", cameraAction); } diff --git a/src/PepperDash.Essentials.MobileControl.Messengers/Messengers/IBasicVolumeControlsMessenger.cs b/src/PepperDash.Essentials.MobileControl.Messengers/Messengers/IBasicVolumeControlsMessenger.cs index 91b01f780..6940bf21f 100644 --- a/src/PepperDash.Essentials.MobileControl.Messengers/Messengers/IBasicVolumeControlsMessenger.cs +++ b/src/PepperDash.Essentials.MobileControl.Messengers/Messengers/IBasicVolumeControlsMessenger.cs @@ -67,7 +67,7 @@ private void SendStatus(string id = null) /// protected override void RegisterActions() { - AddAction("/volumeUp", (id, content) => PressAndHoldHandler.HandlePressAndHold(DeviceKey, content, (b) => + AddAction("/volumeUp", (id, content) => PressAndHoldHandler.HandlePressAndHold(DeviceKey, "/volumeUp", content, (b) => { Debug.LogMessage(Serilog.Events.LogEventLevel.Verbose, "Calling {localDevice} volume up with {value}", DeviceKey, b); try @@ -85,7 +85,7 @@ protected override void RegisterActions() device.MuteToggle(); }); - AddAction("/volumeDown", (id, content) => PressAndHoldHandler.HandlePressAndHold(DeviceKey, content, (b) => + AddAction("/volumeDown", (id, content) => PressAndHoldHandler.HandlePressAndHold(DeviceKey, "/volumeDown", content, (b) => { Debug.LogMessage(Serilog.Events.LogEventLevel.Verbose, "Calling {localDevice} volume down with {value}", DeviceKey, b); diff --git a/src/PepperDash.Essentials.MobileControl.Messengers/Messengers/ILevelControlsMessenger.cs b/src/PepperDash.Essentials.MobileControl.Messengers/Messengers/ILevelControlsMessenger.cs index 6ddb671d3..d44089d62 100644 --- a/src/PepperDash.Essentials.MobileControl.Messengers/Messengers/ILevelControlsMessenger.cs +++ b/src/PepperDash.Essentials.MobileControl.Messengers/Messengers/ILevelControlsMessenger.cs @@ -56,9 +56,9 @@ protected override void RegisterActions() AddAction($"/{key}/muteOff", (id, content) => control.MuteOff()); - AddAction($"/{key}/volumeUp", (id, content) => PressAndHoldHandler.HandlePressAndHold(DeviceKey, content, (b) => control.VolumeUp(b))); + AddAction($"/{key}/volumeUp", (id, content) => PressAndHoldHandler.HandlePressAndHold(DeviceKey, $"/{key}/volumeUp", content, (b) => control.VolumeUp(b))); - AddAction($"/{key}/volumeDown", (id, content) => PressAndHoldHandler.HandlePressAndHold(DeviceKey, content, (b) => control.VolumeDown(b))); + AddAction($"/{key}/volumeDown", (id, content) => PressAndHoldHandler.HandlePressAndHold(DeviceKey, $"/{key}/volumeDown", content, (b) => control.VolumeDown(b))); control.VolumeLevelFeedback.OutputChange += (o, a) => PostStatusMessage(JToken.FromObject(new { diff --git a/src/PepperDash.Essentials.MobileControl.Messengers/Messengers/PressAndHoldHandler.cs b/src/PepperDash.Essentials.MobileControl.Messengers/Messengers/PressAndHoldHandler.cs index 6256142aa..f074e5967 100644 --- a/src/PepperDash.Essentials.MobileControl.Messengers/Messengers/PressAndHoldHandler.cs +++ b/src/PepperDash.Essentials.MobileControl.Messengers/Messengers/PressAndHoldHandler.cs @@ -9,12 +9,23 @@ namespace PepperDash.Essentials.AppServer.Messengers /// /// Handler for press/hold/release messages /// + /// + /// Each hold is tracked by a key, which should identify both the device and the action (for example + /// "camera1/cameraLeft"), so that two holds on one device, such as pan and tilt together, run + /// independently. A hold ends on "released", or when no "held" heartbeat arrives within + /// . + /// public static class PressAndHoldHandler { private const long ButtonHeartbeatInterval = 1000; private static readonly Dictionary _pushedActions = new Dictionary(); + // Messages are handled concurrently, and heartbeat timers expire on their own threads. Every + // start, reset and stop runs under this lock, including the action itself, so a hold's action + // is started and stopped exactly once and in order. + private static readonly object _pushedActionsLock = new object(); + private static readonly Dictionary>> _pushedActionHandlers; static PressAndHoldHandler() @@ -27,73 +38,95 @@ static PressAndHoldHandler() }; } - private static void AddTimer(string deviceKey, Action action) + private static void AddTimer(string key, Action action) { - Debug.LogDebug("Attempting to add timer for {deviceKey}", deviceKey); + Debug.LogDebug("Attempting to add timer for {key}", key); - if (_pushedActions.TryGetValue(deviceKey, out Timer cancelTimer)) + lock (_pushedActionsLock) { - Debug.LogDebug("Timer for {deviceKey} already exists", deviceKey); - return; - } + if (_pushedActions.ContainsKey(key)) + { + Debug.LogDebug("Timer for {key} already exists", key); + return; + } - Debug.LogDebug("Adding timer for {deviceKey} with due time {dueTime}", deviceKey, ButtonHeartbeatInterval); + Debug.LogDebug("Adding timer for {key} with due time {dueTime}", key, ButtonHeartbeatInterval); - action(true); + var cancelTimer = new Timer(ButtonHeartbeatInterval) { AutoReset = false }; + cancelTimer.Elapsed += (s, e) => ExpireTimer(key, cancelTimer, action); - cancelTimer = new Timer(ButtonHeartbeatInterval) { AutoReset = false }; - cancelTimer.Elapsed += (s, e) => - { - Debug.LogDebug("Timer expired for {deviceKey}", deviceKey); + _pushedActions.Add(key, cancelTimer); - action(false); + action(true); - _pushedActions.Remove(deviceKey); - }; - cancelTimer.Start(); + cancelTimer.Start(); + } + } - _pushedActions.Add(deviceKey, cancelTimer); + private static void ExpireTimer(string key, Timer cancelTimer, Action action) + { + lock (_pushedActionsLock) + { + // "released" may have ended this hold already, or a new hold may have replaced it. + if (!_pushedActions.TryGetValue(key, out var current) || current != cancelTimer) + { + return; + } + + Debug.LogDebug("Timer expired for {key}", key); + + _pushedActions.Remove(key); + cancelTimer.Dispose(); + action(false); + } } - private static void ResetTimer(string deviceKey, Action action) + private static void ResetTimer(string key, Action action) { - Debug.LogDebug("Attempting to reset timer for {deviceKey}", deviceKey); + Debug.LogDebug("Attempting to reset timer for {key}", key); - if (!_pushedActions.TryGetValue(deviceKey, out Timer cancelTimer)) + lock (_pushedActionsLock) { - Debug.LogDebug("Timer for {deviceKey} not found", deviceKey); - return; - } + if (!_pushedActions.TryGetValue(key, out Timer cancelTimer)) + { + Debug.LogDebug("Timer for {key} not found", key); + return; + } - Debug.LogDebug("Resetting timer for {deviceKey} with due time {dueTime}", deviceKey, ButtonHeartbeatInterval); + Debug.LogDebug("Resetting timer for {key} with due time {dueTime}", key, ButtonHeartbeatInterval); - cancelTimer.Stop(); - cancelTimer.Interval = ButtonHeartbeatInterval; - cancelTimer.Start(); + cancelTimer.Stop(); + cancelTimer.Interval = ButtonHeartbeatInterval; + cancelTimer.Start(); + } } - private static void StopTimer(string deviceKey, Action action) + private static void StopTimer(string key, Action action) { - Debug.LogDebug("Attempting to stop timer for {deviceKey}", deviceKey); + Debug.LogDebug("Attempting to stop timer for {key}", key); - if (!_pushedActions.TryGetValue(deviceKey, out Timer cancelTimer)) + lock (_pushedActionsLock) { - Debug.LogDebug("Timer for {deviceKey} not found", deviceKey); - return; - } + if (!_pushedActions.TryGetValue(key, out Timer cancelTimer)) + { + Debug.LogDebug("Timer for {key} not found", key); + return; + } - Debug.LogDebug("Stopping timer for {deviceKey} with due time {dueTime}", deviceKey, ButtonHeartbeatInterval); + Debug.LogDebug("Stopping timer for {key}", key); - action(false); - cancelTimer.Stop(); - _pushedActions.Remove(deviceKey); + _pushedActions.Remove(key); + cancelTimer.Stop(); + cancelTimer.Dispose(); + action(false); + } } /// /// Gets the handler for a given press and hold message type /// /// The press and hold message type. - /// The handler for the specified message type. + /// The handler for the specified message type, called with the hold's key and action. public static Action> GetPressAndHoldHandler(string value) { Debug.LogDebug("Getting press and hold handler for {value}", value); @@ -110,13 +143,34 @@ public static Action> GetPressAndHoldHandler(string value) } /// - /// HandlePressAndHold method + /// Handles a press/hold/release message for one action on a device + /// + /// The device the action belongs to. + /// The action's message path, such as "/cameraLeft", so each action on the device is held independently. + /// The message content, with a value of "pressed", "held" or "released". + /// The action to run: true to start, false to stop. + public static void HandlePressAndHold(string deviceKey, string actionPath, JToken content, Action action) + { + HandlePressAndHoldForKey($"{deviceKey}{actionPath}", content, action); + } + + /// + /// Handles a press/hold/release message, tracking one hold per device /// + /// + /// Two actions held at once on the same device share one hold, so the second is ignored. Prefer + /// the overload that takes the action path. + /// public static void HandlePressAndHold(string deviceKey, JToken content, Action action) + { + HandlePressAndHoldForKey(deviceKey, content, action); + } + + private static void HandlePressAndHoldForKey(string key, JToken content, Action action) { var msg = content.ToObject>(); - Debug.LogDebug("Handling press and hold message of {type} for {deviceKey}", msg.Value, deviceKey); + Debug.LogDebug("Handling press and hold message of {type} for {key}", msg.Value, key); var timerHandler = GetPressAndHoldHandler(msg.Value); @@ -125,7 +179,7 @@ public static void HandlePressAndHold(string deviceKey, JToken content, Action PressAndHoldHandler.HandlePressAndHold(DeviceKey, content, (b) => + AddAction("/volumes/master/volumeUp", (id, content) => PressAndHoldHandler.HandlePressAndHold(DeviceKey, "/volumes/master/volumeUp", content, (b) => { if (volumeRoom.CurrentVolumeControls is IBasicVolumeWithFeedback basicVolumeWithFeedback) { @@ -198,7 +198,7 @@ protected override void RegisterActions() } )); - AddAction("/volumes/master/volumeDown", (id, content) => PressAndHoldHandler.HandlePressAndHold(DeviceKey, content, (b) => + AddAction("/volumes/master/volumeDown", (id, content) => PressAndHoldHandler.HandlePressAndHold(DeviceKey, "/volumes/master/volumeDown", content, (b) => { if (volumeRoom.CurrentVolumeControls is IBasicVolumeWithFeedback basicVolumeWithFeedback) { From d94305cb0de59dd2c0b3a1d110608b39ed7a5114 Mon Sep 17 00:00:00 2001 From: Neil Dorin Date: Tue, 29 Sep 2026 14:56:07 -0600 Subject: [PATCH 2/2] fix: per-hold locking and fresh heartbeat timers in PressAndHoldHandler Addresses review on #1493: device callbacks now run under a per-hold lock instead of one global lock, a start that throws no longer leaves the key held, and each heartbeat replaces the hold's timer so an expiry already queued for the old timer can't end a hold that was just extended. A hold stays registered until its stop has run, keeping start/stop ordered per key. Co-Authored-By: Claude Opus 5.5 --- .../Messengers/PressAndHoldHandler.cs | 176 +++++++++++++----- 1 file changed, 134 insertions(+), 42 deletions(-) diff --git a/src/PepperDash.Essentials.MobileControl.Messengers/Messengers/PressAndHoldHandler.cs b/src/PepperDash.Essentials.MobileControl.Messengers/Messengers/PressAndHoldHandler.cs index f074e5967..b8924a4a9 100644 --- a/src/PepperDash.Essentials.MobileControl.Messengers/Messengers/PressAndHoldHandler.cs +++ b/src/PepperDash.Essentials.MobileControl.Messengers/Messengers/PressAndHoldHandler.cs @@ -19,12 +19,28 @@ public static class PressAndHoldHandler { private const long ButtonHeartbeatInterval = 1000; - private static readonly Dictionary _pushedActions = new Dictionary(); + /// + /// One active hold. Its orders that hold's start, heartbeats and stop, and is + /// the only lock held while device code runs, so a slow device never delays another hold. + /// + private sealed class Hold + { + public readonly object Gate = new object(); - // Messages are handled concurrently, and heartbeat timers expire on their own threads. Every - // start, reset and stop runs under this lock, including the action itself, so a hold's action - // is started and stopped exactly once and in order. - private static readonly object _pushedActionsLock = new object(); + /// + /// Replaced on every heartbeat, so an expiry already queued for an earlier timer no longer + /// matches and can't end a hold that was just extended. + /// + public Timer Timer; + + public bool Ended; + } + + // Guards only membership of _holds, never device code. Lock order: _holdsLock may be taken while + // holding a Gate, but a Gate is only taken under _holdsLock when it belongs to a new, unpublished hold. + private static readonly object _holdsLock = new object(); + + private static readonly Dictionary _holds = new Dictionary(); private static readonly Dictionary>> _pushedActionHandlers; @@ -42,42 +58,43 @@ private static void AddTimer(string key, Action action) { Debug.LogDebug("Attempting to add timer for {key}", key); - lock (_pushedActionsLock) + var hold = new Hold(); + + lock (_holdsLock) { - if (_pushedActions.ContainsKey(key)) + if (_holds.ContainsKey(key)) { Debug.LogDebug("Timer for {key} already exists", key); return; } - Debug.LogDebug("Adding timer for {key} with due time {dueTime}", key, ButtonHeartbeatInterval); - - var cancelTimer = new Timer(ButtonHeartbeatInterval) { AutoReset = false }; - cancelTimer.Elapsed += (s, e) => ExpireTimer(key, cancelTimer, action); - - _pushedActions.Add(key, cancelTimer); - - action(true); - - cancelTimer.Start(); + // Take the new hold's gate before publishing it, so a release or heartbeat for this key + // waits until the start has run. + System.Threading.Monitor.Enter(hold.Gate); + _holds.Add(key, hold); } - } - private static void ExpireTimer(string key, Timer cancelTimer, Action action) - { - lock (_pushedActionsLock) + try { - // "released" may have ended this hold already, or a new hold may have replaced it. - if (!_pushedActions.TryGetValue(key, out var current) || current != cancelTimer) + Debug.LogDebug("Adding timer for {key} with due time {dueTime}", key, ButtonHeartbeatInterval); + + try { - return; + action(true); + } + catch + { + // Without this, the key would stay held and every later press would be ignored. + hold.Ended = true; + RemoveHold(key, hold); + throw; } - Debug.LogDebug("Timer expired for {key}", key); - - _pushedActions.Remove(key); - cancelTimer.Dispose(); - action(false); + StartNewTimer(key, hold, action); + } + finally + { + System.Threading.Monitor.Exit(hold.Gate); } } @@ -85,19 +102,24 @@ private static void ResetTimer(string key, Action action) { Debug.LogDebug("Attempting to reset timer for {key}", key); - lock (_pushedActionsLock) + var hold = GetHold(key); + + if (hold == null) { - if (!_pushedActions.TryGetValue(key, out Timer cancelTimer)) + Debug.LogDebug("Timer for {key} not found", key); + return; + } + + lock (hold.Gate) + { + if (hold.Ended) { - Debug.LogDebug("Timer for {key} not found", key); return; } Debug.LogDebug("Resetting timer for {key} with due time {dueTime}", key, ButtonHeartbeatInterval); - cancelTimer.Stop(); - cancelTimer.Interval = ButtonHeartbeatInterval; - cancelTimer.Start(); + StartNewTimer(key, hold, action); } } @@ -105,20 +127,90 @@ private static void StopTimer(string key, Action action) { Debug.LogDebug("Attempting to stop timer for {key}", key); - lock (_pushedActionsLock) + var hold = GetHold(key); + + if (hold == null) + { + Debug.LogDebug("Timer for {key} not found", key); + return; + } + + lock (hold.Gate) { - if (!_pushedActions.TryGetValue(key, out Timer cancelTimer)) + if (hold.Ended) { - Debug.LogDebug("Timer for {key} not found", key); return; } Debug.LogDebug("Stopping timer for {key}", key); - _pushedActions.Remove(key); - cancelTimer.Stop(); - cancelTimer.Dispose(); - action(false); + // Removed only now, once this hold's start has finished, so a new press for the key + // can't start before this one has stopped. + RemoveHold(key, hold); + EndHold(hold, action); + } + } + + private static void ExpireTimer(string key, Hold hold, Timer timer, Action action) + { + lock (hold.Gate) + { + // A release ended the hold, or a heartbeat replaced this timer: this expiry is stale. + if (hold.Ended || hold.Timer != timer) + { + return; + } + + Debug.LogDebug("Timer expired for {key}", key); + + RemoveHold(key, hold); + EndHold(hold, action); + } + } + + // Caller holds hold.Gate. + private static void StartNewTimer(string key, Hold hold, Action action) + { + var previous = hold.Timer; + + var timer = new Timer(ButtonHeartbeatInterval) { AutoReset = false }; + timer.Elapsed += (s, e) => ExpireTimer(key, hold, timer, action); + + hold.Timer = timer; + + previous?.Stop(); + previous?.Dispose(); + + timer.Start(); + } + + // Caller holds hold.Gate. + private static void EndHold(Hold hold, Action action) + { + hold.Ended = true; + + hold.Timer?.Stop(); + hold.Timer?.Dispose(); + + action(false); + } + + private static Hold GetHold(string key) + { + lock (_holdsLock) + { + return _holds.TryGetValue(key, out var hold) ? hold : null; + } + } + + private static void RemoveHold(string key, Hold hold) + { + lock (_holdsLock) + { + if (_holds.TryGetValue(key, out var current) && current == hold) + { + _holds.Remove(key); + } } }