diff --git a/src/main/drivers/pinio.c b/src/main/drivers/pinio.c index 700a329979d..9c9c9caafe4 100644 --- a/src/main/drivers/pinio.c +++ b/src/main/drivers/pinio.c @@ -110,6 +110,23 @@ static bool pinioInitTimerPWM(int slot, IO_t io, const timerHardware_t *timHw, b return true; } +// True if another channel on the same physical timer is dedicated to a fixed-period +// output (WS2811 LED strip or BEEPER tone). pinioInitTimerPWM() has no per-timer +// exclusivity check of its own -- timerGetTCH() will happily hand out a TCH for any +// channel regardless of what else is running on that timer -- so without this guard, +// configuring such a pad as timer-PWM would call timerConfigBase() and reprogram the +// period/prescaler shared by every channel on the timer, corrupting the fixed-period +// output it belongs to. +static bool timerHasFixedPeriodSibling(const timerHardware_t *timHw) +{ + for (int i = 0; i < timerHardwareCount; i++) { + if (timerHardware[i].tim == timHw->tim && (timerHardware[i].usageFlags & (TIM_USE_LED | TIM_USE_BEEPER))) { + return true; + } + } + return false; +} + void pinioInit(void) { int runtimeCount = 0; @@ -126,7 +143,7 @@ void pinioInit(void) bool inverted = (pinioHardware[i].flags & PINIO_FLAGS_INVERTED) != 0; const timerHardware_t *timHw = timerGetByTag(pinioHardware[i].ioTag, TIM_USE_ANY); - if (timHw && IOGetOwner(io) == OWNER_FREE && pinioInitTimerPWM(runtimeCount, io, timHw, inverted)) { + if (timHw && IOGetOwner(io) == OWNER_FREE && !timerHasFixedPeriodSibling(timHw) && pinioInitTimerPWM(runtimeCount, io, timHw, inverted)) { runtimeCount++; continue; } @@ -152,13 +169,13 @@ void pinioInit(void) if (!io || IOGetOwner(io) != OWNER_FREE) { continue; } - if (pinioInitTimerPWM(runtimeCount, io, timHw, false)) { + if (!timerHasFixedPeriodSibling(timHw) && pinioInitTimerPWM(runtimeCount, io, timHw, false)) { runtimeCount++; continue; } // GPIO fallback: pad's timer is already running at a fixed frequency - // for another purpose (e.g. BEEPER), so PWM duty control isn't available. + // for another purpose (e.g. BEEPER or LED), so PWM duty control isn't available. IOInit(io, OWNER_PINIO, RESOURCE_OUTPUT, RESOURCE_INDEX(runtimeCount)); IOConfigGPIO(io, IOCFG_OUT_PP); pinioRuntime[runtimeCount].inverted = false; diff --git a/src/main/drivers/pwm_mapping.c b/src/main/drivers/pwm_mapping.c index 23b5c596111..44a33220c20 100644 --- a/src/main/drivers/pwm_mapping.c +++ b/src/main/drivers/pwm_mapping.c @@ -211,7 +211,7 @@ static bool checkPwmTimerConflicts(const timerHardware_t *timHw) return false; } -static void timerHardwareOverride(timerHardware_t * timer, bool isCanonicalBeeperPad) { +static void timerHardwareOverride(timerHardware_t * timer, bool isCanonicalBeeperPad, bool isCanonicalLedPad) { switch (timerOverrides(timer2id(timer->tim))->outputMode) { case OUTPUT_MODE_MOTORS: timer->usageFlags &= ~(TIM_USE_SERVO|TIM_USE_LED|TIM_USE_BEEPER); @@ -222,8 +222,17 @@ static void timerHardwareOverride(timerHardware_t * timer, bool isCanonicalBeepe timer->usageFlags |= TIM_USE_SERVO; break; case OUTPUT_MODE_LED: - timer->usageFlags &= ~(TIM_USE_MOTOR|TIM_USE_SERVO|TIM_USE_BEEPER); - timer->usageFlags |= TIM_USE_LED; + // Other channels of this timer share its period register, so they can't + // be repurposed as motor/servo/BEEPER either. Only the first (canonical) + // pad is the one ws2811LedStripInit() actually wires up; siblings can still + // drive a GPIO-only PINIO (binary on/off, no PWM), since that doesn't need + // the timer's fixed WS2811-bitrate period. + timer->usageFlags &= ~(TIM_USE_MOTOR|TIM_USE_SERVO|TIM_USE_BEEPER|TIM_USE_LED); + timer->usageFlags |= TIM_USE_PINIO; + if (isCanonicalLedPad) { + timer->usageFlags &= ~TIM_USE_PINIO; + timer->usageFlags |= TIM_USE_LED; + } break; case OUTPUT_MODE_PINIO: timer->usageFlags &= ~(TIM_USE_MOTOR|TIM_USE_SERVO|TIM_USE_LED|TIM_USE_BEEPER); @@ -292,8 +301,14 @@ static void pwmAssignOutput(timMotorServoHardware_t *timOutputs, timerHardware_t pwmClaimTimer(timHw->tim, timHw->usageFlags); break; case MAP_TO_LED_OUTPUT: + // Unlike motor/servo, do NOT pwmClaimTimer() here: that propagates + // this pad's flags to every sibling pad sharing the physical timer, + // which is correct for motor/servo (a multi-channel timer should + // offer multiple motor/servo outputs) but wrong for LED — only this + // one canonical pad is ever driven, and propagating TIM_USE_LED would + // overwrite siblings' correct TIM_USE_PINIO/BEEPER (from the earlier + // canonical-pad pass in timerHardwareOverride()) back to a false LED flag. timHw->usageFlags &= TIM_USE_LED; - pwmClaimTimer(timHw->tim, timHw->usageFlags); break; default: break; @@ -327,6 +342,7 @@ void pwmBuildTimerOutputList(timMotorServoHardware_t *timOutputs) // Apply all timerOverrides upfront so flag state is stable for both passes bool beeperClaimed[HARDWARE_TIMER_DEFINITION_COUNT] = { false }; + bool ledClaimed[HARDWARE_TIMER_DEFINITION_COUNT] = { false }; for (int idx = 0; idx < timerHardwareCount; idx++) { timerHardware_t *timHw = &timerHardware[idx]; uint8_t timId = timer2id(timHw->tim); @@ -335,7 +351,12 @@ void pwmBuildTimerOutputList(timMotorServoHardware_t *timOutputs) beeperClaimed[timId] = true; isCanonicalBeeperPad = true; } - timerHardwareOverride(timHw, isCanonicalBeeperPad); + bool isCanonicalLedPad = false; + if (timerOverrides(timId)->outputMode == OUTPUT_MODE_LED && !ledClaimed[timId]) { + ledClaimed[timId] = true; + isCanonicalLedPad = true; + } + timerHardwareOverride(timHw, isCanonicalBeeperPad, isCanonicalLedPad); } // Assign outputs in priority order: dedicated first, then auto. diff --git a/src/test/unit/pinio_timer_sibling_unittest.cc b/src/test/unit/pinio_timer_sibling_unittest.cc new file mode 100644 index 00000000000..94187e95108 --- /dev/null +++ b/src/test/unit/pinio_timer_sibling_unittest.cc @@ -0,0 +1,432 @@ +/* + * Unit test: pinioInit()'s guard against configuring a PINIO pad as + * timer-PWM when it shares its physical timer with a fixed-period output + * (WS2811 LED strip or BEEPER tone). + * + * Bug context (follow-on issue surfaced by the sibling-pad fix in + * src/main/drivers/pwm_mapping.c, see pwm_mapping_led_unittest.cc / + * pwm_mapping_beeper_unittest.cc): + * + * Once pwm_mapping.c correctly falls sibling pads on an LED/BEEPER timer + * back to TIM_USE_PINIO (instead of falsely claiming TIM_USE_LED), those + * pads reach pinioInit(). pinioInit() calls pinioInitTimerPWM() on any + * TIM_USE_PINIO-flagged pad it finds a TCH for, and pinioInitTimerPWM() + * calls timerConfigBase(tch, PINIO_PWM_PERIOD, PINIO_PWM_BASE_HZ) — but the + * period/prescaler register that timerConfigBase() programs is SHARED by + * every channel on that physical timer. timerGetTCH() has no per-timer + * exclusivity check of its own: it will happily hand back a TCH for the + * PINIO pad regardless of what else (LED strip / BEEPER) is already + * running on that timer. Without a guard, configuring the PINIO sibling as + * PWM would silently reprogram the LED-strip's WS2811 bit-rate period (or + * the BEEPER's tone period), corrupting that output. + * + * Fix: src/main/drivers/pinio.c adds timerHasFixedPeriodSibling(), which + * scans timerHardware[] for any OTHER entry on the SAME physical timer + * (`.tim ==`) carrying TIM_USE_LED or TIM_USE_BEEPER, and gates both + * pinioInitTimerPWM() call sites in pinioInit() on + * `!timerHasFixedPeriodSibling(timHw) && ...`, so such a pad falls back to + * the plain-GPIO path instead. + * + * This file contains: + * 1. A minimal inline reproduction of timerHasFixedPeriodSibling() (and the + * minimal timerHardware_t / usage-flag definitions it needs), since + * pinio.c cannot be linked into the host unit-test binary: it is gated + * by `#ifdef USE_PINIO` and depends on live hardware/timer/IO driver + * infrastructure (IOInit, timerGetTCH, timerConfigBase, etc.) that is + * not mocked for unit tests. + * 2. TEST SUITE TimerHasFixedPeriodSibling — covers: no sibling on the + * timer, LED sibling, BEEPER sibling, a sibling with a matching flag + * but on a DIFFERENT physical timer (verifies the `.tim ==` comparison + * itself is doing real work, not just the flag test), and the pad's own + * entry carrying TIM_USE_PINIO while scanning past it. + * 3. TEST SUITE SourceSync — reads the ACTUAL LIVE + * src/main/drivers/pinio.c off disk at test time and verifies (a) the + * real timerHasFixedPeriodSibling() body still matches this file's + * inline reproduction, and (b) both call sites inside pinioInit() still + * gate on `timerHasFixedPeriodSibling(...)`, so this test cannot + * silently drift from the real implementation and cannot stay green if + * the guard is later removed. This is the same drift-detection pattern + * used by pwm_mapping_led_unittest.cc / pwm_mapping_beeper_unittest.cc + * after pwm_mapping_beeper_unittest.cc was found to have gone stale. + */ + +#include +#include +#include +#include +#include +#include + +#include "gtest/gtest.h" +#include "unittest_macros.h" + +/* ------------------------------------------------------------------------- + * Minimal type/flag definitions — mirrors src/main/drivers/timer.h exactly + * for the bits this function cares about. `tim` stands in for the real + * code's `tmr_type *tim` / `TIM_TypeDef *tim` pointer — in the real driver, + * multiple timerHardware[] pads sharing the same physical timer compare + * equal on pointer identity of `.tim`; here we just use a plain int id and + * compare that, which exercises identical logic. + * ------------------------------------------------------------------------- */ + +typedef enum { + TIM_USE_ANY = 0, + TIM_USE_PPM = (1 << 0), + TIM_USE_PWM = (1 << 1), + TIM_USE_MOTOR = (1 << 2), + TIM_USE_SERVO = (1 << 3), + TIM_USE_LED = (1 << 24), + TIM_USE_BEEPER = (1 << 25), + TIM_USE_PINIO = (1 << 26), +} timerUsageFlag_e; + +/* Minimal timer hardware entry — only the fields timerHasFixedPeriodSibling() + * actually touches (`.tim`, `.usageFlags`). */ +typedef struct timerHardware_s { + int tim; // stands in for the real tmr_type*/TIM_TypeDef* pointer identity + uint32_t usageFlags; +} timerHardware_t; + +/* ------------------------------------------------------------------------- + * Inline reproduction of timerHasFixedPeriodSibling(). + * + * We inline this rather than linking pinio.c because pinio.c is gated by + * `#ifdef USE_PINIO` and depends on IOInit/IOConfigGPIOAF/timerGetTCH/ + * timerConfigBase/timerPWMConfigChannel/timerPWMStart/timerEnable — none of + * which exist in the host unit-test build. + * + * This is a literal translation of the current C source. It operates against + * test-local `timerHardware[]` / `timerHardwareCount` globals (declared + * below, mirroring the real extern globals from timer.h) rather than the + * real firmware ones, so each test can set up its own hardware table. + * + * See the SourceSync test suite at the bottom of this file for a mechanism + * that catches this reproduction going out of sync with the real source. + * ------------------------------------------------------------------------- */ + +static timerHardware_t *g_timerHardware = nullptr; +static int g_timerHardwareCount = 0; + +static bool timerHasFixedPeriodSibling(const timerHardware_t *timHw) +{ + for (int i = 0; i < g_timerHardwareCount; i++) { + if (g_timerHardware[i].tim == timHw->tim && (g_timerHardware[i].usageFlags & (TIM_USE_LED | TIM_USE_BEEPER))) { + return true; + } + } + return false; +} + +/* ========================================================================= + * TEST SUITE: TimerHasFixedPeriodSibling + * ========================================================================= */ + +/* + * TEST 1 — No other pad on the timer at all: must return false. + */ +TEST(TimerHasFixedPeriodSibling, NoSibling_ReturnsFalse) +{ + timerHardware_t pads[1]; + pads[0].tim = 1; pads[0].usageFlags = (uint32_t)TIM_USE_PINIO; + + g_timerHardware = pads; + g_timerHardwareCount = 1; + + EXPECT_FALSE(timerHasFixedPeriodSibling(&pads[0])); +} + +/* + * TEST 2 — A sibling pad on the SAME physical timer carries TIM_USE_LED: + * must return true. + */ +TEST(TimerHasFixedPeriodSibling, LedSiblingOnSameTimer_ReturnsTrue) +{ + timerHardware_t pads[2]; + pads[0].tim = 5; pads[0].usageFlags = (uint32_t)TIM_USE_PINIO; // pad under test + pads[1].tim = 5; pads[1].usageFlags = (uint32_t)TIM_USE_LED; // sibling: LED strip + + g_timerHardware = pads; + g_timerHardwareCount = 2; + + EXPECT_TRUE(timerHasFixedPeriodSibling(&pads[0])) + << "A PINIO pad sharing its timer with an LED-strip pad must be " + "reported as having a fixed-period sibling, so pinioInit() does " + "not reprogram the LED strip's WS2811 period."; +} + +/* + * TEST 3 — A sibling pad on the SAME physical timer carries TIM_USE_BEEPER: + * must return true. + */ +TEST(TimerHasFixedPeriodSibling, BeeperSiblingOnSameTimer_ReturnsTrue) +{ + timerHardware_t pads[2]; + pads[0].tim = 7; pads[0].usageFlags = (uint32_t)TIM_USE_PINIO; // pad under test + pads[1].tim = 7; pads[1].usageFlags = (uint32_t)TIM_USE_BEEPER; // sibling: buzzer + + g_timerHardware = pads; + g_timerHardwareCount = 2; + + EXPECT_TRUE(timerHasFixedPeriodSibling(&pads[0])) + << "A PINIO pad sharing its timer with a BEEPER pad must be reported " + "as having a fixed-period sibling, so pinioInit() does not " + "reprogram the buzzer's tone period."; +} + +/* + * TEST 4 — A DIFFERENT physical timer has an LED pad, but the timer under + * test does not. Must return false. This specifically verifies the `.tim ==` + * comparison is doing real work (i.e. the function is not just checking + * "does ANY entry anywhere have TIM_USE_LED|TIM_USE_BEEPER", which would be + * a much easier, but wrong, bug to accidentally introduce). + */ +TEST(TimerHasFixedPeriodSibling, LedOnDifferentTimer_ReturnsFalse) +{ + timerHardware_t pads[2]; + pads[0].tim = 1; pads[0].usageFlags = (uint32_t)TIM_USE_PINIO; // pad under test, timer 1 + pads[1].tim = 2; pads[1].usageFlags = (uint32_t)TIM_USE_LED; // LED pad, timer 2 (different!) + + g_timerHardware = pads; + g_timerHardwareCount = 2; + + EXPECT_FALSE(timerHasFixedPeriodSibling(&pads[0])) + << "An LED/BEEPER pad on a DIFFERENT physical timer must not cause a " + "false positive -- the comparison must be scoped to the SAME " + "timer (`.tim ==`), not just a global flag scan."; +} + +/* + * TEST 5 — Same as TEST 4 but with BEEPER instead of LED, for symmetry. + */ +TEST(TimerHasFixedPeriodSibling, BeeperOnDifferentTimer_ReturnsFalse) +{ + timerHardware_t pads[2]; + pads[0].tim = 3; pads[0].usageFlags = (uint32_t)TIM_USE_PINIO; + pads[1].tim = 9; pads[1].usageFlags = (uint32_t)TIM_USE_BEEPER; + + g_timerHardware = pads; + g_timerHardwareCount = 2; + + EXPECT_FALSE(timerHasFixedPeriodSibling(&pads[0])); +} + +/* + * TEST 6 — The pad's own entry (in timerHardware[]) carries TIM_USE_PINIO; + * scanning must correctly pass over it (a pad with only TIM_USE_PINIO set + * must not itself be mistaken for a fixed-period sibling) while still + * finding the real LED sibling elsewhere on the same timer. Also exercises + * a 3-pad table so the loop must not stop early. + */ +TEST(TimerHasFixedPeriodSibling, OwnPinioEntryIgnored_SiblingStillFound) +{ + timerHardware_t pads[3]; + pads[0].tim = 4; pads[0].usageFlags = (uint32_t)TIM_USE_PINIO; // pad under test (itself in the table) + pads[1].tim = 4; pads[1].usageFlags = (uint32_t)TIM_USE_PINIO; // another PINIO pad, same timer, not fixed-period + pads[2].tim = 4; pads[2].usageFlags = (uint32_t)TIM_USE_LED; // the real fixed-period sibling + + g_timerHardware = pads; + g_timerHardwareCount = 3; + + EXPECT_TRUE(timerHasFixedPeriodSibling(&pads[0])) + << "Own TIM_USE_PINIO entry and a plain-PINIO sibling must not mask " + "the presence of the real LED sibling further down the table."; + EXPECT_TRUE(timerHasFixedPeriodSibling(&pads[1])) + << "Same check from the second PINIO pad's perspective."; +} + +/* + * TEST 7 — Multiple pads on the timer, none of them fixed-period (all + * MOTOR/SERVO/PINIO): must return false. Guards against a trivial "returns + * true if timer has >1 pad" mis-implementation. + */ +TEST(TimerHasFixedPeriodSibling, MultipleNonFixedPeriodSiblings_ReturnsFalse) +{ + timerHardware_t pads[3]; + pads[0].tim = 6; pads[0].usageFlags = (uint32_t)TIM_USE_PINIO; + pads[1].tim = 6; pads[1].usageFlags = (uint32_t)TIM_USE_MOTOR; + pads[2].tim = 6; pads[2].usageFlags = (uint32_t)TIM_USE_SERVO; + + g_timerHardware = pads; + g_timerHardwareCount = 3; + + EXPECT_FALSE(timerHasFixedPeriodSibling(&pads[0])); +} + +/* ========================================================================= + * TEST SUITE: SourceSync + * + * Reads the ACTUAL LIVE src/main/drivers/pinio.c off disk (not a cached + * copy, not something computed at CMake-configure time) and checks that: + * (a) timerHasFixedPeriodSibling()'s body still matches this file's inline + * reproduction, and + * (b) both pinioInitTimerPWM() call sites inside pinioInit() still gate on + * `timerHasFixedPeriodSibling(...)`. + * + * If a future change to pinio.c alters this logic (or removes the guard) + * without updating this test file, these checks fail loudly in `make check` + * / CI instead of this file silently continuing to pass against dead logic + * -- exactly the class of drift that was previously found in + * pwm_mapping_beeper_unittest.cc (see its header comment for history). + * + * This does not (and structurally cannot, given the USE_PINIO / hardware- + * driver dependencies described above) substitute for actually linking and + * executing the real function. It is a drift-detector, not a full + * integration test. Comments are stripped and whitespace is collapsed + * before comparison so that pure reformatting/typo-fixes in comments do not + * cause false failures; only the executable-statement tokens are checked. + * ========================================================================= */ + +namespace { + +// Strips "//...\n" line comments. Deliberately does not handle /* */ block +// comments or comments inside string/char literals -- the functions we +// check in pinio.c don't use either, and keeping this simple avoids this +// drift-detector itself becoming a source of drift. +std::string stripLineComments(const std::string &text) +{ + std::string result; + result.reserve(text.size()); + for (size_t i = 0; i < text.size();) { + if (text[i] == '/' && i + 1 < text.size() && text[i + 1] == '/') { + while (i < text.size() && text[i] != '\n') { + i++; + } + continue; + } + result += text[i]; + i++; + } + return result; +} + +// Collapses any run of whitespace (including newlines) to a single space, +// and trims leading/trailing whitespace, so indentation/line-wrap changes +// don't cause false failures. +std::string normalizeWhitespace(const std::string &text) +{ + std::string result; + result.reserve(text.size()); + bool lastWasSpace = true; // trims leading whitespace + for (char c : text) { + if (std::isspace(static_cast(c))) { + if (!lastWasSpace) { + result += ' '; + lastWasSpace = true; + } + } else { + result += c; + lastWasSpace = false; + } + } + while (!result.empty() && result.back() == ' ') { + result.pop_back(); + } + return result; +} + +std::string normalizeSource(const std::string &text) +{ + return normalizeWhitespace(stripLineComments(text)); +} + +// Locates the live pinio.c relative to this test file's own path. __FILE__ +// is emitted as an absolute path by this project's CMake/Makefiles +// (verified by the equivalent helper in pwm_mapping_led_unittest.cc / +// pwm_mapping_beeper_unittest.cc), so this resolves correctly regardless of +// the build directory or CWD the test is invoked from. +std::string pinioSourcePath() +{ + std::string thisFile = __FILE__; // .../src/test/unit/pinio_timer_sibling_unittest.cc + size_t lastSlash = thisFile.find_last_of("/\\"); + std::string testUnitDir = (lastSlash == std::string::npos) ? "." : thisFile.substr(0, lastSlash); + return testUnitDir + "/../../main/drivers/pinio.c"; +} + +// Reads and normalizes pinio.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 loadNormalizedPinioSource(std::string *out) +{ + const std::string path = pinioSourcePath(); + 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."; + } + std::ostringstream buffer; + buffer << file.rdbuf(); + if (buffer.str().empty()) { + return ::testing::AssertionFailure() + << "Live source file at " << path << " opened but read as empty."; + } + *out = normalizeSource(buffer.str()); + return ::testing::AssertionSuccess(); +} + +} // namespace + +TEST(SourceSync, TimerHasFixedPeriodSiblingBodyMatchesLiveSource) +{ + std::string normalizedSource; + ASSERT_TRUE(loadNormalizedPinioSource(&normalizedSource)); + + const std::string expectedFunction = normalizeSource( + "static bool timerHasFixedPeriodSibling(const timerHardware_t *timHw) " + "{ " + "for (int i = 0; i < timerHardwareCount; i++) { " + "if (timerHardware[i].tim == timHw->tim && (timerHardware[i].usageFlags & (TIM_USE_LED | TIM_USE_BEEPER))) { " + "return true; " + "} " + "} " + "return false; " + "}"); + + EXPECT_NE(normalizedSource.find(expectedFunction), std::string::npos) + << "timerHasFixedPeriodSibling() in the LIVE src/main/drivers/pinio.c " + "no longer matches this test's inline reproduction. If the " + "function legitimately changed, update the inline " + "timerHasFixedPeriodSibling() in this test file to match, then " + "update this expected string too. THIS is the exact class of " + "drift that made pwm_mapping_beeper_unittest.cc stale for months " + "-- do not let it happen again."; +} + +TEST(SourceSync, Pass1CallSiteGatesOnFixedPeriodSiblingGuard) +{ + std::string normalizedSource; + ASSERT_TRUE(loadNormalizedPinioSource(&normalizedSource)); + + // Pass 1 (pinioHardware[] loop): the condition guarding pinioInitTimerPWM(). + const std::string expectedGuard = normalizeSource( + "if (timHw && IOGetOwner(io) == OWNER_FREE && !timerHasFixedPeriodSibling(timHw) && pinioInitTimerPWM(runtimeCount, io, timHw, inverted)) {"); + + EXPECT_NE(normalizedSource.find(expectedGuard), std::string::npos) + << "pinioInit()'s Pass 1 (pinioHardware[] target-defined pins) call " + "site no longer gates pinioInitTimerPWM() on " + "!timerHasFixedPeriodSibling(timHw). If someone removes this " + "guard, a PINIO pad sharing a timer with an LED strip or BEEPER " + "would silently corrupt that output's period again -- this test " + "must fail loudly if that happens."; +} + +TEST(SourceSync, Pass2CallSiteGatesOnFixedPeriodSiblingGuard) +{ + std::string normalizedSource; + ASSERT_TRUE(loadNormalizedPinioSource(&normalizedSource)); + + // Pass 2 (timerHardware[] loop, TIM_USE_PINIO pads assigned via mixer): + // the condition guarding pinioInitTimerPWM(). + const std::string expectedGuard = normalizeSource( + "if (!timerHasFixedPeriodSibling(timHw) && pinioInitTimerPWM(runtimeCount, io, timHw, false)) {"); + + EXPECT_NE(normalizedSource.find(expectedGuard), std::string::npos) + << "pinioInit()'s Pass 2 (timerHardware[] TIM_USE_PINIO pads assigned " + "via the mixer) call site no longer gates pinioInitTimerPWM() on " + "!timerHasFixedPeriodSibling(timHw). If someone removes this " + "guard, a mixer-assigned PINIO pad sharing a timer with an LED " + "strip or BEEPER would silently corrupt that output's period " + "again -- this test must fail loudly if that happens."; +} diff --git a/src/test/unit/pwm_mapping_beeper_unittest.cc b/src/test/unit/pwm_mapping_beeper_unittest.cc index 1ce0754a618..e71eb691253 100644 --- a/src/test/unit/pwm_mapping_beeper_unittest.cc +++ b/src/test/unit/pwm_mapping_beeper_unittest.cc @@ -1,39 +1,87 @@ /* - * Unit test: timerHardwareOverride() must not corrupt TIM_USE_BEEPER flags. + * Unit test: timerHardwareOverride() must not corrupt/misassign TIM_USE_BEEPER + * flags on shared timers. * - * Bug (pre-fix): - * When a user applies OUTPUT_MODE_SERVOS to a timer that has TIM_USE_BEEPER - * set (e.g. MATEKH743 TIM2/PA15), timerHardwareOverride() would apply the - * servo-mode mask without first checking for the beeper flag. The subsequent - * pwmAssignOutput(servo) call strips everything except TIM_USE_SERVO, leaving - * beeperPwmInit() unable to find its timer. + * History (this file previously tested an intermediate, now-superseded fix — + * see below; it was found to be STALE and was rewritten to match current + * source while adding pwm_mapping_led_unittest.cc): * - * Fix (commit 551bce85d6): - * Guard added at the top of timerHardwareOverride(): - * if (timer->usageFlags & TIM_USE_BEEPER) { return; } + * 1. Bug (original, issue #11492): if a user applied a timer_output_mode + * override (e.g. OUTPUT_MODE_SERVOS) to a timer that also had + * TIM_USE_BEEPER set (e.g. MATEKH743 TIM2/PA15), timerHardwareOverride() + * would apply the servo-mode mask without protecting the beeper flag. + * The subsequent pwmAssignOutput(servo) call then strips everything + * except TIM_USE_SERVO, leaving beeperPwmInit() unable to find its + * timer. + * + * 2. Intermediate fix (commit 7eebdf6345, "pwm_mapping: guard beeper timer + * from timerHardwareOverride()"): guard added at the top of + * timerHardwareOverride() — + * if (timer->usageFlags & TIM_USE_BEEPER) { return; } + * This made ANY timer carrying TIM_USE_BEEPER completely immune to all + * output-mode overrides, regardless of the override applied to it. This + * is what the ORIGINAL version of this test file verified. + * + * 3. CURRENT fix (commits 8c16aeed85 "Fix BEEPER output mode falsely + * flagging sibling timer channels" and 31bb99ea0b "Expose BEEPER-timer + * sibling pads as GPIO-only PINIO outputs"): the step-2 early-return + * guard was replaced entirely. BEEPER is now its own explicit + * OUTPUT_MODE_BEEPER (rather than being inferred from a pre-existing + * TIM_USE_BEEPER flag), and timerHardwareOverride() takes an + * `isCanonicalBeeperPad` bool: only the first (canonical) pad on a + * physical timer set to OUTPUT_MODE_BEEPER gets TIM_USE_BEEPER; sibling + * pads on that same timer fall back to TIM_USE_PINIO (GPIO-only — + * binary on/off, no PWM, since that doesn't need the timer's fixed + * buzzer-tone period). This is the exact same fix pattern later applied + * to LED (see pwm_mapping_led_unittest.cc) via a parallel + * `isCanonicalLedPad` parameter. + * + * NOTE ON DISCOVERED STALENESS: this file previously (incorrectly) still + * tested the step-2 guard-based logic long after the real source had moved + * to step-3 — it kept passing the whole time because it only exercised its + * own frozen, hand-copied logic and never re-checked the live file. It has + * been rewritten here to match the CURRENT source in + * src/main/drivers/pwm_mapping.c, and a SourceSync test suite (see bottom + * of this file) has been added specifically to catch this class of drift + * automatically in the future. * * This file contains: - * 1. A minimal inline reproduction of both the BUGGY and FIXED versions of - * timerHardwareOverride() so that the test is fully self-contained - * (pwm_mapping.c is excluded from the SITL/unit build by its own guard). - * 2. TEST BugReproduction — demonstrates that the bug corrupts TIM_USE_BEEPER. - * This test would FAIL on pre-fix code and PASSES on the fixed code. - * 3. TEST FixVerification — positive assertion that TIM_USE_BEEPER survives - * after a servo-mode override on a beeper timer. - * 4. Negative / regression tests covering normal (non-beeper) timers to - * confirm that the guard does not break the existing override behaviour. + * 1. A minimal inline reproduction of both a BUGGY version (unconditional + * TIM_USE_BEEPER on every pad of a BEEPER-mode timer, no canonical-pad + * tracking — the same bug class fixed by commit 8c16aeed85) and the + * CURRENT FIXED version of timerHardwareOverride(), since pwm_mapping.c + * is excluded from the SITL/unit build by its own + * `#if !defined(SITL_BUILD)` guard and depends on PG (parameter-group) + * infrastructure not available in a host unit build. + * 2. TEST BugReproduction — demonstrates that the old, unconditional-BEEPER + * code gives TIM_USE_BEEPER to every pad sharing a BEEPER-mode timer. + * 3. TEST FixVerification — only the canonical (first) pad on a shared + * timer gets TIM_USE_BEEPER; sibling pads fall back to TIM_USE_PINIO. + * 4. Multi-pad / multi-timer canonical-tracking tests. + * 5. Regression tests for MOTORS/SERVOS/LED/PINIO overrides on the fixed + * function, including timers mixed with a BEEPER-mode timer. + * 6. TEST SUITE SourceSync — reads the ACTUAL LIVE + * src/main/drivers/pwm_mapping.c off disk at test time and asserts the + * real timerHardwareOverride() BEEPER-case source text still matches + * the logic the inline reproduction above assumes, so this file cannot + * silently go stale again the way it did before. */ #include #include +#include +#include +#include +#include #include "gtest/gtest.h" #include "unittest_macros.h" /* ------------------------------------------------------------------------- - * Minimal type/flag definitions — mirrors the real firmware headers. - * We avoid pulling in the real headers because they drag in hundreds of - * platform-specific includes that cannot be satisfied in a host build. + * Minimal type/flag definitions — mirrors the real firmware headers on this + * branch (src/main/drivers/timer.h, src/main/flight/mixer.h). This branch + * has TIM_USE_PINIO / TIM_USE_BEEPER and OUTPUT_MODE_PINIO / OUTPUT_MODE_BEEPER + * (unlike release/9.1). * ------------------------------------------------------------------------- */ /* timerUsageFlag_e — must match src/main/drivers/timer.h exactly */ @@ -45,6 +93,7 @@ typedef enum { TIM_USE_SERVO = (1 << 3), TIM_USE_LED = (1 << 24), TIM_USE_BEEPER = (1 << 25), + TIM_USE_PINIO = (1 << 26), } timerUsageFlag_e; /* outputMode_e — must match src/main/flight/mixer.h exactly */ @@ -53,13 +102,17 @@ typedef enum { OUTPUT_MODE_MOTORS, OUTPUT_MODE_SERVOS, OUTPUT_MODE_LED, - OUTPUT_MODE_PINIO + OUTPUT_MODE_PINIO, + OUTPUT_MODE_BEEPER } outputMode_e; -/* Minimal timer hardware entry — only the fields exercised by the function. */ +/* Minimal timer hardware entry — only the fields exercised by the function. + * `timId` stands in for the real code's timer2id(timer->tim) — in the real + * driver, multiple timerHardware[] pads sharing the same physical timer + * compare equal on timer2id(); here we just store the id directly. */ typedef struct timerHardware_s { uint32_t usageFlags; - /* All other fields (tim, tag, channel, …) are unused in this test. */ + uint8_t timId; } timerHardware_t; /* ------------------------------------------------------------------------- @@ -69,40 +122,52 @@ typedef struct timerHardware_s { * a) pwm_mapping.c is wrapped in #if !defined(SITL_BUILD) and the unit-test * target.h defines SITL_BUILD, so the real file compiles to nothing. * b) The function depends on timerOverrides() (parameter-group accessor) and - * timer2id(), which are platform / PG infrastructure that is not present in - * a host unit-test build. + * timer2id(), which are platform / PG infrastructure that is not present + * in a host unit-test build. * - * The functions below are literal translations of the C source with the PG - * indirection collapsed into a simple `outputMode` parameter. + * The functions below are literal translations of the current C source + * (post BEEPER-fix, post LED-fix) with the PG indirection collapsed into a + * simple `outputMode` parameter, and canonical-pad tracking passed in + * explicitly as bool parameters (mirroring isCanonicalBeeperPad / + * isCanonicalLedPad computed by pwmBuildTimerOutputList()). + * + * See the SourceSync test suite at the bottom of this file for a mechanism + * that catches this reproduction going out of sync with the real source. * ------------------------------------------------------------------------- */ /* - * BUGGY version (pre-fix, equivalent to the code BEFORE commit 551bce85d6). - * - * The OUTPUT_MODE_SERVOS case clears TIM_USE_MOTOR and TIM_USE_LED but does - * NOT clear TIM_USE_BEEPER. The resulting combined flags - * (TIM_USE_SERVO | TIM_USE_BEEPER) will later be stripped by pwmAssignOutput() - * to just TIM_USE_SERVO, silently destroying the beeper timer entry. + * BUGGY version — reproduces the pre-8c16aeed85 BEEPER behaviour: + * OUTPUT_MODE_BEEPER unconditionally sets TIM_USE_BEEPER on every pad + * sharing the timer, with no canonical-pad tracking at all. MOTORS/SERVOS/ + * LED/PINIO cases are included only for contrast; this reproduction focuses + * on the BEEPER bug. */ static void timerHardwareOverride_buggy(timerHardware_t *timer, - outputMode_e outputMode) + outputMode_e outputMode) { - /* NOTE: no beeper guard here — this is intentionally the buggy version */ switch (outputMode) { case OUTPUT_MODE_MOTORS: - timer->usageFlags &= ~((uint32_t)(TIM_USE_SERVO | TIM_USE_LED)); - timer->usageFlags |= (uint32_t)TIM_USE_MOTOR; + timer->usageFlags &= ~(uint32_t)(TIM_USE_SERVO | TIM_USE_LED | TIM_USE_BEEPER); + timer->usageFlags |= (uint32_t)TIM_USE_MOTOR; break; case OUTPUT_MODE_SERVOS: - timer->usageFlags &= ~((uint32_t)(TIM_USE_MOTOR | TIM_USE_LED)); - timer->usageFlags |= (uint32_t)TIM_USE_SERVO; + timer->usageFlags &= ~(uint32_t)(TIM_USE_MOTOR | TIM_USE_LED | TIM_USE_BEEPER); + timer->usageFlags |= (uint32_t)TIM_USE_SERVO; break; case OUTPUT_MODE_LED: - timer->usageFlags &= ~((uint32_t)(TIM_USE_MOTOR | TIM_USE_SERVO)); - timer->usageFlags |= (uint32_t)TIM_USE_LED; + timer->usageFlags &= ~(uint32_t)(TIM_USE_MOTOR | TIM_USE_SERVO | TIM_USE_BEEPER); + timer->usageFlags |= (uint32_t)TIM_USE_LED; break; case OUTPUT_MODE_PINIO: - timer->usageFlags &= ~((uint32_t)(TIM_USE_MOTOR | TIM_USE_SERVO | TIM_USE_LED)); + timer->usageFlags &= ~(uint32_t)(TIM_USE_MOTOR | TIM_USE_SERVO | TIM_USE_LED | TIM_USE_BEEPER); + timer->usageFlags |= (uint32_t)TIM_USE_PINIO; + break; + case OUTPUT_MODE_BEEPER: + /* BUG: unconditionally sets TIM_USE_BEEPER on every pad of this + * timer, even though only one pad is actually wired up by + * beeperPwmInit(). No canonical-pad tracking whatsoever. */ + timer->usageFlags &= ~(uint32_t)(TIM_USE_MOTOR | TIM_USE_SERVO | TIM_USE_LED); + timer->usageFlags |= (uint32_t)TIM_USE_BEEPER; break; default: break; @@ -110,266 +175,554 @@ static void timerHardwareOverride_buggy(timerHardware_t *timer, } /* - * FIXED version (post-fix, matches commit 551bce85d6 exactly). - * - * The beeper guard causes the function to return immediately when - * TIM_USE_BEEPER is set, leaving the flags completely untouched. + * FIXED version — literal translation of the current + * src/main/drivers/pwm_mapping.c timerHardwareOverride() on this branch. + * Kept in sync with the real source by the SourceSync tests below. */ static void timerHardwareOverride_fixed(timerHardware_t *timer, - outputMode_e outputMode) + outputMode_e outputMode, + bool isCanonicalBeeperPad, + bool isCanonicalLedPad) { - /* Guard added by the fix: never modify a beeper timer. */ - if (timer->usageFlags & (uint32_t)TIM_USE_BEEPER) { - return; - } switch (outputMode) { case OUTPUT_MODE_MOTORS: - timer->usageFlags &= ~((uint32_t)(TIM_USE_SERVO | TIM_USE_LED)); - timer->usageFlags |= (uint32_t)TIM_USE_MOTOR; + timer->usageFlags &= ~(uint32_t)(TIM_USE_SERVO | TIM_USE_LED | TIM_USE_BEEPER); + timer->usageFlags |= (uint32_t)TIM_USE_MOTOR; break; case OUTPUT_MODE_SERVOS: - timer->usageFlags &= ~((uint32_t)(TIM_USE_MOTOR | TIM_USE_LED)); - timer->usageFlags |= (uint32_t)TIM_USE_SERVO; + timer->usageFlags &= ~(uint32_t)(TIM_USE_MOTOR | TIM_USE_LED | TIM_USE_BEEPER); + timer->usageFlags |= (uint32_t)TIM_USE_SERVO; break; case OUTPUT_MODE_LED: - timer->usageFlags &= ~((uint32_t)(TIM_USE_MOTOR | TIM_USE_SERVO)); - timer->usageFlags |= (uint32_t)TIM_USE_LED; + // Other channels of this timer share its period register, so they can't + // be repurposed as motor/servo/BEEPER either. Only the first (canonical) + // pad is the one ws2811LedStripInit() actually wires up; siblings can still + // drive a GPIO-only PINIO (binary on/off, no PWM), since that doesn't need + // the timer's fixed WS2811-bitrate period. + timer->usageFlags &= ~(uint32_t)(TIM_USE_MOTOR | TIM_USE_SERVO | TIM_USE_BEEPER | TIM_USE_LED); + timer->usageFlags |= (uint32_t)TIM_USE_PINIO; + if (isCanonicalLedPad) { + timer->usageFlags &= ~(uint32_t)TIM_USE_PINIO; + timer->usageFlags |= (uint32_t)TIM_USE_LED; + } break; case OUTPUT_MODE_PINIO: - timer->usageFlags &= ~((uint32_t)(TIM_USE_MOTOR | TIM_USE_SERVO | TIM_USE_LED)); + timer->usageFlags &= ~(uint32_t)(TIM_USE_MOTOR | TIM_USE_SERVO | TIM_USE_LED | TIM_USE_BEEPER); + timer->usageFlags |= (uint32_t)TIM_USE_PINIO; + break; + case OUTPUT_MODE_BEEPER: + // Other channels of this timer share its period register, so they can't + // be repurposed as motor/servo/LED either. They can still drive a GPIO-only + // PINIO (binary on/off, no PWM), since that doesn't need the timer's period. + timer->usageFlags &= ~(uint32_t)(TIM_USE_MOTOR | TIM_USE_SERVO | TIM_USE_LED | TIM_USE_BEEPER); + timer->usageFlags |= (uint32_t)TIM_USE_PINIO; + if (isCanonicalBeeperPad) { + timer->usageFlags &= ~(uint32_t)TIM_USE_PINIO; + timer->usageFlags |= (uint32_t)TIM_USE_BEEPER; + } break; default: break; } } -/* ========================================================================= - * Helper — simulate what pwmAssignOutput(MAP_TO_SERVO_OUTPUT) does to flags. - * In real code: timHw->usageFlags &= TIM_USE_SERVO; - * We need this to show the full two-step corruption path. - * ========================================================================= */ -static void pwmAssignServo_stripFlags(timerHardware_t *timHw) +/* + * Helper — reproduces the canonical-pad-tracking loop in + * pwmBuildTimerOutputList(): scans pads in ascending array order and applies + * timerHardwareOverride_fixed() with isCanonicalBeeperPad / isCanonicalLedPad + * computed exactly as the real code does (first pad seen per physical timer, + * for a timer whose overridden outputMode matches, wins canonical status). + */ +static void applyFixedOverridesToArray(timerHardware_t *pads, int count, + const outputMode_e *outputModeByPad, + int maxTimId) { - timHw->usageFlags &= (uint32_t)TIM_USE_SERVO; + bool beeperClaimed[32] = { false }; + bool ledClaimed[32] = { false }; + ASSERT_LT(maxTimId, 32); + + for (int idx = 0; idx < count; idx++) { + timerHardware_t *timHw = &pads[idx]; + uint8_t timId = timHw->timId; + outputMode_e mode = outputModeByPad[idx]; + + bool isCanonicalBeeperPad = false; + if (mode == OUTPUT_MODE_BEEPER && !beeperClaimed[timId]) { + beeperClaimed[timId] = true; + isCanonicalBeeperPad = true; + } + bool isCanonicalLedPad = false; + if (mode == OUTPUT_MODE_LED && !ledClaimed[timId]) { + ledClaimed[timId] = true; + isCanonicalLedPad = true; + } + timerHardwareOverride_fixed(timHw, mode, isCanonicalBeeperPad, isCanonicalLedPad); + } } /* ========================================================================= - * TEST SUITE: BeeperTimerProtection + * TEST SUITE: BeeperTimerSiblingPads * ========================================================================= */ -class BeeperTimerProtectionTest : public ::testing::Test { -protected: - timerHardware_t timer; - - void SetUp() override { - /* Simulate MATEKH743 TIM2/PA15: beeper shares a servo-capable timer */ - timer.usageFlags = (uint32_t)(TIM_USE_BEEPER | TIM_USE_SERVO); - } -}; - /* * TEST 1 — BugReproduction * - * This test demonstrates the pre-fix bug. On the BUGGY code path: - * 1. timerHardwareOverride_buggy() applies OUTPUT_MODE_SERVOS, which does - * NOT clear TIM_USE_BEEPER → flags become TIM_USE_SERVO | TIM_USE_BEEPER. - * 2. pwmAssignOutput(servo) strips everything except TIM_USE_SERVO - * → TIM_USE_BEEPER is lost. - * - * The final assertion checks that TIM_USE_BEEPER was lost, which is the - * observable symptom of the bug. This test PASSES (i.e. the bug reproduces), - * which is what we want from the reproduction test. + * Two pads share timer id 5, both overridden to OUTPUT_MODE_BEEPER. On the + * BUGGY code path (no canonical-pad tracking at all), BOTH pads get + * TIM_USE_BEEPER, even though only one of them will ever actually be wired + * up by beeperPwmInit(). This test PASSES (i.e. the bug reproduces) on the + * buggy code — confirming the observable symptom of the bug that commit + * 8c16aeed85 fixed. */ -TEST_F(BeeperTimerProtectionTest, BugReproduction_BeeperFlagLostAfterOverride) +TEST(BeeperTimerSiblingPads, BugReproduction_BothSiblingsGetBeeperFlag) { - /* Step 1: buggy override — does not protect beeper */ - timerHardwareOverride_buggy(&timer, OUTPUT_MODE_SERVOS); - - /* After the buggy override: both SERVO and BEEPER flags should be set - * because OUTPUT_MODE_SERVOS only clears MOTOR and LED, not BEEPER. */ - EXPECT_TRUE(timer.usageFlags & (uint32_t)TIM_USE_SERVO) - << "SERVO flag should be set by the OUTPUT_MODE_SERVOS override"; - EXPECT_TRUE(timer.usageFlags & (uint32_t)TIM_USE_BEEPER) - << "BEEPER flag should still be present at this point (intermediate state)"; - - /* Step 2: simulate pwmAssignOutput(servo) which strips everything - * except TIM_USE_SERVO — this is the second half of the corruption. */ - pwmAssignServo_stripFlags(&timer); - - /* The bug: TIM_USE_BEEPER is now gone. beeperPwmInit() will fail silently. */ - EXPECT_FALSE(timer.usageFlags & (uint32_t)TIM_USE_BEEPER) - << "BUG CONFIRMED: TIM_USE_BEEPER was stripped by pwmAssignOutput " - "because timerHardwareOverride() did not protect it. " - "beeperPwmInit() would fail to find its timer."; + timerHardware_t padA; padA.usageFlags = 0; padA.timId = 5; + timerHardware_t padB; padB.usageFlags = 0; padB.timId = 5; + + timerHardwareOverride_buggy(&padA, OUTPUT_MODE_BEEPER); + timerHardwareOverride_buggy(&padB, OUTPUT_MODE_BEEPER); + + EXPECT_TRUE(padA.usageFlags & (uint32_t)TIM_USE_BEEPER) + << "First pad should get TIM_USE_BEEPER (expected)"; + EXPECT_TRUE(padB.usageFlags & (uint32_t)TIM_USE_BEEPER) + << "BUG CONFIRMED: sibling pad on the SAME timer also got TIM_USE_BEEPER, " + "but beeperPwmInit() only wires up the first pad found for the buzzer. " + "This sibling would be falsely reported as \"Buzzer\" over MSP/in the " + "configurator while being functionally dead (oscilloscope showed a real " + "waveform only on the first pad of the timer)."; } /* - * TEST 2 — FixVerification - * - * The fixed timerHardwareOverride() returns immediately when TIM_USE_BEEPER is - * set, so the servo override is a no-op for beeper timers. TIM_USE_BEEPER must - * survive, allowing beeperPwmInit() to find the timer later. + * TEST 2 — FixVerification (two sibling pads) * - * This test PASSES on the fixed code and would FAIL on the buggy code. + * Same scenario as above, but using the FIXED function with proper + * canonical-pad computation (as pwmBuildTimerOutputList() does). Only the + * first-scanned pad on the shared timer should get TIM_USE_BEEPER; the + * sibling must fall back to TIM_USE_PINIO (GPIO-only), NOT TIM_USE_BEEPER, + * and NOT be left flagless (commit 31bb99ea0b added the PINIO fallback). */ -TEST_F(BeeperTimerProtectionTest, FixVerification_BeeperFlagSurvivesOverride) +TEST(BeeperTimerSiblingPads, FixVerification_OnlyCanonicalPadGetsBeeper) { - const uint32_t originalFlags = timer.usageFlags; - - /* Fixed override: should be a no-op because TIM_USE_BEEPER is set */ - timerHardwareOverride_fixed(&timer, OUTPUT_MODE_SERVOS); - - /* Flags must be completely unchanged — the guard must have returned early. */ - EXPECT_EQ(timer.usageFlags, originalFlags) - << "FIX VERIFIED: timerHardwareOverride() must not modify a timer " - "that has TIM_USE_BEEPER set."; - - EXPECT_TRUE(timer.usageFlags & (uint32_t)TIM_USE_BEEPER) - << "TIM_USE_BEEPER must still be set after the override attempt"; + timerHardware_t pads[2]; + pads[0].usageFlags = 0; pads[0].timId = 5; + pads[1].usageFlags = 0; pads[1].timId = 5; + outputMode_e modes[2] = { OUTPUT_MODE_BEEPER, OUTPUT_MODE_BEEPER }; + + applyFixedOverridesToArray(pads, 2, modes, 6); + + EXPECT_TRUE(pads[0].usageFlags & (uint32_t)TIM_USE_BEEPER) + << "Canonical (first-scanned) pad must get TIM_USE_BEEPER"; + EXPECT_FALSE(pads[0].usageFlags & (uint32_t)TIM_USE_PINIO) + << "Canonical pad must not also carry the PINIO fallback flag"; + + EXPECT_FALSE(pads[1].usageFlags & (uint32_t)TIM_USE_BEEPER) + << "FIX VERIFIED: sibling pad on the same timer must NOT get TIM_USE_BEEPER"; + EXPECT_TRUE(pads[1].usageFlags & (uint32_t)TIM_USE_PINIO) + << "Sibling pad must fall back to TIM_USE_PINIO (GPIO-only), not be left flagless"; } /* - * TEST 3 — FixVerification for OUTPUT_MODE_MOTORS - * - * Confirm the beeper guard works for all output-mode variants, not just SERVOS. + * TEST 3 — Three-pad case: only the first scanned pad is canonical */ -TEST_F(BeeperTimerProtectionTest, FixVerification_BeeperProtectedFromMotorOverride) +TEST(BeeperTimerSiblingPads, ThreePads_OnlyFirstScannedIsCanonical) { - const uint32_t originalFlags = timer.usageFlags; + timerHardware_t pads[3]; + for (int i = 0; i < 3; i++) { + pads[i].usageFlags = 0; + pads[i].timId = 7; + } + outputMode_e modes[3] = { OUTPUT_MODE_BEEPER, OUTPUT_MODE_BEEPER, OUTPUT_MODE_BEEPER }; - timerHardwareOverride_fixed(&timer, OUTPUT_MODE_MOTORS); + applyFixedOverridesToArray(pads, 3, modes, 8); - EXPECT_EQ(timer.usageFlags, originalFlags) - << "OUTPUT_MODE_MOTORS must not modify a beeper timer"; - EXPECT_TRUE(timer.usageFlags & (uint32_t)TIM_USE_BEEPER); + EXPECT_TRUE(pads[0].usageFlags & (uint32_t)TIM_USE_BEEPER) + << "pads[0] (first scanned) must be canonical and get TIM_USE_BEEPER"; + EXPECT_FALSE(pads[0].usageFlags & (uint32_t)TIM_USE_PINIO); + + for (int i = 1; i < 3; i++) { + EXPECT_FALSE(pads[i].usageFlags & (uint32_t)TIM_USE_BEEPER) + << "pads[" << i << "] must NOT be canonical, so must not get TIM_USE_BEEPER"; + EXPECT_TRUE(pads[i].usageFlags & (uint32_t)TIM_USE_PINIO) + << "pads[" << i << "] must fall back to TIM_USE_PINIO"; + } } /* - * TEST 4 — FixVerification for OUTPUT_MODE_LED + * TEST 4 — Two independent BEEPER timers each get their own canonical pad */ -TEST_F(BeeperTimerProtectionTest, FixVerification_BeeperProtectedFromLedOverride) +TEST(BeeperTimerSiblingPads, IndependentTimers_EachGetsOwnCanonicalPad) { - const uint32_t originalFlags = timer.usageFlags; - - timerHardwareOverride_fixed(&timer, OUTPUT_MODE_LED); - - EXPECT_EQ(timer.usageFlags, originalFlags) - << "OUTPUT_MODE_LED must not modify a beeper timer"; - EXPECT_TRUE(timer.usageFlags & (uint32_t)TIM_USE_BEEPER); + timerHardware_t pads[4]; + pads[0].usageFlags = 0; pads[0].timId = 5; // timer A, pad 1 -> canonical + pads[1].usageFlags = 0; pads[1].timId = 5; // timer A, pad 2 -> sibling + pads[2].usageFlags = 0; pads[2].timId = 9; // timer B, pad 1 -> canonical + pads[3].usageFlags = 0; pads[3].timId = 9; // timer B, pad 2 -> sibling + outputMode_e modes[4] = { OUTPUT_MODE_BEEPER, OUTPUT_MODE_BEEPER, OUTPUT_MODE_BEEPER, OUTPUT_MODE_BEEPER }; + + applyFixedOverridesToArray(pads, 4, modes, 10); + + EXPECT_TRUE(pads[0].usageFlags & (uint32_t)TIM_USE_BEEPER); + EXPECT_TRUE(pads[1].usageFlags & (uint32_t)TIM_USE_PINIO); + EXPECT_FALSE(pads[1].usageFlags & (uint32_t)TIM_USE_BEEPER); + + EXPECT_TRUE(pads[2].usageFlags & (uint32_t)TIM_USE_BEEPER) + << "Timer B's first pad must independently become canonical"; + EXPECT_TRUE(pads[3].usageFlags & (uint32_t)TIM_USE_PINIO); + EXPECT_FALSE(pads[3].usageFlags & (uint32_t)TIM_USE_BEEPER); } /* - * TEST 5 — FixVerification for OUTPUT_MODE_PINIO + * TEST 5 — Edge case: a pure BEEPER-only timer (single pad) still gets + * TIM_USE_BEEPER — the canonical-pad logic must not accidentally suppress + * the common single-pad case. */ -TEST_F(BeeperTimerProtectionTest, FixVerification_BeeperProtectedFromPinioOverride) +TEST(BeeperTimerSiblingPads, SinglePadTimer_StillGetsBeeperFlag) { - const uint32_t originalFlags = timer.usageFlags; + timerHardware_t pads[1]; + pads[0].usageFlags = 0; + pads[0].timId = 2; + outputMode_e modes[1] = { OUTPUT_MODE_BEEPER }; - timerHardwareOverride_fixed(&timer, OUTPUT_MODE_PINIO); + applyFixedOverridesToArray(pads, 1, modes, 3); - EXPECT_EQ(timer.usageFlags, originalFlags) - << "OUTPUT_MODE_PINIO must not modify a beeper timer"; - EXPECT_TRUE(timer.usageFlags & (uint32_t)TIM_USE_BEEPER); + EXPECT_TRUE(pads[0].usageFlags & (uint32_t)TIM_USE_BEEPER); + EXPECT_FALSE(pads[0].usageFlags & (uint32_t)TIM_USE_PINIO); } /* ========================================================================= - * Negative / regression tests — normal (non-beeper) timers must still be - * overridden correctly. The guard must NOT prevent normal overrides. + * Negative / regression tests — MOTORS/SERVOS/LED/PINIO overrides must still + * work correctly with the fixed function, including when mixed with a + * BEEPER-mode timer in the same scan. * ========================================================================= */ -class NormalTimerOverrideTest : public ::testing::Test { -protected: +TEST(BeeperTimerRegression, MotorOverride_Unaffected) +{ timerHardware_t timer; + timer.usageFlags = (uint32_t)(TIM_USE_MOTOR | TIM_USE_SERVO); + timer.timId = 1; - /* No TIM_USE_BEEPER: both motor and servo flags set (auto-mode timer). */ - void SetUp() override { - timer.usageFlags = (uint32_t)(TIM_USE_MOTOR | TIM_USE_SERVO); - } -}; + timerHardwareOverride_fixed(&timer, OUTPUT_MODE_MOTORS, + /*isCanonicalBeeperPad=*/false, + /*isCanonicalLedPad=*/false); + + EXPECT_TRUE(timer.usageFlags & (uint32_t)TIM_USE_MOTOR); + EXPECT_FALSE(timer.usageFlags & (uint32_t)TIM_USE_SERVO); + EXPECT_FALSE(timer.usageFlags & (uint32_t)TIM_USE_LED); + EXPECT_FALSE(timer.usageFlags & (uint32_t)TIM_USE_BEEPER); +} -TEST_F(NormalTimerOverrideTest, MotorOverride_SetsMotorClearsServoAndLed) +TEST(BeeperTimerRegression, ServoOverride_Unaffected) { - timerHardwareOverride_fixed(&timer, OUTPUT_MODE_MOTORS); - - EXPECT_TRUE(timer.usageFlags & (uint32_t)TIM_USE_MOTOR) - << "TIM_USE_MOTOR must be set after OUTPUT_MODE_MOTORS override"; - EXPECT_FALSE(timer.usageFlags & (uint32_t)TIM_USE_SERVO) - << "TIM_USE_SERVO must be cleared by OUTPUT_MODE_MOTORS override"; - EXPECT_FALSE(timer.usageFlags & (uint32_t)TIM_USE_LED) - << "TIM_USE_LED must be cleared by OUTPUT_MODE_MOTORS override"; - EXPECT_FALSE(timer.usageFlags & (uint32_t)TIM_USE_BEEPER) - << "TIM_USE_BEEPER must remain clear on a non-beeper timer"; + timerHardware_t timer; + timer.usageFlags = (uint32_t)(TIM_USE_MOTOR | TIM_USE_SERVO); + timer.timId = 2; + + timerHardwareOverride_fixed(&timer, OUTPUT_MODE_SERVOS, + /*isCanonicalBeeperPad=*/false, + /*isCanonicalLedPad=*/false); + + EXPECT_TRUE(timer.usageFlags & (uint32_t)TIM_USE_SERVO); + EXPECT_FALSE(timer.usageFlags & (uint32_t)TIM_USE_MOTOR); + EXPECT_FALSE(timer.usageFlags & (uint32_t)TIM_USE_BEEPER); } -TEST_F(NormalTimerOverrideTest, ServoOverride_SetsServoClearsMotorAndLed) +TEST(BeeperTimerRegression, LedCanonicalPad_StillGetsLedFlag) { - timerHardwareOverride_fixed(&timer, OUTPUT_MODE_SERVOS); - - EXPECT_TRUE(timer.usageFlags & (uint32_t)TIM_USE_SERVO) - << "TIM_USE_SERVO must be set after OUTPUT_MODE_SERVOS override"; - EXPECT_FALSE(timer.usageFlags & (uint32_t)TIM_USE_MOTOR) - << "TIM_USE_MOTOR must be cleared by OUTPUT_MODE_SERVOS override"; - EXPECT_FALSE(timer.usageFlags & (uint32_t)TIM_USE_LED) - << "TIM_USE_LED must be cleared by OUTPUT_MODE_SERVOS override"; + /* Regression: the LED canonical-pad logic (isCanonicalLedPad) must be + * completely unaffected by the BEEPER-specific test coverage here. */ + timerHardware_t timer; + timer.usageFlags = 0; + timer.timId = 3; + + timerHardwareOverride_fixed(&timer, OUTPUT_MODE_LED, + /*isCanonicalBeeperPad=*/false, + /*isCanonicalLedPad=*/true); + + EXPECT_TRUE(timer.usageFlags & (uint32_t)TIM_USE_LED); + EXPECT_FALSE(timer.usageFlags & (uint32_t)TIM_USE_PINIO); } -TEST_F(NormalTimerOverrideTest, LedOverride_SetsLedClearsMotorAndServo) +TEST(BeeperTimerRegression, LedSiblingPad_FallsBackToPinio) { - timerHardwareOverride_fixed(&timer, OUTPUT_MODE_LED); - - EXPECT_TRUE(timer.usageFlags & (uint32_t)TIM_USE_LED) - << "TIM_USE_LED must be set after OUTPUT_MODE_LED override"; - EXPECT_FALSE(timer.usageFlags & (uint32_t)TIM_USE_MOTOR) - << "TIM_USE_MOTOR must be cleared by OUTPUT_MODE_LED override"; - EXPECT_FALSE(timer.usageFlags & (uint32_t)TIM_USE_SERVO) - << "TIM_USE_SERVO must be cleared by OUTPUT_MODE_LED override"; + timerHardware_t timer; + timer.usageFlags = 0; + timer.timId = 3; + + timerHardwareOverride_fixed(&timer, OUTPUT_MODE_LED, + /*isCanonicalBeeperPad=*/false, + /*isCanonicalLedPad=*/false); + + EXPECT_FALSE(timer.usageFlags & (uint32_t)TIM_USE_LED); + EXPECT_TRUE(timer.usageFlags & (uint32_t)TIM_USE_PINIO) + << "Non-canonical LED pad must fall back to PINIO"; } -TEST_F(NormalTimerOverrideTest, PinioOverride_ClearsAllOutputFlags) +TEST(BeeperTimerRegression, PinioOverride_Unaffected) { + timerHardware_t timer; timer.usageFlags = (uint32_t)(TIM_USE_MOTOR | TIM_USE_SERVO | TIM_USE_LED); + timer.timId = 4; - timerHardwareOverride_fixed(&timer, OUTPUT_MODE_PINIO); + timerHardwareOverride_fixed(&timer, OUTPUT_MODE_PINIO, + /*isCanonicalBeeperPad=*/false, + /*isCanonicalLedPad=*/false); - EXPECT_FALSE(timer.usageFlags & (uint32_t)TIM_USE_MOTOR) - << "TIM_USE_MOTOR must be cleared by OUTPUT_MODE_PINIO override"; - EXPECT_FALSE(timer.usageFlags & (uint32_t)TIM_USE_SERVO) - << "TIM_USE_SERVO must be cleared by OUTPUT_MODE_PINIO override"; - EXPECT_FALSE(timer.usageFlags & (uint32_t)TIM_USE_LED) - << "TIM_USE_LED must be cleared by OUTPUT_MODE_PINIO override"; + EXPECT_FALSE(timer.usageFlags & (uint32_t)TIM_USE_MOTOR); + EXPECT_FALSE(timer.usageFlags & (uint32_t)TIM_USE_SERVO); + EXPECT_FALSE(timer.usageFlags & (uint32_t)TIM_USE_LED); + EXPECT_TRUE(timer.usageFlags & (uint32_t)TIM_USE_PINIO); } -TEST_F(NormalTimerOverrideTest, AutoMode_LeavesTimerUnchanged) +TEST(BeeperTimerRegression, AutoMode_LeavesTimerUnchanged) { + timerHardware_t timer; + timer.usageFlags = (uint32_t)(TIM_USE_MOTOR | TIM_USE_SERVO); + timer.timId = 6; const uint32_t originalFlags = timer.usageFlags; - timerHardwareOverride_fixed(&timer, OUTPUT_MODE_AUTO); + timerHardwareOverride_fixed(&timer, OUTPUT_MODE_AUTO, + /*isCanonicalBeeperPad=*/false, + /*isCanonicalLedPad=*/false); EXPECT_EQ(timer.usageFlags, originalFlags) << "OUTPUT_MODE_AUTO (default case) must not change flags"; } +/* + * TEST — Mixed scan: MOTOR timer, BEEPER timer (2 pads), LED timer + * (canonical pad only) all processed together via + * applyFixedOverridesToArray(), matching a realistic + * pwmBuildTimerOutputList() scenario where different timers on the board + * have different output-mode overrides. Confirms none of the per-timer + * canonical tracking (beeperClaimed[] vs ledClaimed[]) interferes across + * timer ids or across output modes — the mirror image of the equivalent test + * in pwm_mapping_led_unittest.cc. + */ +TEST(BeeperTimerRegression, MixedScan_MotorBeeperLedTimersDoNotInterfere) +{ + timerHardware_t pads[5]; + for (int i = 0; i < 5; i++) pads[i].usageFlags = 0; + + pads[0].timId = 1; // MOTOR timer + pads[1].timId = 2; // BEEPER timer, canonical + pads[2].timId = 2; // BEEPER timer, sibling + pads[3].timId = 3; // LED timer, canonical + pads[4].timId = 3; // LED timer, sibling + + outputMode_e modes[5] = { + OUTPUT_MODE_MOTORS, + OUTPUT_MODE_BEEPER, + OUTPUT_MODE_BEEPER, + OUTPUT_MODE_LED, + OUTPUT_MODE_LED, + }; + + applyFixedOverridesToArray(pads, 5, modes, 4); + + EXPECT_TRUE(pads[0].usageFlags & (uint32_t)TIM_USE_MOTOR); + + EXPECT_TRUE(pads[1].usageFlags & (uint32_t)TIM_USE_BEEPER); + EXPECT_FALSE(pads[1].usageFlags & (uint32_t)TIM_USE_PINIO); + + EXPECT_FALSE(pads[2].usageFlags & (uint32_t)TIM_USE_BEEPER); + EXPECT_TRUE(pads[2].usageFlags & (uint32_t)TIM_USE_PINIO); + + EXPECT_TRUE(pads[3].usageFlags & (uint32_t)TIM_USE_LED); + EXPECT_FALSE(pads[3].usageFlags & (uint32_t)TIM_USE_PINIO); + + EXPECT_FALSE(pads[4].usageFlags & (uint32_t)TIM_USE_LED); + EXPECT_TRUE(pads[4].usageFlags & (uint32_t)TIM_USE_PINIO); +} + /* ========================================================================= - * Edge case: timer has ONLY TIM_USE_BEEPER (no motor/servo capability) + * TEST SUITE: SourceSync + * + * Reads the ACTUAL LIVE src/main/drivers/pwm_mapping.c off disk (not a + * cached copy, not something computed at CMake-configure time) and checks + * that specific, logic-relevant fragments of the real timerHardwareOverride() + * and pwmBuildTimerOutputList() are still present verbatim (modulo + * whitespace and comments). If a future change to pwm_mapping.c alters this + * logic without updating the inline reproduction above, these tests fail — + * loudly, in `make check` / CI — instead of the rest of the file silently + * continuing to pass against a stale copy, which is exactly what happened to + * this file for months after commits 8c16aeed85/31bb99ea0b. + * + * This does not (and structurally cannot, given the SITL_BUILD guard around + * the entire file — see block comment above) substitute for actually linking + * and executing the real function. It is a drift-detector, not a full + * integration test. Comments are stripped and whitespace is collapsed before + * comparison so that pure reformatting/typo-fixes in comments do not cause + * false failures; only the executable-statement tokens are checked. * ========================================================================= */ -TEST(BeeperOnlyTimer, OverrideIgnoredForPureBeeper) +namespace { + +// Strips "//...\n" line comments. Deliberately does not handle /* */ block +// comments or comments inside string/char literals — pwm_mapping.c does not +// use either within the functions we check, and keeping this simple avoids +// this drift-detector itself becoming a source of drift. +std::string stripLineComments(const std::string &text) { - timerHardware_t timer; - timer.usageFlags = (uint32_t)TIM_USE_BEEPER; + std::string result; + result.reserve(text.size()); + for (size_t i = 0; i < text.size();) { + if (text[i] == '/' && i + 1 < text.size() && text[i + 1] == '/') { + while (i < text.size() && text[i] != '\n') { + i++; + } + continue; + } + result += text[i]; + i++; + } + return result; +} - const uint32_t originalFlags = timer.usageFlags; +// Collapses any run of whitespace (including newlines) to a single space, +// and trims leading/trailing whitespace, so indentation/line-wrap changes +// don't cause false failures. +std::string normalizeWhitespace(const std::string &text) +{ + std::string result; + result.reserve(text.size()); + bool lastWasSpace = true; // trims leading whitespace + for (char c : text) { + if (std::isspace(static_cast(c))) { + if (!lastWasSpace) { + result += ' '; + lastWasSpace = true; + } + } else { + result += c; + lastWasSpace = false; + } + } + while (!result.empty() && result.back() == ' ') { + result.pop_back(); + } + return result; +} - /* All override modes must be no-ops for a pure beeper timer. */ - timerHardwareOverride_fixed(&timer, OUTPUT_MODE_MOTORS); - EXPECT_EQ(timer.usageFlags, originalFlags); +std::string normalizeSource(const std::string &text) +{ + return normalizeWhitespace(stripLineComments(text)); +} - timerHardwareOverride_fixed(&timer, OUTPUT_MODE_SERVOS); - EXPECT_EQ(timer.usageFlags, originalFlags); +// 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."; + } + std::ostringstream buffer; + buffer << file.rdbuf(); + if (buffer.str().empty()) { + return ::testing::AssertionFailure() + << "Live source file at " << path << " opened but read as empty."; + } + *out = normalizeSource(buffer.str()); + return ::testing::AssertionSuccess(); +} - timerHardwareOverride_fixed(&timer, OUTPUT_MODE_PINIO); - EXPECT_EQ(timer.usageFlags, originalFlags); +} // namespace + +TEST(SourceSync, TimerHardwareOverrideSignatureMatchesLiveSource) +{ + std::string normalizedSource; + ASSERT_TRUE(loadNormalizedPwmMappingSource(&normalizedSource)); + + const std::string expectedSignature = normalizeSource( + "static void timerHardwareOverride(timerHardware_t * timer, " + "bool isCanonicalBeeperPad, bool isCanonicalLedPad) {"); + + EXPECT_NE(normalizedSource.find(expectedSignature), std::string::npos) + << "timerHardwareOverride()'s signature in the LIVE " + "src/main/drivers/pwm_mapping.c no longer matches what this test's " + "inline reproduction assumes. If the signature legitimately " + "changed (e.g. the step-2 guard-based approach was reinstated, or " + "a new parameter was added), update timerHardwareOverride_fixed()/" + "_buggy() in this file (and pwm_mapping_led_unittest.cc) to match, " + "then update this expected string too. THIS is the exact class of " + "drift that made this file stale for months previously — do not " + "let it happen again."; +} + +TEST(SourceSync, BeeperCaseMatchesLiveSource) +{ + std::string normalizedSource; + ASSERT_TRUE(loadNormalizedPwmMappingSource(&normalizedSource)); + + // Comments intentionally omitted here (stripLineComments() removes them + // from the live source before comparison too) so that comment wording + // changes don't cause false failures; only the executable statements are + // checked, which is exactly the logic the FixVerification tests above + // depend on. + const std::string expectedBeeperCase = normalizeSource( + "case OUTPUT_MODE_BEEPER: " + "timer->usageFlags &= ~(TIM_USE_MOTOR|TIM_USE_SERVO|TIM_USE_LED|TIM_USE_BEEPER); " + "timer->usageFlags |= TIM_USE_PINIO; " + "if (isCanonicalBeeperPad) { " + "timer->usageFlags &= ~TIM_USE_PINIO; " + "timer->usageFlags |= TIM_USE_BEEPER; " + "} " + "break;"); + + EXPECT_NE(normalizedSource.find(expectedBeeperCase), std::string::npos) + << "The OUTPUT_MODE_BEEPER case in the LIVE " + "src/main/drivers/pwm_mapping.c no longer matches " + "timerHardwareOverride_fixed()'s BEEPER case in this test file. " + "Update the inline reproduction (and the FixVerification/" + "BugReproduction tests, if the fix strategy itself changed) to " + "match the live source."; +} + +TEST(SourceSync, CanonicalPadTrackingLoopMatchesLiveSource) +{ + std::string normalizedSource; + ASSERT_TRUE(loadNormalizedPwmMappingSource(&normalizedSource)); + + const std::string expectedLoop = normalizeSource( + "bool beeperClaimed[HARDWARE_TIMER_DEFINITION_COUNT] = { false }; " + "bool ledClaimed[HARDWARE_TIMER_DEFINITION_COUNT] = { false }; " + "for (int idx = 0; idx < timerHardwareCount; idx++) { " + "timerHardware_t *timHw = &timerHardware[idx]; " + "uint8_t timId = timer2id(timHw->tim); " + "bool isCanonicalBeeperPad = false; " + "if (timerOverrides(timId)->outputMode == OUTPUT_MODE_BEEPER && !beeperClaimed[timId]) { " + "beeperClaimed[timId] = true; " + "isCanonicalBeeperPad = true; " + "} " + "bool isCanonicalLedPad = false; " + "if (timerOverrides(timId)->outputMode == OUTPUT_MODE_LED && !ledClaimed[timId]) { " + "ledClaimed[timId] = true; " + "isCanonicalLedPad = true; " + "}"); + + EXPECT_NE(normalizedSource.find(expectedLoop), std::string::npos) + << "pwmBuildTimerOutputList()'s canonical-pad-tracking loop in the LIVE " + "src/main/drivers/pwm_mapping.c no longer matches what " + "applyFixedOverridesToArray() in this test file reproduces. Update " + "applyFixedOverridesToArray() to match the live source (e.g. if the " + "beeperClaimed[]/ledClaimed[] scanning strategy changes)."; } diff --git a/src/test/unit/pwm_mapping_led_e2e_unittest.cc b/src/test/unit/pwm_mapping_led_e2e_unittest.cc new file mode 100644 index 00000000000..56f981ccfac --- /dev/null +++ b/src/test/unit/pwm_mapping_led_e2e_unittest.cc @@ -0,0 +1,789 @@ +/* + * End-to-end unit test: pwmBuildTimerOutputList()'s FULL two-loop pipeline + * (canonical-pad override loop + priority-ordered assignment loop) must not + * let a sibling pad on a shared LED timer end up flagged TIM_USE_LED. + * + * This is a companion to pwm_mapping_led_unittest.cc, which only exercises + * timerHardwareOverride() in isolation (the first loop). That is NOT enough + * to catch the real bug found via hardware bootlog tracing on a SPEDIXF405 + * (TIM4, S5+S6 shared): timerHardwareOverride() correctly set + * S5=TIM_USE_LED, S6=TIM_USE_PINIO after the first (canonical-claim) loop, + * but pwmAssignOutput()'s MAP_TO_LED_OUTPUT case, run later in the SECOND + * (assignment) loop, used to call pwmClaimTimer(timHw->tim, timHw->usageFlags) + * after assigning the canonical pad. pwmClaimTimer() propagates the + * just-assigned pad's usageFlags to EVERY sibling pad sharing the same + * physical timer — correct for MOTOR/SERVO (a multi-channel motor timer + * really should offer multiple motor outputs), but wrong for LED, since it + * silently overwrote the sibling's correct TIM_USE_PINIO back to + * TIM_USE_LED. This reintroduced the "sibling pad falsely reported as LED" + * bug via a different, later code path than the one originally fixed in + * timerHardwareOverride(). + * + * Fix (already applied to src/main/drivers/pwm_mapping.c, NOT modified by + * this test file): pwmAssignOutput()'s MAP_TO_LED_OUTPUT case no longer + * calls pwmClaimTimer() — it only masks timHw->usageFlags &= TIM_USE_LED. + * The MAP_TO_MOTOR_OUTPUT / MAP_TO_SERVO_OUTPUT cases are UNCHANGED and + * still call pwmClaimTimer() to propagate flags to sibling pads. + * + * This file contains: + * 1. Inline, faithful reproductions of ALL THREE pieces working together, + * end to end: + * - timerHardwareOverride() (fixed version, with isCanonicalBeeperPad + * / isCanonicalLedPad canonical-pad tracking) + * - pwmClaimTimer() (exact real logic: propagate-usageFlags-to- + * siblings-sharing-a-timer) + * - pwmAssignOutput() in BOTH its FIXED form (current source: LED + * case does NOT call pwmClaimTimer) and its BUGGY/pre-fix form (LED + * case DOES call pwmClaimTimer, exactly like MOTOR/SERVO), so the + * test can demonstrate the bug reproduces against the buggy form + * and is fixed against the current form. + * pwm_mapping.c is excluded from the SITL/host unit-test build (guarded + * by `#if !defined(SITL_BUILD)`), and depends on PG (parameter-group) + * infrastructure not available in a host unit build, so it cannot be + * linked directly — same constraint as pwm_mapping_led_unittest.cc and + * pwm_mapping_beeper_unittest.cc. + * 2. A minimal simulateBuildTimerOutputList() that reproduces + * pwmBuildTimerOutputList()'s two-loop structure: the canonical-claim + * loop, then the priority-ordered (dedicated pass, then auto pass) + * assignment loop, with checkPwmTimerConflicts() stubbed to always + * return false (matching the real hardware trace where + * FEATURE_LED_STRIP was off). + * 3. TEST SUITE LedSiblingE2E — the primary regression test. Two pads + * share one physical timer set to LED mode. After running the FULL + * simulated pipeline (both loops): + * - with the BUGGY pwmAssignOutput (pre-fix): the sibling pad ends up + * incorrectly flagged TIM_USE_LED too (bug reproduction). + * - with the FIXED pwmAssignOutput (current source): the canonical + * pad keeps TIM_USE_LED and the sibling correctly falls back to + * TIM_USE_PINIO, NOT TIM_USE_LED. + * 4. TEST SUITE MotorSiblingPropagationRegression — the inverse/regression + * guard: pwmClaimTimer()'s propagation-to-siblings behavior must still + * happen for MAP_TO_MOTOR_OUTPUT (this fix must not have been + * accidentally broadened to remove propagation for motor/servo too). + * 5. TEST SUITE SourceSync — reads the ACTUAL LIVE + * src/main/drivers/pwm_mapping.c off disk at test time and asserts that + * pwmAssignOutput()'s MAP_TO_LED_OUTPUT case does NOT contain a + * pwmClaimTimer() call, while MAP_TO_MOTOR_OUTPUT / MAP_TO_SERVO_OUTPUT + * still do. This exists because pwm_mapping_beeper_unittest.cc was + * previously found to have drifted silently for months after upstream + * logic changed (see pwm_mapping_led_unittest.cc's SourceSync suite) — + * an inline reproduction is unavoidable here, but it must not be + * allowed to silently go stale. + */ + +#include +#include +#include +#include +#include +#include +#include + +#include "gtest/gtest.h" +#include "unittest_macros.h" + +/* ------------------------------------------------------------------------- + * Minimal type/flag definitions — mirrors the real firmware headers on this + * branch (src/main/drivers/timer.h, src/main/flight/mixer.h, + * src/main/drivers/pwm_mapping.c's internal MAP_TO_* enum). + * ------------------------------------------------------------------------- */ + +typedef enum { + TIM_USE_ANY = 0, + TIM_USE_PPM = (1 << 0), + TIM_USE_PWM = (1 << 1), + TIM_USE_MOTOR = (1 << 2), + TIM_USE_SERVO = (1 << 3), + TIM_USE_LED = (1 << 24), + TIM_USE_BEEPER = (1 << 25), + TIM_USE_PINIO = (1 << 26), +} timerUsageFlag_e; + +#define TIM_IS_MOTOR(flags) ((flags) & (uint32_t)TIM_USE_MOTOR) +#define TIM_IS_SERVO(flags) ((flags) & (uint32_t)TIM_USE_SERVO) +#define TIM_IS_LED(flags) ((flags) & (uint32_t)TIM_USE_LED) + +typedef enum { + OUTPUT_MODE_AUTO = 0, + OUTPUT_MODE_MOTORS, + OUTPUT_MODE_SERVOS, + OUTPUT_MODE_LED, + OUTPUT_MODE_PINIO, + OUTPUT_MODE_BEEPER +} outputMode_e; + +/* Mirrors the anonymous enum at the top of pwm_mapping.c */ +enum { + MAP_TO_NONE, + MAP_TO_MOTOR_OUTPUT, + MAP_TO_SERVO_OUTPUT, + MAP_TO_LED_OUTPUT +}; + +/* Minimal timer hardware entry. `timId` stands in for the real code's + * timer2id(timer->tim) / the `tim` pointer identity itself — in the real + * driver, multiple timerHardware[] pads sharing the same physical timer + * compare equal on timer2id(tim); here we just store the id directly and use + * it as the "same physical timer" key everywhere the real code would compare + * `->tim` pointers (including inside the simulated pwmClaimTimer()). */ +typedef struct timerHardware_s { + uint32_t usageFlags; + uint8_t timId; +} timerHardware_t; + +static const int MAX_PADS = 16; +static const int MAX_TIMID = 16; + +/* ------------------------------------------------------------------------- + * Piece 1: timerHardwareOverride() — literal translation of the current + * (fixed) src/main/drivers/pwm_mapping.c. Identical to the copy in + * pwm_mapping_led_unittest.cc / pwm_mapping_beeper_unittest.cc; duplicated + * here because each *_unittest.cc becomes its own standalone executable + * (see src/test/unit/CMakeLists.txt's `file(GLOB TEST_PROGRAMS *_unittest.cc)` + * + one add_executable() per file), so statics cannot be shared across files. + * ------------------------------------------------------------------------- */ +static void timerHardwareOverride_fixed(timerHardware_t *timer, + outputMode_e outputMode, + bool isCanonicalBeeperPad, + bool isCanonicalLedPad) +{ + switch (outputMode) { + case OUTPUT_MODE_MOTORS: + timer->usageFlags &= ~(uint32_t)(TIM_USE_SERVO | TIM_USE_LED | TIM_USE_BEEPER); + timer->usageFlags |= (uint32_t)TIM_USE_MOTOR; + break; + case OUTPUT_MODE_SERVOS: + timer->usageFlags &= ~(uint32_t)(TIM_USE_MOTOR | TIM_USE_LED | TIM_USE_BEEPER); + timer->usageFlags |= (uint32_t)TIM_USE_SERVO; + break; + case OUTPUT_MODE_LED: + timer->usageFlags &= ~(uint32_t)(TIM_USE_MOTOR | TIM_USE_SERVO | TIM_USE_BEEPER | TIM_USE_LED); + timer->usageFlags |= (uint32_t)TIM_USE_PINIO; + if (isCanonicalLedPad) { + timer->usageFlags &= ~(uint32_t)TIM_USE_PINIO; + timer->usageFlags |= (uint32_t)TIM_USE_LED; + } + break; + case OUTPUT_MODE_PINIO: + timer->usageFlags &= ~(uint32_t)(TIM_USE_MOTOR | TIM_USE_SERVO | TIM_USE_LED | TIM_USE_BEEPER); + timer->usageFlags |= (uint32_t)TIM_USE_PINIO; + break; + case OUTPUT_MODE_BEEPER: + timer->usageFlags &= ~(uint32_t)(TIM_USE_MOTOR | TIM_USE_SERVO | TIM_USE_LED | TIM_USE_BEEPER); + timer->usageFlags |= (uint32_t)TIM_USE_PINIO; + if (isCanonicalBeeperPad) { + timer->usageFlags &= ~(uint32_t)TIM_USE_PINIO; + timer->usageFlags |= (uint32_t)TIM_USE_BEEPER; + } + break; + default: + break; + } +} + +/* ------------------------------------------------------------------------- + * Piece 2: pwmClaimTimer() — exact real logic (propagate usageFlags to every + * pad sharing the same physical timer). Real signature is + * pwmClaimTimer(HAL_Timer_t *tim, uint32_t usageFlags) and it iterates the + * GLOBAL timerHardware[]/timerHardwareCount; here `pads`/`count` stand in for + * that global array, passed explicitly since this is a host unit test with + * no global timer table. + * ------------------------------------------------------------------------- */ +static uint8_t pwmClaimTimer_sim(timerHardware_t *pads, int count, uint8_t timId, uint32_t usageFlags) +{ + uint8_t changed = 0; + for (int idx = 0; idx < count; idx++) { + timerHardware_t *timHw = &pads[idx]; + if (timHw->timId == timId && timHw->usageFlags != usageFlags) { + timHw->usageFlags = usageFlags; + changed++; + } + } + return changed; +} + +/* Output-collection bookkeeping — stands in for timMotorServoHardware_t's + * timMotors[]/timServos[] arrays (pointers into timerHardware[] in the real + * code; here indices into `pads[]`). Note: the real timOutputs struct does + * NOT track LED pads at all — the LED strip driver finds its pad later via + * timerGetByUsageFlag(TIM_USE_LED) — so we don't track LED assignments here + * either; LED "assignment" is verified purely via the pad's final usageFlags, + * matching real-world observability. */ +struct SimOutputs { + int motorPadIdx[MAX_PADS]; + int motorCount; + int servoPadIdx[MAX_PADS]; + int servoCount; +}; + +static bool pwmHasMotorOnTimer_sim(const timerHardware_t *pads, const SimOutputs *outputs, uint8_t timId) +{ + for (int i = 0; i < outputs->motorCount; i++) { + if (pads[outputs->motorPadIdx[i]].timId == timId) { + return true; + } + } + return false; +} + +static bool pwmHasServoOnTimer_sim(const timerHardware_t *pads, const SimOutputs *outputs, uint8_t timId) +{ + for (int i = 0; i < outputs->servoCount; i++) { + if (pads[outputs->servoPadIdx[i]].timId == timId) { + return true; + } + } + return false; +} + +/* ------------------------------------------------------------------------- + * Piece 3: pwmAssignOutput() — provided in TWO forms: + * + * pwmAssignOutput_fixed() — literal translation of the CURRENT (fixed) + * src/main/drivers/pwm_mapping.c: MOTOR/SERVO + * cases call pwmClaimTimer_sim() to propagate + * flags to sibling pads; the LED case does NOT. + * + * pwmAssignOutput_buggy() — reproduces the PRE-FIX behavior: identical + * except the LED case ALSO calls + * pwmClaimTimer_sim(), exactly like MOTOR/SERVO + * used to (and still do). This is the exact bug + * found via hardware bootlog tracing. + * ------------------------------------------------------------------------- */ +static void pwmAssignOutput_fixed(SimOutputs *outputs, timerHardware_t *pads, int count, + timerHardware_t *timHw, int type) +{ + int padIdx = (int)(timHw - pads); + switch (type) { + case MAP_TO_MOTOR_OUTPUT: + timHw->usageFlags &= (uint32_t)TIM_USE_MOTOR; + outputs->motorPadIdx[outputs->motorCount++] = padIdx; + pwmClaimTimer_sim(pads, count, timHw->timId, timHw->usageFlags); + break; + case MAP_TO_SERVO_OUTPUT: + timHw->usageFlags &= (uint32_t)TIM_USE_SERVO; + outputs->servoPadIdx[outputs->servoCount++] = padIdx; + pwmClaimTimer_sim(pads, count, timHw->timId, timHw->usageFlags); + break; + case MAP_TO_LED_OUTPUT: + // FIX: no pwmClaimTimer_sim() call here — see file header comment. + timHw->usageFlags &= (uint32_t)TIM_USE_LED; + break; + default: + break; + } +} + +static void pwmAssignOutput_buggy(SimOutputs *outputs, timerHardware_t *pads, int count, + timerHardware_t *timHw, int type) +{ + int padIdx = (int)(timHw - pads); + switch (type) { + case MAP_TO_MOTOR_OUTPUT: + timHw->usageFlags &= (uint32_t)TIM_USE_MOTOR; + outputs->motorPadIdx[outputs->motorCount++] = padIdx; + pwmClaimTimer_sim(pads, count, timHw->timId, timHw->usageFlags); + break; + case MAP_TO_SERVO_OUTPUT: + timHw->usageFlags &= (uint32_t)TIM_USE_SERVO; + outputs->servoPadIdx[outputs->servoCount++] = padIdx; + pwmClaimTimer_sim(pads, count, timHw->timId, timHw->usageFlags); + break; + case MAP_TO_LED_OUTPUT: + // BUG (pre-fix): propagates this pad's masked flags to every + // sibling sharing the timer, overwriting a correctly-computed + // TIM_USE_PINIO sibling back to TIM_USE_LED. + timHw->usageFlags &= (uint32_t)TIM_USE_LED; + pwmClaimTimer_sim(pads, count, timHw->timId, timHw->usageFlags); + break; + default: + break; + } +} + +typedef void (*pwmAssignOutputFn)(SimOutputs *, timerHardware_t *, int, timerHardware_t *, int); + +/* checkPwmTimerConflicts() stub — the task scenario matches the real + * hardware trace where FEATURE_LED_STRIP was off, so this always returns + * false (never skip a pad due to a conflicting port/feature). */ +static bool checkPwmTimerConflicts_stub(const timerHardware_t * /*timHw*/) +{ + return false; +} + +/* ------------------------------------------------------------------------- + * Piece 4: simulateBuildTimerOutputList() — minimal reproduction of + * pwmBuildTimerOutputList()'s two-loop structure: + * Loop 1: canonical-pad tracking (beeperClaimed[]/ledClaimed[]) + + * timerHardwareOverride() for every pad. + * Loop 2: priority-ordered assignment (priority 0 = dedicated pass, + * priority 1 = auto pass), motor branch then servo branch then + * LED branch (auto-pass only), exactly mirroring the real + * function's branch order and conditions. + * `assignFn` selects pwmAssignOutput_fixed or pwmAssignOutput_buggy. + * ------------------------------------------------------------------------- */ +static void simulateBuildTimerOutputList(timerHardware_t *pads, int count, + const outputMode_e *modeByTimId, + int motorCount, int servoCount, + pwmAssignOutputFn assignFn, + SimOutputs *outputs) +{ + outputs->motorCount = 0; + outputs->servoCount = 0; + + // Loop 1: apply all timerOverrides upfront (canonical-claim tracking). + bool beeperClaimed[MAX_TIMID] = { false }; + bool ledClaimed[MAX_TIMID] = { false }; + for (int idx = 0; idx < count; idx++) { + timerHardware_t *timHw = &pads[idx]; + uint8_t timId = timHw->timId; + ASSERT_LT(timId, MAX_TIMID); + outputMode_e mode = modeByTimId[timId]; + + bool isCanonicalBeeperPad = false; + if (mode == OUTPUT_MODE_BEEPER && !beeperClaimed[timId]) { + beeperClaimed[timId] = true; + isCanonicalBeeperPad = true; + } + bool isCanonicalLedPad = false; + if (mode == OUTPUT_MODE_LED && !ledClaimed[timId]) { + ledClaimed[timId] = true; + isCanonicalLedPad = true; + } + timerHardwareOverride_fixed(timHw, mode, isCanonicalBeeperPad, isCanonicalLedPad); + } + + // Loop 2: assign outputs in priority order (dedicated, then auto). + for (int priority = 0; priority < 2; priority++) { + int motorIdx = outputs->motorCount; + + for (int idx = 0; idx < count; idx++) { + timerHardware_t *timHw = &pads[idx]; + outputMode_e mode = modeByTimId[timHw->timId]; + bool isDedicated = (priority == 0); + + if (checkPwmTimerConflicts_stub(timHw)) { + continue; + } + + // Motors: dedicated (OUTPUT_MODE_MOTORS) first, then auto + if (TIM_IS_MOTOR(timHw->usageFlags) && motorIdx < motorCount + && !pwmHasServoOnTimer_sim(pads, outputs, timHw->timId) + && (isDedicated ? mode == OUTPUT_MODE_MOTORS : mode != OUTPUT_MODE_MOTORS)) { + assignFn(outputs, pads, count, timHw, MAP_TO_MOTOR_OUTPUT); + motorIdx++; + continue; + } + + // Servos: dedicated (OUTPUT_MODE_SERVOS) first, then auto + if (TIM_IS_SERVO(timHw->usageFlags) && outputs->servoCount < servoCount + && !pwmHasMotorOnTimer_sim(pads, outputs, timHw->timId) + && (isDedicated ? mode == OUTPUT_MODE_SERVOS : mode != OUTPUT_MODE_SERVOS)) { + assignFn(outputs, pads, count, timHw, MAP_TO_SERVO_OUTPUT); + continue; + } + + // LEDs: only on the auto pass, and only if timer is uncontested + if (!isDedicated && TIM_IS_LED(timHw->usageFlags) + && !pwmHasMotorOnTimer_sim(pads, outputs, timHw->timId) + && !pwmHasServoOnTimer_sim(pads, outputs, timHw->timId)) { + assignFn(outputs, pads, count, timHw, MAP_TO_LED_OUTPUT); + } + } + } +} + +/* ========================================================================= + * TEST SUITE: LedSiblingE2E + * + * Two pads share one physical timer (timId=4), overridden to + * OUTPUT_MODE_LED. No motors/servos in play (motorCount=servoCount=0), so + * only the LED auto-pass branch is exercised in loop 2 — the exact path + * that exposed the real bug (FEATURE_LED_STRIP off -> checkPwmTimerConflicts + * always false -> LED pad reached via the uncontested-timer auto pass). + * ========================================================================= */ + +TEST(LedSiblingE2E, BugReproduction_FullPipeline_SiblingIncorrectlyGetsLed) +{ + timerHardware_t pads[2]; + pads[0].usageFlags = 0; pads[0].timId = 4; + pads[1].usageFlags = 0; pads[1].timId = 4; + + outputMode_e modeByTimId[MAX_TIMID]; + for (int i = 0; i < MAX_TIMID; i++) modeByTimId[i] = OUTPUT_MODE_AUTO; + modeByTimId[4] = OUTPUT_MODE_LED; + + SimOutputs outputs; + simulateBuildTimerOutputList(pads, 2, modeByTimId, /*motorCount=*/0, /*servoCount=*/0, + pwmAssignOutput_buggy, &outputs); + + EXPECT_TRUE(pads[0].usageFlags & (uint32_t)TIM_USE_LED) + << "Canonical (first-scanned) pad should get TIM_USE_LED"; + EXPECT_TRUE(pads[1].usageFlags & (uint32_t)TIM_USE_LED) + << "BUG REPRODUCED: with the pre-fix pwmAssignOutput() (LED case also " + "calling pwmClaimTimer()), the sibling pad — correctly computed as " + "TIM_USE_PINIO by timerHardwareOverride() in loop 1 — gets silently " + "overwritten back to TIM_USE_LED when the canonical pad is assigned " + "in loop 2. This is the real hardware-observed bug (SPEDIXF405 " + "TIM4 S5+S6)."; + EXPECT_FALSE(pads[1].usageFlags & (uint32_t)TIM_USE_PINIO) + << "Sibling's correct PINIO flag was clobbered by propagation (bug symptom)"; +} + +TEST(LedSiblingE2E, FixVerification_FullPipeline_SiblingKeepsPinio_NotLed) +{ + timerHardware_t pads[2]; + pads[0].usageFlags = 0; pads[0].timId = 4; + pads[1].usageFlags = 0; pads[1].timId = 4; + + outputMode_e modeByTimId[MAX_TIMID]; + for (int i = 0; i < MAX_TIMID; i++) modeByTimId[i] = OUTPUT_MODE_AUTO; + modeByTimId[4] = OUTPUT_MODE_LED; + + SimOutputs outputs; + simulateBuildTimerOutputList(pads, 2, modeByTimId, /*motorCount=*/0, /*servoCount=*/0, + pwmAssignOutput_fixed, &outputs); + + EXPECT_TRUE(pads[0].usageFlags & (uint32_t)TIM_USE_LED) + << "Canonical (first-scanned) pad must have TIM_USE_LED after the full pipeline"; + EXPECT_FALSE(pads[0].usageFlags & (uint32_t)TIM_USE_PINIO) + << "Canonical pad must not also carry the PINIO fallback flag"; + + EXPECT_FALSE(pads[1].usageFlags & (uint32_t)TIM_USE_LED) + << "FIX VERIFIED: sibling pad on the same timer must NOT end up with " + "TIM_USE_LED after the full two-loop pipeline (this is the " + "assertion that would have FAILED against the pre-fix " + "pwmAssignOutput(), which called pwmClaimTimer() for LED too)"; + EXPECT_TRUE(pads[1].usageFlags & (uint32_t)TIM_USE_PINIO) + << "Sibling pad must retain TIM_USE_PINIO (GPIO-only fallback) set by " + "timerHardwareOverride() in loop 1, undisturbed by loop 2's " + "assignment of the canonical pad"; +} + +TEST(LedSiblingE2E, FixVerification_ThreeSiblingPads_OnlyCanonicalKeepsLed) +{ + timerHardware_t pads[3]; + for (int i = 0; i < 3; i++) { pads[i].usageFlags = 0; pads[i].timId = 9; } + + outputMode_e modeByTimId[MAX_TIMID]; + for (int i = 0; i < MAX_TIMID; i++) modeByTimId[i] = OUTPUT_MODE_AUTO; + modeByTimId[9] = OUTPUT_MODE_LED; + + SimOutputs outputs; + simulateBuildTimerOutputList(pads, 3, modeByTimId, /*motorCount=*/0, /*servoCount=*/0, + pwmAssignOutput_fixed, &outputs); + + EXPECT_TRUE(pads[0].usageFlags & (uint32_t)TIM_USE_LED); + for (int i = 1; i < 3; i++) { + EXPECT_FALSE(pads[i].usageFlags & (uint32_t)TIM_USE_LED) + << "pads[" << i << "] must not end up LED-flagged"; + EXPECT_TRUE(pads[i].usageFlags & (uint32_t)TIM_USE_PINIO) + << "pads[" << i << "] must remain PINIO"; + } +} + +/* ========================================================================= + * TEST SUITE: MotorSiblingPropagationRegression + * + * Regression guard: pwmClaimTimer()'s propagate-to-siblings behavior must + * still happen for MAP_TO_MOTOR_OUTPUT (and, by the same code path, for + * MAP_TO_SERVO_OUTPUT). The fix removed the pwmClaimTimer() call ONLY from + * the LED case; it must NOT have been broadened to also drop it from + * motor/servo. + * ========================================================================= */ + +/* + * Direct-call test: proves pwmAssignOutput_fixed()'s MOTOR case still + * propagates via pwmClaimTimer_sim(). This is the test that actually + * *distinguishes* "propagation happened" from "propagation didn't happen" — + * pads[1] never has pwmAssignOutput_fixed() called on it directly; its + * flags can ONLY change as a side effect of pads[0]'s assignment call + * invoking pwmClaimTimer_sim() for the shared timId. Pre-existing + * (non-MOTOR) bits on pads[1] must be overwritten (not merely OR'd/left + * alone), matching pwmClaimTimer()'s real "usageFlags = usageFlags" (full + * overwrite, not `|=`) semantics. + */ +TEST(MotorSiblingPropagationRegression, DirectCall_PropagationOverwritesSiblingFlags) +{ + timerHardware_t pads[2]; + pads[0].usageFlags = (uint32_t)(TIM_USE_MOTOR | TIM_USE_PWM); + pads[0].timId = 7; + pads[1].usageFlags = (uint32_t)(TIM_USE_MOTOR | TIM_USE_PPM); // different pre-existing bit + pads[1].timId = 7; + + SimOutputs outputs; + outputs.motorCount = 0; + outputs.servoCount = 0; + + // Assign ONLY pads[0] — pads[1] is never passed to pwmAssignOutput_fixed(). + pwmAssignOutput_fixed(&outputs, pads, 2, &pads[0], MAP_TO_MOTOR_OUTPUT); + + EXPECT_EQ(pads[0].usageFlags, (uint32_t)TIM_USE_MOTOR) + << "Assigned pad's own flags must be masked down to TIM_USE_MOTOR only"; + EXPECT_EQ(pads[1].usageFlags, (uint32_t)TIM_USE_MOTOR) + << "REGRESSION CHECK: sibling pad's flags must be overwritten to " + "TIM_USE_MOTOR by pwmClaimTimer() propagation alone, even though " + "pwmAssignOutput() was never called on it directly. If this " + "propagation were accidentally removed for MOTOR too, pads[1] " + "would incorrectly retain TIM_USE_PPM here."; + EXPECT_EQ(outputs.motorCount, 1); +} + +/* + * Full-pipeline sanity check: with the current fixed source, a two-pad + * shared MOTOR timer still ends up with both pads assigned as motor outputs. + * (Note: unlike the direct-call test above, this alone would still pass even + * without propagation, since each pad also gets individually masked by its + * own pwmAssignOutput() call in the auto/dedicated pass — the direct-call + * test above is what actually isolates the propagation behavior. This test + * is included for overall pipeline sanity / non-regression of the motor + * assignment path as a whole.) + */ +TEST(MotorSiblingPropagationRegression, FullPipeline_BothMotorPadsAssigned) +{ + timerHardware_t pads[2]; + pads[0].usageFlags = 0; pads[0].timId = 7; + pads[1].usageFlags = 0; pads[1].timId = 7; + + outputMode_e modeByTimId[MAX_TIMID]; + for (int i = 0; i < MAX_TIMID; i++) modeByTimId[i] = OUTPUT_MODE_AUTO; + modeByTimId[7] = OUTPUT_MODE_MOTORS; + + SimOutputs outputs; + simulateBuildTimerOutputList(pads, 2, modeByTimId, /*motorCount=*/2, /*servoCount=*/0, + pwmAssignOutput_fixed, &outputs); + + EXPECT_EQ(outputs.motorCount, 2) << "Both pads on the shared MOTOR timer should be assigned"; + EXPECT_EQ(pads[0].usageFlags, (uint32_t)TIM_USE_MOTOR); + EXPECT_EQ(pads[1].usageFlags, (uint32_t)TIM_USE_MOTOR); +} + +/* ========================================================================= + * TEST SUITE: SourceSync + * + * Reads the ACTUAL LIVE src/main/drivers/pwm_mapping.c off disk and checks + * that pwmAssignOutput()'s MAP_TO_LED_OUTPUT case does NOT call + * pwmClaimTimer(), while MAP_TO_MOTOR_OUTPUT/MAP_TO_SERVO_OUTPUT still do. + * Mirrors the SourceSync pattern in pwm_mapping_led_unittest.cc / + * pwm_mapping_beeper_unittest.cc. This is a drift-detector, not a substitute + * for the behavioral tests above (see those files' header comments for why + * pwm_mapping.c can't be linked directly into a host unit test). + * ========================================================================= */ + +namespace { + +std::string stripLineComments(const std::string &text) +{ + std::string result; + result.reserve(text.size()); + for (size_t i = 0; i < text.size();) { + if (text[i] == '/' && i + 1 < text.size() && text[i + 1] == '/') { + while (i < text.size() && text[i] != '\n') { + i++; + } + continue; + } + result += text[i]; + i++; + } + return result; +} + +std::string normalizeWhitespace(const std::string &text) +{ + std::string result; + result.reserve(text.size()); + bool lastWasSpace = true; + for (char c : text) { + if (std::isspace(static_cast(c))) { + if (!lastWasSpace) { + result += ' '; + lastWasSpace = true; + } + } else { + result += c; + lastWasSpace = false; + } + } + while (!result.empty() && result.back() == ' ') { + result.pop_back(); + } + return result; +} + +std::string normalizeSource(const std::string &text) +{ + return normalizeWhitespace(stripLineComments(text)); +} + +std::string pwmMappingSourcePath() +{ + std::string thisFile = __FILE__; // .../src/test/unit/pwm_mapping_led_e2e_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"; +} + +::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."; + } + std::ostringstream buffer; + buffer << file.rdbuf(); + if (buffer.str().empty()) { + return ::testing::AssertionFailure() + << "Live source file at " << path << " opened but read as empty."; + } + *out = normalizeSource(buffer.str()); + return ::testing::AssertionSuccess(); +} + +// Extracts the substring of `normalizedSource` starting at the (already +// normalized) needle `funcSignature` through its matching closing brace, +// assuming brace-balanced C code with no braces inside string/char literals +// (true for pwmAssignOutput()). Returns empty string if not found or +// unbalanced. +std::string extractBraceBlock(const std::string &normalizedSource, const std::string &funcSignature) +{ + size_t start = normalizedSource.find(funcSignature); + if (start == std::string::npos) { + return ""; + } + size_t bracePos = normalizedSource.find('{', start); + if (bracePos == std::string::npos) { + return ""; + } + int depth = 0; + size_t i = bracePos; + for (; i < normalizedSource.size(); i++) { + if (normalizedSource[i] == '{') depth++; + else if (normalizedSource[i] == '}') { + depth--; + if (depth == 0) { + return normalizedSource.substr(bracePos, i - bracePos + 1); + } + } + } + return ""; // unbalanced -- shouldn't happen for valid C +} + +} // namespace + +TEST(SourceSync, PwmAssignOutputSignatureMatchesLiveSource) +{ + std::string normalizedSource; + ASSERT_TRUE(loadNormalizedPwmMappingSource(&normalizedSource)); + + const std::string expectedSignature = normalizeSource( + "static void pwmAssignOutput(timMotorServoHardware_t *timOutputs, " + "timerHardware_t *timHw, int type)"); + + EXPECT_NE(normalizedSource.find(expectedSignature), std::string::npos) + << "pwmAssignOutput()'s signature in the LIVE src/main/drivers/pwm_mapping.c " + "no longer matches what this test assumes. Update " + "pwm_mapping_led_e2e_unittest.cc's inline reproduction and this " + "expected string to match."; +} + +TEST(SourceSync, LedCaseDoesNotCallPwmClaimTimer) +{ + std::string normalizedSource; + ASSERT_TRUE(loadNormalizedPwmMappingSource(&normalizedSource)); + + std::string body = extractBraceBlock(normalizedSource, + normalizeSource("static void pwmAssignOutput(timMotorServoHardware_t *timOutputs, " + "timerHardware_t *timHw, int type)")); + ASSERT_FALSE(body.empty()) + << "Could not locate/extract pwmAssignOutput()'s body from the live source " + "-- check PwmAssignOutputSignatureMatchesLiveSource for details."; + + size_t ledCasePos = body.find("case MAP_TO_LED_OUTPUT:"); + ASSERT_NE(ledCasePos, std::string::npos) + << "Could not find 'case MAP_TO_LED_OUTPUT:' inside pwmAssignOutput()'s body " + "in the live source."; + + size_t nextCasePos = body.find("case", ledCasePos + 1); + size_t defaultPos = body.find("default:", ledCasePos + 1); + size_t ledCaseEnd = std::min( + nextCasePos == std::string::npos ? body.size() : nextCasePos, + defaultPos == std::string::npos ? body.size() : defaultPos); + std::string ledCaseBody = body.substr(ledCasePos, ledCaseEnd - ledCasePos); + + EXPECT_EQ(ledCaseBody.find("pwmClaimTimer("), std::string::npos) + << "REGRESSION: the live MAP_TO_LED_OUTPUT case in " + "src/main/drivers/pwm_mapping.c now calls pwmClaimTimer() again. " + "This reintroduces the exact bug this test file guards against: " + "propagating this pad's flags to sibling pads on the same timer, " + "overwriting their correct TIM_USE_PINIO back to TIM_USE_LED. " + "LED case body found: " << ledCaseBody; +} + +TEST(SourceSync, MotorAndServoCasesStillCallPwmClaimTimer) +{ + std::string normalizedSource; + ASSERT_TRUE(loadNormalizedPwmMappingSource(&normalizedSource)); + + std::string body = extractBraceBlock(normalizedSource, + normalizeSource("static void pwmAssignOutput(timMotorServoHardware_t *timOutputs, " + "timerHardware_t *timHw, int type)")); + ASSERT_FALSE(body.empty()); + + size_t motorCasePos = body.find("case MAP_TO_MOTOR_OUTPUT:"); + size_t servoCasePos = body.find("case MAP_TO_SERVO_OUTPUT:"); + ASSERT_NE(motorCasePos, std::string::npos); + ASSERT_NE(servoCasePos, std::string::npos); + ASSERT_LT(motorCasePos, servoCasePos) + << "Expected MAP_TO_MOTOR_OUTPUT case to appear before MAP_TO_SERVO_OUTPUT " + "in source order, as in the current file. If this legitimately " + "changed, adjust this test's slicing logic."; + + size_t ledCasePos = body.find("case MAP_TO_LED_OUTPUT:", servoCasePos); + ASSERT_NE(ledCasePos, std::string::npos); + + std::string motorAndServoBody = body.substr(motorCasePos, ledCasePos - motorCasePos); + + // Expect exactly two pwmClaimTimer( calls in the motor+servo span (one each). + int count = 0; + size_t pos = 0; + while ((pos = motorAndServoBody.find("pwmClaimTimer(", pos)) != std::string::npos) { + count++; + pos += std::strlen("pwmClaimTimer("); + } + EXPECT_EQ(count, 2) + << "REGRESSION: expected exactly 2 pwmClaimTimer() calls across the " + "MAP_TO_MOTOR_OUTPUT and MAP_TO_SERVO_OUTPUT cases (one each) in the " + "live source, found " << count << ". This fix must only remove " + "propagation for LED, not for motor/servo. Span checked: " + << motorAndServoBody; +} + +TEST(SourceSync, PwmClaimTimerBodyMatchesLiveSource) +{ + std::string normalizedSource; + ASSERT_TRUE(loadNormalizedPwmMappingSource(&normalizedSource)); + + const std::string expectedBody = normalizeSource( + "uint8_t pwmClaimTimer(HAL_Timer_t *tim, uint32_t usageFlags) { " + "uint8_t changed = 0; " + "for (int idx = 0; idx < timerHardwareCount; idx++) { " + "timerHardware_t *timHw = &timerHardware[idx]; " + "if (timHw->tim == tim && timHw->usageFlags != usageFlags) { " + "timHw->usageFlags = usageFlags; " + "changed++; " + "} " + "} " + "return changed; " + "}"); + + EXPECT_NE(normalizedSource.find(expectedBody), std::string::npos) + << "pwmClaimTimer()'s body in the LIVE src/main/drivers/pwm_mapping.c no " + "longer matches pwmClaimTimer_sim()'s reproduction in this test file " + "(propagate-usageFlags-to-siblings-sharing-a-timer). Update " + "pwmClaimTimer_sim() to match, then update this expected string."; +} diff --git a/src/test/unit/pwm_mapping_led_unittest.cc b/src/test/unit/pwm_mapping_led_unittest.cc new file mode 100644 index 00000000000..3428107afa6 --- /dev/null +++ b/src/test/unit/pwm_mapping_led_unittest.cc @@ -0,0 +1,705 @@ +/* + * Unit test: timerHardwareOverride() must not falsely flag sibling timer + * channels as TIM_USE_LED when OUTPUT_MODE_LED is applied to a shared timer. + * + * Bug (pre-fix, same bug class as the BEEPER bug fixed by commits + * 8c16aeed85 / 31bb99ea0b, see pwm_mapping_beeper_unittest.cc): + * timerHardwareOverride() applied TIM_USE_LED to every pad sharing a timer + * set to OUTPUT_MODE_LED, but the WS2811 LED strip driver + * (light_ws2811strip.c) only ever wires up the FIRST matching pad, found + * via timerGetByUsageFlag(TIM_USE_LED). All channels of one timer share a + * single period register (fixed at the WS2811 bit rate), so only one pad + * can actually drive the LED strip. This caused sibling pads on the same + * timer to be falsely reported/claimed as LED outputs while being + * functionally dead (or worse, stealing a pad that could have been used + * for something else). + * + * Fix (mirrors the BEEPER fix exactly): + * timerHardwareOverride() now takes an `isCanonicalLedPad` bool. The + * OUTPUT_MODE_LED case always clears TIM_USE_MOTOR|TIM_USE_SERVO| + * TIM_USE_BEEPER|TIM_USE_LED, and sets TIM_USE_PINIO as a GPIO-only + * fallback (binary on/off, no PWM — doesn't need the timer's fixed + * WS2811-bitrate period). Only if isCanonicalLedPad is true (i.e. this is + * the first timerHardware[] entry, in ascending scan order, seen for this + * physical timer with outputMode == OUTPUT_MODE_LED) does it clear + * TIM_USE_PINIO and set TIM_USE_LED instead. + * + * pwmBuildTimerOutputList() computes isCanonicalLedPad via a + * `ledClaimed[HARDWARE_TIMER_DEFINITION_COUNT]` array, parallel to the + * existing `beeperClaimed[]` array used for OUTPUT_MODE_BEEPER. + * + * This file contains: + * 1. A minimal inline reproduction of both the BUGGY and FIXED versions of + * timerHardwareOverride() (LED case only, plus MOTORS/SERVOS/BEEPER/ + * PINIO for regression coverage) so the test is fully self-contained. + * pwm_mapping.c is excluded from the SITL/unit build by + * `#if !defined(SITL_BUILD)`, and the real function depends on PG + * (parameter-group) infrastructure not available in a host unit build. + * 2. TEST BugReproduction — demonstrates that the old, unconditional-LED + * code gives TIM_USE_LED to every pad sharing an LED-mode timer. + * 3. TEST FixVerification — only the canonical (first) pad on a shared + * timer gets TIM_USE_LED; sibling pads fall back to TIM_USE_PINIO. + * 4. Multi-pad (3 siblings) test — only the first scanned pad is canonical. + * 5. Regression tests for MOTORS/SERVOS/BEEPER/PINIO overrides on the + * fixed function, including timers mixed with LED-mode timers. + * 6. TEST SUITE SourceSync — reads the ACTUAL LIVE + * src/main/drivers/pwm_mapping.c off disk at test time and asserts the + * real timerHardwareOverride() source text still matches the logic the + * inline reproduction above assumes. This exists specifically because + * the sibling pwm_mapping_beeper_unittest.cc was found to have drifted: + * it kept passing for months after commits 8c16aeed85/31bb99ea0b + * changed the real BEEPER logic, because it only ever exercised its own + * frozen, hand-copied logic and never looked at the live file again. + * An inline reproduction is unavoidable here (see point 1), but it + * should not be allowed to silently go stale — if pwm_mapping.c's real + * OUTPUT_MODE_LED case (or the canonical-pad tracking loop in + * pwmBuildTimerOutputList()) changes without this file being updated to + * match, SourceSync must fail loudly instead of the rest of the suite + * quietly passing against dead logic. + */ + +#include +#include +#include +#include +#include +#include + +#include "gtest/gtest.h" +#include "unittest_macros.h" + +/* ------------------------------------------------------------------------- + * Minimal type/flag definitions — mirrors the real firmware headers on this + * branch (src/main/drivers/timer.h, src/main/flight/mixer.h). Unlike + * release/9.1, this branch has TIM_USE_PINIO / TIM_USE_BEEPER and + * OUTPUT_MODE_PINIO / OUTPUT_MODE_BEEPER. + * ------------------------------------------------------------------------- */ + +/* timerUsageFlag_e — must match src/main/drivers/timer.h exactly */ +typedef enum { + TIM_USE_ANY = 0, + TIM_USE_PPM = (1 << 0), + TIM_USE_PWM = (1 << 1), + TIM_USE_MOTOR = (1 << 2), + TIM_USE_SERVO = (1 << 3), + TIM_USE_LED = (1 << 24), + TIM_USE_BEEPER = (1 << 25), + TIM_USE_PINIO = (1 << 26), +} timerUsageFlag_e; + +/* outputMode_e — must match src/main/flight/mixer.h exactly */ +typedef enum { + OUTPUT_MODE_AUTO = 0, + OUTPUT_MODE_MOTORS, + OUTPUT_MODE_SERVOS, + OUTPUT_MODE_LED, + OUTPUT_MODE_PINIO, + OUTPUT_MODE_BEEPER +} outputMode_e; + +/* Minimal timer hardware entry — only the fields exercised by the function. + * `timId` stands in for the real code's timer2id(timer->tim) — in the real + * driver, multiple timerHardware[] pads sharing the same physical timer + * compare equal on timer2id(); here we just store the id directly. */ +typedef struct timerHardware_s { + uint32_t usageFlags; + uint8_t timId; +} timerHardware_t; + +/* ------------------------------------------------------------------------- + * Inline reproductions of timerHardwareOverride(). + * + * We inline these rather than linking pwm_mapping.c because: + * a) pwm_mapping.c is wrapped in #if !defined(SITL_BUILD) and the unit-test + * target.h defines SITL_BUILD, so the real file compiles to nothing. + * b) The function depends on timerOverrides() (parameter-group accessor) and + * timer2id(), which are platform / PG infrastructure that is not present + * in a host unit-test build. + * + * The functions below are literal translations of the current C source + * (post BEEPER-fix, post LED-fix) with the PG indirection collapsed into a + * simple `outputMode` parameter, and canonical-pad tracking passed in + * explicitly as bool parameters (mirroring isCanonicalBeeperPad / + * isCanonicalLedPad computed by pwmBuildTimerOutputList()). + * + * See the SourceSync test suite at the bottom of this file for a mechanism + * that catches this reproduction going out of sync with the real source. + * ------------------------------------------------------------------------- */ + +/* + * BUGGY version — reproduces the pre-fix LED behaviour: OUTPUT_MODE_LED + * unconditionally sets TIM_USE_LED on every pad sharing the timer, with no + * canonical-pad tracking at all (same bug class as the pre-fix BEEPER code + * in commit history before 8c16aeed85). MOTORS/SERVOS/PINIO/BEEPER cases are + * included only for completeness / contrast; this reproduction focuses on + * the LED bug. + */ +static void timerHardwareOverride_buggy(timerHardware_t *timer, + outputMode_e outputMode) +{ + switch (outputMode) { + case OUTPUT_MODE_MOTORS: + timer->usageFlags &= ~(uint32_t)(TIM_USE_SERVO | TIM_USE_LED | TIM_USE_BEEPER); + timer->usageFlags |= (uint32_t)TIM_USE_MOTOR; + break; + case OUTPUT_MODE_SERVOS: + timer->usageFlags &= ~(uint32_t)(TIM_USE_MOTOR | TIM_USE_LED | TIM_USE_BEEPER); + timer->usageFlags |= (uint32_t)TIM_USE_SERVO; + break; + case OUTPUT_MODE_LED: + /* BUG: unconditionally sets TIM_USE_LED on every pad of this + * timer, even though only one pad is actually wired up by + * ws2811LedStripInit(). No canonical-pad tracking whatsoever. */ + timer->usageFlags &= ~(uint32_t)(TIM_USE_MOTOR | TIM_USE_SERVO | TIM_USE_BEEPER); + timer->usageFlags |= (uint32_t)TIM_USE_LED; + break; + case OUTPUT_MODE_PINIO: + timer->usageFlags &= ~(uint32_t)(TIM_USE_MOTOR | TIM_USE_SERVO | TIM_USE_LED | TIM_USE_BEEPER); + timer->usageFlags |= (uint32_t)TIM_USE_PINIO; + break; + case OUTPUT_MODE_BEEPER: + timer->usageFlags &= ~(uint32_t)(TIM_USE_MOTOR | TIM_USE_SERVO | TIM_USE_LED); + timer->usageFlags |= (uint32_t)TIM_USE_BEEPER; + break; + default: + break; + } +} + +/* + * FIXED version — literal translation of the current + * src/main/drivers/pwm_mapping.c timerHardwareOverride() on this branch. + * Kept in sync with the real source by the SourceSync tests below. + */ +static void timerHardwareOverride_fixed(timerHardware_t *timer, + outputMode_e outputMode, + bool isCanonicalBeeperPad, + bool isCanonicalLedPad) +{ + switch (outputMode) { + case OUTPUT_MODE_MOTORS: + timer->usageFlags &= ~(uint32_t)(TIM_USE_SERVO | TIM_USE_LED | TIM_USE_BEEPER); + timer->usageFlags |= (uint32_t)TIM_USE_MOTOR; + break; + case OUTPUT_MODE_SERVOS: + timer->usageFlags &= ~(uint32_t)(TIM_USE_MOTOR | TIM_USE_LED | TIM_USE_BEEPER); + timer->usageFlags |= (uint32_t)TIM_USE_SERVO; + break; + case OUTPUT_MODE_LED: + // Other channels of this timer share its period register, so they can't + // be repurposed as motor/servo/BEEPER either. Only the first (canonical) + // pad is the one ws2811LedStripInit() actually wires up; siblings can still + // drive a GPIO-only PINIO (binary on/off, no PWM), since that doesn't need + // the timer's fixed WS2811-bitrate period. + timer->usageFlags &= ~(uint32_t)(TIM_USE_MOTOR | TIM_USE_SERVO | TIM_USE_BEEPER | TIM_USE_LED); + timer->usageFlags |= (uint32_t)TIM_USE_PINIO; + if (isCanonicalLedPad) { + timer->usageFlags &= ~(uint32_t)TIM_USE_PINIO; + timer->usageFlags |= (uint32_t)TIM_USE_LED; + } + break; + case OUTPUT_MODE_PINIO: + timer->usageFlags &= ~(uint32_t)(TIM_USE_MOTOR | TIM_USE_SERVO | TIM_USE_LED | TIM_USE_BEEPER); + timer->usageFlags |= (uint32_t)TIM_USE_PINIO; + break; + case OUTPUT_MODE_BEEPER: + // Other channels of this timer share its period register, so they can't + // be repurposed as motor/servo/LED either. They can still drive a GPIO-only + // PINIO (binary on/off, no PWM), since that doesn't need the timer's period. + timer->usageFlags &= ~(uint32_t)(TIM_USE_MOTOR | TIM_USE_SERVO | TIM_USE_LED | TIM_USE_BEEPER); + timer->usageFlags |= (uint32_t)TIM_USE_PINIO; + if (isCanonicalBeeperPad) { + timer->usageFlags &= ~(uint32_t)TIM_USE_PINIO; + timer->usageFlags |= (uint32_t)TIM_USE_BEEPER; + } + break; + default: + break; + } +} + +/* + * Helper — reproduces the canonical-pad-tracking loop in + * pwmBuildTimerOutputList(): scans pads in ascending array order and applies + * timerHardwareOverride_fixed() with isCanonicalBeeperPad / isCanonicalLedPad + * computed exactly as the real code does (first pad seen per physical timer, + * for a timer whose overridden outputMode matches, wins canonical status). + */ +static void applyFixedOverridesToArray(timerHardware_t *pads, int count, + const outputMode_e *outputModeByPad, + int maxTimId) +{ + bool beeperClaimed[32] = { false }; + bool ledClaimed[32] = { false }; + ASSERT_LT(maxTimId, 32); + + for (int idx = 0; idx < count; idx++) { + timerHardware_t *timHw = &pads[idx]; + uint8_t timId = timHw->timId; + outputMode_e mode = outputModeByPad[idx]; + + bool isCanonicalBeeperPad = false; + if (mode == OUTPUT_MODE_BEEPER && !beeperClaimed[timId]) { + beeperClaimed[timId] = true; + isCanonicalBeeperPad = true; + } + bool isCanonicalLedPad = false; + if (mode == OUTPUT_MODE_LED && !ledClaimed[timId]) { + ledClaimed[timId] = true; + isCanonicalLedPad = true; + } + timerHardwareOverride_fixed(timHw, mode, isCanonicalBeeperPad, isCanonicalLedPad); + } +} + +/* ========================================================================= + * TEST SUITE: LedTimerSiblingPads + * ========================================================================= */ + +/* + * TEST 1 — BugReproduction + * + * Two pads share timer id 5, both overridden to OUTPUT_MODE_LED. On the + * BUGGY code path (no canonical-pad tracking at all), BOTH pads get + * TIM_USE_LED, even though only one of them will ever actually be wired up + * by ws2811LedStripInit()/timerGetByUsageFlag(TIM_USE_LED). This test + * PASSES (i.e. the bug reproduces) on the buggy code — confirming the + * observable symptom of the bug. + */ +TEST(LedTimerSiblingPads, BugReproduction_BothSiblingsGetLedFlag) +{ + timerHardware_t padA; padA.usageFlags = 0; padA.timId = 5; + timerHardware_t padB; padB.usageFlags = 0; padB.timId = 5; + + timerHardwareOverride_buggy(&padA, OUTPUT_MODE_LED); + timerHardwareOverride_buggy(&padB, OUTPUT_MODE_LED); + + EXPECT_TRUE(padA.usageFlags & (uint32_t)TIM_USE_LED) + << "First pad should get TIM_USE_LED (expected)"; + EXPECT_TRUE(padB.usageFlags & (uint32_t)TIM_USE_LED) + << "BUG CONFIRMED: sibling pad on the SAME timer also got TIM_USE_LED, " + "but light_ws2811strip.c only wires up the first pad found via " + "timerGetByUsageFlag(TIM_USE_LED). This sibling would be falsely " + "reported as an LED output while being functionally dead."; +} + +/* + * TEST 2 — FixVerification (two sibling pads) + * + * Same scenario as above, but using the FIXED function with proper + * canonical-pad computation (as pwmBuildTimerOutputList() does). Only the + * first-scanned pad on the shared timer should get TIM_USE_LED; the sibling + * must fall back to TIM_USE_PINIO (GPIO-only), NOT TIM_USE_LED, and NOT be + * left flagless (this branch has the PINIO fallback, unlike release/9.1). + */ +TEST(LedTimerSiblingPads, FixVerification_OnlyCanonicalPadGetsLed) +{ + timerHardware_t pads[2]; + pads[0].usageFlags = 0; pads[0].timId = 5; + pads[1].usageFlags = 0; pads[1].timId = 5; + outputMode_e modes[2] = { OUTPUT_MODE_LED, OUTPUT_MODE_LED }; + + applyFixedOverridesToArray(pads, 2, modes, 6); + + EXPECT_TRUE(pads[0].usageFlags & (uint32_t)TIM_USE_LED) + << "Canonical (first-scanned) pad must get TIM_USE_LED"; + EXPECT_FALSE(pads[0].usageFlags & (uint32_t)TIM_USE_PINIO) + << "Canonical pad must not also carry the PINIO fallback flag"; + + EXPECT_FALSE(pads[1].usageFlags & (uint32_t)TIM_USE_LED) + << "FIX VERIFIED: sibling pad on the same timer must NOT get TIM_USE_LED"; + EXPECT_TRUE(pads[1].usageFlags & (uint32_t)TIM_USE_PINIO) + << "Sibling pad must fall back to TIM_USE_PINIO (GPIO-only), not be left flagless"; +} + +/* + * TEST 3 — Three-pad case: only the first scanned pad is canonical + * + * Confirms the canonical tracking generalizes beyond two pads: with three + * pads sharing a timer, only pads[0] should be canonical; pads[1] and + * pads[2] must both fall back to PINIO. + */ +TEST(LedTimerSiblingPads, ThreePads_OnlyFirstScannedIsCanonical) +{ + timerHardware_t pads[3]; + for (int i = 0; i < 3; i++) { + pads[i].usageFlags = 0; + pads[i].timId = 7; + } + outputMode_e modes[3] = { OUTPUT_MODE_LED, OUTPUT_MODE_LED, OUTPUT_MODE_LED }; + + applyFixedOverridesToArray(pads, 3, modes, 8); + + EXPECT_TRUE(pads[0].usageFlags & (uint32_t)TIM_USE_LED) + << "pads[0] (first scanned) must be canonical and get TIM_USE_LED"; + EXPECT_FALSE(pads[0].usageFlags & (uint32_t)TIM_USE_PINIO); + + for (int i = 1; i < 3; i++) { + EXPECT_FALSE(pads[i].usageFlags & (uint32_t)TIM_USE_LED) + << "pads[" << i << "] must NOT be canonical, so must not get TIM_USE_LED"; + EXPECT_TRUE(pads[i].usageFlags & (uint32_t)TIM_USE_PINIO) + << "pads[" << i << "] must fall back to TIM_USE_PINIO"; + } +} + +/* + * TEST 4 — Two independent LED timers each get their own canonical pad + * + * Pads on timer id 5 and pads on timer id 9 are independent: each timer + * should get exactly one canonical (LED) pad, tracked separately via the + * per-timer ledClaimed[] array. + */ +TEST(LedTimerSiblingPads, IndependentTimers_EachGetsOwnCanonicalPad) +{ + timerHardware_t pads[4]; + pads[0].usageFlags = 0; pads[0].timId = 5; // timer A, pad 1 -> canonical + pads[1].usageFlags = 0; pads[1].timId = 5; // timer A, pad 2 -> sibling + pads[2].usageFlags = 0; pads[2].timId = 9; // timer B, pad 1 -> canonical + pads[3].usageFlags = 0; pads[3].timId = 9; // timer B, pad 2 -> sibling + outputMode_e modes[4] = { OUTPUT_MODE_LED, OUTPUT_MODE_LED, OUTPUT_MODE_LED, OUTPUT_MODE_LED }; + + applyFixedOverridesToArray(pads, 4, modes, 10); + + EXPECT_TRUE(pads[0].usageFlags & (uint32_t)TIM_USE_LED); + EXPECT_TRUE(pads[1].usageFlags & (uint32_t)TIM_USE_PINIO); + EXPECT_FALSE(pads[1].usageFlags & (uint32_t)TIM_USE_LED); + + EXPECT_TRUE(pads[2].usageFlags & (uint32_t)TIM_USE_LED) + << "Timer B's first pad must independently become canonical"; + EXPECT_TRUE(pads[3].usageFlags & (uint32_t)TIM_USE_PINIO); + EXPECT_FALSE(pads[3].usageFlags & (uint32_t)TIM_USE_LED); +} + +/* ========================================================================= + * Negative / regression tests — MOTORS/SERVOS/BEEPER overrides must still + * work correctly with the fixed function, including when mixed with an + * LED-mode timer in the same scan. + * ========================================================================= */ + +TEST(LedTimerRegression, MotorOverride_Unaffected) +{ + timerHardware_t timer; + timer.usageFlags = (uint32_t)(TIM_USE_MOTOR | TIM_USE_SERVO); + timer.timId = 1; + + timerHardwareOverride_fixed(&timer, OUTPUT_MODE_MOTORS, + /*isCanonicalBeeperPad=*/false, + /*isCanonicalLedPad=*/false); + + EXPECT_TRUE(timer.usageFlags & (uint32_t)TIM_USE_MOTOR); + EXPECT_FALSE(timer.usageFlags & (uint32_t)TIM_USE_SERVO); + EXPECT_FALSE(timer.usageFlags & (uint32_t)TIM_USE_LED); + EXPECT_FALSE(timer.usageFlags & (uint32_t)TIM_USE_BEEPER); +} + +TEST(LedTimerRegression, ServoOverride_Unaffected) +{ + timerHardware_t timer; + timer.usageFlags = (uint32_t)(TIM_USE_MOTOR | TIM_USE_SERVO); + timer.timId = 2; + + timerHardwareOverride_fixed(&timer, OUTPUT_MODE_SERVOS, + /*isCanonicalBeeperPad=*/false, + /*isCanonicalLedPad=*/false); + + EXPECT_TRUE(timer.usageFlags & (uint32_t)TIM_USE_SERVO); + EXPECT_FALSE(timer.usageFlags & (uint32_t)TIM_USE_MOTOR); + EXPECT_FALSE(timer.usageFlags & (uint32_t)TIM_USE_LED); +} + +TEST(LedTimerRegression, BeeperCanonicalPad_StillGetsBeeperFlag) +{ + /* Regression: the BEEPER canonical-pad logic (isCanonicalBeeperPad) must + * be completely unaffected by the addition of isCanonicalLedPad. */ + timerHardware_t timer; + timer.usageFlags = 0; + timer.timId = 3; + + timerHardwareOverride_fixed(&timer, OUTPUT_MODE_BEEPER, + /*isCanonicalBeeperPad=*/true, + /*isCanonicalLedPad=*/false); + + EXPECT_TRUE(timer.usageFlags & (uint32_t)TIM_USE_BEEPER); + EXPECT_FALSE(timer.usageFlags & (uint32_t)TIM_USE_PINIO); +} + +TEST(LedTimerRegression, BeeperSiblingPad_FallsBackToPinio) +{ + timerHardware_t timer; + timer.usageFlags = 0; + timer.timId = 3; + + timerHardwareOverride_fixed(&timer, OUTPUT_MODE_BEEPER, + /*isCanonicalBeeperPad=*/false, + /*isCanonicalLedPad=*/false); + + EXPECT_FALSE(timer.usageFlags & (uint32_t)TIM_USE_BEEPER); + EXPECT_TRUE(timer.usageFlags & (uint32_t)TIM_USE_PINIO) + << "Non-canonical BEEPER pad must fall back to PINIO (regression check " + "for the pre-existing BEEPER fix, unaffected by the LED fix)"; +} + +TEST(LedTimerRegression, PinioOverride_Unaffected) +{ + timerHardware_t timer; + timer.usageFlags = (uint32_t)(TIM_USE_MOTOR | TIM_USE_SERVO | TIM_USE_LED); + timer.timId = 4; + + timerHardwareOverride_fixed(&timer, OUTPUT_MODE_PINIO, + /*isCanonicalBeeperPad=*/false, + /*isCanonicalLedPad=*/false); + + EXPECT_FALSE(timer.usageFlags & (uint32_t)TIM_USE_MOTOR); + EXPECT_FALSE(timer.usageFlags & (uint32_t)TIM_USE_SERVO); + EXPECT_FALSE(timer.usageFlags & (uint32_t)TIM_USE_LED); + EXPECT_TRUE(timer.usageFlags & (uint32_t)TIM_USE_PINIO); +} + +TEST(LedTimerRegression, AutoMode_LeavesTimerUnchanged) +{ + timerHardware_t timer; + timer.usageFlags = (uint32_t)(TIM_USE_MOTOR | TIM_USE_SERVO); + timer.timId = 6; + const uint32_t originalFlags = timer.usageFlags; + + timerHardwareOverride_fixed(&timer, OUTPUT_MODE_AUTO, + /*isCanonicalBeeperPad=*/false, + /*isCanonicalLedPad=*/false); + + EXPECT_EQ(timer.usageFlags, originalFlags) + << "OUTPUT_MODE_AUTO (default case) must not change flags"; +} + +/* + * TEST — Mixed scan: MOTOR timer, LED timer (2 pads), BEEPER timer (canonical + * pad only) all processed together via applyFixedOverridesToArray(), matching + * a realistic pwmBuildTimerOutputList() scenario where different timers on + * the board have different output-mode overrides. Confirms none of the + * per-timer canonical tracking (ledClaimed[] vs beeperClaimed[]) interferes + * across timer ids or across output modes. + */ +TEST(LedTimerRegression, MixedScan_MotorLedBeeperTimersDoNotInterfere) +{ + timerHardware_t pads[5]; + for (int i = 0; i < 5; i++) pads[i].usageFlags = 0; + + pads[0].timId = 1; // MOTOR timer + pads[1].timId = 2; // LED timer, canonical + pads[2].timId = 2; // LED timer, sibling + pads[3].timId = 3; // BEEPER timer, canonical + pads[4].timId = 3; // BEEPER timer, sibling + + outputMode_e modes[5] = { + OUTPUT_MODE_MOTORS, + OUTPUT_MODE_LED, + OUTPUT_MODE_LED, + OUTPUT_MODE_BEEPER, + OUTPUT_MODE_BEEPER, + }; + + applyFixedOverridesToArray(pads, 5, modes, 4); + + EXPECT_TRUE(pads[0].usageFlags & (uint32_t)TIM_USE_MOTOR); + + EXPECT_TRUE(pads[1].usageFlags & (uint32_t)TIM_USE_LED); + EXPECT_FALSE(pads[1].usageFlags & (uint32_t)TIM_USE_PINIO); + + EXPECT_FALSE(pads[2].usageFlags & (uint32_t)TIM_USE_LED); + EXPECT_TRUE(pads[2].usageFlags & (uint32_t)TIM_USE_PINIO); + + EXPECT_TRUE(pads[3].usageFlags & (uint32_t)TIM_USE_BEEPER); + EXPECT_FALSE(pads[3].usageFlags & (uint32_t)TIM_USE_PINIO); + + EXPECT_FALSE(pads[4].usageFlags & (uint32_t)TIM_USE_BEEPER); + EXPECT_TRUE(pads[4].usageFlags & (uint32_t)TIM_USE_PINIO); +} + +/* ========================================================================= + * TEST SUITE: SourceSync + * + * Reads the ACTUAL LIVE src/main/drivers/pwm_mapping.c off disk (not a + * cached copy, not something computed at CMake-configure time) and checks + * that specific, logic-relevant fragments of the real timerHardwareOverride() + * and pwmBuildTimerOutputList() are still present verbatim (modulo + * whitespace and comments). If a future change to pwm_mapping.c alters this + * logic without updating the inline reproduction above, these tests fail — + * loudly, in `make check` / CI — instead of the rest of the file silently + * continuing to pass against a stale copy. + * + * This does not (and structurally cannot, given the SITL_BUILD guard around + * the entire file — see block comment above) substitute for actually linking + * and executing the real function. It is a drift-detector, not a full + * integration test. Comments are stripped and whitespace is collapsed before + * comparison so that pure reformatting/typo-fixes in comments do not cause + * false failures; only the executable-statement tokens are checked. + * ========================================================================= */ + +namespace { + +// Strips "//...\n" line comments. Deliberately does not handle /* */ block +// comments or comments inside string/char literals — pwm_mapping.c does not +// use either within the functions we check, and keeping this simple avoids +// this drift-detector itself becoming a source of drift. +std::string stripLineComments(const std::string &text) +{ + std::string result; + result.reserve(text.size()); + for (size_t i = 0; i < text.size();) { + if (text[i] == '/' && i + 1 < text.size() && text[i + 1] == '/') { + while (i < text.size() && text[i] != '\n') { + i++; + } + continue; + } + result += text[i]; + i++; + } + return result; +} + +// Collapses any run of whitespace (including newlines) to a single space, +// and trims leading/trailing whitespace, so indentation/line-wrap changes +// don't cause false failures. +std::string normalizeWhitespace(const std::string &text) +{ + std::string result; + result.reserve(text.size()); + bool lastWasSpace = true; // trims leading whitespace + for (char c : text) { + if (std::isspace(static_cast(c))) { + if (!lastWasSpace) { + result += ' '; + lastWasSpace = true; + } + } else { + result += c; + lastWasSpace = false; + } + } + while (!result.empty() && result.back() == ' ') { + result.pop_back(); + } + return result; +} + +std::string normalizeSource(const std::string &text) +{ + return normalizeWhitespace(stripLineComments(text)); +} + +// 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_led_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_led_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"; +} + +// 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."; + } + std::ostringstream buffer; + buffer << file.rdbuf(); + if (buffer.str().empty()) { + return ::testing::AssertionFailure() + << "Live source file at " << path << " opened but read as empty."; + } + *out = normalizeSource(buffer.str()); + return ::testing::AssertionSuccess(); +} + +} // namespace + +TEST(SourceSync, TimerHardwareOverrideSignatureMatchesLiveSource) +{ + std::string normalizedSource; + ASSERT_TRUE(loadNormalizedPwmMappingSource(&normalizedSource)); + + const std::string expectedSignature = normalizeSource( + "static void timerHardwareOverride(timerHardware_t * timer, " + "bool isCanonicalBeeperPad, bool isCanonicalLedPad) {"); + + EXPECT_NE(normalizedSource.find(expectedSignature), std::string::npos) + << "timerHardwareOverride()'s signature in the LIVE " + "src/main/drivers/pwm_mapping.c no longer matches what this test's " + "inline reproduction assumes. If the signature legitimately " + "changed, update timerHardwareOverride_fixed()/_buggy() in " + "pwm_mapping_led_unittest.cc (and pwm_mapping_beeper_unittest.cc) " + "to match, then update this expected string too."; +} + +TEST(SourceSync, LedCaseMatchesLiveSource) +{ + std::string normalizedSource; + ASSERT_TRUE(loadNormalizedPwmMappingSource(&normalizedSource)); + + // Comments intentionally omitted here (stripLineComments() removes them + // from the live source before comparison too) so that comment wording + // changes don't cause false failures; only the executable statements are + // checked, which is exactly the logic the FixVerification tests above + // depend on. + const std::string expectedLedCase = normalizeSource( + "case OUTPUT_MODE_LED: " + "timer->usageFlags &= ~(TIM_USE_MOTOR|TIM_USE_SERVO|TIM_USE_BEEPER|TIM_USE_LED); " + "timer->usageFlags |= TIM_USE_PINIO; " + "if (isCanonicalLedPad) { " + "timer->usageFlags &= ~TIM_USE_PINIO; " + "timer->usageFlags |= TIM_USE_LED; " + "} " + "break;"); + + EXPECT_NE(normalizedSource.find(expectedLedCase), std::string::npos) + << "The OUTPUT_MODE_LED case in the LIVE src/main/drivers/pwm_mapping.c " + "no longer matches timerHardwareOverride_fixed()'s LED case in this " + "test file. This is exactly the kind of drift that let " + "pwm_mapping_beeper_unittest.cc silently stop verifying real " + "behavior after commits 8c16aeed85/31bb99ea0b. Update the inline " + "reproduction (and the FixVerification/BugReproduction tests, if " + "the fix strategy itself changed) to match the live source."; +} + +TEST(SourceSync, CanonicalPadTrackingLoopMatchesLiveSource) +{ + std::string normalizedSource; + ASSERT_TRUE(loadNormalizedPwmMappingSource(&normalizedSource)); + + const std::string expectedLoop = normalizeSource( + "bool beeperClaimed[HARDWARE_TIMER_DEFINITION_COUNT] = { false }; " + "bool ledClaimed[HARDWARE_TIMER_DEFINITION_COUNT] = { false }; " + "for (int idx = 0; idx < timerHardwareCount; idx++) { " + "timerHardware_t *timHw = &timerHardware[idx]; " + "uint8_t timId = timer2id(timHw->tim); " + "bool isCanonicalBeeperPad = false; " + "if (timerOverrides(timId)->outputMode == OUTPUT_MODE_BEEPER && !beeperClaimed[timId]) { " + "beeperClaimed[timId] = true; " + "isCanonicalBeeperPad = true; " + "} " + "bool isCanonicalLedPad = false; " + "if (timerOverrides(timId)->outputMode == OUTPUT_MODE_LED && !ledClaimed[timId]) { " + "ledClaimed[timId] = true; " + "isCanonicalLedPad = true; " + "}"); + + EXPECT_NE(normalizedSource.find(expectedLoop), std::string::npos) + << "pwmBuildTimerOutputList()'s canonical-pad-tracking loop in the LIVE " + "src/main/drivers/pwm_mapping.c no longer matches what " + "applyFixedOverridesToArray() in this test file reproduces. Update " + "applyFixedOverridesToArray() to match the live source (e.g. if the " + "ledClaimed[]/beeperClaimed[] scanning strategy changes)."; +}