LEGO Technice Move hub improvements by vicocz · Pull Request #217 · vicocz/brickcontroller2

vicocz · 2026-04-02T22:23:08Z

No description provided.

Copilot

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 ControlPlusDeviceBase to centralize characteristic discovery/notification setup and hub property requests/parsing.
Add a shared NormalizeAngle helper in LegoWirelessProtocol and refactor parsing / byte conversions to use span-based helpers.
Update TechnicMoveDevice steering 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.

Copilot · 2026-04-02T22:28:48Z

+        if (angle >= 180)
+        {
+            return angle - (360 * ((angle + 180) / 360));
+        }
+        else if (angle < -180)
+        {
+            return angle + (360 * ((180 - angle) / 360));
+        }
+
+        return angle;


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).

Suggested change

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;

Copilot · 2026-04-02T22:28:48Z

+                return;
+            }
+
+            var dataLength = data[0];


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).

Suggested change

var dataLength = data[0];

var dataLength = data[0];

if (dataLength != data.Length)

{

return;

}

Copilot · 2026-04-02T22:28:49Z

                    _virtualPortSendBuffer[7] = (byte)(value2 < 0 ? (255 + value2) : value2);

-                    if (await _bleDevice!.WriteNoResponseAsync(_characteristic!, _virtualPortSendBuffer, token))
+                    if (await WriteAsync(_virtualPortSendBuffer, token))


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.

Suggested change

if (await WriteAsync(_virtualPortSendBuffer, token))

if (await WriteNoResponseAsync(_virtualPortSendBuffer, token))

Copilot · 2026-04-02T22:28:49Z

+                // 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);


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.

Copilot

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.

Comments suppressed due to low confidence (1)

BrickController2/BrickController2/DeviceManagement/ControlPlusDevice.cs:384

