Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors LEGO Powered Up / Control+ Bluetooth device handling by introducing a shared base class for GATT characteristic setup + hub property processing, and adds a PoC calibration flow for Technic Move steering so ABS-position commands work correctly outside PLAYVM mode.
Changes:
- Add
ControlPlusDeviceBaseto centralize characteristic discovery/notification setup and hub property requests/parsing. - Add a shared
NormalizeAnglehelper inLegoWirelessProtocoland refactor parsing / byte conversions to use span-based helpers. - Update
TechnicMoveDevicesteering to apply a calibrated zero offset in non-PLAYVM mode and adjust port setup to query APOS before enabling POS notifications.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| BrickController2/BrickController2/Protocols/LegoWirelessProtocol.cs | Adds shared NormalizeAngle helper for angle math. |
| BrickController2/BrickController2/DeviceManagement/TechnicMoveDevice.cs | Implements non-PLAYVM steering calibration + offset application and LED cleanup on disconnect. |
| BrickController2/BrickController2/DeviceManagement/Lego/RemoteControl.cs | Switches RemoteControl to the new shared Control+ base and reuses hub property handling. |
| BrickController2/BrickController2/DeviceManagement/Lego/ControlPlusDeviceBase.cs | New base class for characteristic setup, write helpers, and hub property request/parse logic. |
| BrickController2/BrickController2/DeviceManagement/ControlPlusDevice.cs | Migrates to new base class, refactors parsing to spans, and routes writes through shared helpers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (angle >= 180) | ||
| { | ||
| return angle - (360 * ((angle + 180) / 360)); | ||
| } | ||
| else if (angle < -180) | ||
| { | ||
| return angle + (360 * ((180 - angle) / 360)); | ||
| } | ||
|
|
||
| return angle; |
There was a problem hiding this comment.
NormalizeAngle does not correctly normalize angles less than -180 for values beyond one full rotation (e.g., angle = -540 currently returns 180, which is outside the intended [-180, 179] range). This can lead to incorrect servo/steering calculations when angles accumulate beyond a single turn. Consider replacing this with a modulo-based normalization that always maps into [-180, 179] (e.g., using ((angle + 180) % 360 + 360) % 360 - 180).
| if (angle >= 180) | |
| { | |
| return angle - (360 * ((angle + 180) / 360)); | |
| } | |
| else if (angle < -180) | |
| { | |
| return angle + (360 * ((180 - angle) / 360)); | |
| } | |
| return angle; | |
| return ((angle + 180) % 360 + 360) % 360 - 180; |
| return; | ||
| } | ||
|
|
||
| var dataLength = data[0]; |
There was a problem hiding this comment.
dataLength is assigned but never used, which will generate an "unused variable" warning. Either remove it or use it to validate data.Length matches the reported length (if that was the intent).
| var dataLength = data[0]; | |
| var dataLength = data[0]; | |
| if (dataLength != data.Length) | |
| { | |
| return; | |
| } |
| _virtualPortSendBuffer[7] = (byte)(value2 < 0 ? (255 + value2) : value2); | ||
|
|
||
| if (await _bleDevice!.WriteNoResponseAsync(_characteristic!, _virtualPortSendBuffer, token)) | ||
| if (await WriteAsync(_virtualPortSendBuffer, token)) |
There was a problem hiding this comment.
This virtual port path switched from a no-response write to WriteAsync (write-with-response). For high-frequency output updates, this can significantly reduce throughput and increase latency. If the protocol doesn’t require an ACK here, consider using WriteNoResponseAsync (or a dedicated helper) to preserve the previous behavior/performance characteristics.
| if (await WriteAsync(_virtualPortSendBuffer, token)) | |
| if (await WriteNoResponseAsync(_virtualPortSendBuffer, token)) |
| // query current APOS | ||
| await WriteAsync([0x05, 0x00, 0x21, portId, 0x00], token); | ||
| await Task.Delay(250, token); //TODO wait for change | ||
|
|
||
| // setup channel to report POS position regularly | ||
| await WriteAsync(inputFormatForRelAngle, token); | ||
| await Task.Delay(250, token); //TODO wait for change | ||
|
|
||
| // need to recalculate base angle to support ABS POS commands | ||
| _calibratedZeroAngle = CalculateCalibratedTarget(channel); |
There was a problem hiding this comment.
Calibration currently relies on fixed delays after requesting APOS/POS (with TODOs) and then immediately computes _calibratedZeroAngle. If the hub response is delayed or dropped, calibration will be computed from stale/default _absolutePositions / _relativePositions values. Consider waiting for the actual port-value message(s) (e.g., via a TaskCompletionSource triggered in OnCharacteristicChanged, or by polling _positionUpdateTimes with a timeout) before calculating the calibrated target.
vicocz
changed the title
No description provided.