fix(t-watch-ultra): build with the esp32s3 flags, not the classic-ESP32 ones - #11619
fix(t-watch-ultra): build with the esp32s3 flags, not the classic-ESP32 ones#11619Ixitxachitl wants to merge 2 commits into
Conversation
…32 ones
The env was the only esp32s3 variant extending ${esp32_base.build_flags} (since
meshtastic#8171). That base adds -D ESP32_FORCE_IRAM_MEMSET -Wl,--wrap=memset
-Wl,--wrap=memcpy, and the wrappers in IramMemcpy.c/IramMemset.c decide whether
the cache is on by reading 0x3FF00040 - DPORT_PRO_CACHE_CTRL_REG on the classic
ESP32, an address the S3 does not map at all (soc.h: DRAM 0x3FC88000-0x3FD00000,
DROM 0x3C000000-0x3E000000, IRAM 0x40370000-0x403E0000, peripherals 0x60000000).
--wrap is link-wide, so every memcpy/memset in the image - including inside the
precompiled WiFi, lwIP and flash driver libraries - branched on that undefined
read. Two long-standing board-specific bugs came from it, both dating to meshtastic#8171,
which introduced the wrong base and the first workaround in the same commit:
* WPA2 networks associated and completed the 4-way handshake, then never got a
DHCP lease, while open networks worked normally (meshtastic#11513).
* Direct flash reads returned 0x00 for data that was correct on flash, so NVS
came up empty every boot and dropped BLE bonds (meshtastic#11530).
Switching the env to esp32s3_base fixes both on hardware: WPA2 gets a lease, and
NVS survives a reboot with the bond intact. The read workaround that meshtastic#11530
needed - -Wl,--wrap=esp_partition_read, -Wl,--wrap=esp_flash_read and
esp_partition_read_mmap_wrap.c - is therefore removed as well.
The module excludes the env inherited from esp32_base go with it, so the board
now matches every other esp32s3 variant: web server and paxcounter are built
(paxcounter still only runs when enabled in config), and MESHTASTIC_EXCLUDE_AUDIO
was already inert here because AudioModule additionally requires USE_SX1280.
-UMESHTASTIC_EXCLUDE_ACCELEROMETER goes too, having only existed to undo an
inherited -D.
Also guards ESP32_FORCE_IRAM_MEMSET behind CONFIG_IDF_TARGET_ESP32, so a variant
cannot enable the classic-ESP32 probe on another target again.
⚡ Try this PR in the Web FlasherNote Building this pull request… the flash button, badges and supported-board |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
💤 Files with no reviewable changes (1)
Included review availability: Your plan provides up to 8 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughThe change limits IRAM cache probes to classic ESP32 targets, updates the T-Watch Ultra S3 build flags, and removes its flash-read workaround wrappers. ChangesESP32 target build compatibility
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This change corrects the T-Watch Ultra’s ESP32-S3 build flags and removes the obsolete flash-read workaround; the reported Wi-Fi, NVS, and device-function tests pass, and no actionable merge-blocking risk remains beyond normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Docstring CoverageExplanation No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 2 files. (1 skipped: 1 unsupported.) ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
3 participants
Summary
variants/esp32s3/t-watch-ultra/platformio.iniwas the only ESP32-S3 variant in the tree extending${esp32_base.build_flags}— the classic ESP32 base — instead of${esp32s3_base.build_flags}. It has been that way since the original board support PR (#8171). Every other S3 board (t-deck, t-watch-s3, tlora-pager, mesh-tab, esp32-s3-pico, elecrow_panel, hackaday-communicator, seeed-sensecap-indicator) uses the S3 base.That one line is the cause of two long-standing, T-Watch-Ultra-only bugs.
Root cause
[esp32_base]adds-D ESP32_FORCE_IRAM_MEMSET -Wl,--wrap=memset -Wl,--wrap=memcpy. The wrappers insrc/platform/esp32/IramMemcpy.c/IramMemset.care a classic-ESP32 boot workaround, and they decide whether the cache is enabled by reading0x3FF00040—DPORT_PRO_CACHE_CTRL_REGon the ESP32.On the ESP32-S3 that address is not mapped at all. From
soc/esp32s3/include/soc/soc.h: DRAM0x3FC88000–0x3FD00000, DROM0x3C000000–0x3E000000, IRAM0x40370000–0x403E0000, peripherals at0x60000000.From the shipped T-Watch Ultra ELF before this change:
--wrapis link-wide, so this sat underneath everymemcpy/memsetin the image, including those inside the precompiled WiFi, lwIP and flash-driver libraries — each one branching on an undefined read.What this fixes
WiFi (#11513) — WPA2 networks associated and completed the 4-way handshake, then never received a DHCP OFFER, while open networks worked normally. Reported independently on a second unit; a T-Watch S3 on the same AP was unaffected.
NVS (#11530, closed) — direct
esp_flash_read/esp_partition_readreturned0x00for data that was physically correct on flash, so NVS initialised empty on every boot and BLE bonds never persisted. This was diagnosed as an IDF 5.5 regression specific to the board's W25Q128JW and worked around with-Wl,--wrap=esp_partition_read,-Wl,--wrap=esp_flash_readandsrc/platform/esp32/esp_partition_read_mmap_wrap.c.It was never a flash-chip regression. #8171 introduced the wrong base and the read workaround in the same commit, so the direct read path had never once been exercised without the broken
memcpywrapper underneath it. With the wrapper gone, direct reads return correct data, NVS survives reboots, and the entire workaround is removed here.Also in this change
esp32_basego with it, so the board now matches every other S3 variant. The web server and paxcounter are built (paxcounter still only runs when enabled in config).MESHTASTIC_EXCLUDE_AUDIOwas already inert here —AudioModuleadditionally requiresUSE_SX1280, and this board is SX1262.-UMESHTASTIC_EXCLUDE_ACCELEROMETERis dropped. It existed only to undo the-Dinherited fromesp32_base; with that gone the macro is simply never defined, so the accelerometer is unchanged.ESP32_FORCE_IRAM_MEMSETis now guarded behindCONFIG_IDF_TARGET_ESP32with an#error, so a variant cannot enable the classic-ESP32 cache probe on another target again. The classic-ESP32 code path itself is untouched.Fixes #11513. Also resolves the underlying cause of #11530.
Testing
Tested on a LilyGo T-Watch Ultra:
The classic-ESP32 wrappers are unchanged apart from a compile-time guard that is a no-op wherever CONFIG_IDF_TARGET_ESP32 is defined (verified against the prebuilt esp32 sdkconfig). No runtime behaviour on those boards is affected, so I have not retested them.
🤝 Attestations
Summary by CodeRabbit