Firmware support for improved mixer wizard (MSP2_INAV_MOTOR_LOCATE command for motor identification)

[sensei-hacker]

User description

In Configurator 9.1 I want to be able to have a very simple, intuitive, motor mixer wizard, where the user just clicks on which motor is beeping and jerking:

This PR is firmware side of that, to have one motor signal.

Adds a new MSP command (MSP2_INAV_MOTOR_LOCATE, 0x2042) that beeps and jerks a specific motor to help users identify motor positions during configuration.

Changes

• New motor_locate module with state machine for motor identification pattern
• MSP2_INAV_MOTOR_LOCATE (0x2042) command handler in fc_msp.c
• Integration with mixer to override motor outputs during locate cycle

How It Works

When the command is received with a motor index:

1. Motor does a brief 100ms jerk at ~12% throttle
2. Followed by 3 DShot beacon beeps
3. Pattern repeats for 2 seconds total
4. Only works when disarmed and with DShot protocol

Safety

• Only activates when aircraft is disarmed
• Automatically stops after 2 seconds
• Low throttle (12%)

Testing

• Built SITL successfully
• MSP command handler tested via configurator connection
• Full motor spin testing requires hardware with DShot ESCs

Related

Companion PR: iNavFlight/inav-configurator#2516

[qodo-code-review]

PR Compliance Guide 🔍

All compliance sections have been disabled in the configurations.

[qodo-code-review]

Suggestion: Mark the locateState struct as volatile to ensure its members are correctly accessed across different execution contexts and prevent potential compiler optimization issues. [possible issue, importance: 8]

