Fix LED output mode falsely flagging sibling timer channels - #11749
Merged
sensei-hacker merged 3 commits intoJul 26, 2026
Merged
Conversation
timerHardwareOverride() applied TIM_USE_LED to every pad sharing a timer set to OUTPUT_MODE_LED, but ws2811LedStripInit() only ever wires up the first matching pad — all channels of one timer share a single period register, so only one can actually be the WS2811 output. This caused sibling pads (e.g. S6 on a timer with S5 as the LED strip) to be reported as "Led" over MSP and shown as such in the configurator Mixer tab while being functionally dead. Confirmed on hardware via bootlog: before the fix, a two-pad shared timer ended with both pads flagged TIM_USE_LED even though only one is ever driven. Track which pad is canonical (first-encountered, matching the scan order ws2811LedStripInit()'s timerGetByUsageFlag() uses) per physical timer; only that pad gets TIM_USE_LED, mirroring the existing BEEPER canonical-pad tracking. Siblings fall back to TIM_USE_PINIO (GPIO on/off), same as BEEPER siblings. A second, subtler bug had to be fixed for this to actually work: pwmAssignOutput()'s MAP_TO_LED_OUTPUT case called pwmClaimTimer(), which propagates the assigned pad's flags to every sibling sharing the same physical timer. That propagation is correct for motor/servo (a multi-channel timer should offer multiple outputs) but was silently re-overwriting the sibling's correct TIM_USE_PINIO back to TIM_USE_LED immediately after the canonical-pad logic had set it correctly. Removed the pwmClaimTimer() call from the LED case; motor/servo keep it.
pwm_mapping_beeper_unittest.cc still hand-reproduced an older, since-superseded timerHardwareOverride() (a simple early-return guard, no canonical-pad tracking, no PINIO fallback) rather than the version that actually landed in commits 8c16aee/31bb99ea0b. It kept passing because it's fully self-contained and only ever exercised its own stale inline copy, so it had stopped verifying real BEEPER behavior. Rewritten to mirror the current source, with a SourceSync suite that reads pwm_mapping.c at test time and asserts the inline reproduction's key logic tokens still match the live file, so this can't silently drift again.
Contributor
PR Summary by QodoFix LED timer overrides incorrectly flagging sibling pads as LED outputs
AI Description
Diagram
High-Level Assessment
Files changed (4)
|
Contributor
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+611
to
+637
| // Locates the live pwm_mapping.c relative to this test file's own path. | ||
| // __FILE__ is emitted as an absolute path by this project's CMake/Makefiles | ||
| // (verified: `c++ ... -c /abs/path/to/pwm_mapping_beeper_unittest.cc`), so | ||
| // this resolves correctly regardless of the build directory or CWD the test | ||
| // is invoked from. | ||
| std::string pwmMappingSourcePath() | ||
| { | ||
| std::string thisFile = __FILE__; // .../src/test/unit/pwm_mapping_beeper_unittest.cc | ||
| size_t lastSlash = thisFile.find_last_of("/\\"); | ||
| std::string testUnitDir = (lastSlash == std::string::npos) ? "." : thisFile.substr(0, lastSlash); | ||
| return testUnitDir + "/../../main/drivers/pwm_mapping.c"; | ||
| } | ||
|
|
||
| timerHardwareOverride_fixed(&timer, OUTPUT_MODE_LED); | ||
| EXPECT_EQ(timer.usageFlags, originalFlags); | ||
| // Reads and normalizes pwm_mapping.c. Fails the calling test LOUDLY (not | ||
| // silently returning empty / skipping) if the file can't be found, since a | ||
| // missing file here means this drift-detector isn't detecting anything. | ||
| ::testing::AssertionResult loadNormalizedPwmMappingSource(std::string *out) | ||
| { | ||
| const std::string path = pwmMappingSourcePath(); | ||
| std::ifstream file(path); | ||
| if (!file.is_open()) { | ||
| return ::testing::AssertionFailure() | ||
| << "Could not open live source file at computed path: " << path | ||
| << " -- SourceSync tests cannot verify anything against dead/absent " | ||
| "source. Check that src/test/unit/ and src/main/drivers/ still " | ||
| "have their expected relative layout."; | ||
| } |
Contributor
There was a problem hiding this comment.
2. Sourcesync path is fragile 🐞 Bug ☼ Reliability
The new SourceSync unit tests compute the on-disk path to src/main/drivers/pwm_mapping.c from __FILE__ and a fixed relative layout, then hard-fail if the file cannot be opened. If a build setup rewrites __FILE__ (e.g., path remapping) or runs unit tests without a full source tree present, these tests will fail even when behavior is correct.
Agent Prompt
### Issue description
SourceSync tests derive the live source path using `__FILE__` and `../../main/drivers/pwm_mapping.c`, and they fail the test if the file is missing/unopenable. This is intentionally strict, but it can produce false negatives under build configurations that don’t preserve a usable `__FILE__` path or when tests run without the source checkout.
### Issue Context
The implementation assumes `__FILE__` is emitted with a directory component that matches the source tree, and assumes the source tree exists at runtime.
### Fix Focus Areas
- src/test/unit/pwm_mapping_beeper_unittest.cc[611-646]
### Suggested fix approach
- Pass the repository source root into unit tests via a compile definition from CMake (e.g., `-DINAV_SOURCE_DIR="${CMAKE_SOURCE_DIR}"`) and build paths from that instead of `__FILE__`.
- Alternatively, if the source file cannot be located, use `GTEST_SKIP()` to avoid failing CI runs that execute tests from a packaged/relocated environment.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Test firmware build ready — commit Download firmware for PR #11749 243 targets built. Find your board's
|
pinioInit() would happily configure a TIM_USE_PINIO-flagged pad as timer-PWM via pinioInitTimerPWM(), which calls timerConfigBase() to set the timer's period/prescaler. That register is shared by every channel on the physical timer, so if the pad's sibling channel is dedicated to a WS2811 LED strip or BEEPER tone, this silently reprograms the shared period. beeperInit() runs before pinioInit() with nothing to re-assert it afterward, so a BEEPER sibling getting PWM-configured as PINIO can permanently corrupt the tone frequency. Add timerHasFixedPeriodSibling() and gate both pinioInitTimerPWM() call sites in pinioInit() on it, so any pad sharing a timer with an LED or BEEPER output falls through to the existing GPIO-only fallback instead. Found via code review of the LED sibling-pad-flags fix in this branch, which relies on siblings correctly falling back to GPIO-only PINIO.
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.
Summary
When a timer's output mode is set to LED,
ws2811LedStripInit()only ever wires up the first (canonical) pad on that timer — but every pad sharing the timer was being flaggedTIM_USE_LED, so sibling pads were reported as "Led" over MSP and shown that way in the configurator's Mixer/Output Mapping tab, despite being functionally dead. This mirrors the sibling-pad bug already fixed for BEEPER in #(8c16aee)/#(31bb99e), applied to LED.Changes
timerHardwareOverride(): track the first-encountered (canonical) pad per physical timer forOUTPUT_MODE_LED, same pattern as the existing BEEPER canonical-pad tracking. Only that pad getsTIM_USE_LED; sibling pads fall back toTIM_USE_PINIO(GPIO on/off) instead of a false LED flag.pwmAssignOutput()'sMAP_TO_LED_OUTPUTcase: removed apwmClaimTimer()call that was propagating the assigned pad's flags to every sibling on the same physical timer. That propagation is correct for motor/servo (a multi-channel timer legitimately offers multiple motor/servo outputs) but was silently re-overwriting the sibling's correctTIM_USE_PINIOback toTIM_USE_LEDimmediately after the canonical-pad logic set it correctly. This was the actual root cause of the symptom visible in the configurator — the first fix alone was not sufficient.pinio.c:pinioInit()would happily configure aTIM_USE_PINIOsibling pad as timer-PWM, which reprograms the timer's shared period register — corrupting an LED/BEEPER output living on the same physical timer (BEEPER especially, since nothing re-asserts its period afterward). AddedtimerHasFixedPeriodSibling()and gated bothpinioInitTimerPWM()call sites on it, so such pads correctly fall through to the GPIO-only fallback. Found via code review of the LED fix above, which depends on siblings actually behaving as GPIO-only.pwm_mapping_led_unittest.cc(isolatedtimerHardwareOverride()coverage),pwm_mapping_led_e2e_unittest.cc(full-pipeline coverage reproducing both the bug and the fix, plus a regression guard proving motor/servo timer-claim propagation is untouched), andpinio_timer_sibling_unittest.cc(coverage for the pinio.c guard, including same-timer vs different-timer scoping).pwm_mapping_beeper_unittest.cc, which had drifted and was testing a superseded version of the BEEPER logic; rewritten to match current source, with a SourceSync suite that reads the livepwm_mapping.cat test-runtime to catch future drift.Testing
pwm_mapping_led_unittest.cc,pwm_mapping_led_e2e_unittest.cc,pwm_mapping_beeper_unittest.cc, andpinio_timer_sibling_unittest.cc.MSP2_INAV_OUTPUT_MAPPING_EXT2and BOOTLOG tracing that only the canonical pad on Timer 4 carriesTIM_USE_LEDand the sibling carriesTIM_USE_PINIO.Related Issues
N/A