Add multiple datagram payload tests with mocks and helpers

[J0EK3R]

Added MouldKing_MK6_DatagramTests to verify datagram payload construction for MK6 devices. Introduced TestableMK6 to expose TryGetTelegram for testing and TestMKPlatformService to simulate RF payload extraction. Used Moq for dependency mocking and added tests for payload identifiers, AppIdentifier inclusion, and output value encoding across channels and addresses. Utilized xUnit and FluentAssertions for test structure and assertions.

[Copilot]

Pull request overview

Adds a new unit-test suite to validate MK6 (MouldKing) datagram/telegram payload construction, using a testable MK6 subclass plus a minimal platform-service stub to surface raw telegram bytes for assertions.

Changes:

• Introduces MouldKing_MK6_DatagramTests to verify connect/command telegram identifiers and app-id insertion.
• Adds parameterized tests to validate output-value encoding/clamping across channels and device addresses.

Comments suppressed due to low confidence (3)

BrickController2/BrickController2.Tests/DeviceManagement/MouldKing/MouldKing_MK6_DatagramTests.cs:102

• This test is marked async and returns Task but contains no awaits. Remove async and return void to avoid CS1998 warnings (and potential warnings-as-errors failures).

    public async Task MK6_TryGetTelegram_CommandDatagram_PayloadIdentifier(string deviceAddress, byte expectedPayloadIdentifier1, byte expectedPayloadIdentifier2)
    {

BrickController2/BrickController2.Tests/DeviceManagement/MouldKing/MouldKing_MK6_DatagramTests.cs:119

• This test is marked async and returns Task but contains no awaits. Remove async and return void to avoid CS1998 warnings (and potential warnings-as-errors failures).

    public async Task MK6_TryGetTelegram_CommandDatagram_AppIdentifier(string deviceAddress)
    {

BrickController2/BrickController2.Tests/DeviceManagement/MouldKing/MouldKing_MK6_DatagramTests.cs:158

• This test is marked async and returns Task but contains no awaits. Remove async and return void to avoid CS1998 warnings (and potential warnings-as-errors failures).

    public async Task MK6_Check_CommandDatagram_Payload(string deviceAddress, float[] setValues, byte[] expectedPayload)
    {

---

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

[J0EK3R]

If you're fine with the code in the current PR, I'll add unit tests for the other devices as well.

[J0EK3R]

While creating the unit tests, I discovered a bug:

The three instances of the MK4 type use the same static byte buffer for the command telegram (Telegram_Base).

• When you open the DevicePage for Instance 2 and move a channel slider to its maximum, the command telegram correctly contains the channel value at its maximum.
• If you then return to the DeviceListPage while the channel’s target value is still at maximum…
• …and afterwards open the DevicePage for Instance 1,

-> the command telegram still contains the maximum channel value from Instance 2, and this value is sent out as part of Instance 1’s command.

The solution is to override MK4.InitDevice() and reset all channel values to zero for all three instance.

[Copilot]

Pull request overview

Copilot reviewed 19 out of 19 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (7)

BrickController2/BrickController2/DeviceManagement/MouldKing/MKBaseNibble.cs:67

• Calling InitDevice() from the constructor invokes virtual/abstract logic (InitDevice -> SetChannelOutput -> ProcessChannelValue / GetTargetPosition) on the most-derived type before the derived constructor has run. This is risky and can also mutate shared static telegram buffers during mere object construction (e.g., MK4/MK5/MK3_8 use static telegram templates). Consider removing this constructor call and relying on BluetoothAdvertisingDevice.ConnectAsync() to invoke InitDevice before starting output, and/or exposing an explicit reset method for tests instead.

        _storedValues = new float[NumberOfChannels]; // initialize output values for all channels

        // initialize all channels in _telegram_Base and _storedValues to zero value
        InitDevice();

BrickController2/BrickController2/DeviceManagement/MouldKing/MKBaseByte.cs:50

• Calling InitDevice() from the constructor runs overridable logic during construction and mutates the (often static/shared) telegram buffers immediately when an instance is created. This can create hard-to-debug cross-instance side effects and duplicates the InitDevice call already performed by BluetoothAdvertisingDevice.ConnectAsync() when output processing starts. Consider removing this constructor call and having tests explicitly trigger initialization/reset.

        _telegram_Base[1] = appId[0];
        _telegram_Base[2] = appId[1];

        // initialize all channels in _telegram_Base and _storedValues to zero value
        InitDevice();

BrickController2/BrickController2/DeviceManagement/JieStar/JieStarBase.cs:61

• Calling InitDevice() from the constructor executes overridable logic during object construction. Since InitDevice ultimately calls abstract ProcessChannelValue implementations, this can run derived code before the derived constructor completes, and it also mutates the (static/shared) telegram templates earlier than before. Consider removing this constructor call and relying on BluetoothAdvertisingDevice.ConnectAsync() to initialize state, plus an explicit reset path for tests if needed.

        _telegram_Base[2] = appId[1];


        // initialize all channels in _telegram_Base and _storedValues to zero value
        InitDevice();

BrickController2/BrickController2/DeviceManagement/MouldKing/MK4.cs:79

• MK4.InitDevice() resets the shared static Telegram_Base bytes for all instances (indexes 3..8) but does not reset the BluetoothAdvertisingDeviceHandler channel-state bitfield for the other instances. If any channels from another MK4 instance were previously non-zero, the handler can keep stale non-zero bits even though the telegram bytes are reset, which can break the reconnect/all-zero detection logic. Resetting should keep the handler state consistent with the telegram content (and should be done under _outputLock for consistency with other telegram writes).

        // Reset the base telegram to its initial state for all channels and all instances to the default value of 0x88.
        // Because the 3 instances of the MK4.0 device are using the same static Telegram_Base, we need to reset the values for all channels of all instances.
        for (int index = 3; index <= 8; index++)
        {
            Telegram_Base[index] = 0x88;

BrickController2/BrickController2.Tests/DeviceManagement/MouldKing/MouldKingDatagramTestsBase.cs:10

• Spelling/grammar in the comment: "an randomly choosen" should be "a randomly chosen" (and "UnitTesting" reads better as "unit testing").

    protected const byte AppIdentifier1 = 0x61; // This is the first byte of an randomly choosen AppIdentifier for UnitTesting
    protected const byte AppIdentifier2 = 0x62; // This is the second byte of an randomly choosen AppIdentifier for UnitTesting

BrickController2/BrickController2.Tests/DeviceManagement/JieStar/JieStarDatagramTestsBase.cs:10

• Spelling/grammar in the comment: "an randomly choosen" should be "a randomly chosen" (and "UnitTesting" reads better as "unit testing").

    protected const byte AppIdentifier1 = 0x61; // This is the first byte of an randomly choosen AppIdentifier for UnitTesting
    protected const byte AppIdentifier2 = 0x62; // This is the second byte of an randomly choosen AppIdentifier for UnitTesting

BrickController2/BrickController2/DeviceManagement/MouldKing/MKBaseByte.cs:50

• The comment mentions initializing "_storedValues", but MKBaseByte doesn't have a _storedValues field. This makes the intent unclear when reading the constructor.

        // initialize all channels in _telegram_Base and _storedValues to zero value
        InitDevice();