Suggested change

	static struct {
	motorLocateState_e state;
	uint8_t motorIndex;
	uint8_t beepCount;
	timeUs_t stateStartTime;
	timeUs_t cycleStartTime;
	} locateState = {
	static volatile struct {
	motorLocateState_e state;
	uint8_t motorIndex;
	uint8_t beepCount;
	timeUs_t stateStartTime;
	timeUs_t cycleStartTime;
	} locateState = {

[sensei-hacker]

Not needed. INAV uses cooperative scheduling (not preemptive). Both writeMotors() (FAST_CODE) and MSP handlers run from main loop context via the scheduler - there's no interrupt contention. FAST_CODE is a linker section attribute that places code in fast RAM, not an indication of interrupt context. All accesses to locateState are single-threaded.

[qodo-code-review]

Question:

Not needed. INAV uses cooperative scheduling (not preemptive). Both writeMotors() (FAST_CODE) and MSP handlers run from main loop context via the scheduler - there's no interrupt contention. FAST_CODE is a linker section attribute that places code in fast RAM, not an indication of interrupt context. All accesses to locateState are single-threaded.

Answer:

Add a project-specific instruction to stop the model from suggesting volatile/atomic/thread-safety changes unless there is explicit evidence of true concurrency (ISR/preemption/multithreading), and to not infer “interrupt context” from attributes like FAST_CODE.

[pr_code_suggestions]
extra_instructions = """\
Concurrency rules for this repo (INAV):
- Assume cooperative scheduling / single-threaded main-loop execution unless the diff explicitly shows interrupt handlers, preemptive RTOS threads, or shared state accessed from ISRs.
- Do NOT suggest adding 'volatile', atomics, locks, or memory barriers for shared state unless there is clear interrupt/preemption/multicore contention shown in the code.
- Do NOT interpret attributes like FAST_CODE / section placement as interrupt context; treat them as placement/linker attributes only.
- If unsure about concurrency context, ask for clarification instead of making a volatile/atomic recommendation.
"""

If you still see these false positives, you can also reduce overall “best-practices” style suggestions by keeping (or ensuring) the default:

[pr_code_suggestions]
focus_only_on_problems = true

Relevant Sources:

• https://qodo-merge-docs.qodo.ai//tools/improve#extra-instructions-and-best-practices
• https://qodo-merge-docs.qodo.ai//tools/improve#configuration-options
• https://qodo-merge-docs.qodo.ai//faq/index#-note-"q:-i-received-an-incorrect-or-irrelevant-suggestion-why"

[qodo-code-review]

Suggestion: Add a check in motorLocateUpdate() to verify that the motor protocol is still DShot, and call motorLocateStop() if it has changed. [general, importance: 6]

Suggested change

	bool motorLocateUpdate(void)
	{
	if (locateState.state == LOCATE_STATE_IDLE) {
	return false;
	}
	bool motorLocateUpdate(void)
	{
	if (locateState.state == LOCATE_STATE_IDLE) {
	return false;
	}
	// Stop if protocol is no longer DShot
	if (!isMotorProtocolDshot()) {
	motorLocateStop();
	return false;
	}

[sensei-hacker]

Not needed. Motor protocol is configured at boot via motorConfig()->motorPwmProtocol and never changes at runtime. motorLocateStart() already validates DShot protocol at startup (line 91-93). There's no scenario where protocol changes mid-operation.

[qodo-code-review]

Suggestion: Avoid using unchecked sbufReadU8 in MSP handlers; use a safe read helper (and require exact payload size) so a malformed frame can’t cause an out-of-bounds read despite dataSize being present. [Learned best practice, importance: 6]

Suggested change

	case MSP2_INAV_MOTOR_LOCATE:
	{
	// Motor locate requires 1 byte: motor index
	if (dataSize < 1) {
	return MSP_RESULT_ERROR;
	}
	uint8_t motorIndex = sbufReadU8(src);
	bool success = motorLocateStart(motorIndex);
	sbufWriteU8(dst, success ? 1 : 0);
	}
	break;
	case MSP2_INAV_MOTOR_LOCATE:
	{
	// Motor locate requires exactly 1 byte: motor index
	if (dataSize != 1) {
	return MSP_RESULT_ERROR;
	}
	uint8_t motorIndex;
	if (!sbufReadU8Safe(&motorIndex, src)) {
	return MSP_RESULT_ERROR;
	}
	const bool success = motorLocateStart(motorIndex);
	sbufWriteU8(dst, success ? 1 : 0);
	}
	break;

[sensei-hacker]

Already handled. Line 1735-1736 checks if (dataSize < 1) return MSP_RESULT_ERROR; before reading. This provides equivalent bounds protection - we validate the buffer has sufficient data before calling sbufReadU8(). The dataSize check is the standard pattern used throughout INAV's MSP handlers.

[qodo-code-review]

Question:

Already handled. Line 1735-1736 checks if (dataSize < 1) return MSP_RESULT_ERROR; before reading. This provides equivalent bounds protection - we validate the buffer has sufficient data before calling sbufReadU8(). The dataSize check is the standard pattern used throughout INAV's MSP handlers.

Answer:

You can reduce this kind of “already handled” suggestion by tightening the /improve guidance so the model first checks for existing guards/patterns around reads before proposing a safer helper.

Add project-specific instructions like:

• Don’t suggest replacing sbufReadU8() with sbufReadU8Safe() (or adding “exact payload size” checks) if there is already an upstream dataSize/length guard immediately before the read and this is the established handler pattern in the codebase.
• Only recommend “safe read helpers” when the code reads without any prior size validation, or when validation is inconsistent with the required payload size.

Example .pr_agent.toml:

[pr_code_suggestions]
extra_instructions = """\
For MSP/packet parsing handlers:
- Before suggesting "use *Safe()" read helpers or "require exact payload size", verify whether the code already validates data length (e.g., `if (dataSize < N) return ...;` or an equivalent established pattern) immediately before the read.
- If an existing length guard is present and matches the bytes read, do NOT suggest replacing `sbufReadU8()` with a safe helper; at most, briefly note optional tightening (e.g., `!= N`) only if the project prefers exact sizes.
- Prefer following the repository’s standard pattern used throughout the handlers; avoid proposing alternative helpers unless there is a concrete bug (missing/incorrect guard).
"""

If these suggestions still appear occasionally, it’s largely an AI context/precision limitation (the model may miss nearby guard lines in a large diff). In that case, also consider filtering mid-importance “best practice” items by increasing the score threshold slightly:

[pr_code_suggestions]
suggestions_score_threshold = 7

Relevant Sources:

• https://qodo-merge-docs.qodo.ai//tools/improve#extra-instructions-and-best-practices
• https://qodo-merge-docs.qodo.ai//tools/improve#configuration-options
• https://qodo-merge-docs.qodo.ai//faq/index#faq
• https://qodo-merge-docs.qodo.ai//core-abilities/self_reflection#local-and-global-metadata-injection-with-multi-stage-analysis

[sensei-hacker]

In testing, the beeping doesn't help much. I may not end up using this method.
Sending a 20ms pulse of 15% via existing MSP motor commands seem to work, achieving essentially the same thing without any firmware change.