Skip to content

Conversation

@KenVanHoeylandt
Copy link
Member

@KenVanHoeylandt KenVanHoeylandt commented Jan 4, 2026

  • Remove unnecessary CS work-around for Cardputers
  • Don't compile WifiMock when not necessary
  • Fix for running apps on devices without SPIRAM
  • Disable backlight dimming by default

@coderabbitai
Copy link

coderabbitai bot commented Jan 4, 2026

📝 Walkthrough

Walkthrough

Removed LCD chip-select constant and ceased passing the LCD pin to SpiSdCardDevice in both m5stack-cardputer variants. Added an ESP_PLATFORM guard to include sdkconfig.h in WifiMock.cpp. Changed DisplaySettings defaults to disable backlight timeout by default. In device.py, made CPU frequency lines static strings and added conditional emission of memory-protection configuration (disabling MEMPROT features) when idf_target is not esp32.

Pre-merge checks

❌ Failed checks (1 warning, 1 inconclusive)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
Title check ❓ Inconclusive The title 'Fixes and improvements' is vague and generic, using non-descriptive terms that don't convey meaningful information about the specific changes in the changeset. Use a more descriptive title that reflects the primary changes, such as 'Remove LCD CS workaround and fix device compatibility issues' or similar.
✅ Passed checks (1 passed)
Check name Status Explanation
Description check ✅ Passed The pull request description is directly related to the changeset, listing specific fixes and improvements that align with the modified files and their changes.

📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3546fe1 and 19322ee.

📒 Files selected for processing (2)
  • Devices/m5stack-cardputer-adv/Source/devices/SdCard.cpp
  • Tactility/Source/settings/DisplaySettings.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
  • Tactility/Source/settings/DisplaySettings.cpp
  • Devices/m5stack-cardputer-adv/Source/devices/SdCard.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). (4)
  • GitHub Check: BuildSdk (generic-esp32c6, esp32c6)
  • GitHub Check: run
  • GitHub Check: Linux
  • GitHub Check: macOS

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2ca8ae6 and 3546fe1.

📒 Files selected for processing (5)
  • Devices/m5stack-cardputer-adv/Source/devices/SdCard.cpp
  • Devices/m5stack-cardputer/Source/devices/SdCard.cpp
  • Tactility/Source/service/wifi/WifiMock.cpp
  • Tactility/Source/settings/DisplaySettings.cpp
  • device.py
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: KenVanHoeylandt
Repo: ByteWelder/Tactility PR: 406
File: Buildscripts/CDN/devices.properties:0-0
Timestamp: 2025-11-02T15:49:07.895Z
Learning: In the ByteWelder/Tactility repository, alphabetical ordering issues (such as entries in Buildscripts/CDN/devices.properties) and similar organizational/formatting concerns should be marked as "minor" severity rather than "major" severity.
📚 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:

  • Devices/m5stack-cardputer-adv/Source/devices/SdCard.cpp
  • Devices/m5stack-cardputer/Source/devices/SdCard.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:

  • Devices/m5stack-cardputer-adv/Source/devices/SdCard.cpp
  • Devices/m5stack-cardputer/Source/devices/SdCard.cpp
📚 Learning: 2025-10-26T12:40:47.243Z
Learnt from: KenVanHoeylandt
Repo: ByteWelder/Tactility PR: 391
File: Boards/ElecrowCrowpanelAdvance28/Source/devices/Display.cpp:37-43
Timestamp: 2025-10-26T12:40:47.243Z
Learning: The Tactility project uses 80 MHz SPI pixel clock frequency for ST7789 displays, which has been tested and confirmed to work stably on all supported devices (CYD variants, Elecrow CrowPanel, LilyGo, M5Stack, Waveshare boards).

Applied to files:

  • Devices/m5stack-cardputer-adv/Source/devices/SdCard.cpp
  • Devices/m5stack-cardputer/Source/devices/SdCard.cpp
📚 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/WifiMock.cpp
🧬 Code graph analysis (1)
Tactility/Source/service/wifi/WifiMock.cpp (2)
Tactility/Source/app/systeminfo/SystemInfo.cpp (2)
  • ESP_PLATFORM (331-358)
  • ESP_PLATFORM (415-571)
Tactility/Source/app/boot/Boot.cpp (1)
  • ESP_PLATFORM (132-150)
🪛 Cppcheck (2.19.0)
Devices/m5stack-cardputer-adv/Source/devices/SdCard.cpp

[error] 20-20: There is an unknown macro here somewhere. Configuration is required. If IRAM is a macro then please configure it.

(unknownMacro)

🔇 Additional comments (4)
Tactility/Source/service/wifi/WifiMock.cpp (1)

1-3: LGTM! Platform-specific header guard is appropriate.

The addition of the ESP_PLATFORM guard to include sdkconfig.h is correct and follows ESP-IDF conventions. This ensures the CONFIG_ESP_WIFI_ENABLED macro is properly defined when building for ESP platforms.

device.py (2)

114-115: Good cleanup: removed unnecessary f-strings.

The change from f-strings to static strings is appropriate since no string interpolation is needed for these CPU frequency configuration lines.


118-121: Configuration is necessary and correctly scoped for dynamic ELF binary loading.

The memory protection disabling is required to support the ELF loader functionality, which enables dynamic execution of compiled ELF binaries. MALLOC_CAP_EXEC is used in Libraries/elf_loader/src/esp_elf_adapter.c when CONFIG_ELF_LOADER_BUS_ADDRESS_MIRROR is enabled (default on ESP32, S2, S3), and memory protection features on these chips prevent executing from dynamically allocated memory. The conditional check limiting this to non-ESP32 targets is correct because the original ESP32 lacks these memory protection features entirely.

Devices/m5stack-cardputer/Source/devices/SdCard.cpp (1)

17-17: Verify the removal of LCD_PIN_CS from the pin list.

The change from passing LCD_PIN_CS to an empty vector is consistent with the parallel change in the -adv variant. Please verify that SPI communication with the SD card functions correctly without LCD_PIN_CS in the pin list.

If you have access to the hardware, please test SD card mounting and file operations to ensure this change doesn't cause SPI conflicts with the LCD.

@KenVanHoeylandt KenVanHoeylandt merged commit 719f7bc into main Jan 4, 2026
48 checks passed
@KenVanHoeylandt KenVanHoeylandt deleted the develop branch January 4, 2026 13:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants