Skip to content

Add an integrated multirotor motor direction wizard - #2794

Closed
Raffi1202 wants to merge 5 commits into
iNavFlight:maintenance-10.xfrom
Raffi1202:motor-direction-wizard
Closed

Raffi1202 wants to merge 5 commits into
iNavFlight:maintenance-10.xfrom
Raffi1202:motor-direction-wizard

Conversation

@Raffi1202

Copy link
Copy Markdown

Problem and behavior

Add an integrated Motor Direction Wizard under Outputs, allowing selection, a short test pulse and Normal/Reverse ESC configuration without leaving INAV Configurator.

The dialog offers Wizard and Individually modes using the existing INAV mixer diagram and motor numbering. The wizard explicitly initializes each selected ESC to Normal, then offers hold-to-test, reverse and next-motor confirmation. Individual selection does not change configuration until Normal or Reverse is selected.

The entire launcher appears only for multirotor/tricopter platforms via FC.isMultirotor(), not motor count. Two-motor airplanes, rovers and boats keep it hidden and do not start direction polling.

Props-off acknowledgement gates all actions. Release, pointer cancellation/leave, keyboard release, blur and dialog/tab close request stop. Firmware independently expires pulses after 1.5 seconds. Existing arming rules remain unchanged; arming cancels the active operation. Ordinary motor tests remain available after the operation. Unsupported firmware disables the launcher. SITL is clearly identified as simulation; hardware status does not claim confirmed ESC storage.

Integration

  • Requires companion firmware PR: Add per-motor DShot direction configuration and bounded test pulses inav#12011.
  • Target: maintenance-10.x. Provisional MSP2 codes 0x2235-0x2237 need maintainer agreement.
  • English and German UI strings included.
  • Inspired by Betaflight's Motor Direction Wizard; no wizard code copied.
  • Global firmware-version acceptance is unchanged. The current firmware branch reports 10.0.0 while Configurator accepts 9.x. The local native test package temporarily accepts 10.x for validation; this packaging-only override is excluded from the PR. Upstream version alignment remains separate.

Validation

  • 293 Node tests and 28 transpiler test suites passed.
  • Native macOS arm64 Electron package built and tested against actual INAV SITL over MSP.
  • Props gate, initialization, reverse, next motor, individual selection, release stop, expiry while held, close/reopen and unchanged ordinary motor-test availability passed with no renderer errors.
  • Native platform tests: hidden for two-motor Flying Wing, Rover and Boat; visible for Tricopter and Quad X.
  • Whitespace checks passed. The repository lint script has no linter configured. The separate web/PWA build exceeds its precache file-size limit on the unchanged baseline too; native packaging succeeds.

Test instructions and limitations

Build this branch and the companion firmware with aligned version acceptance as described above. In the native app, use the companion SITL binary in Demo mode, configure Quad X and open Outputs → Motor direction wizard. Confirm the props warning, run the wizard, hold the test button, reverse and advance. Reopen Individual mode and select another motor. Switch to a two-motor airplane: the entire launcher must disappear.

Physical hardware testing remains outstanding. Remove all propellers before using matching board firmware with supported DShot ESCs. Check selected-output isolation, pulse stops, observed rotation and persistence after ESC/FC power cycle. See docs/development/esc-direction.md for protocol details and the hardware test plan.

@qodo-code-review

Copy link
Copy Markdown
Contributor

ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing

@qodo-free-for-open-source-projects

Copy link
Copy Markdown

PR Summary by Qodo

Add integrated multirotor motor direction wizard

✨ Enhancement 🧪 Tests 📝 Documentation 🕐 40+ Minutes

Grey Divider

AI Description

• Adds guided and individual ESC direction configuration for multirotors in Outputs.
• Enforces props acknowledgement, arming checks, fresh status, and bounded hold-to-test pulses.
• Integrates companion MSP2 commands with localized UI, tests, and hardware-validation guidance.
Diagram

sequenceDiagram
    actor User
    participant Outputs as Outputs Tab
    participant Wizard as Direction Wizard
    participant MSP as MSP Layer
    participant Firmware as INAV Firmware
    participant ESC as DShot ESC
    User->>Outputs: Open tab
    Outputs->>Wizard: Mount on multirotor
    loop Status polling
        Wizard->>MSP: Read direction status
        MSP->>Firmware: MSP2 status
        Firmware-->>MSP: Phase and test state
        MSP-->>Wizard: Parsed status
    end
    User->>Wizard: Acknowledge props removed
    alt Set direction
        Wizard->>MSP: Normal or reverse
        MSP->>Firmware: Direction request
        Firmware->>ESC: Direction and save
    else Hold motor test
        User->>Wizard: Press and hold
        Wizard->>MSP: Start pulse
        MSP->>Firmware: Bounded test request
        Firmware->>ESC: Low-speed output
        User->>Wizard: Release or close
        Wizard->>MSP: Stop pulse
        MSP->>Firmware: Unconditional stop
        Firmware->>ESC: Zero output
    end
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Extend existing motor-test controls
  • ➕ Reuses a familiar Outputs interface.
  • ➕ Could reduce dedicated dialog markup and styling.
  • ➖ Entangles persistent ESC configuration with ordinary transient motor testing.
  • ➖ Provides weaker guided sequencing and safety acknowledgement boundaries.
  • ➖ Makes release, close, and tab-cleanup behavior harder to isolate.
2. Use external ESC configuration tools
  • ➕ Avoids adding provisional MSP2 commands.
  • ➕ Mature ESC tools may expose broader configuration and readback capabilities.
  • ➖ Breaks the integrated Configurator workflow.
  • ➖ Loses INAV mixer mapping and platform-aware motor numbering.
  • ➖ Depends on ESC-specific passthrough tooling and hardware compatibility.

Recommendation: Keep the dedicated wizard and companion MSP2 protocol: this cleanly isolates safety-sensitive lifecycle handling while preserving ordinary motor tests. Merge should remain contingent on final MSP code allocation and physical STM32/AT32 DShot validation, especially output isolation, cancellation, and persistence.

Files changed (14) +721 / -0

Enhancement (8) +370 / -0
escDirection.jsAdd ESC direction status parsing and request guards +27/-0

Add ESC direction status parsing and request guards

• Introduces strict parsing for versioned direction and bounded-test status payloads. Adds centralized safety eligibility checks and validated, tokenized direction payload generation.

js/escDirection.js

escDirectionPanel.jsImplement the motor direction wizard controller +222/-0

Implement the motor direction wizard controller

• Implements multirotor-only mounting, guided and individual workflows, mixer-based motor selection, status polling, and direction writes. Coordinates acknowledgement, arming, stale-state, pending-operation, and hold-to-test safety while ensuring release, blur, close, and disposal request an unconditional stop.

js/escDirectionPanel.js

fc.jsStore ESC direction operation status in FC state +3/-0

Store ESC direction operation status in FC state

• Adds the ESC_DIRECTION state slot and clears it whenever flight-controller state is reset.

js/fc.js

msp.jsPermit safety-critical ESC test stop messages +1/-0

Permit safety-critical ESC test stop messages

• Adds the ESC direction test command to the always-allowed live-write set. This ensures unconditional stop requests are not blocked by unrelated unreadable FC state while firmware still guards starts.

js/msp.js

MSPHelper.jsDecode ESC direction status responses +9/-0

Decode ESC direction status responses

• Parses supported ESC direction status responses into FC.ESC_DIRECTION and recognizes both write-response commands without additional payload decoding.

js/msp/MSPHelper.js

motors.cssStyle the responsive motor direction dialog +61/-0

Style the responsive motor direction dialog

• Adds launcher, modal, mixer map, action-state, accessibility-focus, responsive-layout, and reduced-motion styling for the new wizard.

src/css/tabs/motors.css

outputs.htmlAdd motor direction wizard markup to Outputs +42/-0

Add motor direction wizard markup to Outputs

• Adds the hidden launcher and accessible dialog containing the props acknowledgement, workflow selection, mixer map, hold-to-test control, direction actions, progress, and live status regions.

tabs/outputs.html

outputs.jsIntegrate wizard lifecycle with the Outputs tab +5/-0

Integrate wizard lifecycle with the Outputs tab

• Mounts the direction panel after Outputs initialization and disposes it during tab cleanup so polling and active test requests cannot survive navigation.

tabs/outputs.js

Tests (2) +52 / -0
esc-direction.test.mjsTest direction protocol validation and safety gates +47/-0

Test direction protocol validation and safety gates

• Covers malformed payload rejection, independent direction-write gates, token generation and wrapping, SITL identification, and bounded-test state validation.

tests/esc-direction.test.mjs

msp-parse-failure-recovery.test.mjsCover ESC commands in MSP failure recovery tests +5/-0

Cover ESC commands in MSP failure recovery tests

• Loads the real ESC direction parser into MSP helper tests and verifies the test command remains an always-allowed live write despite its SET-style name.

tests/msp-parse-failure-recovery.test.mjs

Documentation (1) +92 / -0
esc-direction.mdDocument ESC direction protocol and hardware test plan +92/-0

Document ESC direction protocol and hardware test plan

• Documents platform scope, provisional MSP2 payloads, DShot sequencing, safety behavior, limitations, and validation results. It explicitly distinguishes commands sent from confirmed ESC persistence and lists required physical-hardware checks.

docs/development/esc-direction.md

Other (3) +207 / -0
MSPCodes.jsRegister provisional ESC direction MSP2 commands +3/-0

Register provisional ESC direction MSP2 commands

• Defines status, direction-write, and bounded-test command identifiers at provisional codes 0x2235 through 0x2237.

js/msp/MSPCodes.js

messages.jsonAdd German motor direction wizard translations +102/-0

Add German motor direction wizard translations

• Adds German text for safety warnings, mode selection, direction controls, test status, simulation disclosure, unsupported firmware, and persistence limitations.

locale/de/messages.json

messages.jsonAdd English motor direction wizard messages +102/-0

Add English motor direction wizard messages

• Adds English UI text covering the complete wizard workflow, safety requirements, simulation state, errors, and the lack of ESC save readback.

locale/en/messages.json

@qodo-free-for-open-source-projects

qodo-free-for-open-source-projects Bot commented Sep 22, 2026 •

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

Grey Divider


Remediation recommended

1. Two motor writes lack shared encoding ✓ Resolved 📘 Rule violation ≡ Correctness
Description
MSP2_INAV_SET_ESC_DIRECTION and MSP2_INAV_SET_ESC_DIRECTION_TEST are sent using caller-built
byte arrays, while MSPHelper.js adds only response handlers and no registered outbound
serializers. Payload changes must therefore be coordinated across individual call sites, allowing
the two commands' wire formats to diverge during later protocol revisions.
Code

js/escDirectionPanel.js[189]

