From 75ad87b82e3302aaa6de017d9cbae2b794858f05 Mon Sep 17 00:00:00 2001 From: Mario Ruiz Date: Wed, 26 Aug 2026 11:56:35 -0500 Subject: [PATCH 1/8] Set X4 Pro viewable insets to measured values Measured 2026-08-26 on the physical unit with CrossPlay's BEZEL ruler app: the glass hides 10 rows at the top, 1 column each side, 0 at the bottom. Replaces {9, 7, 3, 7}: the 9/3s were the X4's eyeballed values carried over 'pending measurement', and the 7 was a scrollbar-aesthetics constant, not a measurement. Community data (crosspoint-reader #618) puts per-unit top overlap at 5-11 rows, so units that still clip should be re-measured, not padded. --- libs/hardware/BoardConfig/include/BoardConfig.h | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/libs/hardware/BoardConfig/include/BoardConfig.h b/libs/hardware/BoardConfig/include/BoardConfig.h index ee948fe0..7266612d 100644 --- a/libs/hardware/BoardConfig/include/BoardConfig.h +++ b/libs/hardware/BoardConfig/include/BoardConfig.h @@ -1467,11 +1467,14 @@ constexpr BoardProfile XTEINK_X4_PRO = { // button ladder — that earlier assumption was wrong; the ladder pins remain unconfirmed. {1}, 0, // displayControllerVariant: filled by the boot probe - // Bezel overlap: the panel sits recessed, and 7px is the empirically-tuned - // side inset that keeps an edge-hugging scroll indicator visible (was the - // firmware's hardcoded X4 Pro scrollbar inset); top/bottom keep the X4 - // historical values pending measurement. - {9, 7, 3, 7}}; + // Bezel overlap, measured on real hardware (2026-08-26) with the BEZEL + // ruler app: the glass hides 10 rows at the top, 1 column each side, and + // nothing at the bottom -- the panel sits shifted toward its bottom flex + // cable. Per-unit variance is real (upstream #618 measured 5-11 hidden + // top rows across X4 units), so a unit that still clips should be + // re-measured with the ruler rather than padded blindly. The old side + // value (7) was a scrollbar-aesthetics constant, not a measurement. + {10, 1, 0, 1}}; // Largest framebuffer (bytes) over the devices compiled into this build, derived // from the profiles above. The display facade sizes its static framebuffer to From 0ced22080701a1203964a565798254e197f85845 Mon Sep 17 00:00:00 2001 From: Mario Ruiz Date: Wed, 26 Aug 2026 13:52:29 -0500 Subject: [PATCH 2/8] Fill DeviceContext.safeArea from the panel's viewable insets The GfxRenderer adapter now reports the bezel-covered edge pixels (BoardConfig viewableInsets, rotated into the current orientation) through DeviceContext.safeArea, so Screen's content rect -- seeded from safeRect() -- keeps every fui-laid-out screen clear of the glass. Adds Screen::setContentMarginAbsolute() for hosts whose chrome is drawn at absolute renderer coordinates and whose margins already fold the safe area in (they derive from an insets-aware rect): insetting those from safeRect() would apply the safe area twice. --- libs/ui/FreeInkUI/include/FreeInkApp.h | 8 ++++++++ libs/ui/FreeInkUI/include/FreeInkUIGfxRenderer.h | 10 ++++++++++ 2 files changed, 18 insertions(+) diff --git a/libs/ui/FreeInkUI/include/FreeInkApp.h b/libs/ui/FreeInkUI/include/FreeInkApp.h index d56a72fb..a5a27b46 100644 --- a/libs/ui/FreeInkUI/include/FreeInkApp.h +++ b/libs/ui/FreeInkUI/include/FreeInkApp.h @@ -65,6 +65,14 @@ template class Screen { void setContentMargin(Insets margin) { content_ = insetClamped(frame_.safeRect(), margin); } + // Margins measured against the FULL screen frame, for hosts whose chrome is + // drawn at absolute renderer coordinates and whose margins already fold the + // device's safe area in (they derive from an insets-aware safe-area rect). + // setContentMargin() measures from safeRect(); using it with absolute + // margins applies the safe area twice. + void setContentMarginAbsolute(Insets margin) { + content_ = insetClamped(frame_.screen(), margin); + } void insetContent(Insets margin) { content_ = insetClamped(content_, margin); } diff --git a/libs/ui/FreeInkUI/include/FreeInkUIGfxRenderer.h b/libs/ui/FreeInkUI/include/FreeInkUIGfxRenderer.h index df7147d4..f37235cd 100644 --- a/libs/ui/FreeInkUI/include/FreeInkUIGfxRenderer.h +++ b/libs/ui/FreeInkUI/include/FreeInkUIGfxRenderer.h @@ -48,6 +48,16 @@ class GfxRendererTarget final : public DrawTarget { device.width = static_cast(renderer.getScreenWidth()); device.height = static_cast(renderer.getScreenHeight()); device.orientation = static_cast(renderer.getOrientation()); + // The panel's bezel-covered edge pixels (BoardConfig viewableInsets, + // rotated into this orientation). Screen seeds its content rect from + // safeRect(), so everything laid out through it clears the glass; + // deliberate full-bleed paint keeps using screen(). + { + int top = 0, right = 0, bottom = 0, left = 0; + renderer.getOrientedViewableTRBL(&top, &right, &bottom, &left); + device.safeArea = Insets{static_cast(top), static_cast(right), + static_cast(bottom), static_cast(left)}; + } // Same inverse-of-render-rotation selector as DisplayTarget: this makes // touchToLogical() agree case-for-case with GfxRenderer::tapToLogical, so // FreeInkUI components and the firmware's own tap path map taps identically. From 2b717e95d9e746688ae843a9f028faea65b8f9b2 Mon Sep 17 00:00:00 2001 From: Mario Ruiz Date: Wed, 26 Aug 2026 14:28:49 -0500 Subject: [PATCH 3/8] Add Screen::header(props, rect) Themed header at an explicit rect, sharing the anchor overload's theme substitution: for chrome that positions the band itself, e.g. a band whose visible top tracks the device safe area while the layout below keeps its own geometry. Does not consume body space. --- libs/ui/FreeInkUI/include/FreeInkApp.h | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/libs/ui/FreeInkUI/include/FreeInkApp.h b/libs/ui/FreeInkUI/include/FreeInkApp.h index a5a27b46..4c75ff7f 100644 --- a/libs/ui/FreeInkUI/include/FreeInkApp.h +++ b/libs/ui/FreeInkUI/include/FreeInkApp.h @@ -140,6 +140,14 @@ template class Screen { void header(const HeaderProps &props, LayoutAnchor anchor = LayoutAnchor::Top) { + header(props, take(anchor, theme_.headerHeight)); + } + + // Themed header at an explicit rect, for chrome that positions the band + // itself (e.g. a band whose visible top tracks the device safe area while + // the layout below keeps its own geometry). Does not consume body space -- + // pair with takeTop when the band should be reserved. + void header(const HeaderProps &props, Rect rect) { HeaderProps themed = props; if (textStyleUnset(themed.titleText)) { themed.titleText = theme_.titleText; @@ -165,7 +173,7 @@ template class Screen { themed.styles.normal.borderWidth = theme_.headerUnderline; } themed.minTouchSize = theme_.minTouchSize; - ui::header(frame_, take(anchor, theme_.headerHeight), themed); + ui::header(frame_, rect, themed); } // Sub-screen chrome: leading back button + centered title, with an optional From 1eecf93d1045bf7ea5e351145116e3f27d31f38b Mon Sep 17 00:00:00 2001 From: Mario Ruiz Date: Mon, 31 Aug 2026 20:19:34 -0500 Subject: [PATCH 4/8] fix(x4pro): the frontlight PWM must fit RC_FAST, or it never lights Since v1.11.1 the X4 Pro frontlight cannot be turned on. Not dimmer, not flickering: the LEDC channels are never attached, so no code path that writes a duty reaches the pads. The panel opens on the top-edge swipe, the sun icon fills, the power double-click toggles, the reader menu says ON, and the settings file records frontlightOn=1. Nothing lights up. Under FREEINK_FRONTLIGHT_LS the timer is clocked from RC_FAST so the PWM survives light sleep. RC_FAST is ~17.5 MHz on the S3, and LEDC forms its divisor as (clk << 8) / (freq * 2^bits), rejecting any result at or below 255. So a (frequency, resolution) pair is achievable only while freq * 2^bits fits inside the clock. The X4 Pro profile carried 10 kHz / 10-bit from first bring-up, which needs 10.24 MHz and fits. Upstream 13d23ba ("Update X4 Pro display driver") moved it to the 25 kHz the stock firmware passes to the same initializer. That needs 25.6 MHz, ledc_timer_config() returns ESP_FAIL, attachChannel() returns false, and begin() logs ok=0 and carries on. 25 kHz is not a mistake upstream made. It is what stock 7.0.8 really uses, and stock can afford it because it clocks LEDC from APB and lets the light die in sleep. The fork chose the other side of that trade deliberately, which is what FREEINK_FRONTLIGHT_LS is, and 25 kHz is simply incompatible with it. Anyone re-recovering the value from stock should read this before reverting the revert. Back to 10 kHz. Not 25 kHz at 9-bit, which also fits: the gamma table's exponent is chosen so 1% is exactly 1 LSB on a 10-bit range, and setBrightnessLevel's level-1 night minimum is the dimmest step that range can express. Halving it to buy a frequency nothing here needs is a worse trade. But 10 kHz is only today's value of the fix. The fix is the static_assert: the constraint now holds the profile this build ships, exempting PMIC frontlights (viaPm1Pwm) and boards with none, since neither reaches the timer. It asserts the RELATIONSHIP rather than the frequency, so raising the resolution breaks the build as loudly as raising the frequency does: 10 kHz at 11-bit is 20.48 MHz and fails it too. An assert pinned to freq == 10000 would have caught this regression and no other. Checked against a mirror of ledc_calculate_divisor and LEDC_IS_DIV_INVALID on six pairs including both boundaries; the guard and the IDF rule agree on all six. Nothing in the gate could have caught the original. It is a device-only failure with no crash, no wrong pixel and no failed build, and the only reason it surfaced is that Mario tried to turn the light on. So the runtime path now says WHY it gave up, at ERR with the numbers, instead of leaving ok=0 to be interpreted: a session read that exact log on that exact device the night this shipped and learned nothing from it. Verified: the arithmetic above, and the x4pro and gh_release_x4pro builds under check.sh --committed. NOT verified on hardware: the device was asleep and off the network while this was written. --- .../BoardConfig/include/BoardConfig.h | 16 +++++-- .../src/FrontlightManager.cpp | 47 +++++++++++++++---- 2 files changed, 52 insertions(+), 11 deletions(-) diff --git a/libs/hardware/BoardConfig/include/BoardConfig.h b/libs/hardware/BoardConfig/include/BoardConfig.h index 69fbe20c..5c306989 100644 --- a/libs/hardware/BoardConfig/include/BoardConfig.h +++ b/libs/hardware/BoardConfig/include/BoardConfig.h @@ -1561,13 +1561,23 @@ constexpr BoardProfile XTEINK_X4_PRO = { // Frontlight: dual warm/cold LEDC PWM with color temperature (NVS lightWarmValue/ // lightColdValue/lightCT/lightBri/lightOn). Recovered from the OEM LEDC init (IROM // 0x420a2130 → helper 0x420a20c0): two channels — GPIO8 on LEDC ch4 and GPIO9 on ch5 — - // The original bring-up dump used 10 kHz; stock 7.0.8 passes 25 kHz / 10-bit to - // the frontlight initializer on the same pins. Use that directly recovered value. // Both channels are active-HIGH (init drives the pin LOW = off, brightness raises // duty). // GPIO8 is the hardware-confirmed cool channel and GPIO9 the warm channel; // FrontlightManager mixes them for color-temperature control. - {8, 25000, 10, true, 9}, + // + // 10 kHz, and NOT the 25 kHz stock 7.0.8 passes to the same initializer, because + // this build does not clock LEDC the way stock does. Under FREEINK_FRONTLIGHT_LS + // the timer runs off RC_FAST (~17.5 MHz) so the PWM survives light sleep, and LEDC + // can only divide that down while freq * 2^bits fits inside it. 25 kHz * 1024 = + // 25.6 MHz does not fit: ledc_timer_config() returns ESP_FAIL, the channels are + // never attached, and the frontlight cannot be turned on at all, with no crash and + // no failed build. That is what 25 kHz shipped as in v1.11.1. 10 kHz * 1024 = + // 10.24 MHz fits, and keeps the full 10-bit range setBrightnessLevel's level-1 + // night minimum and the gamma table are both tuned for. Stock affords 25 kHz by + // clocking LEDC from APB and letting the light die in sleep. + // The static_assert in FrontlightManager.cpp holds this pair to the constraint. + {8, 10000, 10, true, 9}, NO_AUDIO, NO_LEDS, NO_FLIP, // panel mount transform pending hardware; native SSD1677 scan is 800x480 landscape diff --git a/libs/hardware/FrontlightManager/src/FrontlightManager.cpp b/libs/hardware/FrontlightManager/src/FrontlightManager.cpp index 1ee59e94..01550259 100644 --- a/libs/hardware/FrontlightManager/src/FrontlightManager.cpp +++ b/libs/hardware/FrontlightManager/src/FrontlightManager.cpp @@ -8,6 +8,7 @@ #ifdef FREEINK_FRONTLIGHT_LS #include #include +#include // esp_sleep_sub_mode_config lives in a private IDF header (no public API exists // for balancing the refcounted RC_FAST keep-on the LEDC driver takes for // KEEP_ALIVE channels — the driver manages it through this same header). Pinned @@ -85,12 +86,38 @@ uint32_t physicalDuty(uint32_t logicalDuty, uint32_t full, bool activeHigh) { // sleep current), mark the // channels KEEP_ALIVE, and disable the GPIO sleep-isolation override on the // output pins (a documented gotcha: sleep entry reconfigures the pad and kills -// the PWM even when the clock survives). RC_FAST at 10 kHz supports up to -// 10-bit resolution (17.5 MHz / 10 kHz = 1750 >= 1024), so the board profiles' -// full duty range — including setBrightnessLevel's level-1 minimum step — stays -// expressible. Uses the IDF driver directly (fixed LEDC_TIMER_0 + the channel -// ids below) because the Arduino helpers don't expose sleep_mode; safe here -// because frontlight boards using this flag have no other LEDC consumer. +// the PWM even when the clock survives). +// Uses the IDF driver directly (fixed LEDC_TIMER_0 + the channel ids below) +// because the Arduino helpers don't expose sleep_mode; safe here because +// frontlight boards using this flag have no other LEDC consumer. +// +// RC_FAST also bounds which (frequency, resolution) pairs exist at all. LEDC +// forms its divisor as (clk << 8) / (freq * 2^bits) and rejects any result at +// or below 255 (LEDC_IS_DIV_INVALID), so a pair is achievable exactly while +// freq * 2^bits stays within the clock. Miss it and ledc_timer_config() +// returns ESP_FAIL, attachChannel() below leaves the channels unattached, and +// the frontlight cannot be turned on AT ALL: no crash, no failed build, every +// UI control still moving and the sun icon still toggling. That shipped on the +// X4 Pro in v1.11.1, when an upstream profile edit took its frequency from +// 10 kHz to 25 kHz and 25 kHz * 1024 = 25.6 MHz stopped fitting in 17.5 MHz. +// Hardware is the worst place to find this, so the build refuses the pair. +constexpr bool fitsRcFast(const uint32_t freq, const uint8_t bits) { + // <= is the conservative form: equality yields divisor 256, one above the + // reject threshold. The true boundary is 256/255 higher, and the driver + // divides by a CALIBRATED RC_FAST that drifts from this nominal figure, so + // the extra 0.4% is not worth claiming. + return (static_cast(freq) << bits) <= SOC_CLK_RC_FAST_FREQ_APPROX; +} +// Scoped to the profile this build ships, and only when it drives LEDC pins: a +// PMIC frontlight (viaPm1Pwm) and a board with none never reach the timer. +static_assert(BoardConfig::DEFAULT_DEVICE.frontlight.gpio == BoardConfig::PIN_UNASSIGNED || + BoardConfig::DEFAULT_DEVICE.frontlight.viaPm1Pwm || + fitsRcFast(BoardConfig::DEFAULT_DEVICE.frontlight.pwmFrequency, + BoardConfig::DEFAULT_DEVICE.frontlight.pwmResolutionBits), + "FREEINK_FRONTLIGHT_LS: this board's frontlight pwmFrequency * 2^pwmResolutionBits " + "exceeds RC_FAST, so LEDC cannot attach the channels and the light would never turn " + "on. Lower the frequency or the resolution in its BoardConfig profile."); + bool attachChannel(int8_t gpio, uint8_t ch, uint32_t freq, uint8_t bits) { ledc_timer_config_t timer = {}; timer.speed_mode = LEDC_LOW_SPEED_MODE; @@ -99,8 +126,12 @@ bool attachChannel(int8_t gpio, uint8_t ch, uint32_t freq, uint8_t bits) { timer.freq_hz = freq; timer.clk_cfg = LEDC_USE_RC_FAST_CLK; if (ledc_timer_config(&timer) != ESP_OK) { - // freq/bits exceed RC_FAST — leave the light unconfigured rather than - // silently falling back to a clock that freezes in light sleep. + // Unreachable for the shipped profile (the static_assert above), so this is + // the belt for a runtime-selected board. Say WHY: begin()'s ok=0 on its own + // names no cause, and that is what took this one three releases to find. + LOG_ERR("FrontlightMgr", "RC_FAST cannot clock %u Hz at %u-bit (needs %llu Hz <= %u); light stays dark", freq, + bits, static_cast(static_cast(freq) << bits), + static_cast(SOC_CLK_RC_FAST_FREQ_APPROX)); return false; } ledc_channel_config_t chan = {}; From 74d30f92bde6c9dc49e1ee461380f3d2930364fb Mon Sep 17 00:00:00 2001 From: Mario Ruiz Date: Mon, 31 Aug 2026 20:26:40 -0500 Subject: [PATCH 5/8] fix: a frontlight that did not come up must not look like one nobody used begin() returned void, so no consumer could tell a light that configured from one that did not. Every signal short of a human eye reported success: the firmware logged "Frontlight up: 3% warm=100% off" on a device whose LEDC channels had never attached, the panel opened, the sun icon filled, and the settings file recorded frontlightOn=1. That is how the X4 Pro shipped two releases with a light that could not be turned on, and why a session reading /api/dev/log on the affected unit learned nothing from it. begin() now returns whether the light can actually be driven, and every path says so explicitly: false when the board has none, when the EEGO A4's LM3630A does not ACK, when the primary pin is unassigned, or when the LEDC channels would not configure. The static_assert in the previous commit proves THIS build cannot ship the bad pair. It says nothing about another board, another clock source, or any failure that only exists at runtime. A checked return covers those, and it is the half that generalises. --- .../FrontlightManager/include/FrontlightManager.h | 12 ++++++++++-- .../FrontlightManager/src/FrontlightManager.cpp | 15 +++++++++------ 2 files changed, 19 insertions(+), 8 deletions(-) diff --git a/libs/hardware/FrontlightManager/include/FrontlightManager.h b/libs/hardware/FrontlightManager/include/FrontlightManager.h index bdcb0e0f..295b1270 100644 --- a/libs/hardware/FrontlightManager/include/FrontlightManager.h +++ b/libs/hardware/FrontlightManager/include/FrontlightManager.h @@ -18,8 +18,16 @@ class FrontlightManager { public: - // Bring up the PWM channel(s). No-op if the board has no frontlight. - void begin(); + // Bring up the PWM channel(s), and say whether the light can now be driven. + // + // FALSE means every later setBrightness()/on()/setColorTemperature() will be + // accepted and do nothing: the board has no frontlight, or its channels + // would not configure. Check it. Until 2026-08-31 this returned void, so a + // consumer had no way to tell a light that came up from one that did not, + // and the X4 Pro shipped two releases whose LEDC channels never attached + // while the firmware logged "Frontlight up" on the way past. Every signal + // short of a human eye reported success, which is why it survived a release. + bool begin(); // Set brightness as a 0-100 percentage, mapped to duty through a perceptual // gamma-1.6554 curve (1% is the smallest non-zero duty step, not 1% linear diff --git a/libs/hardware/FrontlightManager/src/FrontlightManager.cpp b/libs/hardware/FrontlightManager/src/FrontlightManager.cpp index 01550259..0a4999e7 100644 --- a/libs/hardware/FrontlightManager/src/FrontlightManager.cpp +++ b/libs/hardware/FrontlightManager/src/FrontlightManager.cpp @@ -165,14 +165,14 @@ void writeChannel(int8_t /*gpio*/, uint8_t ch, uint32_t duty) { ledcWrite(ch, du } // namespace #endif -void FrontlightManager::begin() { +bool FrontlightManager::begin() { #if FREEINK_CAP_FRONTLIGHT const auto& fl = BoardConfig::ACTIVE.frontlight; #if FREEINK_DEVICE_EEGO_A4 const auto& i2c = BoardConfig::ACTIVE.i2cFrontlight; if (BoardConfig::ACTIVE.board == BoardConfig::Board::EegoA4 && i2c.controller == BoardConfig::I2cFrontlightController::Lm3630a) { - if (i2c.sda < 0 || i2c.scl < 0 || i2c.enable < 0 || i2c.address == 0) return; + if (i2c.sda < 0 || i2c.scl < 0 || i2c.enable < 0 || i2c.address == 0) return false; Wire.begin(i2c.sda, i2c.scl, i2c.i2cHz); Wire.setTimeOut(256); pinMode(i2c.enable, OUTPUT); @@ -187,20 +187,20 @@ void FrontlightManager::begin() { Wire.beginTransmission(i2c.address); const bool detected = Wire.endTransmission() == 0; digitalWrite(i2c.enable, LOW); - if (!detected) return; + if (!detected) return false; _begun = true; _brightness = 0; - return; + return true; } #endif if (fl.viaPm1Pwm) { pm1FrontlightAttach(fl.pwmFrequency); _begun = true; setBrightness(0); - return; + return true; } - if (fl.gpio == BoardConfig::PIN_UNASSIGNED) return; + if (fl.gpio == BoardConfig::PIN_UNASSIGNED) return false; bool attachOk = attachChannel(fl.gpio, LEDC_CH_COOL, fl.pwmFrequency, fl.pwmResolutionBits); if (fl.gpioWarm != BoardConfig::PIN_UNASSIGNED) { @@ -238,6 +238,9 @@ void FrontlightManager::begin() { _begun = true; setBrightness(0); LOG_INF("FrontlightMgr", "begin: attached gpio=%d warm=%d ok=%d", fl.gpio, fl.gpioWarm, attachOk ? 1 : 0); + return attachOk; +#else + return false; #endif } From 979fac9619dfbc2be3182b8b2061fff750b913dc Mon Sep 17 00:00:00 2001 From: Mario Ruiz Date: Mon, 31 Aug 2026 21:02:36 -0500 Subject: [PATCH 6/8] docs: two releases, not three The comment beside attachChannel() said this took three releases to find. It shipped in exactly two, v1.11.1 and v1.12.0, about six hours apart on 2026-08-30/31. The commit messages were corrected against the tag dates when they were written and this copy was not, which makes it a derived fact written as a literal inside the fix for derived facts written as literals. --- libs/hardware/FrontlightManager/src/FrontlightManager.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/libs/hardware/FrontlightManager/src/FrontlightManager.cpp b/libs/hardware/FrontlightManager/src/FrontlightManager.cpp index 0a4999e7..264fd424 100644 --- a/libs/hardware/FrontlightManager/src/FrontlightManager.cpp +++ b/libs/hardware/FrontlightManager/src/FrontlightManager.cpp @@ -128,7 +128,8 @@ bool attachChannel(int8_t gpio, uint8_t ch, uint32_t freq, uint8_t bits) { if (ledc_timer_config(&timer) != ESP_OK) { // Unreachable for the shipped profile (the static_assert above), so this is // the belt for a runtime-selected board. Say WHY: begin()'s ok=0 on its own - // names no cause, and that is what took this one three releases to find. + // names no cause, which is most of why this one reached two published + // releases with a session reading the log on an affected device. LOG_ERR("FrontlightMgr", "RC_FAST cannot clock %u Hz at %u-bit (needs %llu Hz <= %u); light stays dark", freq, bits, static_cast(static_cast(freq) << bits), static_cast(SOC_CLK_RC_FAST_FREQ_APPROX)); From 7d15d586143701456e58143934ea551897056a08 Mon Sep 17 00:00:00 2001 From: Mario Ruiz Date: Mon, 31 Aug 2026 21:43:40 -0500 Subject: [PATCH 7/8] feat(sd): free space that can say it does not know Nothing above SDCardManager surfaced free space, so every app on the fork that writes to the card writes blind. Trivia declined to ship a 6.21MB download onto the one card holding a live Anki collection for exactly that reason, and it was right to. The numbers existed. sdTotalBytes() and sdUsedBytes() have been here all along -- with ZERO callers anywhere, which is the only reason the next part is safe to leave alone. sdUsedBytes() RETURNS 0 WHEN THE QUERY FAILS. Zero used bytes is also what an empty card reports, so sdTotalBytes() - sdUsedBytes() renders a failed query as an almost-empty card: the one direction an error can point that turns a safety check into permission to write anyway. Worse, line 386 marks that zero as a VALID cached answer for the full 20s TTL, so a retry inside the window gets the same confident zero without re-querying. It does not report the lie once, it memoizes it. So sdFreeBytes(uint64_t&) reports failure as FALSE with the out-param untouched, and never as a number. False means unknown, and a caller must not read it as room. Both accessors now share one cached FAT walk. freeClusterCount() on FAT32 without a valid FSInfo walks the whole table, seconds on a large card, and running it twice for one question was the obvious way to make this unusable in the download path it exists for. sdUsedBytes() keeps its shape because its contract is upstream's and changing the signature would diverge a shared header for no caller. What it no longer does is fail quietly: the failing path now says so on Serial, in the file's own idiom, once per refresh so the TTL bounds it. Trivia's point, and correct -- wrapping a trap leaves it armed for whoever reaches past the wrapper. Verified: zero existing callers, counted rather than assumed. --- .../SDCardManager/include/SDCardManager.h | 21 +++++++ .../SDCardManager/src/SDCardManager.cpp | 59 ++++++++++++++----- 2 files changed, 65 insertions(+), 15 deletions(-) diff --git a/libs/hardware/SDCardManager/include/SDCardManager.h b/libs/hardware/SDCardManager/include/SDCardManager.h index 136442c5..c96e70f7 100644 --- a/libs/hardware/SDCardManager/include/SDCardManager.h +++ b/libs/hardware/SDCardManager/include/SDCardManager.h @@ -37,7 +37,22 @@ class SDCardManager { // Returns used space in bytes, cached with a 20-second TTL (freeClusterCount // scans the FAT and is too slow to call on every frame). 0 if not mounted or // the cluster count cannot be determined. + // Bytes in use. RETURNS 0 WHEN IT CANNOT ANSWER, which is also what an empty + // card returns, and caches that for the TTL -- so do not subtract it from + // sdTotalBytes() to get free space. Use sdFreeBytes(), which cannot make + // that mistake. Kept in this shape because it predates sdFreeBytes and its + // contract is upstream's; the failure now announces itself on Serial. uint64_t sdUsedBytes(); + + // Free bytes on the card, and whether the volume could actually answer. + // + // Separate from sdUsedBytes() on purpose. That one returns 0 when + // freeClusterCount() fails, which is indistinguishable from a card with + // nothing on it -- so `sdTotalBytes() - sdUsedBytes()` reports a FAILURE as + // an almost-empty card. That is the one direction the error can point that + // turns "check before writing" into "write anyway", which is the whole + // reason a caller asks. Here a failure is false and out is untouched. + bool sdFreeBytes(uint64_t& out); std::vector listFiles(const char* path = "/", int maxFiles = 200); // Read the entire file at `path` into a String. Returns empty string on failure. String readFile(const char* path); @@ -100,6 +115,12 @@ class SDCardManager { uint64_t cachedUsedBytes = 0; uint32_t cachedUsedBytesAt = 0; bool cachedUsedBytesValid = false; + uint64_t cachedFreeBytes = 0; + // Whether the last refresh actually got an answer, as opposed to caching a + // zero it could not vouch for. + bool cachedFreeBytesValid = false; + // One FAT walk per TTL, shared by both accessors above. + bool refreshFreeClusters(); // All filesystem ops route through one FsVolume& so the backend is swappable. // SPI boards: `sd` (SdFs is-a FsVolume). SDMMC boards: a bare FsVolume mounted diff --git a/libs/hardware/SDCardManager/src/SDCardManager.cpp b/libs/hardware/SDCardManager/src/SDCardManager.cpp index 8a2af346..5890846b 100644 --- a/libs/hardware/SDCardManager/src/SDCardManager.cpp +++ b/libs/hardware/SDCardManager/src/SDCardManager.cpp @@ -369,23 +369,52 @@ bool SDCardManager::openFileForWrite(const char* moduleName, const String& path, uint64_t SDCardManager::sdTotalBytes() const { return cachedTotalBytes; } -uint64_t SDCardManager::sdUsedBytes() { - if (!initialized) return 0; +// Shared by sdUsedBytes() and sdFreeBytes() so both read ONE cached answer. +// freeClusterCount() walks the FAT on FAT32 without a valid FSInfo, which is +// seconds on a large card, so it must not run twice for one question. +bool SDCardManager::refreshFreeClusters() { + if (!initialized) return false; const uint32_t now = millis(); - if (!cachedUsedBytesValid || (now - cachedUsedBytesAt) >= USED_BYTES_CACHE_TTL_MS) { - const int32_t freeClusters = vol().freeClusterCount(); - const uint64_t clusterCount = vol().clusterCount(); - if (freeClusters < 0) { - cachedUsedBytes = 0; - } else { - const uint64_t cappedFree = (static_cast(freeClusters) > clusterCount) - ? clusterCount - : static_cast(freeClusters); - cachedUsedBytes = (clusterCount - cappedFree) * vol().bytesPerCluster(); - } - cachedUsedBytesValid = true; - cachedUsedBytesAt = now; + if (cachedUsedBytesValid && (now - cachedUsedBytesAt) < USED_BYTES_CACHE_TTL_MS) { + return cachedFreeBytesValid; + } + const int32_t freeClusters = vol().freeClusterCount(); + const uint64_t clusterCount = vol().clusterCount(); + cachedUsedBytesAt = now; + cachedUsedBytesValid = true; + if (freeClusters < 0) { + // sdUsedBytes() reports this as 0 used, which is also what an empty card + // reports, and it is cached as a valid answer for the whole TTL -- so a + // retry inside the window gets the same confident zero without asking + // again. Nothing in this fork reads it, and sdFreeBytes() below refuses to + // conflate the two, but a future caller reaching past it would get a + // plausible number. Say so out loud rather than leaving the trace to the + // absence of one. Once per refresh, so the TTL bounds the noise. + if (Serial) Serial.printf("[%lu] [SD] free-cluster query FAILED; used-bytes will read 0, which is a lie\n", millis()); + cachedUsedBytes = 0; + cachedFreeBytes = 0; + cachedFreeBytesValid = false; + return false; } + const uint64_t cappedFree = + (static_cast(freeClusters) > clusterCount) ? clusterCount : static_cast(freeClusters); + cachedUsedBytes = (clusterCount - cappedFree) * vol().bytesPerCluster(); + cachedFreeBytes = cappedFree * vol().bytesPerCluster(); + cachedFreeBytesValid = true; + return true; +} + +bool SDCardManager::sdFreeBytes(uint64_t& out) { + if (!refreshFreeClusters()) return false; + out = cachedFreeBytes; + return true; +} + +uint64_t SDCardManager::sdUsedBytes() { + // Unchanged contract: 0 when the volume cannot answer, which several callers + // already treat as "unknown or empty". sdFreeBytes() is the one that tells + // those two apart. + refreshFreeClusters(); return cachedUsedBytes; } From c444616478ace8621734242347ab05679a4902f2 Mon Sep 17 00:00:00 2001 From: Mario Ruiz Date: Mon, 31 Aug 2026 23:55:06 -0500 Subject: [PATCH 8/8] fix(sd): a failed free-cluster query must not be cached as an answer refreshFreeClusters() set cachedUsedBytesValid = true BEFORE the failure branch, so a failed walk was memoised for the whole 20s TTL. The guard at the top then returned the stale false without re-querying, and every caller's retry was dead for twenty seconds. That lands hardest on the screen that exists to be retried. Trivia shows "NO ROOM" with the numbers and a TRY AGAIN button. A user deletes a book, presses it inside the window, is told there is still no room, and reasonably concludes deleting did not help. The screen punishes the correct response, and no wording rescues a button that cannot work. A cache stores ANSWERS, and "the card did not answer" is not one. So the failure branch now clears cachedUsedBytesValid and only the success path sets it. The next call re-queries, which is exactly what a person pressing TRY AGAIN is asking for. Chosen over the two alternatives considered. Invalidating the cache from the retry path, and a force flag on the query, both push cache knowledge into every caller; the flag additionally hands apps a documented way to defeat the TTL, which exists to stop a seconds-long FAT walk running per chunk of a download. Leaving a failure uncached costs one re-query per button press, and a button press is user-paced. Also corrects the comment above it, which said the failure "is cached as a valid answer for the whole TTL". True when written, false now, and it would have outlived the behaviour it described. Found by review, not by a suite: the code was gated and mutation-tested with the defect present, because no test presses a button twice inside twenty seconds. Docs: none affected -- checked; behaviour is described in the comment above the branch and in the workspace memory. --- .../SDCardManager/src/SDCardManager.cpp | 30 +++++++++++++++---- 1 file changed, 24 insertions(+), 6 deletions(-) diff --git a/libs/hardware/SDCardManager/src/SDCardManager.cpp b/libs/hardware/SDCardManager/src/SDCardManager.cpp index 5890846b..4dc970a5 100644 --- a/libs/hardware/SDCardManager/src/SDCardManager.cpp +++ b/libs/hardware/SDCardManager/src/SDCardManager.cpp @@ -381,21 +381,39 @@ bool SDCardManager::refreshFreeClusters() { const int32_t freeClusters = vol().freeClusterCount(); const uint64_t clusterCount = vol().clusterCount(); cachedUsedBytesAt = now; - cachedUsedBytesValid = true; if (freeClusters < 0) { // sdUsedBytes() reports this as 0 used, which is also what an empty card - // reports, and it is cached as a valid answer for the whole TTL -- so a - // retry inside the window gets the same confident zero without asking - // again. Nothing in this fork reads it, and sdFreeBytes() below refuses to - // conflate the two, but a future caller reaching past it would get a + // reports. Nothing in this fork reads it, and sdFreeBytes() below refuses + // to conflate the two, but a future caller reaching past it would get a // plausible number. Say so out loud rather than leaving the trace to the - // absence of one. Once per refresh, so the TTL bounds the noise. + // absence of one. + // + // This used to read "and it is cached as a valid answer for the whole TTL, + // so a retry inside the window gets the same confident zero". That was + // true, and it was the bug fixed just below: the failure is no longer + // memoised, so a retry re-queries. The log line is therefore once per + // FAILED ATTEMPT rather than once per TTL -- user-paced, since only a + // button press drives it. if (Serial) Serial.printf("[%lu] [SD] free-cluster query FAILED; used-bytes will read 0, which is a lie\n", millis()); cachedUsedBytes = 0; cachedFreeBytes = 0; cachedFreeBytesValid = false; + // Do NOT memoise the failure. A cache stores ANSWERS, and "the card did not + // answer" is not one -- caching it made the guard above return the stale + // false for the whole TTL without re-querying, so an app's TRY AGAIN could + // not succeed for 20 seconds. That is worst on a NO ROOM screen: the user + // deletes a book, retries inside the window, is told there is still no + // room, and reasonably concludes deleting did not help. The screen punishes + // the correct response, and no wording rescues a button that cannot work. + // + // Chosen over a force flag on the retry path: a flag puts cache knowledge + // in every caller and can be used to defeat the TTL entirely, which is the + // seconds-long walk this cache exists to prevent. Leaving a failure + // uncached costs one re-query per user-paced button press. + cachedUsedBytesValid = false; return false; } + cachedUsedBytesValid = true; const uint64_t cappedFree = (static_cast(freeClusters) > clusterCount) ? clusterCount : static_cast(freeClusters); cachedUsedBytes = (clusterCount - cappedFree) * vol().bytesPerCluster();