Conversation
There was a problem hiding this comment.
Pull request overview
This PR hardens BLE reconnection behavior (especially on Android) by ensuring cached GATT/service/characteristic references are cleared during disconnect and by reducing reliance on stale Android BluetoothGatt state across reconnects. It also refines LEGO Control+/TechnicMove handling by making port index mapping safer and by adding a feedback-driven completion path for PLAYVM calibration.
Changes:
- Introduces a
OnDeviceDisconnecting()hook onBluetoothDeviceand implements it across device types to clear cached characteristic references before native disconnect/cleanup. - Adds an Android BLE “refresh service cache” reflection call during GATT disconnect/close to mitigate stale service/characteristic caching between reconnects.
- Updates ControlPlus/TechnicMove logic: safer port-id-to-channel mapping, a dedicated output-feedback callback hook, and a PLAYVM calibration wait based on hub feedback (with timeout fallback).
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| BrickController2/BrickController2/DeviceManagement/Wedo2Device.cs | Clears cached characteristic references on disconnect to avoid stale native objects after reconnect. |
| BrickController2/BrickController2/DeviceManagement/Vengit/SBrickLightDevice.cs | Clears cached characteristic references on disconnect. |
| BrickController2/BrickController2/DeviceManagement/TechnicMoveDevice.cs | Filters unsupported configurations, adds feedback-based PLAYVM calibration wait, and updates port/channel mapping for non-index port IDs. |
| BrickController2/BrickController2/DeviceManagement/SBrickDevice.cs | Clears cached characteristic references on disconnect. |
| BrickController2/BrickController2/DeviceManagement/PfxBrickDevice.cs | Clears cached characteristic references on disconnect. |
| BrickController2/BrickController2/DeviceManagement/MouldKing/MK_DIY.cs | Clears cached characteristic reference on disconnect. |
| BrickController2/BrickController2/DeviceManagement/Lego/RemoteControl.cs | Clears cached characteristic reference on disconnect. |
| BrickController2/BrickController2/DeviceManagement/ControlPlusDevice.cs | Adds disconnect cleanup, safe port-id mapping via TryGetChannelIndex, feedback hook for message 0x82, and debug-only logging. |
| BrickController2/BrickController2/DeviceManagement/CircuitCubeDevice.cs | Clears cached characteristic references on disconnect. |
| BrickController2/BrickController2/DeviceManagement/BuwizzDevice.cs | Clears cached characteristic reference on disconnect. |
| BrickController2/BrickController2/DeviceManagement/BuWizz3Device.cs | Clears cached characteristic references on disconnect. |
| BrickController2/BrickController2/DeviceManagement/BuWizz2Device.cs | Clears cached characteristic references on disconnect. |
| BrickController2/BrickController2/DeviceManagement/BluetoothDevice.cs | Adds abstract OnDeviceDisconnecting() and invokes it as part of the disconnect sequence; makes disconnect callback invocation more robust. |
| BrickController2/BrickController2.Android/PlatformServices/BluetoothLE/BluetoothLEDevice.cs | Adds reflective BluetoothGatt.refresh() call before close/dispose to reduce stale service caching across reconnects. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
BrickController2/BrickController2/DeviceManagement/TechnicMoveDevice.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
This PR hardens Bluetooth LE reconnection behavior by introducing a disconnect hook to clear cached GATT characteristic references (avoiding stale native Android objects), and adds an Android-specific GATT service-cache refresh on disconnect. It also improves Control+ port-to-channel mapping safety and enhances Technic Move PLAYVM calibration handling.
Changes:
- Add
OnDeviceDisconnecting()lifecycle hook to clear cachedIGattCharacteristicreferences across BLE devices before native disconnect. - Add Android
BluetoothGatt.refresh()(reflection) on disconnect to reduce stale service caching between reconnects. - Improve Control+ message parsing with
TryGetChannelIndex(...)and update Technic Move PLAYVM calibration to wait for hub feedback (with timeout fallback).
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| BrickController2/BrickController2/DeviceManagement/BluetoothDevice.cs | Adds OnDeviceDisconnecting() hook and ensures callback handling is safe during disconnect. |
| BrickController2/BrickController2/DeviceManagement/ControlPlusDevice.cs | Switches to TryGetChannelIndex to avoid invalid indexing and adds a disconnect cleanup for cached characteristic. |
| BrickController2/BrickController2/DeviceManagement/TechnicMoveDevice.cs | Filters unsupported channel configs, improves init suppression for LED/PLAYVM, and waits for PLAYVM calibration feedback instead of fixed delays. |
| BrickController2/BrickController2/DeviceManagement/Wedo2Device.cs | Clears cached characteristics on disconnect. |
| BrickController2/BrickController2/DeviceManagement/SBrickDevice.cs | Clears cached characteristics on disconnect. |
| BrickController2/BrickController2/DeviceManagement/Vengit/SBrickLightDevice.cs | Clears cached characteristics on disconnect. |
| BrickController2/BrickController2/DeviceManagement/PfxBrickDevice.cs | Clears cached characteristics on disconnect. |
| BrickController2/BrickController2/DeviceManagement/MouldKing/MK_DIY.cs | Clears cached characteristic on disconnect. |
| BrickController2/BrickController2/DeviceManagement/Lego/RemoteControl.cs | Clears cached characteristic on disconnect. |
| BrickController2/BrickController2/DeviceManagement/CircuitCubeDevice.cs | Clears cached characteristics on disconnect. |
| BrickController2/BrickController2/DeviceManagement/BuwizzDevice.cs | Clears cached characteristic on disconnect. |
| BrickController2/BrickController2/DeviceManagement/BuWizz2Device.cs | Clears cached characteristics on disconnect. |
| BrickController2/BrickController2/DeviceManagement/BuWizz3Device.cs | Clears cached characteristics on disconnect. |
| BrickController2/BrickController2.Android/PlatformServices/BluetoothLE/BluetoothLEDevice.cs | Adds reflected BluetoothGatt.refresh() before closing GATT to mitigate stale service cache on Android. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
There was a problem hiding this comment.
Pull request overview
This PR aims to prevent stale Android BLE GATT object reuse across reconnects by explicitly clearing cached characteristic references on disconnect, while also refactoring LEGO Control+ BLE device handling into a shared base and updating Technic Move output processing.
Changes:
- Add a disconnect hook (
OnDeviceDisconnecting) to clear cachedIGattCharacteristicreferences across multiple BLE device implementations. - Introduce
ControlPlusDeviceBaseto centralize LEGO Wireless Protocol characteristic discovery, notifications, and hub property requests; updateControlPlusDeviceand LEGO Remote Control to use it. - Extend
OutputValuesGroup(renameCommmit→Commit, addInitialize(int)andClear()) and refactor Technic Move output sending (including PLAYVM support).
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| BrickController2/BrickController2/Protocols/LegoWirelessProtocol.cs | Adds ToByte(Half) conversion helper used by new Half-based output tracking. |
| BrickController2/BrickController2/DeviceManagement/Wedo2Device.cs | Clears cached characteristics on disconnect via new hook. |
| BrickController2/BrickController2/DeviceManagement/Vengit/SBrickLightDevice.cs | Clears cached characteristics on disconnect; fixes Commit() typo usage. |
| BrickController2/BrickController2/DeviceManagement/TechnicMoveDevice.cs | Major refactor: PLAYVM output grouping and send loop, light handling, servo reset/calibration flow. |
| BrickController2/BrickController2/DeviceManagement/SBrickDevice.cs | Clears cached characteristics on disconnect. |
| BrickController2/BrickController2/DeviceManagement/PfxBrickDevice.cs | Clears cached characteristics on disconnect; fixes Commit() typo usage. |
| BrickController2/BrickController2/DeviceManagement/MouldKing/MK_DIY.cs | Clears cached characteristic on disconnect. |
| BrickController2/BrickController2/DeviceManagement/Lego/RemoteControl.cs | Moves Remote Control onto shared Control+ base; routes hub property requests through base helpers. |
| BrickController2/BrickController2/DeviceManagement/Lego/ControlPlusDeviceBase.cs | New shared base for LEGO Control+ devices: service validation, notification parsing, hub property requests. |
| BrickController2/BrickController2/DeviceManagement/IO/OutputValuesGroup.cs | Adds Initialize(int) + Clear(), renames Commmit()→Commit(), fixes comment typo. |
| BrickController2/BrickController2/DeviceManagement/Device.cs | CheckChannel now returns the validated channel index. |
| BrickController2/BrickController2/DeviceManagement/ControlPlusDevice.cs | Refactors to inherit from ControlPlusDeviceBase; renames ResetSendAttempts; uses shared write helpers. |
| BrickController2/BrickController2/DeviceManagement/CircuitCubeDevice.cs | Clears cached characteristics on disconnect. |
| BrickController2/BrickController2/DeviceManagement/BuwizzDevice.cs | Clears cached characteristic on disconnect. |
| BrickController2/BrickController2/DeviceManagement/BuWizz3Device.cs | Clears cached characteristics on disconnect. |
| BrickController2/BrickController2/DeviceManagement/BuWizz2Device.cs | Clears cached characteristics on disconnect. |
| BrickController2/BrickController2/DeviceManagement/BluetoothDevice.cs | Adds OnDeviceDisconnecting() hook and improves disconnect callback handling. |
| BrickController2/BrickController2.Tests/DeviceManagement/IO/OutputValuesGroupTests.cs | Updates test to use renamed Commit(). |
| BrickController2/BrickController2.Android/PlatformServices/BluetoothLE/BluetoothLEDevice.cs | Refreshes Android GATT service cache (reflection) before close/dispose on disconnect. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| var absPosition = ToInt32(data, dataIndex); | ||
| _absolutePositions[channel] = absPosition; | ||
|
|
||
| dataIndex += 2; |
There was a problem hiding this comment.
In case 0x46 (combined mode), ABS position is read with ToInt32(data, dataIndex) but dataIndex is only advanced by 2. This mismatch can corrupt parsing and can read past the end of the payload. Use ToInt16 + += 2 or ToInt32 + += 4 (with length checks), depending on the expected protocol format.
| var absPosition = ToInt32(data, dataIndex); | |
| _absolutePositions[channel] = absPosition; | |
| dataIndex += 2; | |
| if ((dataIndex + 1) < data.Length) | |
| { | |
| var absPosition = ToInt16(data, dataIndex); | |
| _absolutePositions[channel] = absPosition; | |
| dataIndex += 2; | |
| } | |
| else | |
| { | |
| // Not enough data to read ABS position safely; skip parsing it. | |
| } |
BrickController2/BrickController2/DeviceManagement/TechnicMoveDevice.cs
Outdated
Show resolved
Hide resolved
BrickController2/BrickController2/DeviceManagement/IO/OutputValuesGroup.cs
Outdated
Show resolved
Hide resolved
BrickController2/BrickController2/DeviceManagement/IO/OutputValuesGroup.cs
Show resolved
Hide resolved
BrickController2/BrickController2/Protocols/LegoWirelessProtocol.cs
Outdated
Show resolved
Hide resolved
| case 0x45: // Port value (single mode) | ||
| lock (_positionLock) | ||
| { | ||
| if (data.Length == 6) | ||
| { |
There was a problem hiding this comment.
This class uses _positionLock/_absolutePositions/_relativePositions/_positionsUpdated/_positionUpdateTimes, but those members are not declared in ControlPlusDeviceBase (or its base classes). As written this won’t compile; move the position state into ControlPlusDeviceBase (and expose it to derived types) or delegate the storage logic to derived classes via virtual/abstract methods.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| await SendPortOutput_HubLedAsync(HUB_LED_COLOR_LIGHT_BLUE, token); | ||
|
|
||
| // wait for initialization to complete before sending any commands | ||
| await WaitForInitializationAsync(token); |
There was a problem hiding this comment.
WaitForInitializationAsync(token) returns a boolean indicating whether initialization completed before timeout, but the return value is currently ignored. If initialization times out, subsequent writes (lights off / PLAYVM setup) may run before ports are ready; consider checking the result and either retrying or aborting setup with a clear failure path.
| await WaitForInitializationAsync(token); | |
| var initialized = await WaitForInitializationAsync(token); | |
| if (!initialized) | |
| { | |
| return false; | |
| } |
BrickController2/BrickController2/DeviceManagement/TechnicMoveDevice.cs
Outdated
Show resolved
Hide resolved
BrickController2/BrickController2/DeviceManagement/TechnicMoveDevice.cs
Outdated
Show resolved
Hide resolved
| /// </summary> | ||
| protected async Task<bool> WaitForInitializationAsync(CancellationToken token) | ||
| { | ||
| _initializationTcs = new TaskCompletionSource<bool>(); |
There was a problem hiding this comment.
WaitForInitializationAsync creates a TaskCompletionSource<bool> without TaskCreationOptions.RunContinuationsAsynchronously. Since completion can occur on a timer/notification callback thread, running continuations inline can cause unexpected reentrancy; consider creating the TCS with RunContinuationsAsynchronously (as done elsewhere in this PR).
| _initializationTcs = new TaskCompletionSource<bool>(); | |
| _initializationTcs = new TaskCompletionSource<bool>(TaskCreationOptions.RunContinuationsAsynchronously); |
| { | ||
| Characteristic = null; |
There was a problem hiding this comment.
OnDeviceDisconnecting() only nulls Characteristic. Any pending initialization debounce timer/CTS and _initializationTcs remain alive across disconnects, which can leak and/or complete unexpectedly after reconnect. Consider cancelling/disposing _debounceCts and clearing _initializationTcs/state here.
| { | |
| Characteristic = null; | |
| { | |
| // Clear characteristic reference for this connection | |
| Characteristic = null; | |
| // Cancel and dispose any pending debounce timer to avoid it firing after disconnect | |
| var debounceCts = _debounceCts; | |
| if (debounceCts is not null) | |
| { | |
| _debounceCts = null; | |
| try | |
| { | |
| debounceCts.Cancel(); | |
| } | |
| catch (ObjectDisposedException) | |
| { | |
| // If already disposed, ignore | |
| } | |
| debounceCts.Dispose(); | |
| } | |
| // Cancel any pending initialization task so it cannot complete after reconnect | |
| var initializationTcs = _initializationTcs; | |
| if (initializationTcs is not null && !initializationTcs.Task.IsCompleted) | |
| { | |
| initializationTcs.TrySetCanceled(); | |
| } | |
| _initializationTcs = null; |
| private void ResetDebounceTimer() | ||
| { | ||
| // Cancel the previous timer | ||
| _debounceCts?.Cancel(); | ||
| _debounceCts = new CancellationTokenSource(); | ||
|
|
||
| // Start a new timer | ||
| Task.Delay(BurstTimeout, _debounceCts.Token).ContinueWith(t => | ||
| { | ||
| // If the task was NOT canceled, it means timeout passed with no new ports | ||
| if (!t.IsCanceled && _initializationTcs != null && !_initializationTcs.Task.IsCompleted) | ||
| { | ||
| _initializationTcs.TrySetResult(true); | ||
| } | ||
| }); | ||
| } | ||
|
|
There was a problem hiding this comment.
ResetDebounceTimer uses Task.Delay(...).ContinueWith(...) without specifying a scheduler, so it can capture TaskScheduler.Current and run the continuation on an unexpected context. Consider using TaskScheduler.Default (or rewriting as an async fire-and-forget method) and disposing the previous _debounceCts after cancellation to avoid CTS buildup.
| private void ResetDebounceTimer() | |
| { | |
| // Cancel the previous timer | |
| _debounceCts?.Cancel(); | |
| _debounceCts = new CancellationTokenSource(); | |
| // Start a new timer | |
| Task.Delay(BurstTimeout, _debounceCts.Token).ContinueWith(t => | |
| { | |
| // If the task was NOT canceled, it means timeout passed with no new ports | |
| if (!t.IsCanceled && _initializationTcs != null && !_initializationTcs.Task.IsCompleted) | |
| { | |
| _initializationTcs.TrySetResult(true); | |
| } | |
| }); | |
| } | |
| private async Task RunDebounceTimerAsync(CancellationTokenSource debounceCts) | |
| { | |
| try | |
| { | |
| await Task.Delay(BurstTimeout, debounceCts.Token).ConfigureAwait(false); | |
| // If the task was NOT canceled, it means timeout passed with no new ports | |
| if (!debounceCts.IsCancellationRequested && _initializationTcs != null && !_initializationTcs.Task.IsCompleted) | |
| { | |
| _initializationTcs.TrySetResult(true); | |
| } | |
| } | |
| catch (TaskCanceledException) | |
| { | |
| // Swallow cancellation exceptions; cancellation is expected behavior. | |
| } | |
| finally | |
| { | |
| debounceCts.Dispose(); | |
| } | |
| } | |
| private void ResetDebounceTimer() | |
| { | |
| // Cancel and dispose the previous timer | |
| var previousCts = _debounceCts; | |
| if (previousCts != null) | |
| { | |
| try | |
| { | |
| previousCts.Cancel(); | |
| } | |
| finally | |
| { | |
| previousCts.Dispose(); | |
| } | |
| } | |
| // Start a new timer | |
| _debounceCts = new CancellationTokenSource(); | |
| var currentCts = _debounceCts; | |
| // Fire-and-forget debounce timer using the default task scheduler | |
| _ = RunDebounceTimerAsync(currentCts); | |
| } |
BrickController2/BrickController2/DeviceManagement/IO/OutputValuesGroup.cs
Show resolved
Hide resolved
BrickController2/BrickController2/DeviceManagement/IO/OutputValuesGroup.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
BrickController2/BrickController2/DeviceManagement/Lego/ControlPlusDeviceBase.cs
Outdated
Show resolved
Hide resolved
| _virtualPortSendBuffer[6] = (byte)(value1 < 0 ? (255 + value1) : value1); | ||
| _virtualPortSendBuffer[7] = (byte)(value2 < 0 ? (255 + value2) : value2); |
There was a problem hiding this comment.
SendOutputValueVirtualAsync still converts negative motor values using 255 + value while GetOutputCommand now uses LegoWirelessProtocol.ToByte(value) (two's-complement via value & 0xFF). These mappings differ (e.g., -1 becomes 254 vs 255), so virtual-port outputs can behave differently from non-virtual outputs. Consider using the same conversion helper for both paths to keep behavior consistent.
| _virtualPortSendBuffer[6] = (byte)(value1 < 0 ? (255 + value1) : value1); | |
| _virtualPortSendBuffer[7] = (byte)(value2 < 0 ? (255 + value2) : value2); | |
| _virtualPortSendBuffer[6] = ToByte(value1); | |
| _virtualPortSendBuffer[7] = ToByte(value2); |
| /// </summary> | ||
| public void NotifyPortAttached() | ||
| { | ||
| _debounceCts?.Cancel(); |
There was a problem hiding this comment.
DeviceInitializationWaiter.NotifyPortAttached() cancels the previous CancellationTokenSource but never disposes it. This can leak native resources over repeated reconnects/attach bursts. Dispose the old CTS after canceling (and consider setting it to null before creating a new one) to avoid accumulation.
| _debounceCts?.Cancel(); | |
| var oldCts = _debounceCts; | |
| if (oldCts != null) | |
| { | |
| oldCts.Cancel(); | |
| oldCts.Dispose(); | |
| } |
| Task.Delay(BurstTimeout, _debounceCts.Token).ContinueWith(t => | ||
| { | ||
| if (!t.IsCanceled && _initializationTcs is { Task.IsCompleted: false }) | ||
| { | ||
| _initializationTcs.TrySetResult(true); | ||
| } | ||
| }); |
There was a problem hiding this comment.
NotifyPortAttached() uses Task.Delay(...).ContinueWith(...) without specifying a TaskScheduler. ContinueWith defaults to TaskScheduler.Current, which can unexpectedly marshal the continuation onto a captured context (e.g., UI) depending on where NotifyPortAttached is called. Prefer using an async method with ConfigureAwait(false), or specify TaskScheduler.Default (and appropriate TaskContinuationOptions) so this always runs on the threadpool.
| Task.Delay(BurstTimeout, _debounceCts.Token).ContinueWith(t => | |
| { | |
| if (!t.IsCanceled && _initializationTcs is { Task.IsCompleted: false }) | |
| { | |
| _initializationTcs.TrySetResult(true); | |
| } | |
| }); | |
| _ = DebounceAsync(_debounceCts.Token); | |
| } | |
| private async Task DebounceAsync(CancellationToken token) | |
| { | |
| try | |
| { | |
| await Task.Delay(BurstTimeout, token).ConfigureAwait(false); | |
| } | |
| catch (TaskCanceledException) | |
| { | |
| return; | |
| } | |
| if (_initializationTcs is { Task.IsCompleted: false }) | |
| { | |
| _initializationTcs.TrySetResult(true); | |
| } |
No description provided.