+        write(MSPCodes.MSP2_INAV_SET_ESC_DIRECTION_TEST, [selected, 1, lastTestToken], 'ESC_DIRECTION_TEST_ACK').then(async () => {
Evidence
Compliance rule 353263 requires outbound MSP commands to have matching registered serialization and
deserialization handling. The panel sends both new write commands with locally assembled arrays,
while the new MSP helper cases only deserialize their empty acknowledgements and repository-wide
references show no outbound registrations for either command.

Rule 353263: Ensure MSP commands have matching serialize/deserialize handlers
js/escDirectionPanel.js[141-141]
js/escDirectionPanel.js[163-189]
js/msp/MSPHelper.js[1965-1971]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The new motor-direction write commands construct their outbound payloads outside the shared MSP serialization layer, even though their inbound acknowledgements are handled in `MSPHelper.js`.
## Fix Focus Areas
- js/msp/MSPHelper.js[1962-1971]
- js/escDirectionPanel.js[133-189]
## Recommended Fix
Add registered outbound serializers for both motor-direction write commands in the MSP helper layer, including validation of motor, direction, run, and token fields. Route each panel write through those serializers while preserving the existing byte layouts and acknowledgement handling.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. The motor map keeps fixed sizing ✓ Resolved 📘 Rule violation ✧ Quality
Description
#esc-wizard-map fixes its container to 260px in both dimensions rather than using fluid sizing
with a maximum constraint. A later layout or viewport change must add another breakpoint and
duplicate both dimensions to keep the diagram within its available space.
Code

src/css/tabs/motors.css[513]

+#esc-wizard-map { width: 260px; height: 260px; position: relative; margin: auto; }
Evidence
Compliance rule 353257 prohibits fixed absolute widths on container elements. The new
#esc-wizard-map wrapper sets width: 260px and height: 260px, making its container dimensions
fixed rather than fluid.

Rule 353257: Avoid fixed width on container elements
src/css/tabs/motors.css[513-513]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The motor-map container uses fixed pixel width and height, requiring breakpoint-specific overrides instead of adapting to its available space.
## Fix Focus Areas
- src/css/tabs/motors.css[513-513]
- src/css/tabs/motors.css[539-539]
## Recommended Fix
Replace the fixed width and height with a fluid width, an appropriate `max-width`, and `aspect-ratio: 1`; remove the redundant fixed map dimensions from the media query while preserving the square diagram.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


3. Outputs adds constant serial traffic ✓ Resolved 🐞 Bug ➹ Performance
Description
mountEscDirection registers poll at a 300 ms interval and never suspends it when the dialog is
closed or when the status command is reported as unsupported. Every multirotor visit therefore keeps
submitting capability requests for the tab's lifetime, including accepted older firmware that can
never support the wizard, adding avoidable queue and serial traffic alongside existing Outputs
telemetry.
Code

js/escDirectionPanel.js[212]

+    interval.add('esc_direction_poll', poll, 300, true);
Evidence
The panel starts a repeating 300 ms interval unconditionally for every multirotor and only removes
it during tab cleanup. Unsupported responses merely clear FC.ESC_DIRECTION, so they do not latch
unsupported capability or terminate polling; the development notes also establish that the globally
accepted older firmware does not contain this maintenance-10.x protocol.

js/escDirectionPanel.js[70-87]
js/escDirectionPanel.js[212-217]
js/msp/MSPHelper.js[1962-1964]
docs/development/esc-direction.md[89-92]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The ESC direction status command is polled every 300 ms for the entire lifetime of the multirotor Outputs tab, even when the dialog is closed or the firmware has already rejected the command as unsupported.
## Fix Focus Areas
- js/escDirectionPanel.js[70-109]
- js/escDirectionPanel.js[176-184]
- js/escDirectionPanel.js[212-217]
## Recommended Fix
Probe capability once when mounting, stop polling after an unsupported response, and run frequent polling only while the dialog is open or an operation is pending or active. Trigger an immediate status refresh when opening the dialog and retain cleanup removal of any active interval.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


View medium (7)
4. Two new commands lack state fields ✓ Resolved 📘 Rule violation ≡ Correctness
Description
MSP2_INAV_SET_ESC_DIRECTION and MSP2_INAV_SET_ESC_DIRECTION_TEST are added to MSPCodes, but
FC declares only the shared ESC_DIRECTION status property. Their acknowledgement cases discard
responses, leaving neither write command with a distinct initialized destination in
flight-controller state.
Code

js/msp/MSPCodes.js[R270-271]

+    MSP2_INAV_SET_ESC_DIRECTION:       0x2236,
+    MSP2_INAV_SET_ESC_DIRECTION_TEST:  0x2237,
Evidence
Rule 353262 requires a distinct initialized state property for every newly introduced MSP command.
The PR declares three commands, but FC adds only ESC_DIRECTION, while the two write-command
handlers immediately break without updating corresponding state.

Rule 353262: Add state property in js/fc.js for each new MSP command
js/msp/MSPCodes.js[269-271]
js/fc.js[113-116]
js/msp/MSPHelper.js[1962-1967]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Two new write commands lack distinct initialized properties in flight-controller state, while only their shared status command is represented.
## Fix Focus Areas
- js/fc.js[113-116]
- js/msp/MSPHelper.js[1962-1967]
- js/msp/MSPCodes.js[269-271]
## Recommended Fix
Add appropriately named, initialized FC properties for both write commands, reset them in `resetState()`, and update the corresponding response handlers with normalized acknowledgement state.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


5. The wizard dialog uses fixed sizing ✓ Resolved 📘 Rule violation ✧ Quality
Description
#esc-direction-dialog sets width: 720px even though it is a container with a viewport-based
max-width. At ordinary viewport widths the dialog cannot size intrinsically, and future content or
host-layout changes must override the hard-coded width.
Code

src/css/tabs/motors.css[490]

+#esc-direction-dialog { --esc-ink: #303842; width: 720px; height: fit-content; min-height: 0; max-width: calc(100vw - 48px); max-height: calc(100vh - 48px); position: fixed; inset: 0; margin: auto; padding: 0; border: 1px solid #d3d7dc; border-radius: 8px; color: var(--esc-ink); background: #fff; box-shadow: 0 20px 70px #0005; overflow: auto; font: 13px/1.5 "open_sans", sans-serif; }
Evidence
Rule 353257 prohibits fixed absolute widths on container elements and recommends combining
responsive width with max-width. The new dialog is a content container and explicitly fixes its
width at 720 pixels.

Rule 353257: Avoid fixed width on container elements
src/css/tabs/motors.css[490-490]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The new dialog container uses a fixed 720-pixel width instead of responsive intrinsic sizing.
## Fix Focus Areas
- src/css/tabs/motors.css[490-490]
## Recommended Fix
Replace the fixed width with `width: 100%` or an intrinsic width and retain an appropriate `max-width` constraint for the intended desktop size.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


6. Wizard text uses fixed pixel sizes ✓ Resolved 📘 Rule violation ☑ Accessibility
Description
The new wizard CSS declares text sizes in pixels through both font shorthand and repeated
font-size properties. This affects headings, status text, labels, hints, and text buttons, so
user-relative type settings cannot consistently control the feature's typography.
Code

src/css/tabs/motors.css[494]

+#esc-direction-dialog h2 { margin: 0; padding: 0; font-size: 19px; font-weight: 600; line-height: 1.4; color: var(--esc-ink); }
Evidence
Rule 353258 requires changed CSS font sizes to use relative units. The wizard styles introduce
numerous pixel sizes, including 19px for the dialog heading and pixel sizes for paragraphs, labels,
status text, and buttons.

Rule 353258: Use relative units instead of pixels for CSS font sizes
src/css/tabs/motors.css[485-499]
src/css/tabs/motors.css[503-524]
src/css/tabs/motors.css[534-538]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The wizard introduces numerous pixel-based font sizes, preventing its typography from following relative sizing conventions.
## Fix Focus Areas
- src/css/tabs/motors.css[485-538]
## Recommended Fix
Convert every textual pixel font size, including sizes in `font` shorthand declarations, to equivalent `rem` or `em` values while preserving the existing visual hierarchy.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


7. Two text buttons use forced widths ✓ Resolved 📘 Rule violation ✧ Quality
Description
#esc-direction-test and #esc-wizard-next each set width: 100% instead of relying on natural
button sizing or their surrounding layout. Both controls carry translated text, so later
localization and layout changes must override these declarations before the controls can size to
their content.
Code

src/css/tabs/motors.css[526]

+#esc-direction-panel #esc-direction-test { width: 100%; min-height: 42px; background: #303b45; color: #fff; border-color: #303b45; touch-action: none; user-select: none; }
Evidence
Rule 353259 prohibits specified widths on text buttons and allows fixed sizing only for small
icon-only controls. The motor-test and next-step controls are translated text buttons, but both are
assigned width: 100%.

Rule 353259: Avoid fixed width on button elements in CSS
src/css/tabs/motors.css[526-526]
src/css/tabs/motors.css[531-531]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Two translated text buttons are forced to a specified width rather than using natural content sizing.
## Fix Focus Areas
- src/css/tabs/motors.css[526-531]
## Recommended Fix
Remove the fixed `width` declarations or use `width: auto`, then use the existing flex or grid layout to control placement without constraining translated button labels.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


8. Screen readers announce English close ✓ Resolved 📘 Rule violation ✧ Quality
Description
#esc-direction-close hardcodes aria-label="Close" while the surrounding dialog obtains its
labels through i18next. Opening the wizard under the German locale therefore leaves this control's
accessible name in English even though a localized close message is available.
Code

tabs/outputs.html[221]

+                <header><h2 id="esc-wizard-title" data-i18n="escDirectionTitle"></h2><button type="button" id="esc-direction-close" aria-label="Close">×</button></header>
Evidence
Rule 353261 requires translatable HTML labels and button text to use data-i18n. The new icon-only
close button instead hardcodes its accessible label as English, while both English and German locale
files already provide escWizardClose.

Rule 353261: Use data-i18n attributes for all i18next-translated HTML text
tabs/outputs.html[221-221]
locale/en/messages.json[7844-7846]
locale/de/messages.json[6896-6898]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The dialog's icon-only close button has a hardcoded English accessible label instead of an i18next translation binding.
## Fix Focus Areas
- tabs/outputs.html[221-221]
- locale/en/messages.json[7844-7846]
- locale/de/messages.json[6896-6898]
## Recommended Fix
Replace the hardcoded `aria-label` with the project's i18next attribute-translation syntax, binding the accessible label to the existing localized close key while retaining the multiplication-sign icon.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


9. Dropped motor commands look successful ✓ Resolved 🐞 Bug ☼ Reliability
Description
The apply, start, and sendStop continuations treat every fulfilled MSP.promise() as an
acknowledgement even though _enqueue calls onFinish(false) after exhausting its retries. Under
queue contention or CLI activation, this misreports delayed direction operations, leaves controls
held after failed test starts, and lets release, blur, close, or cleanup clear held without
ensuring another stop is sent, so an accepted test can remain active until the firmware’s 1.5-second
timeout.
Code

js/escDirectionPanel.js[R132-134]

+        return MSP.promise(MSPCodes.MSP2_INAV_SET_ESC_DIRECTION_TEST, [255, 0, 0]).catch(() => {
+            if (!disposed) message('escDirectionUncertain');
+        }).finally(() => { stopping = false; if (!disposed) { refresh(); poll(); } });
Evidence
The MSP queue invokes its completion callback with false after exhausting enqueue retries, and
MSP.promise() forwards that value as a fulfillment rather than a rejection. The three new command
paths do not inspect the fulfilled value; in particular, sendStop() catches only rejections and
still runs its cleanup for false, despite the protocol allowing an accepted test to remain active
for up to 1.5 seconds.

js/msp.js[500-539]
js/escDirectionPanel.js[110-160]
js/msp.js[480-505]
js/msp.js[527-541]
js/escDirectionPanel.js[130-140]
docs/development/esc-direction.md[28-30]
docs/development/esc-direction.md[60-62]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
ESC direction command handlers treat the MSP queue’s `false` completion value as a successful acknowledgement. This leaves UI state and messages inconsistent when direction, start, or stop commands were never queued, and a failed stop can clear local held state without ensuring another unconditional stop is sent.
## Fix Focus Areas
- js/escDirectionPanel.js[118-160]
- js/msp.js[480-541]
- tests/esc-direction.test.mjs[1-47]
## Recommended Fix
Inspect the fulfilled value from every ESC direction write made through `MSP.promise()`. Treat `false` like a rejected command by clearing `pending`, `starting`, and `held` as appropriate, reporting uncertainty, and preserving the compensating stop behavior for failed or delayed test starts. For a failed stop, retry or otherwise schedule another unconditional `[255, 0, 0]` stop request until one completes successfully or firmware status confirms that the test is inactive. Add tests covering `false` results for direction, start, and stop commands, including the existing start/stop race behavior.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


10. Pilots can see the wrong motor diagram ✓ Resolved 🐞 Bug ≡ Correctness
Description
drawMotors copies the current preview src while update_model assigns the selected mixer's
image through an unresolved dynamic import. If the user enters a mode before that import completes,
the wizard retains custom.svg for the active session and omits the expected directional diagram
while presenting spatial motor controls.
Code

js/escDirectionPanel.js[R50-51]

+        const image = $('#motor-mixer-preview-img').attr('src');
+        $('#esc-wizard-image').attr('src', image || '');
Evidence
The Outputs markup initializes the source image to custom.svg, while update_model() replaces it
only in a dynamic-import continuation. The panel immediately polls and copies whatever source
exists, and subsequent polling redraws only when the reported motor count changes, so completion of
the image import does not update the open wizard.

tabs/outputs.html[177-183]
tabs/outputs.js[541-549]
js/escDirectionPanel.js[49-57]
js/escDirectionPanel.js[81-103]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The wizard can copy the initial custom-mixer placeholder before the asynchronous selected-mixer image finishes loading, leaving its active diagram stale.
## Fix Focus Areas
- js/escDirectionPanel.js[49-69]
- tabs/outputs.js[541-549]
- tabs/outputs.html[177-183]
## Recommended Fix
Update the wizard image whenever the main mixer preview's selected image finishes loading, or await the mixer image import before enabling wizard entry. Remove any added preview-image event handler during panel disposal and add a test where image loading completes after the initial status poll and mode entry.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Tip of the day
💡 Did you know, you can keep summaries lean with Finding overflow, which tucks the rest behind 'View more'

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

Comment thread js/msp/MSPCodes.js
Comment thread src/css/tabs/motors.css Outdated
Comment thread src/css/tabs/motors.css Outdated
Comment thread src/css/tabs/motors.css Outdated
Comment thread tabs/outputs.html Outdated
Comment thread js/escDirectionPanel.js Outdated
Comment thread js/escDirectionPanel.js Outdated
@github-actions

github-actions Bot commented Sep 22, 2026 •

Copy link
Copy Markdown

Configurator test build ready — commit 30757eb

Download build artifacts for PR #2794

Available platforms (scroll to the Artifacts section at the bottom of the run page):

  • Windows x64 (ZIP, MSI) and x32 (ZIP, MSI)
  • macOS arm64 (ZIP, DMG) and x64 (ZIP, DMG)
  • Linux x64 (DEB, RPM, ZIP) and aarch64 (DEB, RPM, ZIP)

A GitHub login is required to download artifacts. Build is for testing only.

@sensei-hacker

Copy link
Copy Markdown
Member

I notice your bot seems to be copy-pasting several existing features, copying my own PRs from a month ago, a year ago, or two years ago.

@Raffi1202

Copy link
Copy Markdown
Author

/agentic_review

Comment thread src/css/tabs/motors.css Outdated
Comment thread js/escDirectionPanel.js Outdated
@qodo-free-for-open-source-projects

Copy link
Copy Markdown

Code review by qodo was updated up to the latest commit 79162a4

@Raffi1202

Copy link
Copy Markdown
Author

/agentic_review

Comment thread js/escDirectionPanel.js Outdated
@qodo-free-for-open-source-projects

Copy link
Copy Markdown

Code review by qodo was updated up to the latest commit d31bbd7

Raphael Hunziker and others added 2 commits September 23, 2026 11:09
Start and stop of MSP2_INAV_SET_ESC_DIRECTION_TEST built their payloads
by hand at the call sites. escDirectionTestPayload now validates motor
and token against the same limits as the firmware, and the stop payload
is a constant that cannot throw inside the stop retries. Both live in
js/escDirection.js beside escDirectionPayload. The bytes sent are
unchanged.
- Move settling of the pending direction write out of the status poll
  callback into settlePendingWrite(); the logic is unchanged.
- Use an optional chain for the acknowledgement check.
- Darken four text colours to at least 4.5:1 against their background.
- Use <output> for the two status lines and give the dialog heading,
  motor heading and safety label fallback text; i18n replaces it on load.
@sonarqubecloud

Copy link
Copy Markdown

@Raffi1202

Copy link
Copy Markdown
Author

The dialog duplicates the mixer motor wizard from #2580. Following b14ckyy's review on iNavFlight/inav#12011 the direction check moves into that wizard as one step, with a firmware setting instead of ESC config commands. Closing this PR; replacement follows once the firmware side is done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants