Skip to content

Conversation

@sensei-hacker
Copy link
Member

@sensei-hacker sensei-hacker commented Jan 7, 2026

User description

Summary

Redesigns the motor mixer wizard from a dropdown-based interface to a guided step-by-step flow, making motor identification more intuitive and less error-prone.

motor-wizard-demo

Changes

  • Add MSP2_INAV_MOTOR_LOCATE (0x2042) command support
  • New wizard UI with three screens:
    • Intro screen with safety checklist
    • Progress screen with visual step indicators and position buttons
    • Completion screen with apply button
  • Position buttons show visual feedback (blue=waiting, green=assigned)
  • Auto-advances through motors with 2-second locate cycles
  • Modal size increased to 500x560 to avoid scrolling

Testing

  • Verified wizard UI flow with SITL connection
  • Tested intro → progress → completion screen transitions
  • Verified position button states (waiting/assigned)
  • Verified progress bar indicators (active/complete)

Note: MSP2_INAV_MOTOR_LOCATE motor spin requires firmware support and DShot protocol - not testable in SITL.

Related

Requires corresponding firmware PR for MSP2_INAV_MOTOR_LOCATE implementation.


PR Type

Enhancement, New Feature


Description

  • Redesign motor wizard from dropdown UI to guided step-by-step flow

  • Add MSP2_INAV_MOTOR_LOCATE (0x2042) command support for motor identification

  • Implement three-screen wizard: intro with safety checklist, progress with visual indicators, completion screen

  • Add interactive position buttons with visual feedback (blue=waiting, green=assigned)

  • Auto-advance through motors with 2-second locate cycles and modal size increased to 500x560


Diagram Walkthrough

flowchart LR
  A["Intro Screen<br/>Safety Checklist"] -->|Start Wizard| B["Progress Screen<br/>Motor Locating"]
  B -->|Click Position| C["Next Motor<br/>or Complete"]
  C -->|All Motors Done| D["Completion Screen<br/>Apply Mapping"]
  D -->|Apply| E["Motor Rules<br/>Saved"]
Loading

File Walkthrough

Relevant files
Configuration changes
MSPCodes.js
Add motor locate MSP command code                                               

js/msp/MSPCodes.js

  • Add MSP2_INAV_MOTOR_LOCATE (0x2042) command code definition
+2/-0     
Enhancement
MSPHelper.js
Add motor locate MSP message handling                                       

js/msp/MSPHelper.js

  • Add case handler for MSP2_INAV_MOTOR_LOCATE response (single byte:
    1=success, 0=failure)
  • Add sendMotorLocate() function to send motor locate command with motor
    index
+9/-1     
mixer.js
Implement guided motor identification wizard state machine

tabs/mixer.js

  • Replace dropdown-based wizard with state machine-driven guided flow
  • Implement wizard state tracking (currentMotor, motorPositions,
    locateInterval, isActive)
  • Add resetWizard() to initialize wizard state and UI
  • Add startLocatingMotor() to send locate commands every 2 seconds and
    update progress display
  • Add onPositionClicked() to record motor positions and advance to next
    motor
  • Add wizardComplete() to show completion screen
  • Add event handlers for start button, position buttons, and apply
    button
  • Add modal onOpen/onClose callbacks for cleanup and preview image sync
  • Increase modal dimensions from 480x410 to 500x560
+146/-31
mixer.html
Redesign wizard HTML to three-screen guided flow                 

tabs/mixer.html

  • Replace dropdown table UI with three-section wizard layout (intro,
    progress, complete)
  • Add intro section with safety checklist and start button
  • Add progress section with motor number display and progress bar
    indicators
  • Add completion section with apply button
  • Add motor preview area with four clickable position buttons
    (rear-right, front-right, rear-left, front-left)
  • Remove old wizard-execute-button and modal__buttons--upbottom
    container
+58/-56 
mixer.css
Add comprehensive motor wizard styling and animations       

src/css/tabs/mixer.css

  • Add comprehensive wizard styling for sections, checklist, and prompt
    text
  • Add progress bar styles with active (red pulsing) and complete (green)
    states
  • Add motor preview container and position button styles with waiting
    (blue glowing) and assigned (green) states
  • Add animations: pulse for active progress step, glow for waiting
    position buttons
  • Add absolute positioning for four position buttons at quad corners
    (rear-right, front-right, rear-left, front-left)
  • Add wizard-buttons container styling
+144/-0 
Documentation
messages.json
Add internationalization messages for wizard UI                   

locale/en/messages.json

  • Add new i18n messages for wizard intro, safety steps, motor locating
    prompt, and completion
  • Add messages for start button and position button hints
  • Total of 7 new message keys for wizard UI text
+30/-0   

Redesign motor wizard from dropdown-based UI to guided step-by-step flow:

- Add MSP2_INAV_MOTOR_LOCATE (0x2042) support for motor spin identification
- New wizard UI with intro screen, progress indicators, and completion screen
- Position buttons show assigned motors with visual feedback
- Auto-advances through motors with 2-second locate cycles
- Buttons placed inside sections for proper show/hide behavior

Requires firmware support for MSP2_INAV_MOTOR_LOCATE command.
@qodo-code-review
Copy link
Contributor

qodo-code-review bot commented Jan 7, 2026

PR Compliance Guide 🔍

All compliance sections have been disabled in the configurations.

Comment on lines 670 to 700
function onPositionClicked(positionIndex) {
if (!wizardState.isActive) return;

// Validate mixer settings
if (!validateMixerWizard()) {
return;
// Stop the locate command
if (wizardState.locateInterval) {
clearInterval(wizardState.locateInterval);
wizardState.locateInterval = null;
}

FC.MOTOR_RULES.flush();
// Record this motor's position
wizardState.motorPositions[wizardState.currentMotor] = positionIndex;

for (let i = 0; i < 4; i++) {
const $selects = $(".wizard-motor-select");
let rule = -1;
// Update the button to show the motor number
const $btn = $(`#wizardPos${positionIndex}`);
$btn.removeClass('waiting').addClass('assigned');
$btn.find('.position-label').text(wizardState.currentMotor + 1);

$selects.each(function () {
if (parseInt($(this).find(":selected").attr("id"), 10) == i) {
rule = parseInt($(this).attr("data-motor"), 10);
}
});
// Remove waiting state from all buttons
$('.wizard-position-btn').removeClass('waiting');

// Move to next motor or complete
wizardState.currentMotor++;

const r = currentMixerPreset.motorMixer[rule];
if (wizardState.currentMotor >= 4) {
// All motors identified
wizardComplete();
} else {
// Start locating next motor
startLocatingMotor(wizardState.currentMotor);
}
}
Copy link
Contributor

@qodo-code-review qodo-code-review bot Jan 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: In onPositionClicked, send an explicit command to stop the motor from twitching immediately after a position is selected to improve user feedback. [general, importance: 7]

Suggested change
function onPositionClicked(positionIndex) {
if (!wizardState.isActive) return;
// Validate mixer settings
if (!validateMixerWizard()) {
return;
// Stop the locate command
if (wizardState.locateInterval) {
clearInterval(wizardState.locateInterval);
wizardState.locateInterval = null;
}
FC.MOTOR_RULES.flush();
// Record this motor's position
wizardState.motorPositions[wizardState.currentMotor] = positionIndex;
for (let i = 0; i < 4; i++) {
const $selects = $(".wizard-motor-select");
let rule = -1;
// Update the button to show the motor number
const $btn = $(`#wizardPos${positionIndex}`);
$btn.removeClass('waiting').addClass('assigned');
$btn.find('.position-label').text(wizardState.currentMotor + 1);
$selects.each(function () {
if (parseInt($(this).find(":selected").attr("id"), 10) == i) {
rule = parseInt($(this).attr("data-motor"), 10);
}
});
// Remove waiting state from all buttons
$('.wizard-position-btn').removeClass('waiting');
// Move to next motor or complete
wizardState.currentMotor++;
const r = currentMixerPreset.motorMixer[rule];
if (wizardState.currentMotor >= 4) {
// All motors identified
wizardComplete();
} else {
// Start locating next motor
startLocatingMotor(wizardState.currentMotor);
}
}
function onPositionClicked(positionIndex) {
if (!wizardState.isActive) return;
// Stop the locate command
if (wizardState.locateInterval) {
clearInterval(wizardState.locateInterval);
wizardState.locateInterval = null;
}
// Send command to stop motor twitching immediately
mspHelper.sendMotorLocate(255, function() {});
// Record this motor's position
wizardState.motorPositions[wizardState.currentMotor] = positionIndex;
// Update the button to show the motor number
const $btn = $(`#wizardPos${positionIndex}`);
$btn.removeClass('waiting').addClass('assigned');
$btn.find('.position-label').text(wizardState.currentMotor + 1);
// Remove waiting state from all buttons
$('.wizard-position-btn').removeClass('waiting');
// Move to next motor or complete
wizardState.currentMotor++;
if (wizardState.currentMotor >= 4) {
// All motors identified
wizardComplete();
} else {
// Start locating next motor
startLocatingMotor(wizardState.currentMotor);
}
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is already implemented. The onPositionClicked function calls stopMotors() (line 677), which clears the interval AND sends mspHelper.sendMotorLocate(255) to stop motor twitching immediately.

Comment on lines 769 to 776
// Clean up when modal closes
motorWizardModal.options.onClose = function() {
if (wizardState.locateInterval) {
clearInterval(wizardState.locateInterval);
wizardState.locateInterval = null;
}
wizardState.isActive = false;
};
Copy link
Contributor

@qodo-code-review qodo-code-review bot Jan 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: In the modal's onClose handler, send an explicit command to stop any active motor twitching to ensure a clean exit from the wizard. [general, importance: 7]

Suggested change
// Clean up when modal closes
motorWizardModal.options.onClose = function() {
if (wizardState.locateInterval) {
clearInterval(wizardState.locateInterval);
wizardState.locateInterval = null;
}
wizardState.isActive = false;
};
// Clean up when modal closes
motorWizardModal.options.onClose = function() {
if (wizardState.locateInterval) {
clearInterval(wizardState.locateInterval);
wizardState.locateInterval = null;
// Send command to stop any motor twitching
mspHelper.sendMotorLocate(255, function() {});
}
wizardState.isActive = false;
};

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is already implemented. The motorWizardModal.options.onClose handler (line 776) calls stopMotors(), which clears the interval and sends the stop command.

Comment on lines +1624 to +1626
case MSPCodes.MSP2_INAV_MOTOR_LOCATE:
// Response is single byte: 1=success, 0=failure
break;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: In the MSP2_INAV_MOTOR_LOCATE response handler, parse the success/failure byte from the response and invoke the provided callback with the result. [possible issue, importance: 8]

Suggested change
case MSPCodes.MSP2_INAV_MOTOR_LOCATE:
// Response is single byte: 1=success, 0=failure
break;
case MSPCodes.MSP2_INAV_MOTOR_LOCATE:
// Response is single byte: 1=success, 0=failure
const success = new Uint8Array(data.buffer)[0] === 1;
// Call the callback passed to sendMotorLocate
if (typeof callback === 'function') {
callback(success);
}
break;

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not strictly needed for current functionality. The wizard uses a "fire and forget" approach - if a motor doesn't twitch, the user simply won't click on it and can restart. All current callers pass empty callbacks function() {}. Parsing the response could be useful for future error handling but isn't required for the wizard to work correctly.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question:

Not strictly needed for current functionality. The wizard uses a fire and forget approach - if a motor doesn't twitch, the user simply won't click on it and can restart. All current callers pass empty callbacks function() {}. Parsing the response could be useful for future error handling but isn't required for the wizard to work correctly.

Answer:

To reduce suggestions like “parse response byte and call callback” when the current product behavior is intentionally fire-and-forget (and callbacks are no-ops), add a repo-level constraint to the /improve prompt so the model avoids “future-proofing” changes unless they are required for correctness.

Add to .pr_agent.toml (or wiki config):

[pr_code_suggestions]
extra_instructions="""\
- Prefer changes required for current, user-visible correctness/bugs.
- Avoid “future error handling”, “nice-to-have”, or speculative refactors unless they prevent a real bug.
- If a callback/result is currently unused or callers pass no-op callbacks, do not suggest wiring/parsing return values unless it fixes an actual failure mode or is explicitly requested in the PR description/ticket.
- Treat fire-and-forget workflows as acceptable; don't propose adding response parsing/propagation unless there is evidence of missing error handling causing issues.
"""

Optional noise-reduction if you still see many “not strictly needed” items:

[pr_code_suggestions]
suggestions_score_threshold = 7  # use carefully; higher values may hide useful findings

Relevant Sources:

Comment on lines 637 to 668
function startLocatingMotor(motorIndex) {
// Update progress display
$('#wizard-current-motor').text(motorIndex + 1);

// Update progress bar
$('.wizard-progress-step').each(function() {
const step = parseInt($(this).attr('data-step'), 10);
$(this).removeClass('active complete');
if (step < motorIndex) {
$(this).addClass('complete');
} else if (step === motorIndex) {
$(this).addClass('active');
}
});

// Enable clicking on unassigned positions
$('.wizard-position-btn').each(function() {
const pos = parseInt($(this).attr('data-position'), 10);
const isAssigned = Object.values(wizardState.motorPositions).includes(pos);
if (!isAssigned) {
$(this).addClass('waiting');
}
});

// Send locate command and repeat every 2 seconds
const sendLocate = function() {
mspHelper.sendMotorLocate(motorIndex, function() {});
};

sendLocate(); // Send immediately
wizardState.locateInterval = setInterval(sendLocate, 2000);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Guard motorIndex/positionIndex to be finite and within 0..3, and avoid sending MSP commands or indexing currentMixerPreset.motorMixer[...] when inputs/state are missing to prevent runtime errors and invalid motor commands. [Learned best practice, importance: 5]

Suggested change
function startLocatingMotor(motorIndex) {
// Update progress display
$('#wizard-current-motor').text(motorIndex + 1);
// Update progress bar
$('.wizard-progress-step').each(function() {
const step = parseInt($(this).attr('data-step'), 10);
$(this).removeClass('active complete');
if (step < motorIndex) {
$(this).addClass('complete');
} else if (step === motorIndex) {
$(this).addClass('active');
}
});
// Enable clicking on unassigned positions
$('.wizard-position-btn').each(function() {
const pos = parseInt($(this).attr('data-position'), 10);
const isAssigned = Object.values(wizardState.motorPositions).includes(pos);
if (!isAssigned) {
$(this).addClass('waiting');
}
});
// Send locate command and repeat every 2 seconds
const sendLocate = function() {
mspHelper.sendMotorLocate(motorIndex, function() {});
};
sendLocate(); // Send immediately
wizardState.locateInterval = setInterval(sendLocate, 2000);
}
function startLocatingMotor(motorIndex) {
if (!Number.isFinite(motorIndex) || motorIndex < 0 || motorIndex > 3) return;
// Update progress display
$('#wizard-current-motor').text(motorIndex + 1);
...
const sendLocate = function() {
mspHelper.sendMotorLocate(motorIndex, function() {});
};
sendLocate(); // Send immediately
wizardState.locateInterval = setInterval(sendLocate, 2000);
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Low priority - the code controls all motorIndex values internally (0→1→2→3 sequence). Invalid values can't occur in normal wizard operation since the index is managed by the state machine, not user input.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question:

Low priority - the code controls all motorIndex values internally (0→1→2→3 sequence). Invalid values can't occur in normal wizard operation since the index is managed by the state machine, not user input.

Answer:

To reduce low-signal “defensive guard” suggestions for values that are strictly internal/state-machine controlled, add a repo-level instruction for /improve to only request range/null checks when an input is user-controlled, external, or crosses a trust boundary (API, UI input, network, persisted state), and otherwise treat such guards as low priority.

Recommended .pr_agent.toml:

[pr_code_suggestions]
extra_instructions = """\
- Prefer suggestions that fix concrete bugs/regressions visible in the diff.
- Do NOT suggest defensive range/NaN/null guards for variables that are clearly controlled internally (e.g., state-machine indices, enum-like counters) unless there is evidence in the diff that they can be corrupted or originate from user input/external IO.
- Only recommend input validation (bounds checks, Number.isFinite, null checks) when the value is user-provided, external, asynchronous, or otherwise untrusted.
- If suggesting a guard anyway, explicitly justify the realistic failure mode and mark it as low priority.
"""

If you still see too much noise, tighten filtering:

[pr_code_suggestions]
suggestions_depth = "selective"          # fewer, higher-signal suggestions
suggestions_score_threshold = 7          # filter out lower-importance items

Relevant Sources:

Comment on lines 661 to 730
// Send locate command and repeat every 2 seconds
const sendLocate = function() {
mspHelper.sendMotorLocate(motorIndex, function() {});
};

sendLocate(); // Send immediately
wizardState.locateInterval = setInterval(sendLocate, 2000);
}

$("#wizard-execute-button").on('click', function () {
function onPositionClicked(positionIndex) {
if (!wizardState.isActive) return;

// Validate mixer settings
if (!validateMixerWizard()) {
return;
// Stop the locate command
if (wizardState.locateInterval) {
clearInterval(wizardState.locateInterval);
wizardState.locateInterval = null;
}

FC.MOTOR_RULES.flush();
// Record this motor's position
wizardState.motorPositions[wizardState.currentMotor] = positionIndex;

for (let i = 0; i < 4; i++) {
const $selects = $(".wizard-motor-select");
let rule = -1;
// Update the button to show the motor number
const $btn = $(`#wizardPos${positionIndex}`);
$btn.removeClass('waiting').addClass('assigned');
$btn.find('.position-label').text(wizardState.currentMotor + 1);

$selects.each(function () {
if (parseInt($(this).find(":selected").attr("id"), 10) == i) {
rule = parseInt($(this).attr("data-motor"), 10);
}
});
// Remove waiting state from all buttons
$('.wizard-position-btn').removeClass('waiting');

// Move to next motor or complete
wizardState.currentMotor++;

const r = currentMixerPreset.motorMixer[rule];
if (wizardState.currentMotor >= 4) {
// All motors identified
wizardComplete();
} else {
// Start locating next motor
startLocatingMotor(wizardState.currentMotor);
}
}

function wizardComplete() {
wizardState.isActive = false;

// Mark all progress steps complete
$('.wizard-progress-step').removeClass('active').addClass('complete');

$('#wizard-progress').addClass('is-hidden');
$('#wizard-complete').removeClass('is-hidden');
}

// Position button click handler
$('.wizard-position-btn').on('click', function() {
if (!$(this).hasClass('waiting')) return;
const positionIndex = parseInt($(this).attr('data-position'), 10);
onPositionClicked(positionIndex);
});

// Start button click handler
$('#wizard-start-button').on('click', function() {
wizardState.isActive = true;
wizardState.currentMotor = 0;
wizardState.motorPositions = {};

$('#wizard-intro').addClass('is-hidden');
$('#wizard-progress').removeClass('is-hidden');

// Start locating first motor
startLocatingMotor(0);
});
Copy link
Contributor

@qodo-code-review qodo-code-review bot Jan 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Clear any existing locateInterval before creating a new one, and make the start handler idempotent (ignore clicks while active) so repeated clicks don’t create overlapping polling loops. [Learned best practice, importance: 6]

Suggested change
// Send locate command and repeat every 2 seconds
const sendLocate = function() {
mspHelper.sendMotorLocate(motorIndex, function() {});
};
sendLocate(); // Send immediately
wizardState.locateInterval = setInterval(sendLocate, 2000);
}
$("#wizard-execute-button").on('click', function () {
function onPositionClicked(positionIndex) {
if (!wizardState.isActive) return;
// Validate mixer settings
if (!validateMixerWizard()) {
return;
// Stop the locate command
if (wizardState.locateInterval) {
clearInterval(wizardState.locateInterval);
wizardState.locateInterval = null;
}
FC.MOTOR_RULES.flush();
// Record this motor's position
wizardState.motorPositions[wizardState.currentMotor] = positionIndex;
for (let i = 0; i < 4; i++) {
const $selects = $(".wizard-motor-select");
let rule = -1;
// Update the button to show the motor number
const $btn = $(`#wizardPos${positionIndex}`);
$btn.removeClass('waiting').addClass('assigned');
$btn.find('.position-label').text(wizardState.currentMotor + 1);
$selects.each(function () {
if (parseInt($(this).find(":selected").attr("id"), 10) == i) {
rule = parseInt($(this).attr("data-motor"), 10);
}
});
// Remove waiting state from all buttons
$('.wizard-position-btn').removeClass('waiting');
// Move to next motor or complete
wizardState.currentMotor++;
const r = currentMixerPreset.motorMixer[rule];
if (wizardState.currentMotor >= 4) {
// All motors identified
wizardComplete();
} else {
// Start locating next motor
startLocatingMotor(wizardState.currentMotor);
}
}
function wizardComplete() {
wizardState.isActive = false;
// Mark all progress steps complete
$('.wizard-progress-step').removeClass('active').addClass('complete');
$('#wizard-progress').addClass('is-hidden');
$('#wizard-complete').removeClass('is-hidden');
}
// Position button click handler
$('.wizard-position-btn').on('click', function() {
if (!$(this).hasClass('waiting')) return;
const positionIndex = parseInt($(this).attr('data-position'), 10);
onPositionClicked(positionIndex);
});
// Start button click handler
$('#wizard-start-button').on('click', function() {
wizardState.isActive = true;
wizardState.currentMotor = 0;
wizardState.motorPositions = {};
$('#wizard-intro').addClass('is-hidden');
$('#wizard-progress').removeClass('is-hidden');
// Start locating first motor
startLocatingMotor(0);
});
function startLocatingMotor(motorIndex) {
...
// Send locate command and repeat every 2 seconds
const sendLocate = function() {
mspHelper.sendMotorLocate(motorIndex, function() {});
};
if (wizardState.locateInterval) {
clearInterval(wizardState.locateInterval);
wizardState.locateInterval = null;
}
sendLocate(); // Send immediately
wizardState.locateInterval = setInterval(sendLocate, 2000);
}
$('#wizard-start-button').on('click', function() {
if (wizardState.isActive) return;
wizardState.isActive = true;
wizardState.currentMotor = 0;
wizardState.motorPositions = {};
...
// Start locating first motor
startLocatingMotor(0);
});

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. Added:

  1. if (wizardState.isActive) return; at start of click handler to make it idempotent
  2. Interval cleanup in startLocatingMotor() before creating new interval to prevent overlaps

- Add stopMotors() function that sends motor index 255 to stop locate
- Add Emergency Stop button (red) visible during wizard operation
- Call stopMotors() when:
  - User clicks a position (between motors)
  - User clicks Emergency Stop button
  - Modal closes for any reason
  - Wizard is reset
- Add warning button CSS style (modal__button--warning)
- Add i18n translation for stop button

Addresses Qodo review feedback about proper motor cleanup.
- Add idempotent check to start button (ignore if wizard already active)
- Clear existing interval in startLocatingMotor() before creating new one

Prevents multiple polling loops if user double-clicks start button.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant