Conversation
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 |
PR Summary by QodoAdd per-motor DShot direction configuration and bounded test pulses
AI Description
Diagram
High-Level Assessment
Files changed (10)
|
Code Review by Qodo
1. Turtle mode runs motors forward
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Comment on lines
+578
to
+580
| if (run == 0) { | ||
| directionConfig.testActive = false; | ||
| return true; |
There was a problem hiding this comment.
1. Stopped tests can pulse once more 🐞 Bug ≡ Correctness
pwmDshotDirectionTest clears testActive without clearing the test value cached in motors[].value, and arming cancellation has the same stale-cache behavior. If pwmCompleteMotorUpdate runs before the mixer writes fresh values, it packages and transmits the selected motor's previous DShot120 value after an explicit stop or arming cancellation.
Agent Prompt
## Issue description
Stopping or cancelling a direction test clears only the state flag, leaving DShot120 cached for transmission in the selected motor output.
## Fix Focus Areas
- src/main/drivers/pwm_output.c[578-580]
- src/main/drivers/pwm_output.c[621-634]
## Recommended Fix
On every explicit or arming-triggered cancellation, clear the cached test outputs or retain direction-output ownership long enough to emit a zero frame before returning to the normal command path. Ensure the first frame after cancellation cannot reuse the previous DShot120 value.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+588
to
+590
| // Exercise the same command sequencer through real MSP in the built-in demo. | ||
| // SITL has no physical ESCs; it explicitly advertises simulated output. | ||
| static dshotDirection_t sitlDirectionConfig; |
There was a problem hiding this comment.
2. Sitl cannot compile this feature 🐞 Bug ≡ Correctness
target.c declares dshotDirection_t and invokes its helpers without including drivers/pwm_output.h, while its new calls to getMotorCount and areMotorsRunning also lack flight/mixer.h. Compiling the SITL translation unit therefore encounters an unknown direction type and undeclared APIs as soon as the new implementation is enabled.
Agent Prompt
## Issue description
The new SITL implementation uses DShot direction and mixer APIs without including the headers that declare them.
## Fix Focus Areas
- src/main/target/SITL/target.c[45-53]
- src/main/target/SITL/target.c[588-636]
## Recommended Fix
Include `drivers/pwm_output.h` for the direction state and helper declarations and `flight/mixer.h` for `getMotorCount` and `areMotorsRunning`, then verify the SITL target compiles without implicit declarations.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
Author
|
/agentic_review |
|
RAM / Flash usage vs. base commit
See RAM/flash optimization guide for techniques to reduce usage. |
Comment on lines
+590
to
+592
| if (dshotDirectionBusy(&directionConfig)) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
1. Turtle mode runs motors forward 🐞 Bug ≡ Correctness
sendDShotCommand() drops every command while directionConfig is busy, rather than preserving the turtle-mode reverse command. A direction operation may start while disarmed, and turtle arming then sets the armed and turtle flags immediately, so the next scheduler pass cancels the operation while the ESCs never receive their required reverse command.
Agent Prompt
## Issue description
DShot commands are discarded while the direction sequencer is active. Turtle-mode arming depends on queuing the ESC reverse command before enabling turtle mode, so dropping that command lets the craft enter turtle mode with its ESC direction unchanged.
## Fix Focus Areas
- src/main/drivers/pwm_output.c[589-594]
- src/main/fc/fc_core.c[606-616]
## Recommended Fix
Do not silently discard DShot commands while a direction operation is active. Queue the command so that, after the armed-state cancellation releases sequencer ownership, the normal DShot command scheduler sends its required repetitions; alternatively explicitly cancel and enqueue before accepting turtle-mode arming. Preserve the existing exclusion that prevents a newly started direction operation from interrupting already queued commands.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+635
to
+638
| dshotDirectionTestFrame(&sitlDirectionConfig, micros()); | ||
| const int16_t command = dshotDirectionFrame(&sitlDirectionConfig, micros()); | ||
| if (command > 0) { | ||
| fprintf(stderr, "[ESC DEMO] motor=%u command=%d (simulated, no hardware)\n", sitlDirectionConfig.motor + 1, command); |
There was a problem hiding this comment.
4. Simulator tests never pulse a motor 🐞 Bug ≡ Correctness
sitlDshotDirectionUpdate() advances the test and direction state machines but discards their resulting output values, only printing positive configuration commands. SITL's simulators read the mixer's motor[] values directly, so a direction operation leaves ordinary simulated outputs in place and a DShot120 test pulse produces no simulated motor output or selected-motor isolation.
Agent Prompt
## Issue description
The SITL hook advances the shared DShot direction sequencer but does not publish the resulting isolated motor output to the simulator. As a result, SITL does not model either the selected test pulse or the required zeroing of all other motors.
## Fix Focus Areas
- src/main/target/SITL/target.c[630-640]
- src/main/flight/mixer.c[520-601]
## Recommended Fix
In the SITL update hook, translate the sequencer's selected DShot test output to the simulator motor-value range and set every unselected `motor[]` entry to its stopped value while the sequence or pulse owns outputs. Preserve ordinary mixer values once the operation completes or is cancelled, and retain the existing simulated-command logging.
ⓘ 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 2af2025 |
|
Test firmware build ready — commit Download firmware for PR #12011 250 targets built. Find your board's
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem and behavior
Add per-motor DShot direction configuration and a bounded test pulse for an integrated Configurator wizard. Commands 7/8 and SAVE_SETTINGS (12) target only the selected motor; other outputs stay at zero during the operation. Settings live in the ESC. Completion means commands sent, not confirmed persistence.
Existing arming rules are unchanged: no new arming flag or persistent lock. Requests are rejected while armed or ordinary motor testing is active. Arming cancels the operation before the next output frame and restores normal output ownership. Ordinary motor testing works normally after the operation. Cancellation retains tokens so delayed retries cannot restart it.
The fixed DShot120 test pulse expires after 1.5 seconds independently of USB/UI. Duplicate tokens cannot repeat a save or extend/restart a pulse. SITL uses the same sequencer and reports simulation explicitly.
Integration
Validation
Hardware test plan and limitations
Physical hardware has not been tested. With all propellers removed:
See docs/development/msp/esc-direction.md for protocol details.