SendOutputValueVirtualAsync now uses WriteAsync (write-with-response) for virtual port updates. Virtual port writes can be high-frequency; using write-with-response can significantly reduce throughput compared to WriteNoResponseAsync and may increase latency/jitter. If the hub supports it, consider switching this back to no-response writes (and keep the existing send delay if needed to avoid flooding).

        private async Task<bool> SendOutputValueVirtualAsync(int virtualChannel, int channel1, int channel2, int value1, int value2, CancellationToken token)
        {
            try
            {
                if (_lastOutputValues[channel1] != value1 || _lastOutputValues[channel2] != value2)
                {
                    _virtualPortSendBuffer[3] = (byte)virtualChannel;
                    _virtualPortSendBuffer[6] = (byte)(value1 < 0 ? (255 + value1) : value1);
                    _virtualPortSendBuffer[7] = (byte)(value2 < 0 ? (255 + value2) : value2);

                    if (await WriteAsync(_virtualPortSendBuffer, token))
                    {
                        _lastOutputValues[channel1] = value1;
                        _lastOutputValues[channel2] = value2;

                        await Task.Delay(SEND_DELAY, token);
                        return true;

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-28T22:04:33Z

+            // in non PLAYVM mode, need to apply calibrated base angle as offset to reach correct position
+            var absPosition = _calibratedZeroAngle + ChannelConfigs[channel].ServoBaseAngle + value;


SendServoValue is using the already-byte-encoded output value as an angle delta (... + value). Since the upstream value is a percent in [-100..100], this should be scaled by ChannelConfigs[channel].MaxServoAngle (and must preserve sign) before adding to the absolute position; otherwise steering will be wrong, especially for negative values.

Suggested change

// in non PLAYVM mode, need to apply calibrated base angle as offset to reach correct position

var absPosition = _calibratedZeroAngle + ChannelConfigs[channel].ServoBaseAngle + value;

var percentValue = unchecked((sbyte)(byte)value);

var servoDelta = ChannelConfigs[channel].MaxServoAngle * percentValue / 100;

// in non PLAYVM mode, need to apply calibrated base angle as offset to reach correct position

var absPosition = _calibratedZeroAngle + ChannelConfigs[channel].ServoBaseAngle + servoDelta;

Copilot · 2026-04-28T22:04:34Z

+
+                result = result && await SendPortOutput_ValueAsync(channel, value, token);


SendAllOutputValuesAsync sends each A/B/C output twice: once via the outputType switch and again unconditionally with SendPortOutput_ValueAsync. This duplicates BLE writes (and for servo channels likely sends an incorrect extra direct-mode value command). Remove the unconditional second send, and ensure servo channels only send the servo command variant.

Suggested change

result = result && await SendPortOutput_ValueAsync(channel, value, token);

Copilot · 2026-04-28T22:04:34Z

+    /// <summary>Returns a new state with an updated absolute position.</summary>
+    public ChannelPositionState WithAbsolutePosition(int absolutePosition) =>
+        this with { AbsolutePosition = absolutePosition, IsUpdated = true, UpdateTime = DateTime.Now };


WithAbsolutePosition sets IsUpdated = true and updates UpdateTime. In the existing servo-speed logic (ControlPlusDevice.CalculateServoSpeed), IsUpdated is treated as “new relative position arrived”; marking absolute updates this way can cause servo speed to be computed off stale relative data or to suppress servo writes because UpdateTime looks fresh. Consider only setting IsUpdated/UpdateTime on relative updates, or track absolute-vs-relative updates separately.

Suggested change

/// <summary>Returns a new state with an updated absolute position.</summary>

public ChannelPositionState WithAbsolutePosition(int absolutePosition) =>

this with { AbsolutePosition = absolutePosition, IsUpdated = true, UpdateTime = DateTime.Now };

/// <summary>Returns a new state with an updated absolute position without marking a new relative update.</summary>

public ChannelPositionState WithAbsolutePosition(int absolutePosition) =>

this with { AbsolutePosition = absolutePosition };

Copilot · 2026-04-28T22:04:34Z

+                    foreach (KeyValuePair<int, Half> change in changes)
+                    {
+                        var value = ToByte(change.Value);
+                        var channelOutputType = change.Key < NumberOfChannels
+                            ? ChannelConfigs[change.Key].OutputType
+                            : ChannelOutputType.NormalMotor;
+
+                        result = change.Key switch
+                        {
+                            // Light channels 1 - 6 require absolute value
+                            >= CHANNEL_1 and <= CHANNEL_6 => await SendPortOutput_6LedAsync(ledIndex: change.Key - CHANNEL_1, value, token),
+                            // all channels command
+                            int.MaxValue => await SendAllOutputValuesAsync(value, token),
+                            // classic output command for A, B, C channels (with servo support)
+                            CHANNEL_C when channelOutputType == ChannelOutputType.ServoMotor => await SendServoValue(change.Key, value, token),
+                            _ => await SendPortOutput_ValueAsync(change.Key, value, token),


SendOutputValuesAsync converts the channel value to a byte (ToByte(change.Value)) before deciding whether to send a servo command. This loses the sign (e.g., -100 becomes 156) and also makes it easy to accidentally use the wrapped byte value in servo angle math. Keep the signed/int value for servo calculations (including scaling by MaxServoAngle), and only convert to byte when building motor/LED commands.

Copilot

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 7 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-29T21:22:32Z

+            var channelPositions = ChannelRelativePositions.Get(channel);
+
+            if (channelPositions.IsUpdated)
            {
-                if (_positionsUpdated[channel])
-                {
-                    var diffAngle = Math.Abs(_relativePositions[channel] - targetAngle);
-                    _positionsUpdated[channel] = false;
+                var diffAngle = Math.Abs(channelPositions.Current - targetAngle);
+                ChannelRelativePositions.Update(channel, x => x.ConsumeUpdate());



CalculateServoSpeed does a Get() and then a separate Update(...ConsumeUpdate()). If a new position update arrives between those calls, ConsumeUpdate() can clear the new update and you’ll compute speed from a stale snapshot. To avoid losing updates, consider an atomic “read-and-consume” operation (e.g., a TryConsumeUpdate method on ChannelStateStore) so the snapshot you use is the same one you clear.

Copilot · 2026-04-29T21:22:33Z

+        var configs = channelConfigurations
+            .Where(c => IsOutputTypeSupported(c.Channel, c.ChannelOutputType))
+            .ToDictionary(c => c.Channel, c => c);


channelConfigurations.Where(...).ToDictionary(c => c.Channel, ...) will throw if multiple controller actions target the same device/channel (which can happen when building channel configs from controller actions). Previously duplicates would effectively overwrite; now this can fail the whole connect path. Consider de-duplicating (e.g., GroupBy channel and take the last/highest-priority config) or building the dictionary with an overwrite strategy instead of ToDictionary.

Suggested change

var configs = channelConfigurations

.Where(c => IsOutputTypeSupported(c.Channel, c.ChannelOutputType))

.ToDictionary(c => c.Channel, c => c);

// If multiple configurations target the same channel, keep the last one.

var configs = new Dictionary<int, ChannelConfiguration>();

foreach (var channelConfiguration in channelConfigurations.Where(c => IsOutputTypeSupported(c.Channel, c.ChannelOutputType)))

{

configs[channelConfiguration.Channel] = channelConfiguration;

}

Copilot · 2026-04-29T21:22:33Z

+            case MESSAGE_TYPE_PORT_VALUE_COMBINED: // Port value (combined mode)
+                {
+                    if (!TryGetChannelIndex(portId: data[3], out var channel))
+                    {
+                        break;
+                    }
+
+                    var modeMask = data[5];
+                    var currentData = data.Slice(6); // start at index 6
+
+                    if ((modeMask & 0x01) != 0)
+                    {
+                        var absPosition = ToInt16(currentData);


The combined port-value handler reads data[5] and slices at index 6 without validating data.Length. A short/partial BLE notification would cause an IndexOutOfRangeException in the notification callback. Add explicit length checks (e.g., data.Length >= 6 before reading the mode mask, and ensure enough bytes exist before ToInt16(currentData) when ABS is present).

Copilot · 2026-04-29T21:22:33Z

+                    return; // position stable for the required duration
+                }
+            }
+            Debug.WriteLine($"Position time outed having {lastPosition}.");


Spelling/grammar: "Position time outed having ..." is awkward and looks like a typo. Consider changing it to "Position timed out (last position: ...)" or similar for clearer debug output.

Suggested change

Debug.WriteLine($"Position time outed having {lastPosition}.");

Debug.WriteLine($"Position timed out (last position: {lastPosition}).");

Copilot · 2026-04-29T21:22:34Z

+                            // classic output command for A, B, C channels (with servo support)
+                            CHANNEL_C when channelOutputType == ChannelOutputType.ServoMotor => await SendServoValue(change.Key, value, token),
+                            _ => await SendPortOutput_ValueAsync(change.Key, value, token),


value here is a byte produced via ToByte(change.Value), which encodes signed motor power in two’s-complement. Passing that byte into SendServoValue loses the sign (implicit byte→int yields 0–255) and also mixes “motor power” encoding with “servo angle” semantics. For servo output, keep the original signed percentage (e.g., from change.Value) and convert it to a target angle using the channel’s max servo angle before building the GotoAbsPosition command.

Copilot · 2026-04-29T21:22:34Z

+            var portId = GetPortId(channel);
+            // in non PLAYVM mode, need to apply calibrated base angle as offset to reach correct position
+            var absPosition = _calibratedZeroAngle + ChannelConfigs[channel].ServoBaseAngle + value; //TODO MAX SERVO ANGLE
+            var cmd = BuildPortOutput_GotoAbsPosition(portId, absPosition, servoSpeed: 50);
+            return await WriteAsync(cmd, token);


SendServoValue currently treats its value parameter as a direct angle delta and adds it to _calibratedZeroAngle + ServoBaseAngle, but the caller passes a raw byte intended for direct motor output (-100..100 encoded as 0–255). This will produce incorrect absolute positions (especially for negative values) and can exceed expected ranges. Consider changing the API to accept a signed percentage (or angle) explicitly, convert percent→degrees using MaxServoAngle, and only then apply the calibrated/base offsets.

Copilot · 2026-04-29T21:22:34Z

+                // query current APOS
+                await WriteAsync([0x05, 0x00, 0x21, portId, 0x00], token);
+                await Task.Delay(250, token); //TODO wait for change


This hard-coded message [0x05, 0x00, 0x21, portId, 0x00] is a protocol-specific constant (0x21) with no local explanation. To make the intent clearer and avoid future drift, consider defining a named constant / helper in LegoWirelessProtocol for the “port information request” message type (and/or the whole request builder) and use it here.

Copilot

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 8 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-30T21:02:44Z

+                // setup channel to for APOS, but no notifications
+                var inputFormatForAbsAngleDisabled = BuildPortInputFormatSetup(portId, PORT_MODE_3, notification: PORT_VALUE_NOTIFICATION_DISABLED);
+                await WriteAsync(inputFormatForAbsAngleDisabled, token);
+                await Task.Delay(50, 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
+                var inputFormatForRelAngle = BuildPortInputFormatSetup(portId, PORT_MODE_2);
+                await WriteAsync(inputFormatForRelAngle, token);
+                await Task.Delay(250, token); //TODO wait for change
+
+                // need to recalculate zero angle to support ABS POS commands
+                _calibratedZeroAngle = CalculateCalibratedTarget(channel);
+


In non-PLAYVM mode, SetupChannelForPortInformationAsync computes _calibratedZeroAngle = CalculateCalibratedTarget(channel) immediately after fixed delays, but it never waits for ChannelAbsPositions / ChannelRelativePositions to actually be updated from the APOS query / POS notifications. If those messages haven't arrived yet, calibration will be computed from default (0) positions. Consider awaiting a first update (e.g., wait until ChannelAbsPositions.Get(channel).IsUpdated and/or ChannelRelativePositions.Get(channel).IsUpdated changes) instead of using fixed Task.Delay placeholders.

Copilot · 2026-04-30T21:02:44Z

+            case MESSAGE_TYPE_PORT_VALUE_COMBINED: // Port value (combined mode)
+                {
+                    if (!TryGetChannelIndex(portId: data[3], out var channel))
+                    {
+                        break;
+                    }
+
+                    var modeMask = data[5];
+                    var currentData = data.Slice(6); // start at index 6
+
+                    if ((modeMask & 0x01) != 0)
+                    {
+                        var absPosition = ToInt16(currentData);
+                        ChannelAbsPositions.Update(channel, pos => pos.WithPosition(absPosition));
+
+                        currentData = currentData.Slice(2);
+                    }
+
+                    if ((modeMask & 0x02) != 0)
+                    {
+                        // TODO: Read the post value format response and determine the value length accordingly
+                        int relPosition = currentData.Length switch
+                        {
+                            >= 4 => ToInt32(currentData),
+                            >= 2 => ToInt16(currentData),
+                            _ => currentData[0]
+                        };
+                        ChannelRelativePositions.Update(channel, pos => pos.WithPosition(relPosition));
+                    }


MESSAGE_TYPE_PORT_VALUE_COMBINED parsing assumes data.Length >= 6 and that the payload contains enough bytes for each mode (ToInt16(currentData) / ToInt32(currentData)), but there are no length checks before indexing data[5] or slicing/reading. A shorter/malformed notification would throw and potentially break the BLE notification handler. Add explicit minimum-length guards before accessing data[5] and before reading 2/4 bytes from currentData.

Copilot · 2026-04-30T21:02:44Z

+            case MESSAGE_TYPE_HUB_ATTACHED_IO: // Hub attached I/O
+                {
+                    Dump("HUB_ATTACHED_IO", data);
+
+                    if (TryGetChannelIndex(portId: data[3], out var channel))
+                    {
+                        byte eventType = data[4]; // 0x01 = Attached, 0x00 = Detached, 0x02 = Attached Virtual
+
+                        if (eventType == 0x01 || eventType == 0x02)
+                        {
+                            var deviceId = ToUInt16(data.Slice(5));
+                            AttachedHubs.Update(channel, info => info.WithDevice(deviceId)); // store portId as "position" for simplicity
+                        }
+                        else if (eventType == 0x00)
+                        {
+                            AttachedHubs.Remove(channel);
+                        }


MESSAGE_TYPE_HUB_ATTACHED_IO handling reads data[4] and ToUInt16(data.Slice(5)) without verifying the message is long enough (needs at least 7 bytes for the device id). Add a data.Length guard (and consider validating the expected event payload size) to avoid ArgumentOutOfRangeException on unexpected messages.

Copilot · 2026-04-30T21:02:45Z

+using System.Numerics;
+
+namespace BrickController2.DeviceManagement.Lego;
+
+/// <summary>
+/// Immutable snapshot of a motor channel's position feedback.


ChannelAttachmentInfo's XML summary says it's a snapshot of a motor channel's position feedback, but the record represents attachment/device-id info. Also using System.Numerics; is unused here. Updating the summary and removing the unused using will reduce confusion and warnings.

Suggested change

using System.Numerics;

namespace BrickController2.DeviceManagement.Lego;

/// <summary>

/// Immutable snapshot of a motor channel's position feedback.

namespace BrickController2.DeviceManagement.Lego;

/// <summary>

/// Immutable snapshot of a channel's attachment state, including device ID and update metadata.

Copilot · 2026-04-30T21:02:45Z

+    public override Task<DeviceConnectionResult> ConnectAsync(
+        bool reconnect,
+        Action<Device> onDeviceDisconnected,
+        IEnumerable<ChannelConfiguration> channelConfigurations,
+        bool startOutputProcessing,
+        bool requestDeviceInformation,
+        CancellationToken token)
+    {
+        // reset output values & positions
+        ResetOutputValues();
+        ChannelAbsPositions.Clear();
+        ChannelRelativePositions.Clear();
+        AttachedHubs.Clear();
+
+        // Initialize configuration per channel
+
+        // build dictionary, but for supported ones only
+        var configs = channelConfigurations
+            .Where(c => IsOutputTypeSupported(c.Channel, c.ChannelOutputType))
+            .ToDictionary(c => c.Channel, c => c);
+
+        for (int i = 0; i < NumberOfChannels; i++)
+        {
+            configs.TryGetValue(i, out var config);
+
+            ChannelConfigs[i] = config.ChannelOutputType switch
+            {
+                ChannelOutputType.ServoMotor => new()
+                {
+                    OutputType = ChannelOutputType.ServoMotor,
+                    MaxServoAngle = config.MaxServoAngle,
+                    ServoBaseAngle = config.ServoBaseAngle
+                },
+                ChannelOutputType.StepperMotor => new()
+                {
+                    OutputType = ChannelOutputType.StepperMotor,
+                    StepperAngle = config.StepperAngle
+                },
+                _ => new()
+            };
+        }
+
+        return base.ConnectAsync(reconnect, onDeviceDisconnected, channelConfigurations, startOutputProcessing, requestDeviceInformation, token);
+    }


This new WirelessProtocolBasedDevice base class introduces non-trivial connection/config parsing logic (e.g., channel-config normalization and message parsing). There are existing unit tests for OutputValuesGroup and LEGO device discovery, but no tests covering these new behaviors (notably: duplicate channel configurations, and parsing of combined/attached-IO messages). Adding focused unit tests around ConnectAsync config handling and TryProcessMessageData length-guarding would help prevent regressions.

Copilot · 2026-04-30T21:02:45Z

+        var configs = channelConfigurations
+            .Where(c => IsOutputTypeSupported(c.Channel, c.ChannelOutputType))
+            .ToDictionary(c => c.Channel, c => c);


ConnectAsync builds configs via ToDictionary(c => c.Channel, ...). channelConfigurations can include multiple entries for the same channel (e.g., multiple actions targeting the same device/channel), which will throw ArgumentException at runtime. Prefer grouping/deduping (e.g., GroupBy(Channel) and take last/first, or DistinctBy) before creating the dictionary, or populate ChannelConfigs via a simple loop that tolerates duplicates.

Suggested change

var configs = channelConfigurations

.Where(c => IsOutputTypeSupported(c.Channel, c.ChannelOutputType))

.ToDictionary(c => c.Channel, c => c);

var configs = new Dictionary<int, ChannelConfiguration>();

foreach (var config in channelConfigurations.Where(c => IsOutputTypeSupported(c.Channel, c.ChannelOutputType)))

{

configs[config.Channel] = config;

}

Copilot · 2026-04-30T21:02:46Z

+        var stableSince = Stopwatch.StartNew();
+        var lastUpdated = AttachedHubs.Max(x => x.UpdateTime);
+
+        try
+        {
+            while (!linkedCts.Token.IsCancellationRequested)
+            {
+                await Task.Delay(interval, linkedCts.Token);
+
+                var currentValue = AttachedHubs.Max(x => x.UpdateTime);
+                if (currentValue != lastUpdated)
+                {
+                    lastUpdated = currentValue;
+                    stableSince.Restart();
+                }
+                else if (stableSince.Elapsed >= stabilityTimeout)
+                {
+                    Dump("Hub connection: HUBS", AttachedHubs.Count);
+                    return; // position stable for the required duration
+                }


AwaitForHubConnectedAsync returns once AttachedHubs.Max(UpdateTime) is stable for 250ms, but if no HUB_ATTACHED_IO messages arrive then lastUpdated stays at DateTime.MinValue and the method still returns after ~250ms (false positive). Consider requiring at least one attachment update (e.g., AttachedHubs.Count > 0 or lastUpdated != DateTime.MinValue) before treating the connection as "stable", otherwise wait until timeout elapses.

Copilot · 2026-04-30T21:02:46Z

+            // in non PLAYVM mode, need to apply calibrated base angle as offset to reach correct position
+            var absPosition = _calibratedZeroAngle + ChannelConfigs[channel].ServoBaseAngle + value; //TODO MAX SERVO ANGLE


SendServoValue computes absPosition by adding the raw output value directly to the calibrated/base angles, but the stored output values are percentages (-100..100) in the rest of the codebase (and PLAYVM path scales by MaxServoAngle). This makes non-PLAYVM servo output inconsistent and ignores MaxServoAngle (noted by the TODO). Consider scaling value to degrees using GetMaxServoAngle(channel) (and clamping to valid range) before building the Goto-ABS command.

Suggested change

// in non PLAYVM mode, need to apply calibrated base angle as offset to reach correct position

var absPosition = _calibratedZeroAngle + ChannelConfigs[channel].ServoBaseAngle + value; //TODO MAX SERVO ANGLE

var maxServoAngle = GetMaxServoAngle(channel);

var clampedValue = Math.Clamp(value, -100, 100);

var relativeAngle = maxServoAngle * clampedValue / 100;

// in non PLAYVM mode, apply calibrated base angle plus the scaled servo offset

var absPosition = _calibratedZeroAngle + ChannelConfigs[channel].ServoBaseAngle + relativeAngle;

Vit Nemecky added 5 commits April 1, 2026 23:43

Technic Move Hub improvements

4d66de2

nitpicks

6b118ee

work save

089422d

try servo via GOTO ABS

1d9a263

reset led in the ned

239d7cf

vicocz requested a review from Copilot April 2, 2026 22:23

Copilot started reviewing on behalf of vicocz April 2, 2026 22:24 View session

Copilot AI reviewed Apr 2, 2026

View reviewed changes

vicocz changed the title ~~[PoC] Local/tehnice move hub improvements~~ [PoC] LEGO Technice Move hub improvements Apr 3, 2026

work save

2b905c1

vicocz changed the title ~~[PoC] LEGO Technice Move hub improvements~~ LEGO Technice Move hub improvements Apr 28, 2026

keep current positions in base class

977da82

vicocz requested a review from Copilot April 28, 2026 21:58

Copilot started reviewing on behalf of vicocz April 28, 2026 21:58 View session

Copilot AI reviewed Apr 28, 2026

View reviewed changes

work save

f4c22b9

vicocz requested a review from Copilot April 29, 2026 21:16

Copilot started reviewing on behalf of vicocz April 29, 2026 21:17 View session

Copilot AI reviewed Apr 29, 2026

View reviewed changes

Vit Nemecky added 3 commits April 29, 2026 23:51

nitpicks

8c77896

work save - wrrking somehow

b8899f1

clean up + finetune

e428158

vicocz requested a review from Copilot April 30, 2026 20:55

Copilot started reviewing on behalf of vicocz April 30, 2026 20:56 View session

Copilot AI reviewed Apr 30, 2026

View reviewed changes

nitpicks

d7bccda

	if (await WriteAsync(_virtualPortSendBuffer, token))
	if (await WriteNoResponseAsync(_virtualPortSendBuffer, token))

		// in non PLAYVM mode, need to apply calibrated base angle as offset to reach correct position
		var absPosition = _calibratedZeroAngle + ChannelConfigs[channel].ServoBaseAngle + value;


		result = result && await SendPortOutput_ValueAsync(channel, value, token);

-        var configs = channelConfigurations
-            .Where(c => IsOutputTypeSupported(c.Channel, c.ChannelOutputType))
-            .ToDictionary(c => c.Channel, c => c);
+        // If multiple configurations target the same channel, keep the last one.
+        var configs = new Dictionary<int, ChannelConfiguration>();
+        foreach (var channelConfiguration in channelConfigurations.Where(c => IsOutputTypeSupported(c.Channel, c.ChannelOutputType)))
+        {
+            configs[channelConfiguration.Channel] = channelConfiguration;
+        }

	Debug.WriteLine($"Position time outed having {lastPosition}.");
	Debug.WriteLine($"Position timed out (last position: {lastPosition}).");

-            // in non PLAYVM mode, need to apply calibrated base angle as offset to reach correct position
-            var absPosition = _calibratedZeroAngle + ChannelConfigs[channel].ServoBaseAngle + value; //TODO MAX SERVO ANGLE
+            var maxServoAngle = GetMaxServoAngle(channel);
+            var clampedValue = Math.Clamp(value, -100, 100);
+            var relativeAngle = maxServoAngle * clampedValue / 100;
+            // in non PLAYVM mode, apply calibrated base angle plus the scaled servo offset
+            var absPosition = _calibratedZeroAngle + ChannelConfigs[channel].ServoBaseAngle + relativeAngle;

Conversation

vicocz commented Apr 2, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 29, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 29, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 29, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 29, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 29, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 29, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 29, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!