Cardputer adv and more #395
Conversation
📝 WalkthroughWalkthroughAdds M5Stack Cardputer Advanced support: new hardware Configuration, display and SD card factory functions, CardputerKeyboard LVGL input implementation, and CardputerPower ADC metric implementation. Introduces board-specific sdkconfig defaults and CI matrix entry. Moves PWM backlight initialization into M5stack configuration and exposes a backlight GPIO constant. Updates TCA8418 driver with initMatrix and revised key-event decoding. Adjusts Lilygo TLora Pager keyboard mappings and input handling. Adds Adafruit TCA8418 license and README entry, changes wifi.enableOnBoot default to false, and updates various includes and CMake files. Pre-merge checks❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
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: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
Boards/M5stackCardputer/Source/Configuration.cpp (1)
1-1: Remove stale include.The
#include "InitBoot.h"on line 1 references a file that no longer exists in this directory.InitBoot.hwas removed during refactoring and only exists for other boards. The boot initialization is now defined locally in this file (theinitBoot()function). Remove the include.Drivers/TCA8418/Source/Tca8418.cpp (1)
126-142: Critical: Hardcoded column divisor breaks TpagerKeyboard key mapping.The new code hardcodes a divisor of 10 for both row and column calculations, but the boards configure the TCA8418 with different dimensions:
- CardputerKeyboard: 7 rows × 8 columns
- TpagerKeyboard: 4 rows × 11 columns
With the equation
key_row = buffer / 10; key_col = buffer % 10, TpagerKeyboard will produce invalid row indices (0-4 when only 0-3 are valid), causing incorrect key mapping or crashes.Fix: Use
num_colsinstead of the hardcoded 10:key_row = buffer / num_cols; key_col = buffer % num_cols;Also verify the
buffer--decrement matches the hardware's encoding scheme (appears to convert from 1-based to 0-based indexing).Boards/LilygoTLoraPager/Source/devices/TpagerKeyboard.h (1)
1-10: Make header self-contained and guard queue deletion.Include FreeRTOS queue type and avoid deleting null on OOM.
#pragma once #include <Tactility/hal/keyboard/KeyboardDevice.h> +#include "freertos/queue.h" +#include <cassert> ... - ~TpagerKeyboard() override { - vQueueDelete(queue); - } + ~TpagerKeyboard() override { + if (queue) vQueueDelete(queue); + }Optionally assert allocation in the ctor:
- explicit TpagerKeyboard(const std::shared_ptr<Tca8418>& tca) : keypad(tca) { - queue = xQueueCreate(20, sizeof(char)); - } + explicit TpagerKeyboard(const std::shared_ptr<Tca8418>& tca) : keypad(tca) { + queue = xQueueCreate(20, sizeof(char)); + assert(queue && "Queue allocation failed"); + }
🧹 Nitpick comments (16)
Boards/M5stackCardputer/Source/Configuration.cpp (1)
14-16: Consider documenting the magic number.The hardcoded value
512for the PWM backlight initialization could benefit from a named constant or inline comment explaining its purpose (e.g., duty cycle resolution, initial brightness level).For example:
+// PWM resolution: 512 steps (9-bit) bool initBoot() { return driver::pwmbacklight::init(LCD_PIN_BACKLIGHT, 512); }Or:
+constexpr auto LCD_BACKLIGHT_PWM_RESOLUTION = 512; + bool initBoot() { - return driver::pwmbacklight::init(LCD_PIN_BACKLIGHT, 512); + return driver::pwmbacklight::init(LCD_PIN_BACKLIGHT, LCD_BACKLIGHT_PWM_RESOLUTION); }Boards/LilygoTLoraPager/Source/devices/TpagerKeyboard.cpp (1)
61-75: Add bounds checks before indexing keymaps.Protect against unexpected row/col from the keypad driver.
for (int i = 0; i < keypad->pressed_key_count; i++) { auto row = keypad->pressed_list[i].row; auto col = keypad->pressed_list[i].col; auto hold = keypad->pressed_list[i].hold_time; - if ((row == 2) && (col == 0)) { + if (row >= KB_ROWS || col >= KB_COLS) continue; + if ((row == 2) && (col == 0)) { sym_pressed = true; } - if ((row == 2) && (col == 8)) { + if ((row == 2) && (col == 8)) { shift_pressed = true; } }Apply the same guard in the later pressed and released loops.
sdkconfig.board.m5stack-cardputer-adv (1)
3-4: Stack sizes look sane; keep an eye on free heap.6144 for main and 3072 for system event tasks match LVGL/network loads; verify runtime margins with heap/stack watermark logs.
sdkconfig.board.cyd-e32r32p (1)
27-31: WL/FATFS sector config: validate alignment with flash geometry.Setting WL sector size/mode to 512 SAFE is fine for FATFS logical sectors but WL backing must align with flash erase/block size. Please confirm:
- Partition type is WL-backed FATFS, and
- partitions-4mb.csv sizes are multiples of WL sector/erase size,
- mount/format succeeds without implicit up/down-scaling by WL.
Boards/M5stackCardputerAdv/Source/devices/CardputerPower.h (1)
11-15: Avoid magic numbers for battery model.3.3f/4.2f are board-specific. Expose constants (e.g., kBattMinV, kBattMaxV) or read from config. Document divider ratio used elsewhere (×2 in cpp).
Boards/M5stackCardputerAdv/Source/Configuration.cpp (4)
14-16: Backlight init: confirm PWM resolution and default brightness scale.Passing 512 implies 10‑bit (0–1023). Ensure PwmBacklight::init matches this board’s timer/resolution; otherwise set explicit resolution or clamp.
18-26: Device creation order is fine; minor: share TCA8418 via weak_ptr to avoid cycles.If any device holds back-references, consider weak_ptr to prevent accidental cycles. Otherwise LGTM.
33-67: I2C topology: Main OK; Port A note on pin/resource sharing.Port A I2C uses GPIO 2/1 which are also assigned to UART below. Since Port A I2C starts Disabled, add a comment noting it shares pins with UART and must not be enabled simultaneously.
i2c::Configuration { .name = "Port A", // Grove + // Shares pins with UART Port A (GPIO2/1). Enable either I2C or UART, not both. .port = I2C_NUM_1,
120-144: UART vs Port A I2C pin multiplexing.Same pins as I2C Port A. Consider documenting mutual exclusivity here as well, or provide a runtime guard to prevent enabling both.
Boards/M5stackCardputerAdv/Source/devices/Display.h (2)
17-17: LV_COLOR_DEPTH dependency in header.LCD_SPI_TRANSFER_SIZE_LIMIT uses LV_COLOR_DEPTH, but this header doesn’t include LVGL headers. Either:
- include <lvgl.h>/<lvgl_types.h>, or
- move this constant to Display.cpp, or
- compute bytes/pixel from a local config (e.g., 2 for RGB565).
Moving to .cpp reduces header coupling.
-constexpr auto LCD_SPI_TRANSFER_SIZE_LIMIT = LCD_BUFFER_SIZE * LV_COLOR_DEPTH / 8; +// Define in Display.cpp after including LVGL, or hardcode bytes/pixel if fixed (RGB565 -> *2).
8-16: Constants look sane for ST7789 landscape; minor: add static_asserts.Add static_asserts for buffer sizing not exceeding driver limits and pin validity for ESP32‑S3 build.
Boards/M5stackCardputerAdv/Source/devices/CardputerPower.cpp (2)
8-25: Calibration: add fallback characterization when eFuse not present.When efuse check isn’t OK, you can still call esp_adc_cal_characterize with default Vref to get usable readings instead of returning 0 mV. Improves usability on uncalibrated boards.
70-81: Avoid double ADC reads and magic scaling factor.You read and convert twice and multiply by 2 inline. Compute once and use a named kDividerRatio (e.g., 2.0f) derived from the resistor values. Also consider saturating to plausible battery ranges before estimating charge.
- switch (type) { + const uint32_t sense_mv = adcReadValue(); + constexpr float kDividerRatio = 2.0f; // TODO: derive from actual resistors + const uint32_t batt_mv = static_cast<uint32_t>(sense_mv * kDividerRatio); + switch (type) { case MetricType::BatteryVoltage: - data.valueAsUint32 = adcReadValue() * 2; + data.valueAsUint32 = batt_mv; return true; case MetricType::ChargeLevel: - data.valueAsUint8 = chargeFromAdcVoltage.estimateCharge(adcReadValue() * 2); + data.valueAsUint8 = chargeFromAdcVoltage.estimateCharge(batt_mv); return true;Boards/M5stackCardputerAdv/Source/devices/CardputerKeyboard.cpp (3)
5-11: Remove unused headers/constants.driver/gpio.h and BACKLIGHT aren’t used here. Keep includes lean.
-#include "driver/gpio.h" -#include "freertos/queue.h" +#include "freertos/queue.h" @@ -constexpr auto BACKLIGHT = GPIO_NUM_46;
52-62: Optional bounds guard in remap.If keypad ever reports out‑of‑range values, guard to prevent OOB on keymaps.
void CardputerKeyboard::remap(uint8_t& row, uint8_t& col) { + // Defensive: original wiring is 7x8 + if (row >= 7 || col >= 8) return;Please confirm TCA8418 row/col bounds for this wiring.
133-150: Harden startLvgl against misuse.Guard queue/init and duplicate start; minor but prevents crashes.
bool CardputerKeyboard::startLvgl(lv_display_t* display) { + if (!queue || kbHandle) return false; keypad->init(7, 8); - assert(inputTimer == nullptr); + assert(inputTimer == nullptr); inputTimer = std::make_unique<tt::Timer>(tt::Timer::Type::Periodic, [this] { processKeyboard(); });Please confirm keypad->init(7,8) is correct for the Advanced variant.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
Boards/LilygoTLoraPager/Source/devices/TpagerKeyboard.cpp(3 hunks)Boards/LilygoTLoraPager/Source/devices/TpagerKeyboard.h(1 hunks)Boards/M5stackCardputer/Source/Configuration.cpp(2 hunks)Boards/M5stackCardputer/Source/InitBoot.cpp(0 hunks)Boards/M5stackCardputer/Source/InitBoot.h(0 hunks)Boards/M5stackCardputer/Source/devices/Display.h(1 hunks)Boards/M5stackCardputerAdv/CMakeLists.txt(1 hunks)Boards/M5stackCardputerAdv/Source/Configuration.cpp(1 hunks)Boards/M5stackCardputerAdv/Source/devices/CardputerKeyboard.cpp(1 hunks)Boards/M5stackCardputerAdv/Source/devices/CardputerKeyboard.h(1 hunks)Boards/M5stackCardputerAdv/Source/devices/CardputerPower.cpp(1 hunks)Boards/M5stackCardputerAdv/Source/devices/CardputerPower.h(1 hunks)Boards/M5stackCardputerAdv/Source/devices/Display.cpp(1 hunks)Boards/M5stackCardputerAdv/Source/devices/Display.h(1 hunks)Boards/M5stackCardputerAdv/Source/devices/SdCard.cpp(1 hunks)Boards/M5stackCardputerAdv/Source/devices/SdCard.h(1 hunks)Buildscripts/board.cmake(1 hunks)Data/data/settings/wifi.properties(1 hunks)Documentation/ideas.md(1 hunks)Drivers/TCA8418/Adafruit_TCA8418-license.txt(1 hunks)Drivers/TCA8418/README.md(1 hunks)Drivers/TCA8418/Source/Tca8418.cpp(5 hunks)Drivers/TCA8418/Source/Tca8418.h(2 hunks)Firmware/Kconfig(1 hunks)sdkconfig.board.cyd-e32r28t(2 hunks)sdkconfig.board.cyd-e32r32p(2 hunks)sdkconfig.board.m5stack-cardputer-adv(1 hunks)
💤 Files with no reviewable changes (2)
- Boards/M5stackCardputer/Source/InitBoot.h
- Boards/M5stackCardputer/Source/InitBoot.cpp
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-26T12:44:21.833Z
Learnt from: KenVanHoeylandt
PR: ByteWelder/Tactility#391
File: Boards/CYD-2432S028RV3/Source/devices/Display.h:11-19
Timestamp: 2025-10-26T12:44:21.833Z
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:
Boards/M5stackCardputer/Source/devices/Display.h
🧬 Code graph analysis (10)
Boards/M5stackCardputerAdv/Source/devices/SdCard.h (2)
Tactility/Include/Tactility/hal/sdcard/SdCardDevice.h (1)
SdCardDevice(14-67)Boards/M5stackCardputerAdv/Source/devices/SdCard.cpp (2)
createSdCard(10-25)createSdCard(10-10)
Boards/M5stackCardputer/Source/Configuration.cpp (1)
Tactility/Include/Tactility/Tactility.h (1)
hal(38-43)
Boards/M5stackCardputerAdv/Source/devices/CardputerPower.cpp (1)
Boards/M5stackCardputerAdv/Source/devices/CardputerPower.h (1)
adcReadValue(17-23)
Drivers/TCA8418/Source/Tca8418.cpp (1)
Tactility/Source/hal/i2c/I2cDevice.cpp (2)
writeRegister(19-21)writeRegister(19-19)
Boards/LilygoTLoraPager/Source/devices/TpagerKeyboard.h (1)
Drivers/TCA8418/Source/Tca8418.h (1)
Tca8418(48-52)
Boards/M5stackCardputerAdv/Source/devices/Display.h (2)
Tactility/Include/Tactility/hal/display/DisplayDevice.h (1)
DisplayDevice(15-47)Boards/M5stackCardputerAdv/Source/devices/Display.cpp (2)
createDisplay(6-31)createDisplay(6-6)
Boards/M5stackCardputerAdv/Source/devices/CardputerPower.h (3)
Tactility/Include/Tactility/hal/power/PowerDevice.h (1)
PowerDevice(8-47)Drivers/EstimatedPower/Source/ChargeFromVoltage.h (1)
ChargeFromVoltage(5-22)Boards/M5stackCardputerAdv/Source/devices/CardputerPower.cpp (10)
adcInitCalibration(8-25)adcInitCalibration(8-8)adcReadValue(27-38)adcReadValue(27-27)ensureInitialized(40-57)ensureInitialized(40-40)supportsMetric(59-68)supportsMetric(59-59)getMetric(70-85)getMetric(70-70)
Drivers/TCA8418/Source/Tca8418.h (1)
Drivers/TCA8418/Source/Tca8418.cpp (2)
initMatrix(26-56)initMatrix(26-26)
Boards/M5stackCardputerAdv/Source/devices/CardputerKeyboard.h (3)
Boards/M5stackCardputer/Source/devices/CardputerKeyboard.h (1)
CardputerKeyboard(8-26)Tactility/Include/Tactility/hal/keyboard/KeyboardDevice.h (1)
KeyboardDevice(11-24)Boards/M5stackCardputerAdv/Source/devices/CardputerKeyboard.cpp (12)
processKeyboard(64-131)processKeyboard(64-64)readCallback(39-50)readCallback(39-39)remap(52-62)remap(52-52)startLvgl(133-150)startLvgl(133-133)stopLvgl(152-160)stopLvgl(152-152)isAttached(162-164)isAttached(162-162)
Boards/M5stackCardputerAdv/Source/Configuration.cpp (2)
Boards/M5stackCardputerAdv/Source/devices/SdCard.cpp (2)
createSdCard(10-25)createSdCard(10-10)Boards/M5stackCardputerAdv/Source/devices/Display.cpp (2)
createDisplay(6-31)createDisplay(6-6)
🪛 Clang (14.0.6)
Boards/M5stackCardputerAdv/Source/devices/SdCard.h
[error] 3-3: 'Tactility/hal/sdcard/SdCardDevice.h' file not found
(clang-diagnostic-error)
Boards/M5stackCardputerAdv/Source/devices/Display.h
[error] 3-3: 'Tactility/hal/display/DisplayDevice.h' file not found
(clang-diagnostic-error)
Boards/M5stackCardputerAdv/Source/devices/CardputerPower.h
[error] 3-3: 'Tactility/hal/power/PowerDevice.h' file not found
(clang-diagnostic-error)
Boards/M5stackCardputerAdv/Source/devices/CardputerKeyboard.h
[error] 3-3: 'Tactility/hal/keyboard/KeyboardDevice.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). (20)
- GitHub Check: build (m5stack-stickc-plus, esp32)
- GitHub Check: build (elecrow-crowpanel-advance-28, esp32s3)
- GitHub Check: build (m5stack-cores3, esp32s3)
- GitHub Check: build (elecrow-crowpanel-basic-50, esp32s3)
- GitHub Check: build (m5stack-stickc-plus2, esp32)
- GitHub Check: build (lilygo-tdongle-s3, esp32s3)
- GitHub Check: build (cyd-4848s040c, esp32s3)
- GitHub Check: build (elecrow-crowpanel-basic-28, esp32)
- GitHub Check: build (cyd-e32r32p, esp32)
- GitHub Check: build (elecrow-crowpanel-basic-35, esp32)
- GitHub Check: build (elecrow-crowpanel-advance-35, esp32s3)
- GitHub Check: build (cyd-e32r28t, esp32)
- GitHub Check: build (cyd-jc2432w328c, esp32)
- GitHub Check: build (cyd-jc8048w550c, esp32s3)
- GitHub Check: run
- GitHub Check: build (cyd-2432s028rv3, esp32)
- GitHub Check: build (cyd-2432s028r, esp32)
- GitHub Check: build (cyd-2432s024c, esp32)
- GitHub Check: build (cyd-2432s028r, esp32)
- GitHub Check: build (lilygo-tdeck, esp32s3)
🔇 Additional comments (22)
Boards/M5stackCardputer/Source/devices/Display.h (1)
12-12: LGTM!The constant follows the codebase style (using
constexpr auto) and is properly integrated with the PWM backlight initialization in Configuration.cpp.Boards/M5stackCardputer/Source/Configuration.cpp (1)
35-35: I2C port change appears intentional.The change from
I2C_NUM_1toI2C_NUM_0aligns with the PR's board configuration updates.Boards/M5stackCardputerAdv/CMakeLists.txt (1)
1-7: LGTM! Build configuration follows project patterns.The glob-based source collection and dependency list are appropriate for the Cardputer Advanced board variant.
Note:
GLOB_RECURSEwon't automatically detect new source files until CMake is re-run, but this pattern is consistent with the project's approach.Drivers/TCA8418/Source/Tca8418.cpp (1)
25-56: Good addition from Adafruit library.The
initMatrix()implementation properly handles keypad matrix configuration with appropriate bounds checking and register mask building.Drivers/TCA8418/Source/Tca8418.h (2)
10-12: Helpful documentation addition.The datasheet link provides useful reference for understanding the TCA8418 driver.
29-29: Private helper method properly declared.The
initMatrix()declaration correctly matches its implementation in the .cpp file.Data/data/settings/wifi.properties (1)
1-1: WiFi auto-start disabled by default.This changes the default behavior so WiFi won't automatically enable at boot. This could be intentional for battery conservation or privacy, but verify this aligns with the intended user experience.
Drivers/TCA8418/README.md (1)
5-5: Helpful documentation addition.The Adafruit TCA8418 library reference provides a useful additional resource for understanding the driver implementation.
Documentation/ideas.md (1)
9-9: Planned improvement documented.This TODO captures a planned refactor for the original Cardputer's keyboard mapping, which aligns with the keyboard integration work in this PR.
Buildscripts/board.cmake (1)
64-65: Board mapping correctly added.The M5Stack Cardputer Advanced board is properly registered following the existing pattern and alphabetical ordering.
Drivers/TCA8418/Adafruit_TCA8418-license.txt (1)
1-26: Proper attribution and licensing.Including the Adafruit BSD license is correct practice for code adapted from their TCA8418 library (referenced in
Tca8418.cppline 25).Firmware/Kconfig (1)
54-56: LGTM: new board choice entry is correct.Entry aligns with existing pattern; no issues spotted.
sdkconfig.board.cyd-e32r28t (1)
27-33: Verify TLS 1.3 support and memory impact on this target/IDF.Enabling CONFIG_MBEDTLS_SSL_PROTO_TLS1_3 on ESP32 can raise RAM use and may not be supported on some IDF versions.
Please confirm successful build/handshake and heap headroom with this flag enabled.
Boards/LilygoTLoraPager/Source/devices/TpagerKeyboard.h (1)
33-39: Constructor/destructor changes look good.explicit ctor prevents accidental conversions; override clarifies polymorphic destruction.
Boards/M5stackCardputerAdv/Source/devices/Display.cpp (2)
7-20: Validate panel geometry (gapX/gapY/swap/mirror).The 53px gapX workaround may vary with rotation/theme; please verify full-screen widgets have no clip/fill artifacts.
22-28: SPI clock sanity-check.62.5 MHz can be marginal on some ST7789 panels and flex cables. If you see sporadic pixel noise, drop to 40–60 MHz.
sdkconfig.board.m5stack-cardputer-adv (1)
47-49: Confirm 120 MHz QIO flash frequency stability.Some modules are only reliable at 80 MHz. If you see random faults, drop to 80 MHz.
sdkconfig.board.cyd-e32r32p (2)
2-4: Stack size bump: verify RTOS headroom and WDT margins.6144 main/3072 system-event seem reasonable for Wi‑Fi scans. Confirm total RAM usage at peak (Wi‑Fi + LVGL + drivers) and WDT timeouts after the change. Recommend a quick run of heap and task watermark telemetry on scan and TLS connect.
32-32: TLS 1.3 enablement: check memory and compatibility.TLS1.3 increases heap and may drop RSA-only endpoints. Ensure:
- Handshake heap fits under your caps (PSRAM vs internal).
- Needed cipher suites/signatures are enabled.
- Legacy servers still reachable (fallback to 1.2 if required).
Boards/M5stackCardputerAdv/Source/Configuration.cpp (1)
70-117: SPI configs look correct; ensure LVGL lock pairing.Display SPI uses lvgl lock; SD SPI has none. Confirm the display driver only uses the provided lock and SD path is LVGL‑agnostic to avoid deadlocks.
Boards/M5stackCardputerAdv/Source/devices/CardputerPower.cpp (1)
44-51: Channel/width/atten: request verification of the actual battery sense pin.ADC1_CHANNEL_9 with 11 dB atten is fine for wide range. Please confirm it maps to the Cardputer Adv battery divider pin on the S3 and that ADC_WIDTH_BIT_DEFAULT resolves to supported width for this target.
Boards/M5stackCardputerAdv/Source/devices/CardputerKeyboard.cpp (1)
1-4: Build include path check.Static analysis flagged Tactility/hal/keyboard/KeyboardDevice.h not found. Verify include paths or casing in your CMake/component config.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (7)
Boards/M5stackCardputerAdv/Source/devices/CardputerKeyboard.cpp (4)
14-35: Fix keymap element type to avoid truncating LVGL key codes.The keymaps use
charwhich truncates LV_KEY_* constants (typically 16-bit values). This causes incorrect key codes to be queued and delivered to LVGL.Based on learnings
37-48: Use wider key type in readCallback to match LVGL constants.The
keypressvariable should use a wider type (uint16_t or lv_key_t) to handle LV_KEY_* constants without truncation.Based on learnings
62-124: Replace static modifier state with instance members and use non-blocking enqueue.The static local variables (shift_pressed, sym_pressed, cap_toggle, cap_toggle_armed) are shared across instances and persist incorrectly. Additionally, the blocking xQueueSend at line 104 can stall the timer context.
Based on learnings
145-153: Make stopLvgl idempotent with null checks.The assert() at line 146 will fail on repeated calls and can be compiled out in release builds, leading to nullptr dereference.
Based on learnings
Boards/M5stackCardputerAdv/Source/devices/CardputerKeyboard.h (3)
5-7: Include FreeRTOS headers for QueueHandle_t.The header uses
QueueHandle_t(line 12) but doesn't include the required FreeRTOS headers, leading to compilation errors in some contexts.Based on learnings
12-12: Initialize queue to nullptr and add instance modifier state members.The
queuemember should be initialized to nullptr. Additionally, the modifier state (shift_pressed, sym_pressed, cap_toggle, cap_toggle_armed) should be instance members rather than static locals in processKeyboard().Based on learnings
29-35: Fix queue element size and destructor UAF issues.The queue is created with
sizeof(char)but should use a wider type for LV_KEY_* constants. The destructor must stop LVGL and timers before deleting the queue to prevent use-after-free.Based on learnings
🧹 Nitpick comments (2)
Boards/M5stackCardputerAdv/Source/devices/CardputerKeyboard.cpp (1)
8-8: Remove unused BACKLIGHT constant.This constant is defined but never used in the file. It appears to be leftover from TpagerKeyboard.
-constexpr auto BACKLIGHT = GPIO_NUM_46; -Boards/M5stackCardputer/Source/Configuration.cpp (1)
13-15: Replace magic number with a named constant.The hardcoded value
512lacks context. Define it as a named constant (e.g.,LCD_BACKLIGHT_PWM_FREQUENCYorLCD_BACKLIGHT_PWM_RESOLUTION) to clarify its purpose and improve maintainability.Apply this diff to improve clarity:
+namespace { + constexpr uint32_t LCD_BACKLIGHT_PWM_RESOLUTION = 512; +} + bool initBoot() { - return driver::pwmbacklight::init(LCD_PIN_BACKLIGHT, 512); + return driver::pwmbacklight::init(LCD_PIN_BACKLIGHT, LCD_BACKLIGHT_PWM_RESOLUTION); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Boards/LilygoTLoraPager/Source/devices/TpagerKeyboard.cpp(3 hunks)Boards/M5stackCardputer/Source/Configuration.cpp(2 hunks)Boards/M5stackCardputerAdv/Source/devices/CardputerKeyboard.cpp(1 hunks)Boards/M5stackCardputerAdv/Source/devices/CardputerKeyboard.h(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
Boards/LilygoTLoraPager/Source/devices/TpagerKeyboard.cpp (1)
Boards/M5stackCardputerAdv/Source/devices/CardputerKeyboard.cpp (2)
readCallback(37-48)readCallback(37-37)
Boards/M5stackCardputerAdv/Source/devices/CardputerKeyboard.cpp (1)
Boards/LilygoTLoraPager/Source/devices/TpagerKeyboard.cpp (10)
readCallback(41-52)readCallback(41-41)processKeyboard(54-117)processKeyboard(54-54)startLvgl(119-143)startLvgl(119-119)stopLvgl(145-157)stopLvgl(145-145)isAttached(159-161)isAttached(159-159)
Boards/M5stackCardputerAdv/Source/devices/CardputerKeyboard.h (2)
Tactility/Include/Tactility/hal/keyboard/KeyboardDevice.h (1)
KeyboardDevice(11-24)Boards/M5stackCardputerAdv/Source/devices/CardputerKeyboard.cpp (12)
processKeyboard(62-124)processKeyboard(62-62)readCallback(37-48)readCallback(37-37)remap(50-60)remap(50-50)startLvgl(126-143)startLvgl(126-126)stopLvgl(145-153)stopLvgl(145-145)isAttached(155-157)isAttached(155-155)
🪛 Clang (14.0.6)
Boards/M5stackCardputerAdv/Source/devices/CardputerKeyboard.h
[error] 3-3: 'Tactility/hal/keyboard/KeyboardDevice.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). (20)
- GitHub Check: build (waveshare-s3-touch-lcd-128, esp32s3)
- GitHub Check: build (waveshare-s3-touch-lcd-147, esp32s3)
- GitHub Check: build (elecrow-crowpanel-basic-50, esp32s3)
- GitHub Check: build (lilygo-tlora-pager, esp32s3)
- GitHub Check: build (lilygo-tdeck, esp32s3)
- GitHub Check: build (elecrow-crowpanel-basic-28, esp32)
- GitHub Check: build (elecrow-crowpanel-basic-35, esp32)
- GitHub Check: build (elecrow-crowpanel-advance-50, esp32s3)
- GitHub Check: build (cyd-4848s040c, esp32s3)
- GitHub Check: build (cyd-jc8048w550c, esp32s3)
- GitHub Check: build (cyd-jc2432w328c, esp32)
- GitHub Check: build (cyd-2432s032c, esp32)
- GitHub Check: build (cyd-8048s043c, esp32s3)
- GitHub Check: build (cyd-e32r32p, esp32)
- GitHub Check: build (cyd-2432s028r, esp32)
- GitHub Check: build (cyd-e32r28t, esp32)
- GitHub Check: build (cyd-2432s028rv3, esp32)
- GitHub Check: build (cyd-2432s028r, esp32)
- GitHub Check: Linux
- GitHub Check: macOS
🔇 Additional comments (5)
Boards/M5stackCardputer/Source/Configuration.cpp (1)
34-34: No changes needed—GPIO configuration is correct.The apparent GPIO "conflict" is intentional: I2C is initialized with
.initMode = i2c::InitMode::Disabledand.isMutable = true, allowing runtime protocol switching on Port A. UART is the default active protocol, while I2C can be enabled at runtime if needed. This follows the M5Stack Cardputer design where the Grove connector supports either I2C or UART, not both simultaneously. TheI2C_NUM_0port assignment is correct for the hardware.Boards/LilygoTLoraPager/Source/devices/TpagerKeyboard.cpp (4)
18-39: LGTM!The keymap reorganization is consistent across all three variants (lowercase, uppercase, symbol). Modifier keys at positions (2,0) for sym and (2,8) for shift correctly map to null characters, and special keys (ENTER, BACKSPACE, space) are consistently positioned across all layouts.
41-52: LGTM!The change to non-blocking queue receive (0 timeout) is correct for LVGL input device callbacks, which must return quickly without blocking. The implementation properly sets
LV_INDEV_STATE_RELEASEDwhen no key is available and matches the pattern used in CardputerKeyboard.cpp.
94-94: LGTM!The change from blocking (
portMAX_DELAY) to a 50ms timeout prevents the keyboard processing task from hanging indefinitely if the queue is full. Given the 20ms timer period (line 139) and non-blocking queue reads inreadCallback(line 45), this timeout provides sufficient time for the queue to drain while maintaining responsive keyboard handling.
68-73: Coordinate mapping verified as hardware-valid; out-of-bounds issue resolved.The sym (row 2, col 0) and shift (row 2, col 8) coordinates are now valid for the TCA8418 controller (which supports rows 0-7 and columns 0-9). The old coordinate (col 10) was out-of-bounds, confirming this is a legitimate hardware fix. Both coordinates correctly map to null entries in all keymaps and are consistently used in press/release detection (lines 68, 71, 101, 104). Code is internally consistent and correct.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
Boards/M5stackCardputerAdv/Source/devices/CardputerKeyboard.cpp (3)
12-33: Fix keymap element type to avoid truncating LVGL key codes.The keymap arrays use
charwhich cannot represent LVGL key constants likeLV_KEY_BACKSPACE(line 13),LV_KEY_DEL(line 21),LV_KEY_ESC(line 29),LV_KEY_PREV,LV_KEY_NEXT,LV_KEY_LEFT,LV_KEY_RIGHT, andLV_KEY_ENTERwithout truncation. These constants are typically defined as values > 255.Apply this diff to use a wider type:
+using KeyCode = uint16_t; // or int to match lv_key_t + // Lowercase Keymap -static constexpr char keymap_lc[KB_COLS][KB_ROWS] = { +static constexpr KeyCode keymap_lc[KB_COLS][KB_ROWS] = { {'`', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '_', '=', LV_KEY_BACKSPACE}, {'\t', 'q', 'w', 'e', 'r', 't', 'y', 'u', 'i', 'o', 'p', '[', ']', '\\'}, {'\0', '\0', 'a', 's', 'd', 'f', 'g', 'h', 'j', 'k', 'l', ';', '\'', LV_KEY_ENTER}, {'\0', '\0', '\0', 'z', 'x', 'c', 'v', 'b', 'n', 'm', ',', '.', '/', ' '} }; // Uppercase Keymap -static constexpr char keymap_uc[KB_COLS][KB_ROWS] = { +static constexpr KeyCode keymap_uc[KB_COLS][KB_ROWS] = { {'~', '!', '@', '#', '$', '%', '^', '&', '*', '(', ')', '-', '+', LV_KEY_DEL}, {'\t', 'Q', 'W', 'E', 'R', 'T', 'Y', 'U', 'I', 'O', 'P', '{', '}', '|'}, {'\0', '\0', 'A', 'S', 'D', 'F', 'G', 'H', 'J', 'K', 'L', ':', '"', LV_KEY_ENTER}, {'\0', '\0', '\0', 'Z', 'X', 'C', 'V', 'B', 'N', 'M', '<', '>', '?', ' '} }; // Symbol Keymap -static constexpr char keymap_sy[KB_COLS][KB_ROWS] = { +static constexpr KeyCode keymap_sy[KB_COLS][KB_ROWS] = { {LV_KEY_ESC, '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0'}, {'\t', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0'}, {'\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', LV_KEY_PREV, '\0', LV_KEY_ENTER}, {'\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', LV_KEY_LEFT, LV_KEY_NEXT, LV_KEY_RIGHT, '\0'} };Also update the queue element size in the header (line 31) and the local variable type in readCallback (line 37) and processKeyboard (line 93).
35-46: Match keypress type to keymap element type.The
readCallbackuseschar keypresswhich must match the wider type used in the keymaps to avoid truncation when dequeuing LVGL key codes.Apply this diff:
void CardputerKeyboard::readCallback(lv_indev_t* indev, lv_indev_data_t* data) { auto keyboard = static_cast<CardputerKeyboard*>(lv_indev_get_user_data(indev)); - char keypress = 0; + KeyCode keypress = 0; if (xQueueReceive(keyboard->queue, &keypress, 0) == pdPASS) { data->key = keypress; data->state = LV_INDEV_STATE_PRESSED; } else { data->key = 0; data->state = LV_INDEV_STATE_RELEASED; } }
88-103: Update chr type to match keymap elements.Line 93 declares
char chrwhich must use the wider type to avoid truncating LVGL key codes retrieved from the keymaps.Apply this diff:
// Process regular key input given the processed modifiers for (int i = 0; i < keypad->pressed_key_count; i++) { auto row = keypad->pressed_list[i].row; auto column = keypad->pressed_list[i].col; remap(row, column); - char chr = '\0'; + KeyCode chr = 0; if (sym_pressed) { chr = keymap_sy[row][column]; } else if (shift_pressed || cap_toggle) { chr = keymap_uc[row][column]; } else { chr = keymap_lc[row][column]; } - if (chr != '\0') xQueueSend(queue, &chr, 50 / portTICK_PERIOD_MS); + if (chr != 0) xQueueSend(queue, &chr, 50 / portTICK_PERIOD_MS); }Boards/M5stackCardputerAdv/Source/devices/CardputerKeyboard.h (2)
30-32: Update queue element size to match wider key type.The queue is created with
sizeof(char)but must use a wider type (e.g.,uint16_torint) to accommodate LVGL key codes without truncation. This must match the keymap array element type updated in the .cpp file.Apply this diff:
explicit CardputerKeyboard(const std::shared_ptr<Tca8418>& tca) : keypad(tca) { - queue = xQueueCreate(20, sizeof(char)); + queue = xQueueCreate(20, sizeof(uint16_t)); // or sizeof(KeyCode) }
34-36: Destructor must ensure LVGL device and timer are stopped to prevent UAF.The destructor only deletes the queue but doesn't verify that
kbHandleandinputTimerare stopped. If LVGL callbacks or the timer fire after destruction, they'll access freed memory (use-after-free).Move the destructor out-of-line and ensure proper teardown:
In the header:
- ~CardputerKeyboard() override { - vQueueDelete(queue); - } + ~CardputerKeyboard() override;In the .cpp file:
CardputerKeyboard::~CardputerKeyboard() { // Ensure LVGL indev and timer are stopped before deleting queue if (kbHandle || inputTimer) { stopLvgl(); } if (queue) { vQueueDelete(queue); queue = nullptr; } }
🧹 Nitpick comments (1)
Boards/M5stackCardputerAdv/Source/devices/CardputerPower.h (1)
12-12: Consider typical Li-ion minimum voltage for broader charge range.The minimum voltage of 3.3V is higher than the typical Li-ion minimum (3.0-3.2V). This means the charge level won't report very low percentages, which may be intentional for battery longevity but could affect UX if users expect to see lower charge readings.
If a wider charge estimation range is desired:
- ChargeFromVoltage chargeFromAdcVoltage = ChargeFromVoltage(3.3f, 4.2f); + ChargeFromVoltage chargeFromAdcVoltage = ChargeFromVoltage(3.2f, 4.2f);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
Boards/LilygoTLoraPager/Source/devices/SdCard.h(1 hunks)Boards/LilygoTLoraPager/Source/devices/TpagerKeyboard.cpp(4 hunks)Boards/LilygoTLoraPager/Source/devices/TpagerKeyboard.h(3 hunks)Boards/M5stackCardputer/Source/devices/CardputerPower.h(1 hunks)Boards/M5stackCardputer/Source/devices/SdCard.h(1 hunks)Boards/M5stackCardputerAdv/Source/devices/CardputerKeyboard.cpp(1 hunks)Boards/M5stackCardputerAdv/Source/devices/CardputerKeyboard.h(1 hunks)Boards/M5stackCardputerAdv/Source/devices/CardputerPower.h(1 hunks)Boards/M5stackCardputerAdv/Source/devices/SdCard.h(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- Boards/M5stackCardputer/Source/devices/CardputerPower.h
🚧 Files skipped from review as they are similar to previous changes (1)
- Boards/LilygoTLoraPager/Source/devices/TpagerKeyboard.h
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-27T22:52:15.531Z
Learnt from: KenVanHoeylandt
PR: ByteWelder/Tactility#395
File: Boards/M5stackCardputerAdv/Source/devices/CardputerKeyboard.cpp:145-153
Timestamp: 2025-10-27T22:52:15.531Z
Learning: In the Tactility codebase, for LVGL device lifecycle methods (e.g., stopLvgl(), startLvgl()), the maintainer prefers using assert() to catch double-calls and programming errors rather than adding defensive null-checks. The intent is to crash on misuse during development rather than silently handle it, avoiding the overhead of defensive checks in every driver.
Applied to files:
Boards/M5stackCardputerAdv/Source/devices/CardputerKeyboard.cpp
🧬 Code graph analysis (5)
Boards/LilygoTLoraPager/Source/devices/TpagerKeyboard.cpp (1)
Boards/M5stackCardputerAdv/Source/devices/CardputerKeyboard.cpp (2)
readCallback(35-46)readCallback(35-35)
Boards/M5stackCardputerAdv/Source/devices/CardputerPower.h (3)
Tactility/Include/Tactility/hal/power/PowerDevice.h (1)
PowerDevice(8-47)Drivers/EstimatedPower/Source/ChargeFromVoltage.h (1)
ChargeFromVoltage(5-22)Boards/M5stackCardputerAdv/Source/devices/CardputerPower.cpp (10)
adcInitCalibration(8-25)adcInitCalibration(8-8)adcReadValue(27-38)adcReadValue(27-27)ensureInitialized(40-57)ensureInitialized(40-40)supportsMetric(59-68)supportsMetric(59-59)getMetric(70-85)getMetric(70-70)
Boards/M5stackCardputerAdv/Source/devices/CardputerKeyboard.cpp (1)
Boards/LilygoTLoraPager/Source/devices/TpagerKeyboard.cpp (10)
readCallback(40-51)readCallback(40-40)processKeyboard(53-116)processKeyboard(53-53)startLvgl(118-142)startLvgl(118-118)stopLvgl(144-156)stopLvgl(144-144)isAttached(158-160)isAttached(158-158)
Boards/M5stackCardputerAdv/Source/devices/CardputerKeyboard.h (2)
Tactility/Include/Tactility/hal/keyboard/KeyboardDevice.h (1)
KeyboardDevice(11-24)Boards/M5stackCardputerAdv/Source/devices/CardputerKeyboard.cpp (12)
processKeyboard(60-122)processKeyboard(60-60)readCallback(35-46)readCallback(35-35)remap(48-58)remap(48-48)startLvgl(124-141)startLvgl(124-124)stopLvgl(143-151)stopLvgl(143-143)isAttached(153-155)isAttached(153-153)
Boards/M5stackCardputerAdv/Source/devices/SdCard.h (2)
Tactility/Include/Tactility/hal/sdcard/SdCardDevice.h (1)
SdCardDevice(14-67)Boards/M5stackCardputerAdv/Source/devices/SdCard.cpp (2)
createSdCard(10-25)createSdCard(10-10)
🪛 Clang (14.0.6)
Boards/M5stackCardputerAdv/Source/devices/CardputerPower.h
[error] 3-3: 'Tactility/hal/power/PowerDevice.h' file not found
(clang-diagnostic-error)
Boards/LilygoTLoraPager/Source/devices/SdCard.h
[error] 3-3: 'Tactility/hal/sdcard/SdCardDevice.h' file not found
(clang-diagnostic-error)
Boards/M5stackCardputer/Source/devices/SdCard.h
[error] 3-3: 'Tactility/hal/sdcard/SdCardDevice.h' file not found
(clang-diagnostic-error)
Boards/M5stackCardputerAdv/Source/devices/CardputerKeyboard.h
[error] 3-3: 'Tactility/hal/keyboard/KeyboardDevice.h' file not found
(clang-diagnostic-error)
Boards/M5stackCardputerAdv/Source/devices/SdCard.h
[error] 3-3: 'Tactility/hal/sdcard/SdCardDevice.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). (20)
- GitHub Check: build (waveshare-s3-touch-lcd-128, esp32s3)
- GitHub Check: build (waveshare-s3-touch-43, esp32s3)
- GitHub Check: build (lilygo-tdongle-s3, esp32s3)
- GitHub Check: build (m5stack-core2, esp32)
- GitHub Check: build (elecrow-crowpanel-basic-50, esp32s3)
- GitHub Check: build (elecrow-crowpanel-advance-28, esp32s3)
- GitHub Check: build (lilygo-tdeck, esp32s3)
- GitHub Check: build (cyd-8048s043c, esp32s3)
- GitHub Check: build (cyd-jc8048w550c, esp32s3)
- GitHub Check: build (cyd-4848s040c, esp32s3)
- GitHub Check: build (cyd-e32r32p, esp32)
- GitHub Check: build (cyd-2432s032c, esp32)
- GitHub Check: build (cyd-jc2432w328c, esp32)
- GitHub Check: build (cyd-2432s024c, esp32)
- GitHub Check: build (cyd-2432s028r, esp32)
- GitHub Check: build (lilygo-tdeck, esp32s3)
- GitHub Check: build (cyd-2432s028r, esp32)
- GitHub Check: run
- GitHub Check: Linux
- GitHub Check: macOS
🔇 Additional comments (8)
Boards/M5stackCardputer/Source/devices/SdCard.h (1)
3-4: LGTM! Clean modernization of includes.The switch to system-style include brackets for the Tactility library header and the addition of
<memory>forstd::shared_ptrsupport are both correct and follow best practices.Note: The static analysis error about the file not being found is a false positive—the analyzer lacks the full build context and include paths that are available during compilation.
Boards/LilygoTLoraPager/Source/devices/SdCard.h (1)
3-4: LGTM! Necessary header inclusion and appropriate include style.The addition of
<memory>is essential forstd::shared_ptrusage on line 8, and switching to system-style includes<...>for the Tactility library header is appropriate. The static analysis error about the file not being found is a false positive due to misconfigured include paths in the analysis environment.Boards/M5stackCardputerAdv/Source/devices/SdCard.h (1)
1-8: Previous feedback successfully addressed; static analysis error is a false positive.The header has been added (line 4) and the include path now uses angle brackets (line 3) as suggested in the previous review. The static analysis error about the header not being found is a false positive—the include path is correct and matches the actual file structure, as confirmed by the successful compilation of the implementation file
SdCard.cpp.Boards/M5stackCardputerAdv/Source/devices/CardputerPower.h (2)
1-11: Well-structured header with dependencies addressed.The inclusion of
<string>properly addresses the past review comment. The header includes are appropriate, and the class declaration withfinalis good practice.Note: The static analysis error about
PowerDevice.hnot found is a false positive due to analyzer include path configuration.
22-29: Clean public interface following PowerDevice contract.The public methods correctly override the base class interface with appropriate const qualifiers. The inline implementations for
getName()andgetDescription()are simple and clear.Boards/LilygoTLoraPager/Source/devices/TpagerKeyboard.cpp (3)
5-5: LGTM!The include is necessary for the
gpio_num_ttype used throughout the file.
44-44: LGTM!Non-blocking queue receive is appropriate for a periodic callback and matches the pattern used in CardputerKeyboard.
67-73: LGTM!Special key coordinate detection is consistent between pressed and released states, and aligns with the updated keymap structure.
Also applies to: 100-106
Uh oh!
There was an error while loading. Please reload this page.