Conversation
…s 20/21 Replaces the ESC-config approach of iNavFlight#12011. A uint16 bitmask in motorConfig tells each DShot ESC to spin normal (20) or reversed (21). The command queue now carries one command per motor so one frame addresses all ESCs at once. The ESC does not store these commands, so they are re-sent on every arm ahead of the first throttle frame, when the setting changes, and every two seconds while disarmed. Turtle mode inverts relative to the configured mask and disarm restores the mask instead of broadcasting "normal". All-zero mask sends nothing, so existing setups behave as before. PG_MOTOR_CONFIG bumped to 12 for the new field.
|
ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing |
PR Summary by QodoAdd per-motor DShot spin direction control
AI Description
Diagram
High-Level Assessment
Files changed (9)
|
Code Review by Qodo
1. Motors keep old direction after clearing
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
| if (motorConfig()->dshotReversedMotors && !STATE(IN_FLIGHT_EMERG_REARM)) { | ||
| dshotSpinDirectionApply(false); |
There was a problem hiding this comment.
1. Motors keep old direction after clearing 🐞 Bug ≡ Correctness
tryArm() queues the configured directions only when dshotReversedMotors is nonzero, even though clearing the mask may require command 20 to undo a previously sent reversed state. If the pilot arms before the 100 ms update poll processes that change, the armed-state guard prevents correction and affected motors continue using their former direction.
Agent Prompt
## Issue description
Clearing `dshotReversedMotors` and immediately arming can skip the all-normal direction frame because `tryArm()` checks only whether the new mask is nonzero. Track whether a direction was previously sent or centralize the arm-time decision so a transition from nonzero to zero is applied before throttle output.
## Fix Focus Areas
- src/main/fc/fc_core.c[613-619]
- src/main/flight/mixer.c[1023-1044]
- src/main/flight/mixer.h[149-153]
## Recommended Fix
Add an arm-time helper that sends the configured mask when it is nonzero or differs from the last sent mask, and invoke it for every non-emergency DShot arm. This preserves silence for untouched all-zero configurations while ensuring a newly cleared mask queues command 20 before the first throttle frame.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| ); | ||
|
|
||
| PG_REGISTER_WITH_RESET_TEMPLATE(motorConfig_t, motorConfig, PG_MOTOR_CONFIG, 11); | ||
| PG_REGISTER_WITH_RESET_TEMPLATE(motorConfig_t, motorConfig, PG_MOTOR_CONFIG, 12); |
There was a problem hiding this comment.
2. Upgrades erase existing motor settings 🐞 Bug ≡ Correctness
Registering PG_MOTOR_CONFIG as version 12 makes pgLoad() reject every stored version-11 record after first resetting the complete group. Upgrading existing installations therefore defaults the PWM protocol, rate, minimum command, idle offset, and pole count along with initializing the new direction field.
Agent Prompt
## Issue description
The motor parameter-group version bump causes existing version-11 records to be discarded because the loader restores data only on an exact version match. Since the new field is appended, preserve old fields while allowing the new tail field to retain its reset default.
## Fix Focus Areas
- src/main/flight/mixer.c[94-104]
- src/main/config/parameter_group.c[86-94]
## Recommended Fix
Keep the motor group compatible with version 11 when appending `dshotReversedMotors`, relying on the existing reset-before-partial-copy behavior to initialize the new tail field to zero. If a version bump is mandatory, add an explicit version-11 migration that copies every prior field and defaults only the new field.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| pendingCmd[i] = (reversedMotorMask & (1u << i)) ? DSHOT_CMD_SPIN_DIRECTION_REVERSED : DSHOT_CMD_SPIN_DIRECTION_NORMAL; | ||
| } | ||
| pendingCmdReps = 10; |
There was a problem hiding this comment.
3. Changed motor directions can be delayed 🐞 Bug ≡ Correctness
sendDShotSpinDirection() overwrites pendingCmd[] and resets pendingCmdReps even when a prior direction frame is still being transmitted. A DShot beacon command can arrive during those ten direction repeats, replacing the remaining packets on RP2350 boards and leaving the new direction unapplied until a later refresh or arm.
Agent Prompt
## Issue description
The RP2350 DShot implementation has only one mutable pending-command slot. A beacon command can overwrite a newly queued per-motor spin-direction command before its required ten transmissions finish, so the ESC may not receive enough direction packets to apply the setting.
## Fix Focus Areas
- src/main/drivers/pwm_output_rp2350.c[195-197]
- src/main/drivers/pwm_output_rp2350.c[347-352]
## Recommended Fix
Replace the single pending command state with a small FIFO of per-motor command frames, including the per-frame repeat count. Enqueue both `sendDShotCommand()` and `sendDShotSpinDirection()` requests, and only load the next frame after the active frame has completed all repeats.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| sendDShotSpinDirection(mask); | ||
| dshotSpinDirectionSent = mask; | ||
| dshotSpinDirectionSentAtUs = micros(); |
There was a problem hiding this comment.
3. Full queues retain stale motor direction 🐞 Bug ☼ Reliability
dshotSpinDirectionApply records the requested mask and timestamp even when the void sendDShotSpinDirection call is discarded by a full circular buffer. After eight frames are queued, this suppresses the changed-mask retry for two seconds, so affected electronic speed controllers continue using their previous direction.
Agent Prompt
## Issue description
The generic DShot queue silently rejects pushes when full, but the direction lifecycle records rejected frames as sent. This can suppress retries and leave motors using stale directions.
## Fix Focus Areas
- src/main/common/circular_queue.c[34-42]
- src/main/drivers/pwm_output.c[493-500]
- src/main/flight/mixer.c[1032-1044]
- src/main/fc/fc_core.c[584-619]
## Recommended Fix
Make direction-frame enqueueing report success and update the sent mask and timestamp only after acceptance. Ensure normal and turtle arming do not complete until their required direction frame has been accepted, while disarmed failures remain eligible for retry.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit 69a8cfb |
|
RAM / Flash usage vs. base commit
See RAM/flash optimization guide for techniques to reduce usage. |
|
Test firmware build ready — commit Download firmware for PR #12022 249 targets built. Find your board's
|
Bluejay and BLHeli_S keep a runtime command 21 through a DShot signal loss. After clearing the mask and rebooting the FC with the battery connected, nothing sent command 20 again and the motor stayed reversed. Every arm except an in-flight rearm now sends the configured directions, as Betaflight does. Reversible-motor setups with an all-zero mask stay silent as before.
|
Pushed 6db9c5b: every arm now sends the directions, also with an all-zero mask, as Betaflight does. Bluejay and BLHeli_S keep a runtime 21 through an FC reboot, so clearing the mask and rebooting left the motor reversed. @b14ckyy one design point I'd like your call on. With Options:
I'd take 1 plus a docs line. |
|
Thanks, this is the shape we discussed and it reads well. I did a full pass with clean A/B builds on F405, F722SE, F765 and H743 (+336 to +1156 B flash, +120 to +160 B RAM, ITCM -200 to -280 B thanks to the NOINLINE), unit tests pass, and I checked the ESC side against the AM32, Bluejay and BLHeli_S sources: all three accept 20/21 only once the ESC itself is armed at zero throttle, and repeating the frame every 2 s has no side effects on any of them. INAV never sends command 12, so nothing gets written to the ESC. Design call: option 1 plus the docs line. While armed, with all motors at stop, not in turtle and a non-zero mask, re-send every 250 ms. That leaves at most a 250 ms window after the ESC arms and up to ~20 ms throttle hold-off if the pilot opens throttle during a frame, which is fine. Use the turtle-aware apply there, otherwise an armed turtle would get un-inverted. Please don't use PG version: please drop the bump and keep version 11. The new uint16 sits at offset 10, past the old 10-byte record, so Must fix:
Should fix: Nits: Then a bench test with props off on at least two ESC firmwares (set a bit, arm, battery after USB, turtle in and out), and I'll take it for 11.x. |
2 participants
Replaces #12011 after b14ckyy's review there: no ESC configuration is written any more. One setting, no new MSP.
What it does
dshot_reversed_motors(uint16 bitmask, bit 0 = motor 1) inmotorConfig. A set bit gives that ESCSPIN_DIRECTION_REVERSED(21), a clear bitSPIN_DIRECTION_NORMAL(20). Read and written through the standard settings MSP.uint8_t cmd[MAX_MOTORS]+ repeats), so a single frame addresses every ESC.sendDShotCommand()keeps its all-motor semantics for the beacon;sendDShotSpinDirection(mask)builds the per-motor frame. Same on RP2350.tryArm()sendsmask ^ all motors,disarm()restores the mask instead of broadcasting 20.PG_MOTOR_CONFIG11 -> 12 resetsmin_command,motor_pwm_rate,motor_pwm_protocolandmotor_poleson upgrade;diff allrestores them by name.Forward merge from 10.x
Branched from
maintenance-11.xas it is today. Mergingmaintenance-10.xforward conflicts in mixer.h/mixer.c with the SRXL2 fields (struct tail, reset template): keep both blocks and version 12. The bump makes the order safe; without it, a field at offset 10 would read a 10.x config's SRXL2 defaults (7, 1) as mask 0x0107.Not done
No hardware test: SITL has no DShot. CI builds all targets. Needs a bench test with props off on BLHeli_32 / AM32 / Bluejay: set a bit, arm, watch the motor; battery after USB; turtle in and out.
Configurator part follows as a step in the Mixer tab motor wizard (#2580) instead of the separate dialog from #2794.