Add base support for channel macro actions

[vicocz]

No description provided.

[Copilot]

🟡 Changes recommended

Macro persistence currently breaks SQLite mapping, and several validation, UI state, refresh, and localization paths are incomplete.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds channel macro actions to controller profiles and refactors dynamic macro discovery.

Changes:

• Adds macro selection, persistence, validation, and playback.
• Introduces reusable Bluetooth macro caching/discovery.
• Refactors PFx macro support and protocol namespace.

File summaries

File	Description
DevicePageViewModel.cs	Refreshes dynamic macros.
ControllerProfilePageViewModel.cs	Handles missing-macro validation.
ControllerActionPageViewModel.cs	Adds macro-action configuration.
ControllerActionPage.xaml	Adds macro selector controls.
PfxProtocol.cs	Moves the PFx protocol namespace.
PfxBrickDevice.cs	Adopts dynamic macro discovery.
Device.cs	Adds the macro retrieval API.
BluetoothDeviceWithMacros.cs	Implements macro caching.
ICreationManager.cs	Extends action persistence parameters.
CreationManager.cs	Stores macro selections.
Creation.cs	Collects macro references.
ControllerButtonType.cs	Adds the Macro action type.
ControllerAction.cs	Adds macro fields.
PlayLogic.cs	Validates and invokes macros.
CreationValidationResult.cs	Adds missing-macro status.
PfxProtocolTests.cs	Updates the protocol namespace.

Review details

Suppressed comments (3)

BrickController2/BrickController2/UI/Pages/ControllerActionPage.xaml:243

• MacroChoice is not defined in any translation resource, so this label renders the raw key in every locale. Add the key to all resource files.

                                    <Label Grid.Column="0" Text="{extensions:Translate MacroChoice}" VerticalOptions="Center"/>

BrickController2/BrickController2/UI/ViewModels/ControllerActionPageViewModel.cs:444

• SelectMacro is not present in any translation resource, so the selection dialog title is displayed as the raw key. Add it to all localization resource files.

            var result = await _dialogService.ShowSelectionDialogAsync(
                labels,
                Translate("SelectMacro"),
                Translate("Cancel"),
                DisappearingToken);

BrickController2/BrickController2/UI/ViewModels/ControllerActionPageViewModel.cs:475

• SelectMacroChoice is not present in any translation resource, so the selection dialog title is displayed as the raw key. Add it to all localization resource files.

            var result = await _dialogService.ShowSelectionDialogAsync(
                labels,
                Translate("SelectMacroChoice"),
                Translate("Cancel"),
                DisappearingToken);

• Files reviewed: 16/16 changed files
• Comments generated: 13
• Review effort level: Balanced

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

Macro persistence can break SQLite initialization, and several macro selection and execution paths contain functional defects.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Files not reviewed (1)

• BrickController2/BrickController2/Resources/TranslationResources.Designer.cs: Generated file

Suppressed comments (5)

Previously missed (1) — in code that hasn't changed since the last review.

BrickController2/BrickController2/DeviceManagement/FxBricks/PfxBrickDevice.cs:171

• The stop command runs only after disabling notifications. If DisableNotificationAsync throws, BluetoothDevice.DisconnectInternalAsync swallows the exception and proceeds to disconnect without sending AllOff, leaving outputs active despite this method's stated guarantee. Send AllOff before notification cleanup (or put it in a finally).

BrickController2/BrickController2/CreationManagement/ControllerAction.cs:132

• ControllerAction is mapped directly by sqlite-net (CreationRepository.InitAsync calls CreateTableAsync<ControllerAction>()), but sqlite-net cannot map a property declared as object. Adding this property therefore makes table creation/migration fail before creations can be loaded, even when no macro action exists. Store the choice in supported columns (for example, a serialized text blob plus type information) and expose the boxed value as an ignored convenience property; the import/export path must preserve the same type as well.

        public object? MacroChoiceValue
        {
            get { return _macroChoiceValue; }
            set { _macroChoiceValue = value; RaisePropertyChanged(); }

BrickController2/BrickController2/UI/ViewModels/ControllerActionPageViewModel.cs:456

• Selecting a macro changes this command's predicate from false to true, but SafeCommand only updates bound controls when RaiseCanExecuteChanged is called. For a newly created action, the macro-choice button therefore remains disabled after choosing a macro with choices.

                    RaisePropertyChanged(nameof(SelectedMacro));
                    RaisePropertyChanged(nameof(SelectedMacroDisplayName));
                    RaisePropertyChanged(nameof(SelectedMacroChoiceDisplayName));

BrickController2/BrickController2/UI/ViewModels/ControllerActionPageViewModel.cs:473

• SelectMacroChoice is not defined in the translation resources, so this title is guaranteed to render as the raw resource key. Add the corresponding neutral/localized resource (and regenerate the designer).

                Translate("SelectMacroChoice"),

BrickController2/BrickController2/UI/ViewModels/ControllerActionPageViewModel.cs:546

• MacroChoice<T> permits any struct value, and the existing macro catalog already uses float, but this conversion silently replaces every type except int and string with null. Preserve the descriptor's boxed value so valid choice types are not discarded.

            Action.MacroChoiceValue = (choice?.BoxedValue) switch
            {
                int intValue => intValue,
                string stringValue => stringValue,
                _ => null,
            };

• Files reviewed: 21/22 changed files
• Comments generated: 3
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Macro persistence, dynamic discovery, command enablement, and queued execution contain blocking correctness issues.

Get a fresh assessment by requesting another Copilot review.

Review details

Files not reviewed (1)

• BrickController2/BrickController2/Resources/TranslationResources.Designer.cs: Generated file

Suppressed comments (4)

BrickController2/BrickController2/CreationManagement/ControllerAction.cs:132

• ControllerAction is mapped by sqlite-net (CreationRepository.InitAsync calls CreateTableAsync<ControllerAction>()), but sqlite-net cannot map a property declared as System.Object. This causes database initialization/inserts to fail with an unsupported-type error, so creations cannot be loaded or saved once this model is scanned. Persist the choice through a supported scalar/blob representation (including enough type information to reconstruct it) and keep the runtime object property ignored, updating repository serialization as needed.

        public object? MacroChoiceValue
        {
            get { return _macroChoiceValue; }
            set { _macroChoiceValue = value; RaisePropertyChanged(); }

BrickController2/BrickController2/UI/ViewModels/ControllerActionPageViewModel.cs:462

• Selecting a macro changes the predicate of SelectMacroChoiceCommand, but that command never raises CanExecuteChanged. Because it is initially disabled when SelectedMacro is null, the choice button remains disabled after selecting a macro with choices. Notify the command after updating the selected macro.

                    Action.MacroId = macro.Id;
                    SetSelectedChoice(macro.Choices.Count > 0 ? macro.Choices[0] : null);
                    RaisePropertyChanged(nameof(SelectedMacro));
                    RaisePropertyChanged(nameof(SelectedMacroDisplayName));
                    RaisePropertyChanged(nameof(SelectedMacroChoiceDisplayName));

BrickController2/BrickController2/UI/ViewModels/ControllerActionPageViewModel.cs:448

• SelectMacro is not defined in any translation resource, so TranslationHelper falls back to displaying the raw key as the dialog title. Add the resource entry (and regenerate the designer/localizations) before using it here.

                Translate("SelectMacro"),

BrickController2/BrickController2/UI/ViewModels/ControllerActionPageViewModel.cs:552

• This silently discards every valid MacroChoice<T> value except int and string. The macro API is generic, and the existing PFx descriptors already use float choices, so a channel macro with a float, bool, enum, or other supported value would be saved as null and invoked with the wrong parameter. Preserve the descriptor's boxed value; serialization should handle its type separately.

            Action.MacroChoiceValue = (choice?.BoxedValue) switch
            {
                int intValue => intValue,
                string stringValue => stringValue,
                _ => null,
            };

• Files reviewed: 21/22 changed files
• Comments generated: 4
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Unsupported SQLite persistence and command-queue lifecycle issues can prevent initialization or leave macro operations incomplete.

Get a fresh assessment by requesting another Copilot review.

Review details

Files not reviewed (1)

• BrickController2/BrickController2/Resources/TranslationResources.Designer.cs: Generated file

Suppressed comments (6)

Previously missed (1) — in code that hasn't changed since the last review.

BrickController2/BrickController2/UI/ViewModels/ControllerActionPageViewModel.cs:540

• Refreshing the cache can make SelectedMacro appear/disappear or change its choices, but the choice command is not told to reevaluate its predicate. The button can therefore remain enabled or disabled based on the pre-refresh descriptor.

BrickController2/BrickController2/CreationManagement/ControllerAction.cs:132

• sqlite-net maps public read/write properties when CreateTableAsync<ControllerAction>() runs, but System.Object is not a supported SQLite column type. This therefore breaks creation-database initialization for every user, even before a macro action is saved. Persist the choice using supported columns (for example, serialized value plus a type discriminator) and expose an ignored typed facade if needed.

        public object? MacroChoiceValue
        {
            get { return _macroChoiceValue; }
            set { _macroChoiceValue = value; RaisePropertyChanged(); }

BrickController2/BrickController2/UI/ViewModels/ControllerActionPageViewModel.cs:170

• Changing devices also changes all three macro-command predicates, but property notifications do not trigger ICommand.CanExecuteChanged. After switching from a non-macro device, the macro picker can remain disabled; the choice command can likewise retain the previous device's state. Explicitly requery these commands (null-conditionally because this setter runs before command construction).

                RaisePropertyChanged(nameof(AvailableMacros));
                RaisePropertyChanged(nameof(HasMacros));
                RaisePropertyChanged(nameof(SelectedMacro));
                RaisePropertyChanged(nameof(SelectedMacroDisplayName));
                RaisePropertyChanged(nameof(SelectedMacroChoiceDisplayName));

BrickController2/BrickController2/UI/ViewModels/ControllerActionPageViewModel.cs:462

• Selecting a macro changes the choice command's predicate, but only property notifications are raised. If the command was disabled while no macro was selected, MAUI is not notified to reevaluate it, leaving the parameter picker disabled even when the selected macro has choices.

                    RaisePropertyChanged(nameof(SelectedMacro));
                    RaisePropertyChanged(nameof(SelectedMacroDisplayName));
                    RaisePropertyChanged(nameof(SelectedMacroChoiceDisplayName));

BrickController2/BrickController2/DeviceManagement/FxBricks/PfxBrickDevice.cs:250

• If output processing is canceled while Resolve is awaiting a file-directory response, it throws and the outer catch exits without completing this dequeued command. BeforeDisconnectAsync can only drain commands still in the queue, so the caller awaiting ExecuteMacroAsync can remain pending forever. Complete/cancel the dequeued item's TCS in a per-item catch/finally before letting cancellation stop the loop.

                if (_macroCommandQueue.TryDequeue(out var queued) &&
                    !queued.Completion.Task.IsCompleted)
                {
                    var resolvedCommand = await queued.Resolve(token).ConfigureAwait(false);

BrickController2/BrickController2/DeviceManagement/FxBricks/PfxProtocol.cs:170

• The new named-file request and response parser are not covered, although the surrounding protocol helpers and invalid-response cases are comprehensively tested in PfxProtocolTests.cs:77-185. Add tests for exact filename encoding/framing and for valid, not-found, short, and wrong-header responses so protocol regressions are caught without hardware.

• Files reviewed: 21/22 changed files
• Comments generated: 2
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Unsupported SQLite persistence and macro refresh and queue failure paths can prevent loading or reliable execution.

Get a fresh assessment by requesting another Copilot review.

Review details

Files not reviewed (1)

• BrickController2/BrickController2/Resources/TranslationResources.Designer.cs: Generated file

Suppressed comments (3)

Previously missed (2) — in code that hasn't changed since the last review.

BrickController2/BrickController2/DeviceManagement/FxBricks/PfxProtocol.cs:152

• The protocol test suite covers the existing command encoders and response parsers, but neither new named-file operation is exercised. Add framing tests for GetNamedFileId and valid/short/wrong-header/not-found boundary cases for ParseNamedFileId; otherwise a byte-layout regression will only be detectable against hardware.
  BrickController2/BrickController2/UI/ViewModels/DevicePageViewModel.cs:384
• Dynamic macro discovery uses DisappearingToken instead of the progress callback's cancellation token. Pressing Cancel after connection succeeds therefore does not cancel the potentially long directory scan, so the progress dialog can remain blocked. Pass the callback token to GetMacrosAsync.

BrickController2/BrickController2/CreationManagement/ControllerAction.cs:132

• ControllerAction is mapped directly by sqlite-net, but System.Object is not a supported SQLite column type. CreateTableAsync<ControllerAction>() will therefore fail while creating or migrating the table, preventing creations from loading. Persist the choice through supported typed/discriminated columns (or an ignored runtime property backed by a serialized column) so its concrete value type is preserved.

        public object? MacroChoiceValue
        {
            get { return _macroChoiceValue; }
            set { _macroChoiceValue = value; RaisePropertyChanged(); }

• Files reviewed: 23/24 changed files
• Comments generated: 2
• Review effort level: Balanced

[Copilot]

🟢 Approval recommended

The implementation is coherent across UI, persistence, validation, execution, localization, and automated tests.

Review details

Files not reviewed (1)

• BrickController2/BrickController2/Resources/TranslationResources.Designer.cs: Generated file

• Files reviewed: 28/29 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Constructor failure, numeric persistence, and command-queue cancellation paths can break macro configuration or device processing.

Get a fresh assessment by requesting another Copilot review.

Review details

Files not reviewed (1)

• BrickController2/BrickController2/Resources/TranslationResources.Designer.cs: Generated file

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

BrickController2/BrickController2/DeviceManagement/Macros/MacroChoiceValue.cs:12

• Storing the payload as object does not preserve numeric CLR types through the Newtonsoft JSON used by creation export and text blobs: integers deserialize as Int64 and floating values as Double. Consequently an int/float choice no longer equals the original wrapper after reload, and the equality lookup in SelectedMacroChoiceDisplayName fails, leaving saved numeric choices blank. Add typed serialization or normalize supported numeric values consistently on construction/deserialization.

• Files reviewed: 28/29 changed files
• Comments generated: 3
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Macro playback has queue races, ineffective cancellation, dynamic-discovery validation gaps, and unintended channel output writes.

Get a fresh assessment by requesting another Copilot review.

Review details

Files not reviewed (1)

• BrickController2/BrickController2/Resources/TranslationResources.Designer.cs: Generated file

Suppressed comments (4)

Previously missed (2) — in code that hasn't changed since the last review.

BrickController2/BrickController2/BusinessLogic/PlayLogic.cs:253

• A macro button still returns 0 through ProcessButtonEvent, after which ProcessGameControllerEvent unconditionally calls device.SetOutput(channel, outputValue). Every macro press therefore also writes zero to the channel and can stop/override unrelated channel output; macro actions should invoke the macro without entering the numeric-output path.
  BrickController2/BrickController2/DeviceManagement/Macros/MacroChoiceValue.cs:2
• This attribute comes from System.Text.Json, but creation export and text-blob persistence use Json.NET, so it is ignored and HasValue is serialized as an extra property. Import Json.NET's JsonIgnoreAttribute instead so the derived property is actually excluded.

BrickController2/BrickController2/DeviceManagement/FxBricks/PfxBrickDevice.cs:435

• The command processor receives only the output-loop token, while the caller's token merely cancels the completion task. If cancellation occurs after dequeue (for example during the file-ID lookup), the BLE command still executes even though the caller observed cancellation. Carry the caller token in the queued item and cancel/skip resolution and writing when either token is canceled.

        _macroCommandQueue.Enqueue(new QueuedMacroCommand(resolve, tcs));

        if (token.CanBeCanceled)
        {
            var registration = token.Register(() => tcs.TrySetCanceled(token));
            tcs.Task.ContinueWith(_ => registration.Dispose(), TaskScheduler.Default);

BrickController2/BrickController2/BusinessLogic/PlayLogic.cs:61

• Dynamic macro descriptors are unavailable after an app restart: BluetoothDeviceWithMacros initially caches only static macros and cannot discover while disconnected, but creation validation runs before PlayerPageViewModel connects devices. Consequently, any saved dynamic channel-macro action is always rejected as MissingMacro and never reaches playback. Dynamic descriptors need to be discovered after connection before definitive validation/execution.

            else if (macroReferences.Any(mr =>
            {
                var device = _deviceManager.GetDeviceById(mr.DeviceId);
                return device == null         // device not found
                    || !device.SupportsMacros // no macro support
                    || !device.AvailableMacros.Any(m => m.Id == mr.MacroId && m.Scope == mr.Scope);

• Files reviewed: 28/29 changed files
• Comments generated: 1
• Review effort level: Balanced