Implement support for ESP32 C6 and P4 SOCs #421
Conversation
📝 WalkthroughWalkthroughAdds generic device support for ESP32, ESP32-C6, ESP32-P4, and ESP32-S3 via new CMakeLists, device.properties, and Configuration.cpp files. Removes .github/workflows/build-sdk.yml and updates .github/workflows/build.yml to add a BuildSdk matrix and new job dependency chain. Gates ESP‑NOW and WiFi sources behind CONFIG_ESP_WIFI_ENABLED and adds sdkconfig.h includes where ESP_PLATFORM is defined. Introduces Xtensa-specific panic/backtrace handling and PC processing, changes QrUrl to use std::vector, adjusts CPU-affinity logic, refines idf_component.yml target rules, and makes LVGL writes conditional (adds has_group) in device.py. Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
Tactility/Source/service/espnow/EspNowWifi.cpp (1)
110-110: Fix closing comment to match opening guard.The closing comment says
ESP_PLATFORMbut this#endifcloses theCONFIG_ESP_WIFI_ENABLEDguard from Line 5, not theESP_PLATFORMguard from Line 1.Apply this diff:
-#endif // ESP_PLATFORM +#endif // CONFIG_ESP_WIFI_ENABLEDTactility/Source/service/espnow/EspNow.cpp (1)
84-84: Fix closing comment to match opening guard.The closing comment says
ESP_PLATFORMbut this#endifcloses theCONFIG_ESP_WIFI_ENABLEDguard from Line 5.Apply this diff:
-#endif // ESP_PLATFORM +#endif // CONFIG_ESP_WIFI_ENABLEDTactility/Source/service/wifi/WifiEsp.cpp (1)
968-968: Fix closing comment to match opening guard.The closing comment says
ESP_PLATFORMbut this#endifcloses theCONFIG_ESP_WIFI_ENABLEDguard from Line 5.Apply this diff:
-#endif // ESP_PLATFORM +#endif // CONFIG_ESP_WIFI_ENABLEDTactility/Source/service/espnow/EspNowService.cpp (1)
210-210: Fix closing comment to match opening guard.The closing comment says
ESP_PLATFORMbut this#endifcloses theCONFIG_ESP_WIFI_ENABLEDguard from Line 5.Apply this diff:
-#endif // ESP_PLATFORM +#endif // CONFIG_ESP_WIFI_ENABLEDTactilityCore/Source/CpuAffinity.cpp (1)
56-78: Add CONFIG_ESP_WIFI_ENABLED guard to the assert at lines 96-99The review's analysis is correct. When WiFi is disabled (
!CONFIG_ESP_WIFI_ENABLED),.wifiis hardcoded to0, whilegetEspMainSchedulerAffinity()can returnNoneif the old WiFi pinning macros (CONFIG_ESP32_WIFI_TASK_PINNED_TO_CORE_*) are not defined. The assert then compares0 == Noneand fails, even though WiFi isn't in use. This is a legitimate configuration that shouldn't trigger the assertion.The fix is to gate the assertion with
CONFIG_ESP_WIFI_ENABLED:#if CONFIG_FREERTOS_NUMBER_OF_CORES == 2 && defined(CONFIG_ESP_WIFI_ENABLED) // WiFi uses the main dispatcher to defer operations in the background assert(esp.wifi == esp.mainDispatcher); #endif
♻️ Duplicate comments (1)
Devices/generic-esp32/Source/Configuration.cpp (1)
1-3: SameConfiguration.hinclude concern as on generic‑esp32c6This file mirrors the generic‑esp32c6 configuration. Please ensure
<Tactility/hal/Configuration.h>is available and on the include path for this target as well, so Clang/IDF can resolve it consistently.
🧹 Nitpick comments (9)
Tactility/Source/service/wifi/WifiMock.cpp (1)
1-1: Guard change correctly targets “WiFi disabled” builds; comment is now misleading.Switching to
#ifndef CONFIG_ESP_WIFI_ENABLEDmakes this mock implementation compile when WiFi support is off (or on non-ESP hosts), which matches the overall CONFIG_ESP_WIFI_ENABLED gating pattern. The trailing#endif // ESP_PLATFORMcomment no longer reflects the actual guard macro though and could trip people up later.Consider updating the comment to match the new condition:
-#ifndef CONFIG_ESP_WIFI_ENABLED +#ifndef CONFIG_ESP_WIFI_ENABLED @@ -#endif // ESP_PLATFORM +#endif // !CONFIG_ESP_WIFI_ENABLEDAlso applies to: 162-162
Tactility/Private/Tactility/service/espnow/EspNowService.h (1)
2-10: Header gating aligns with WiFi feature flag; fix closing comment and verify call‑sites.Wrapping the entire class in
#ifdef CONFIG_ESP_WIFI_ENABLEDplus includingsdkconfig.honly underESP_PLATFORMcleanly limits this service to WiFi‑enabled ESP builds. WhenCONFIG_ESP_WIFI_ENABLEDis unset, the header is effectively empty, which is fine as long as all includes/uses are also guarded by the same macro.The closing comment is now misleading though:
-#ifdef CONFIG_ESP_WIFI_ENABLED +// ... body ... @@ -#endif // ESP_PLATFORM +#endif // CONFIG_ESP_WIFI_ENABLEDAlso double‑check that any translation units including this header don’t assume EspNowService exists when
CONFIG_ESP_WIFI_ENABLEDis off; otherwise you’ll get surprising compilation differences between targets.Also applies to: 7-7, 68-75
Devices/generic-esp32c6/CMakeLists.txt (1)
1-7: Component registration for generic-esp32c6 matches existing patternUsing
GLOB_RECURSEand registering the component withREQUIRES Tactilityis consistent with other device CMakeLists in this repo. If you ever want stricter build reproducibility, consider listing sources explicitly instead of usingGLOB_RECURSE, but it’s fine as-is.Tactility/Source/app/crashdiagnostics/QrUrl.cpp (1)
19-53: Harden reverse-iteration against empty call stacksThe vector-based
stack_bufferis a nice RAII improvement, but the reverse loopfor (int i = crash_data.callstackLength - 1; i >= 0; --i) { ... }relies on
callstackLengthbeing a non-negative signed type. If it’s unsigned (or ever becomes unsigned),callstackLength == 0will underflow before assigning toi, and the subsequent indexing intostack_bufferwould be undefined.To make this robust and future-proof, consider guarding or changing the loop shape, e.g.:
- for (int i = crash_data.callstackLength - 1; i >= 0; --i) { + for (int i = static_cast<int>(crash_data.callstackLength); i-- > 0;) { uint32_t pc = stack_buffer[(i * 2)]; stream << std::hex << pc; #if CRASH_DATA_INCLUDES_SP uint32_t sp = stack_buffer[(i * 2) + 1]; stream << std::hex << sp; #endif }This works correctly even when
callstackLengthis 0 and avoids any dependence on its signedness.Devices/generic-esp32p4/CMakeLists.txt (1)
1-7: generic-esp32p4 component wiring is consistent with other generic devicesThe globbed source collection and
idf_component_register(... REQUIRES Tactility)match the pattern used for the other generic ESP32 variants. As with esp32c6, explicit source lists would give slightly better build determinism, but the current approach is acceptable and consistent..github/workflows/build.yml (1)
25-27: Address actionlint warnings by addingdescriptionto composite actionsStatic analysis (
actionlint) reports that the composite actions you invoke here are missing adescriptionfield in theiraction.ymlfiles:
.github/actions/build-sdk/action.yml.github/actions/build-firmware/action.yml.github/actions/bundle-firmware/action.yml.github/actions/publish-firmware/action.ymlTo silence these and improve action metadata, add a short
description:to each of thoseaction.ymlfiles. No changes to this workflow YAML are required.Also applies to: 75-76, 88-89, 96-101
TactilityCore/Source/kernel/PanicHandler.cpp (1)
1-95: Xtensa-specific panic handler and RISC-V stub are well factoredRestricting the wrapped
esp_panic_handlertoESP_PLATFORM && CONFIG_IDF_TARGET_ARCH_XTENSA, combined with the CMake link-option guard, cleanly confines this logic to Xtensa targets. The new validation usingesp_cpu_process_stack_pc+esp_ptr_executable(and theEXCCAUSE_INSTR_PROHIBITEDspecial case) should produce much cleaner call stacks and a reliablecallstackCorruptedflag. The#elif defined(ESP_PLATFORM)stub that returns anemptyCrashDatafor RISC-V/other arches is a sensible interim step and integrates correctly withgetUrlFromCrashData().If/when you want to implement real RISC-V crash capture (per the TODO), I can help sketch a frame-pointer/EH-frame-based approach.
Tactility/Include/Tactility/service/espnow/EspNow.h (1)
7-59: Fix misleading#endifcomment forCONFIG_ESP_WIFI_ENABLEDguardThe closing
#endifat line 59 is commenting// ESP_PLATFORM, but it actually closes the#ifdef CONFIG_ESP_WIFI_ENABLEDstarted at line 7. For clarity, consider updating it:-#endif // ESP_PLATFORM +#endif // CONFIG_ESP_WIFI_ENABLEDThis keeps the preprocessor structure easier to follow as the header evolves.
Devices/generic-esp32s3/CMakeLists.txt (1)
1-7: Component registration matches other ESP32 variants; minor note onGLOB_RECURSEThe ESP32‑S3 component wiring (sources, include dir,
REQUIRES Tactility) is consistent with the other generic ESP32 devices, which is good for maintainability. The only tradeoff withfile(GLOB_RECURSE ...)is that adding new source files may not trigger CMake reconfiguration automatically, but since this pattern is already used elsewhere in the repo, keeping it here for consistency makes sense.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (33)
.github/workflows/build-sdk.yml(0 hunks).github/workflows/build.yml(2 hunks)CMakeLists.txt(1 hunks)Devices/generic-esp32/CMakeLists.txt(1 hunks)Devices/generic-esp32/Source/Configuration.cpp(1 hunks)Devices/generic-esp32/device.properties(1 hunks)Devices/generic-esp32c6/CMakeLists.txt(1 hunks)Devices/generic-esp32c6/Source/Configuration.cpp(1 hunks)Devices/generic-esp32c6/device.properties(1 hunks)Devices/generic-esp32p4/CMakeLists.txt(1 hunks)Devices/generic-esp32p4/Source/Configuration.cpp(1 hunks)Devices/generic-esp32p4/device.properties(1 hunks)Devices/generic-esp32s3/CMakeLists.txt(1 hunks)Devices/generic-esp32s3/Source/Configuration.cpp(1 hunks)Devices/generic-esp32s3/device.properties(1 hunks)Firmware/idf_component.yml(2 hunks)Tactility/Include/Tactility/service/espnow/EspNow.h(1 hunks)Tactility/Private/Tactility/service/espnow/EspNowService.h(1 hunks)Tactility/Private/Tactility/service/espnow/EspNowWifi.h(2 hunks)Tactility/Source/Tactility.cpp(5 hunks)Tactility/Source/app/chat/ChatApp.cpp(1 hunks)Tactility/Source/app/crashdiagnostics/QrUrl.cpp(1 hunks)Tactility/Source/hal/sdcard/SdmmcDevice.cpp(1 hunks)Tactility/Source/service/espnow/EspNow.cpp(1 hunks)Tactility/Source/service/espnow/EspNowService.cpp(1 hunks)Tactility/Source/service/espnow/EspNowWifi.cpp(1 hunks)Tactility/Source/service/wifi/WifiEsp.cpp(1 hunks)Tactility/Source/service/wifi/WifiMock.cpp(1 hunks)TactilityC/Source/symbols/gcc_soft_float.cpp(2 hunks)TactilityC/Source/tt_init.cpp(2 hunks)TactilityCore/Source/CpuAffinity.cpp(3 hunks)TactilityCore/Source/kernel/PanicHandler.cpp(3 hunks)device.py(2 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/build-sdk.yml
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-11-01T19:57:58.233Z
Learnt from: NellowTCS
Repo: ByteWelder/Tactility PR: 405
File: Tactility/Source/hal/usb/UsbTusb.cpp:12-12
Timestamp: 2025-11-01T19:57:58.233Z
Learning: In ESP-IDF projects like Tactility, the wear levelling header can be included directly as `#include <wear_levelling.h>` without requiring a directory prefix like `wear_levelling/wear_levelling.h`.
Applied to files:
Tactility/Source/service/wifi/WifiEsp.cppTactility/Source/hal/sdcard/SdmmcDevice.cppTactility/Source/service/wifi/WifiMock.cppTactility/Source/service/espnow/EspNowService.cppTactility/Private/Tactility/service/espnow/EspNowService.hTactilityC/Source/tt_init.cppTactility/Source/app/chat/ChatApp.cppTactility/Private/Tactility/service/espnow/EspNowWifi.hTactility/Source/service/espnow/EspNow.cppTactility/Include/Tactility/service/espnow/EspNow.hTactility/Source/service/espnow/EspNowWifi.cppTactility/Source/Tactility.cpp
📚 Learning: 2025-10-26T12:44:21.858Z
Learnt from: KenVanHoeylandt
Repo: ByteWelder/Tactility PR: 391
File: Boards/CYD-2432S028RV3/Source/devices/Display.h:11-19
Timestamp: 2025-10-26T12:44:21.858Z
Learning: In the Tactility codebase, prefer `constexpr auto` for constant declarations (such as LCD pin configurations, resolutions, and buffer sizes) rather than explicit type annotations like `gpio_num_t`, `spi_host_device_t`, `uint16_t`, or `size_t`.
Applied to files:
Tactility/Source/hal/sdcard/SdmmcDevice.cppTactility/Source/Tactility.cpp
📚 Learning: 2025-11-11T17:22:20.238Z
Learnt from: KenVanHoeylandt
Repo: ByteWelder/Tactility PR: 411
File: sdkconfig.board.waveshare-s3-touch-lcd-43:39-39
Timestamp: 2025-11-11T17:22:20.238Z
Learning: In Tactility sdkconfig.board.* files, board names (CONFIG_TT_BOARD_NAME) that include screen size measurements intentionally use escaped quotes (e.g., "WaveShare S3 Touch LCD 4.3\"") where the \" represents the inch symbol (") in the final board name string. This is not a syntax error but deliberate formatting.
Applied to files:
Tactility/Source/Tactility.cpp
📚 Learning: 2025-10-27T22:33:23.840Z
Learnt from: KenVanHoeylandt
Repo: ByteWelder/Tactility PR: 394
File: Boards/WaveshareEsp32C6Lcd147/Source/WaveshareEsp32C6Lcd147.cpp:49-50
Timestamp: 2025-10-27T22:33:23.840Z
Learning: In the Tactility project, when configuring SPI for displays that use esp_lvgl_port, the `.lock` field must be set to `tt::lvgl::getSyncLock()` rather than `tt::hal::spi::getLock()`, because esp_lvgl_port creates and manages its own synchronization lock for display operations.
Applied to files:
Tactility/Source/Tactility.cpp
🧬 Code graph analysis (3)
Tactility/Source/app/crashdiagnostics/QrUrl.cpp (1)
TactilityCore/Source/kernel/PanicHandler.cpp (4)
getRtcCrashData(82-82)getRtcCrashData(82-82)getRtcCrashData(93-93)getRtcCrashData(93-93)
Tactility/Private/Tactility/service/espnow/EspNowWifi.h (1)
Tactility/Source/app/boot/Boot.cpp (1)
ESP_PLATFORM(132-150)
Tactility/Include/Tactility/service/espnow/EspNow.h (1)
Tactility/Source/app/boot/Boot.cpp (1)
ESP_PLATFORM(132-150)
🪛 actionlint (1.7.8)
.github/workflows/build.yml
27-27: description is required in metadata of "Build" action at "/home/jailuser/git/.github/actions/build-sdk/action.yml"
(action)
76-76: description is required in metadata of "Build" action at "/home/jailuser/git/.github/actions/build-firmware/action.yml"
(action)
89-89: description is required in metadata of "Bundle All" action at "/home/jailuser/git/.github/actions/bundle-firmware/action.yml"
(action)
101-101: description is required in metadata of "Publish Firmware" action at "/home/jailuser/git/.github/actions/publish-firmware/action.yml"
(action)
🪛 Clang (14.0.6)
Devices/generic-esp32s3/Source/Configuration.cpp
[error] 1-1: 'Tactility/hal/Configuration.h' file not found
(clang-diagnostic-error)
Devices/generic-esp32c6/Source/Configuration.cpp
[error] 1-1: 'Tactility/hal/Configuration.h' file not found
(clang-diagnostic-error)
Devices/generic-esp32/Source/Configuration.cpp
[error] 1-1: 'Tactility/hal/Configuration.h' file not found
(clang-diagnostic-error)
Devices/generic-esp32p4/Source/Configuration.cpp
[error] 1-1: 'Tactility/hal/Configuration.h' file not found
(clang-diagnostic-error)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: BuildSdk (generic-esp32p4, esp32p4)
- GitHub Check: BuildSdk (generic-esp32s3, esp32s3)
- GitHub Check: BuildSdk (generic-esp32c6, esp32c6)
- GitHub Check: BuildSdk (generic-esp32, esp32)
- GitHub Check: run
- GitHub Check: macOS
- GitHub Check: Linux
🔇 Additional comments (22)
Tactility/Source/hal/sdcard/SdmmcDevice.cpp (2)
1-1: LGTM! SOC-specific guard prevents compilation on unsupported targets.The addition of
SOC_SDMMC_HOST_SUPPORTEDalongsideESP_PLATFORMensures this SDMMC implementation only compiles on ESP32 variants with SDMMC hardware support, preventing build failures on targets like ESP32-C6 and ESP32-P4 that lack this peripheral.
3-119: LGTM! Implementation is solid.The SDMMC device implementation correctly uses ESP-IDF APIs with proper locking, error handling, and state management. The comment explaining SPI bus contention between the SD card and screen (lines 99-103) is particularly helpful context for the timeout-based locking pattern.
device.py (1)
55-56: LGTM! Clean helper implementation.The function correctly checks for section existence using Python's
inoperator, improving code readability.Devices/generic-esp32p4/device.properties (1)
1-8: ESP32-P4 device properties look consistent.Vendor/name/target/flash/SPI RAM settings follow the same pattern as the other generic ESP32 variants; nothing problematic stands out.
Devices/generic-esp32c6/device.properties (1)
1-8: ESP32-C6 device properties are in line with other generic devices.The general and hardware sections mirror the other generic ESP32 profiles and should integrate cleanly with the existing device discovery logic.
TactilityC/Source/symbols/gcc_soft_float.cpp (1)
1-1: Confirm intent of disabling gcc_soft_float symbols on ESP32‑P4.
#ifndef CONFIG_IDF_TARGET_ESP32P4means this symbol table is now compiled for all targets except ESP32‑P4. That’s a behavioral change: previously these exports were available everywhere, now P4 builds will not see them.Please confirm:
- ESP32‑P4 really must not provide these soft‑float symbols (e.g., because the toolchain/runtime already does, or P4 uses different FP support), and
- Any code that references
gcc_soft_float_symbols(e.g., registration in init code) is also excluded for P4 so you don’t hit unresolved symbols or dead code paths.If the intent was “P4‑only”, this should instead be
#ifdef CONFIG_IDF_TARGET_ESP32P4, or a more explicit target list.Also applies to: 285-288
Devices/generic-esp32/device.properties (1)
1-8: Baseline generic ESP32 properties look good.This mirrors the structure and values of the other generic ESP32* profiles and should integrate cleanly with the device discovery/configuration code.
Tactility/Source/app/chat/ChatApp.cpp (1)
1-5: ChatApp correctly gated behind ESP‑IDF WiFi config.Including
sdkconfig.hunderESP_PLATFORMand then guarding the whole ChatApp with#ifdef CONFIG_ESP_WIFI_ENABLEDcleanly limits this UI/app to builds where WiFi/ESP‑NOW is enabled, avoiding host or no‑WiFi configurations that lackesp_wifi.h/ESP‑NOW support. The rest of the implementation can now safely assume WiFi is present.Also applies to: 151-151
Devices/generic-esp32/CMakeLists.txt (1)
1-7: Component registration for generic ESP32 is consistent with the new pattern.Globbing
Source/*.c*and registering viaidf_component_register(... REQUIRES Tactility)matches the approach used for the other generic ESP32* devices, so this should integrate smoothly into the ESP‑IDF build.CMakeLists.txt (1)
51-54: Xtensa-only wrap aligns correctly with PanicHandler gatingGuarding
-Wl,--wrap=esp_panic_handlerbehindCONFIG_IDF_TARGET_ARCH_XTENSAis the right move and matches the new Xtensa-only implementation inPanicHandler.cpp, preventing link issues on RISC-V targets.TactilityC/Source/tt_init.cpp (1)
34-36: ESP32P4 soft-float exclusion is consistent and safeConditionally excluding
gcc_soft_floatforCONFIG_IDF_TARGET_ESP32P4in both the include and theall_symbolsvector cleanly disables those stubs on P4 while keeping other targets unchanged. This looks coherent with the guarded implementation insymbols/gcc_soft_float.cpp; just confirm that no ESP32P4-only binaries rely on these symbols anymore.Also applies to: 593-599
Tactility/Source/app/crashdiagnostics/QrUrl.cpp (1)
3-15: Arch-specific CPU header usage is consistent with PanicHandlerSelecting
esp_cpu_utils.hfor Xtensa and falling back toesp_cpu.hfor non-Xtensa keeps this module aligned with the new panic-handling logic and avoids pulling in Xtensa-only APIs on RISC-V. The include style change to<Tactility/...>is also consistent with the rest of the project’s public headers.Firmware/idf_component.yml (1)
2-13: Target-scoped component rules look correct; ensure boards align with themConverting these display and touch components to
version + rulesand scoping them toesp32/esp32s3(withst7701includingesp32p4) is a good way to keep unsupported targets from pulling in incompatible drivers as you add C6/P4 support. Please just double-check that no esp32c6/esp32p4 board configs still depend on any of the components now limited to[esp32, esp32s3]; otherwise those boards will silently lose their display dependencies.Also applies to: 16-18, 30-31
.github/workflows/build.yml (1)
14-31: BuildSdk gate correctly protects the heavier firmware matrixIntroducing the
BuildSdkmatrix over the four generic devices and makingBuildFirmwaredepend on it means any SDK build failure short-circuits the much larger board matrix, which is exactly the resource-saving behavior described in the PR. The downstreamBundleFirmwareand publish jobs chaining offBuildFirmwarealso look correct.Also applies to: 71-107
Tactility/Source/Tactility.cpp (1)
1-3: LGTM! Clean gating of WiFi-dependent features.The changes correctly introduce
CONFIG_ESP_WIFI_ENABLEDguards for ESP-NOW and chat functionality while maintainingESP_PLATFORMguards for other platform-specific features. The pattern is consistent across manifest declarations and registration calls.Also applies to: 45-48, 73-75, 146-148, 245-249
Devices/generic-esp32s3/device.properties (1)
1-9: LGTM! Well-formed device properties.The device properties file follows the standard format and correctly defines a generic ESP32-S3 device without a display section, aligning with the PR's objective to make displays optional.
Devices/generic-esp32s3/Source/Configuration.cpp (1)
1-3: LGTM! Minimal configuration stub is appropriate.This minimal configuration with an empty
hardwareConfigurationobject is correct for a generic device variant. The static analysis error about the missing header is a false positive that will resolve during the actual ESP-IDF build process.Devices/generic-esp32p4/Source/Configuration.cpp (1)
1-3: LGTM! Minimal configuration stub is appropriate.This minimal configuration with an empty
hardwareConfigurationobject is correct for a generic device variant. The static analysis error about the missing header is a false positive that will resolve during the actual ESP-IDF build process.Tactility/Private/Tactility/service/espnow/EspNowWifi.h (1)
3-19: ESP‑NOW WiFi gating looks good; ensure call sites are also guardedThe combination of
#ifdef ESP_PLATFORMforsdkconfig.hand#ifdef CONFIG_ESP_WIFI_ENABLEDaround the API is consistent with the new pattern and should prevent host builds from pulling in ESP‑IDF headers. Please double‑check that all callers ofinitWifi/deinitWifiare also conditionally compiled withCONFIG_ESP_WIFI_ENABLED(or equivalent) so you don’t hit missing‑symbol builds when WiFi is disabled.Devices/generic-esp32c6/Source/Configuration.cpp (1)
1-3: No issues found—configuration is correctThe header
<Tactility/hal/Configuration.h>is properly resolved: Tactility's CMakeLists.txt exposesInclude/as a public directory, and generic-esp32c6 depends on Tactility, gaining access to its includes. The symbol definition matches the pattern across other generic devices and is correctly namespaced. The static analysis concern appears to be a false positive; the build system integration is sound.TactilityCore/Source/CpuAffinity.cpp (2)
1-3: sdkconfig.h inclusion guarded by ESP_PLATFORM looks correctIncluding
sdkconfig.honly whenESP_PLATFORMis defined is a clean way to get Kconfig constants without breaking non-ESP/simulator builds. No issues here.
43-53: FreeRTOS timer affinity mapping and single-core fallback look fineThe extra single-core branch (
CONFIG_FREERTOS_NUMBER_OF_CORES == 1) returning core0is consistent with FreeRTOS/ESP-IDF behavior where “no affinity” collapses to core 0 in unicore builds.(esp32.ai) The explicit handling ofCONFIG_FREERTOS_TIMER_TASK_NO_AFFINITYvs*_CPU0/*_CPU1, plus thestatic_assert(false);fallback, gives a clear compile-time failure if new unsupported variants appear.No changes needed here from a correctness perspective.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Tactility/Source/hal/sdcard/SdmmcDevice.cpp(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-01T19:57:58.233Z
Learnt from: NellowTCS
Repo: ByteWelder/Tactility PR: 405
File: Tactility/Source/hal/usb/UsbTusb.cpp:12-12
Timestamp: 2025-11-01T19:57:58.233Z
Learning: In ESP-IDF projects like Tactility, the wear levelling header can be included directly as `#include <wear_levelling.h>` without requiring a directory prefix like `wear_levelling/wear_levelling.h`.
Applied to files:
Tactility/Source/hal/sdcard/SdmmcDevice.cpp
🔇 Additional comments (1)
Tactility/Source/hal/sdcard/SdmmcDevice.cpp (1)
1-4: LGTM!The addition of
sdkconfig.his necessary to access theCONFIG_SOC_SDMMC_HOST_SUPPORTEDmacro used in the guard below. The include is properly placed within theESP_PLATFORMguard and before the macro is referenced.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Tactility/Source/hal/sdcard/SdmmcDevice.cpp (1)
1-5: Compile-time gating now correctly usesSOC_SDMMC_HOST_SUPPORTEDandsoc_caps.h.The SDMMC implementation is now properly compiled only when building for ESP-IDF (
ESP_PLATFORM) on SoCs that actually have an SDMMC host (SOC_SDMMC_HOST_SUPPORTED), andsoc/soc_caps.his safely included only on ESP builds. This resolves the earlierCONFIG_SOC_SDMMC_HOST_SUPPORTEDmacro issue and aligns with ESP-IDF capability-gating patterns.If you want to simplify slightly, you could flatten the double
ESP_PLATFORMcheck to something like:#if defined(ESP_PLATFORM) #include <soc/soc_caps.h> #if defined(SOC_SDMMC_HOST_SUPPORTED) // ... #endif #endifbut this is purely cosmetic; the current form is functionally fine.
Please just confirm against the ESP-IDF versions you target that
SOC_SDMMC_HOST_SUPPORTEDis consistently provided bysoc/soc_caps.hon all intended SoCs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Tactility/Source/hal/sdcard/SdmmcDevice.cpp(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-01T19:57:58.233Z
Learnt from: NellowTCS
Repo: ByteWelder/Tactility PR: 405
File: Tactility/Source/hal/usb/UsbTusb.cpp:12-12
Timestamp: 2025-11-01T19:57:58.233Z
Learning: In ESP-IDF projects like Tactility, the wear levelling header can be included directly as `#include <wear_levelling.h>` without requiring a directory prefix like `wear_levelling/wear_levelling.h`.
Applied to files:
Tactility/Source/hal/sdcard/SdmmcDevice.cpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: run
- GitHub Check: BuildSdk (generic-esp32c6, esp32c6)
- GitHub Check: BuildSdk (generic-esp32p4, esp32p4)
- GitHub Check: BuildSdk (generic-esp32s3, esp32s3)
- GitHub Check: Linux
- GitHub Check: BuildSdk (generic-esp32, esp32)
- GitHub Check: macOS
device.pyto make the[display]section optional