feat: Add basic M5Stack PaperS3 support #380
Conversation
|
It's possible that pushing pixel to the display is taking too long, triggering the watchdog. The watchdog triggers at My next step would be to put logging in I haven't reviewed everything yet, but I went through some files and everything looks very neat and is easy to read. Good job 👍 |
fonix232
force-pushed
the
feat/add_m5stack_papers3_support
branch
from
b003a1e to
ea68ea4
Compare
|
Pixel pushing shouldn't take long. Nonetheless I had a number of other issues with FastEPD in the end (clearing the display would also cause watchdog issues or ProhibitedStore exceptions), so I went back to my original EPDiy idea. Please see the updated drivers :) Also I can't take full credit for it, a lot of the code was AI generated. My C/C++ is generally speaking... Not so good. But, the Tactility codebase on the other hand is good enough that Claude was able to replicate behaviour, tailored to the libraries in question. Mind you I did review the results, but I could never remember to e.g. free up allocated buffers, or to create them to begin with! I've been able to use this last version quite well, though there's a few outstanding things to do:
1 I'd go even a step beyond and recommend a theming system. LVGL already supports runtime theming, so if a device config could return themes somehow - or, even better idea, let's create a theme constructor that reads data from a regular file, allowing loading styles from simple YAML/JSON/XML/TOML (my vote goes for TOML, it's really straightforward), which means they can be loaded from the file system, without the need to recompile things. |
|
Well, EPDiy seems to have its own problems too. Upon trying to activate WiFi, another WDT happens: This is coming from the EPDiy library definitely, but the specific line it's getting caught on, just does some buffer black magic: |
Since you're using EpDiy, someone had implemented this board for that driver, but the PR hasn't been updated. |
|
@NellowTCS I'm aware - the reason why I added my fork of EPDiy as the submodule instead of vroland's is because I've already merged that PR on my fork with the fixes needed to resolve the merge conflicts 🙂 |
ah, cool! This library will probably be really helpful for me as well in the future, so thanks for adding it!!! |
Hmm, I think the WDT is because our LVGL flush is blocking everything when WiFi kicks in. Bit of an oversimplification but this is what i think happens:
Maybe make the flush async? Basically, push the area + pixel data into a worker/task and call lv_disp_flush_ready() immediately so LVGL can keep running? (This is what I've had to do in a driver for another board, so it should work.) |
|
Sorry, my phone keyboard glitched and I accidentally closed the PR. Avoid async draw calls until you find out why it is slow. Debug it by attaching a debugger and pause the app where it hangs. |
KenVanHoeylandt
added
the
board
|
FYI: I simplified the way the boards are defined:
|
|
@NellowTCS thanks for the recommendations, I'll give it a go. @KenVanHoeylandt awesome work, I'll adapt my PR tonight to these changes. I've also done some experimentation on implementing a theming system using cleartext files (basically a custom LVGL theme base class that can be applied and re-applied based on internal flags, which can load the theme details from a TOML file similar to how current properties are loaded, then applies those styles to the active screen entity), but let me open a separate PR and FR for that 🙂 |
|
Oh, and one more thing: I've rewrote the LVGL flush function to utilise EPDiy's In specific scenarios (so far I managed to reproduce it on dropdown lists), the drawn image gets weirdly slanted, almost as if the rows became shorter and shorter... I'll attach a photo later today. |
fonix232
force-pushed
the
feat/add_m5stack_papers3_support
branch
from
ea68ea4 to
828e849
Compare
📝 WalkthroughWalkthroughThis pull request introduces support for the M5Stack PaperS3 device, an ESP32S3-based e-ink display system. Changes include adding the epdiy library as a submodule, creating a new EPDiyDisplay driver component that integrates e-paper control with LVGL rendering, and implementing device-specific factories for power management, display, touch input, and SD card access. Configuration files and metadata for the M5Stack PaperS3 are added to the Devices directory structure. Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Tip 📝 Customizable high-level summaries are now available!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example:
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 |
|
Just pushed an update. Added some extra YIELDs to EPDiy to get rid of the WDT triggers, and it seems to be working - however, the recent changes to @KenVanHoeylandt could you look into this? I'm still registering the touch driver properly, I can see in the logs that it gets initialised, but even with the logging block added to |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Nitpick comments (9)
Drivers/EspLcdCompat/Source/EspLcdTouch.cpp (1)
14-14: Remove redundant log message.The generic "Touch event callback triggered" message is redundant since the specific event type is logged on lines 23, 25, or 27. Removing this reduces log verbosity.
Apply this diff:
static void touch_event_callback(lv_event_t* e) { - TT_LOG_I(TAG, "Touch event callback triggered"); lv_event_code_t code = lv_event_get_code(e);Drivers/EPDiyDisplay/README.md (1)
1-3: Consider enriching the README with additional context (optional).While the current description is accurate, the README could benefit from additional context for developers integrating this driver, such as links to the EPDiy library, key dependencies, configuration pointers, or known limitations—especially given the watchdog and rendering optimization challenges discussed in the PR comments.
If you'd like to expand the documentation, consider adding sections covering:
- Brief overview of EPDiy library and what it provides
- Dependencies (esp_lvgl_port, epdiy library version, etc.)
- Known limitations or caveats (e.g., regarding flush performance or rendering artifacts mentioned in the PR)
- Link to EPDiy documentation or related resources
Boards/M5StackPaperS3/Source/devices/Display.hpp (1)
12-15: Consider using constexpr constants instead of macros.Since the project uses C++23, consider replacing these preprocessor macros with
constexprconstants for better type safety and scope control.-#define PAPERS3_EPD_HORIZONTAL_RESOLUTION 540 -#define PAPERS3_EPD_VERTICAL_RESOLUTION 960 -#define PAPERS3_EPD_DRAW_BUFFER_HEIGHT (PAPERS3_EPD_VERTICAL_RESOLUTION / 10) -#define PAPERS3_EPD_DRAW_BUFFER_SIZE (PAPERS3_EPD_HORIZONTAL_RESOLUTION * PAPERS3_EPD_DRAW_BUFFER_HEIGHT) +namespace { + constexpr int PAPERS3_EPD_HORIZONTAL_RESOLUTION = 540; + constexpr int PAPERS3_EPD_VERTICAL_RESOLUTION = 960; + constexpr int PAPERS3_EPD_DRAW_BUFFER_HEIGHT = PAPERS3_EPD_VERTICAL_RESOLUTION / 10; + constexpr int PAPERS3_EPD_DRAW_BUFFER_SIZE = PAPERS3_EPD_HORIZONTAL_RESOLUTION * PAPERS3_EPD_DRAW_BUFFER_HEIGHT; +}Boards/M5StackPaperS3/CMakeLists.txt (1)
1-1: Consider avoidingGLOB_RECURSEfor source file discovery.Using
GLOB_RECURSEprevents CMake from automatically detecting when source files are added or removed from the filesystem, requiring manual reconfiguration. This can lead to incomplete builds when new files are added.Consider either:
- Explicitly listing source files in CMakeLists.txt
- Using a pattern that triggers CMake reconfiguration
- Documenting that developers must manually reconfigure after adding files
-file(GLOB_RECURSE SOURCE_FILES Source/*.c*) +# Explicitly list source files or document reconfiguration requirement +set(SOURCE_FILES + Source/Init.cpp + Source/Configuration.cpp + Source/devices/Display.cpp + Source/devices/Power.cpp + Source/devices/SdCard.cpp +)Boards/M5StackPaperS3/Source/Init.cpp (1)
12-20: Consider simplifying sincepowerOn()never fails.The error handling for
powerOn()is effectively unreachable since the function always returnstrue. While this pattern may be intentional for consistency with other boards or future expansion, consider simplifying if power-on will never require a sequence.If no future power-on logic is anticipated:
bool initBoot() { ESP_LOGI(TAG, LOG_MESSAGE_POWER_ON_START); - if (!powerOn()) { - TT_LOG_E(TAG, LOG_MESSAGE_POWER_ON_FAILED); - return false; - } - return true; }sdkconfig.board.m5stack-papers3 (1)
66-67: Track ESP-IDF flash frequency bug resolution.The comment indicates a temporary workaround for an ESP-IDF bug. Consider creating a tracking issue to revert this change once the upstream bug is resolved.
Would you like me to open a tracking issue for monitoring the ESP-IDF flash frequency bug and reverting to 80MHz when fixed?
Boards/M5StackPaperS3/Source/devices/Power.cpp (1)
181-199: Shutdown pulses may be incorrect; device-specific timing likely needs “hold-high”.Many PMICs expect holding the line high for N ms (e.g., 2–3 s) rather than pulsing. Pulsing can fail sporadically and blocks the caller for ~1 s here.
If the PMIC expects a hold, consider:
- for (int i = 0; i < POWER_OFF_PULSE_COUNT; i++) { - gpio_set_level(powerOffPin, 1); - vTaskDelay(pdMS_TO_TICKS(POWER_OFF_PULSE_DURATION_MS)); - gpio_set_level(powerOffPin, 0); - vTaskDelay(pdMS_TO_TICKS(POWER_OFF_PULSE_DURATION_MS)); - } - gpio_set_level(powerOffPin, 1); + const int hold_ms = 2000; // adjust per spec + gpio_set_level(powerOffPin, 1); + vTaskDelay(pdMS_TO_TICKS(hold_ms)); + // PMIC should remove power; don't pull low afterward.Drivers/EPDiyDisplay/Source/EpdiyDisplay.h (1)
51-59: Consider preallocating a reusable packed buffer (member) to avoid per-flush allocations.Add members to hold a reusable 4‑bpp buffer; allocate on demand and free in destructor. This reduces fragmentation and WDT risk.
Example members (to be used by .cpp):
private: std::unique_ptr<Configuration> configuration; std::shared_ptr<tt::Mutex> lock; lv_display_t* _Nullable lvglDisplay = nullptr; EpdiyHighlevelState highlevelState = {}; uint8_t* framebuffer = nullptr; + uint8_t* packedBuf = nullptr; + size_t packedBufCap = 0; bool initialized = false; bool powered = false;And free in destructor:
~EpdiyDisplay() override; +// in destructor body: +if (packedBuf) { heap_caps_free(packedBuf); packedBuf = nullptr; packedBufCap = 0; }Drivers/EPDiyDisplay/Source/EpdiyDisplay.cpp (1)
64-69: Initial full clear can be costly; consider deferring or making conditional.Full-screen epd_clear() at startup adds several seconds and risks WDT when system loads Wi‑Fi/tasks. Make it conditional (first boot, dirty flag) or perform after LVGL is up.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
.gitmodules(1 hunks)Boards/M5StackPaperS3/CMakeLists.txt(1 hunks)Boards/M5StackPaperS3/Source/Configuration.cpp(1 hunks)Boards/M5StackPaperS3/Source/Init.cpp(1 hunks)Boards/M5StackPaperS3/Source/devices/Display.cpp(1 hunks)Boards/M5StackPaperS3/Source/devices/Display.hpp(1 hunks)Boards/M5StackPaperS3/Source/devices/Power.cpp(1 hunks)Boards/M5StackPaperS3/Source/devices/Power.hpp(1 hunks)Boards/M5StackPaperS3/Source/devices/SdCard.cpp(1 hunks)Boards/M5StackPaperS3/Source/devices/SdCard.hpp(1 hunks)Buildscripts/board.cmake(1 hunks)CMakeLists.txt(1 hunks)Drivers/EPDiyDisplay/CMakeLists.txt(1 hunks)Drivers/EPDiyDisplay/README.md(1 hunks)Drivers/EPDiyDisplay/Source/EpdiyDisplay.cpp(1 hunks)Drivers/EPDiyDisplay/Source/EpdiyDisplay.h(1 hunks)Drivers/EPDiyDisplay/Source/EpdiyDisplayHelper.h(1 hunks)Drivers/EspLcdCompat/Source/EspLcdTouch.cpp(2 hunks)Drivers/EstimatedPower/Source/ChargeFromAdcVoltage.h(1 hunks)Firmware/Kconfig(1 hunks)Libraries/epdiy(1 hunks)sdkconfig.board.m5stack-papers3(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
Drivers/EPDiyDisplay/Source/EpdiyDisplayHelper.h (3)
Drivers/EPDiyDisplay/Source/EpdiyDisplay.cpp (2)
EpdiyDisplay(11-18)EpdiyDisplay(20-27)Tactility/Include/Tactility/hal/touch/TouchDevice.h (1)
TouchDevice(12-30)Drivers/RgbDisplay/Source/RgbDisplay.h (1)
Configuration(22-58)
Boards/M5StackPaperS3/Source/devices/Power.cpp (2)
Boards/M5StackPaperS3/Source/devices/Power.hpp (4)
PaperS3Power(31-36)PaperS3Power(37-37)type(43-43)type(44-44)Drivers/EstimatedPower/Source/ChargeFromAdcVoltage.h (1)
ChargeFromAdcVoltage(6-52)
Boards/M5StackPaperS3/Source/devices/Power.hpp (1)
Boards/M5StackPaperS3/Source/devices/Power.cpp (2)
PaperS3Power(36-51)PaperS3Power(53-56)
Boards/M5StackPaperS3/Source/Configuration.cpp (4)
Boards/M5StackPaperS3/Source/devices/Power.cpp (2)
createPower(202-240)createPower(202-202)Boards/M5StackPaperS3/Source/devices/Display.cpp (2)
createDisplay(22-25)createDisplay(22-22)Boards/M5StackPaperS3/Source/devices/SdCard.cpp (2)
createSdCard(10-22)createSdCard(10-10)Boards/M5StackPaperS3/Source/Init.cpp (2)
initBoot(12-20)initBoot(12-12)
Boards/M5StackPaperS3/Source/devices/SdCard.hpp (1)
Tactility/Include/Tactility/hal/sdcard/SdCardDevice.h (1)
SdCardDevice(14-67)
Drivers/EPDiyDisplay/Source/EpdiyDisplay.h (3)
Drivers/EPDiyDisplay/Source/EpdiyDisplay.cpp (34)
EpdiyDisplay(11-18)EpdiyDisplay(20-27)flushCallback(214-220)flushCallback(214-214)flushInternal(314-369)flushInternal(314-314)rotationEventCallback(409-422)rotationEventCallback(409-409)handleRotationChange(424-438)handleRotationChange(424-424)epdRotationToLvgl(371-389)epdRotationToLvgl(371-371)lvglRotationToEpd(391-407)lvglRotationToEpd(391-391)start(29-70)start(29-29)stop(72-97)stop(72-72)setPowerOn(99-113)setPowerOn(99-99)startLvgl(116-191)startLvgl(116-116)stopLvgl(193-212)stopLvgl(193-193)clearScreen(224-240)clearScreen(224-224)clearArea(242-255)clearArea(242-242)updateScreen(257-278)updateScreen(257-257)updateArea(280-301)updateArea(280-280)setAllWhite(303-311)setAllWhite(303-303)Tactility/Include/Tactility/hal/display/DisplayDevice.h (1)
DisplayDevice(15-47)Tactility/Include/Tactility/hal/touch/TouchDevice.h (1)
TouchDevice(12-30)
Drivers/EPDiyDisplay/Source/EpdiyDisplay.cpp (1)
Drivers/EPDiyDisplay/Source/EpdiyDisplay.h (1)
start(86-91)
🪛 Clang (14.0.6)
Boards/M5StackPaperS3/Source/devices/Display.hpp
[error] 3-3: 'Tactility/hal/display/DisplayDevice.h' file not found
(clang-diagnostic-error)
Boards/M5StackPaperS3/Source/devices/Power.hpp
[error] 3-3: 'memory' file not found
(clang-diagnostic-error)
Boards/M5StackPaperS3/Source/Configuration.cpp
[error] 1-1: 'Tactility/hal/Configuration.h' file not found
(clang-diagnostic-error)
Boards/M5StackPaperS3/Source/devices/SdCard.hpp
[error] 3-3: 'Tactility/hal/sdcard/SdCardDevice.h' file not found
(clang-diagnostic-error)
Drivers/EPDiyDisplay/Source/EpdiyDisplay.h
[error] 3-3: 'Tactility/hal/display/DisplayDevice.h' file not found
(clang-diagnostic-error)
Boards/M5StackPaperS3/Source/Init.cpp
[error] 1-1: 'Tactility/kernel/SystemEvents.h' file not found
(clang-diagnostic-error)
🔇 Additional comments (24)
Drivers/EspLcdCompat/Source/EspLcdTouch.cpp (1)
89-95: LGTM!The event callback registration is properly implemented and correctly gated by the
LOG_TOUCH_EVENTSmacro. Once the macro is set to 0, this debug functionality will be cleanly disabled.Libraries/epdiy (2)
1-1: Address watchdog and rendering artifacts before stabilization.The PR objectives document persistent display issues: watchdog triggers, LVGL task hangs, and rendering artifacts (slanted dropdown rows in UI elements). Before merging this submodule pin:
- Ensure the LVGL flush callback implementation (EpdiyDisplay::flushInternal) is non-blocking. The PR discussion flagged that malloc + per-pixel drawing while holding a mutex blocks other tasks.
- Verify the rendering artifact fix is captured in the pinned commit or in your board's display driver configuration.
- Consider adding a regression test or pre-merge validation that boots the device, initializes the display, and renders a test pattern without triggering the watchdog.
This may warrant staying in "work in progress" status until display stability is confirmed on hardware.
1-1: Submodule configuration has issues: stale commit hash, misconfigured remote URL, and unrolled test commit.The original review comment references commit
4d1f8196..., but the current submodule HEAD is828e849("test: Add touch logging"); the original commit does not exist in the submodule history.More critically:
- .gitmodules specifies
https://github.com/fonix232/epdiyas the submodule URL- But the submodule's actual remote is configured to
https://github.com/ByteWelder/Tactility.git(the parent repository)- The test commit at HEAD is marked "Roll this commit back once touch is working"—this appears to be a work-in-progress that should not be merged as-is
The required upstream fixes from vroland/epdiy#392 (M5PaperS3 support) are present in the commit history (merge commit
8115ca4), so the core requirement is met. However, before merging:
- Verify the submodule remote URL configuration is intentional or fix it to match .gitmodules
- Roll back the test commit (
828e849) or confirm it should remain- Document why the fork approach was chosen over upstream tracking
CMakeLists.txt (1)
42-42: LGTM!The addition of the EPDiy library path is straightforward and follows the existing pattern for other library components.
.gitmodules (1)
19-21: Document the rationale for using a fork.The submodule points to your personal fork (fonix232/epdiy) rather than the official vroland/epdiy repository. While the PR comments mention merging necessary fixes, consider documenting:
- Why the fork is required (specific fixes/features needed)
- Whether these changes should be upstreamed
- Any maintenance implications for future updates
Have you considered contributing these fixes upstream to avoid long-term fork maintenance?
Drivers/EstimatedPower/Source/ChargeFromAdcVoltage.h (1)
43-51: LGTM!The new accessor is well-documented and appropriately warns users about careful usage. Exposing the ADC handle enables configuring additional channels on the same ADC unit, which aligns with the M5Stack PaperS3 power management requirements.
Buildscripts/board.cmake (1)
66-67: LGTM!The board mapping follows the established pattern and is correctly placed in the elseif chain.
Boards/M5StackPaperS3/Source/devices/SdCard.hpp (1)
1-7: LGTM!The header follows standard patterns with appropriate include guards and a clean factory function declaration.
Drivers/EPDiyDisplay/CMakeLists.txt (1)
1-6: LGTM!The component registration follows ESP-IDF conventions with appropriate dependencies for an LVGL-integrated display driver.
Firmware/Kconfig (1)
56-57: LGTM!The Kconfig entry follows the established pattern and is logically placed with other M5Stack boards.
Boards/M5StackPaperS3/Source/devices/Display.hpp (1)
1-17: LGTM!The header is well-structured with appropriate includes and a clean factory function declaration. The display resolution constants are clearly defined for the M5Stack PaperS3's e-ink panel.
Boards/M5StackPaperS3/Source/Init.cpp (1)
7-10: LGTM: Appropriately documented no-op initialization.The comment clearly explains that no power-on sequence is needed for this board, making the trivial implementation intentional and clear.
Boards/M5StackPaperS3/Source/devices/SdCard.cpp (1)
10-22: LGTM: SD card configuration is correct.The SPI SD card configuration properly sets the CS pin, uses
GPIO_NUM_NCfor unused pins, and enables auto-mounting at boot.Boards/M5StackPaperS3/Source/devices/Display.cpp (2)
8-8: Helpful comment for future hardware integration.The note about interrupt and reset pins provides useful context for future enhancements.
22-25: LGTM: Clean factory pattern for display creation.The display factory correctly obtains the touch device and delegates to the EPDiy helper.
sdkconfig.board.m5stack-papers3 (2)
3-4: LGTM: Stack size increases address WiFi stability.The increased stack sizes for event and main tasks are well-documented and address known WiFi crashes.
69-69: Verify LVGL refresh period of 0 is appropriate for e-ink.Setting
CONFIG_LV_DISP_DEF_REFR_PERIOD=0causes immediate refresh attempts. For e-ink displays with slow refresh rates, this might cause unnecessary CPU usage or contention. Based on the PR discussion about WDT timeouts during display refresh, consider if a non-zero value would be more appropriate.Drivers/EPDiyDisplay/Source/EpdiyDisplayHelper.h (1)
22-42: LGTM: Clean factory implementation.The factory method provides sensible defaults and constructs the EPDiy display configuration correctly.
Boards/M5StackPaperS3/Source/Configuration.cpp (2)
14-20: LGTM: Device factory aggregation is clean.The
createDevices()function correctly aggregates all device factories (power, display, SD card) into a single initialization vector.
22-86: Hardware configuration looks appropriate for M5Stack PaperS3.The I2C, SPI, and initialization configurations match the board's hardware specifications. The external I2C being disabled by default is a sensible choice.
Boards/M5StackPaperS3/Source/devices/Power.hpp (3)
11-20: Excellent hardware documentation.The detailed comments about GPIO pins, ADC channels, voltage divider, and charging voltage thresholds provide valuable context for future maintainers.
21-54: LGTM: Well-designed power management class.The class design demonstrates good practices:
- Clear ownership semantics with
unique_ptr- Appropriate separation of concerns (ADC voltage monitoring vs charge detection)
- Initialization state tracking with flags
- Clean interface overrides
31-36: Constructor parameter order is clear and logical.The constructor parameters flow logically from the ChargeFromAdcVoltage dependency to the additional hardware configuration (ADC handle, charge channel, power-off pin).
Boards/M5StackPaperS3/Source/devices/Power.cpp (1)
11-19: Verify PaperS3 pin/channel mapping and power-off semantics.Confirm VBAT/charge-status channels and POWER_OFF pin are correct for PaperS3; the charge pin may be digital on some variants, and shutdown may require a long hold, not pulses.
Would you validate against the PaperS3 schematic/datasheet and share confirmed GPIO→ADC channel mapping and shutdown timing?
Also applies to: 27-35
Coderabbitai pointed out the issue. |
fonix232
force-pushed
the
feat/add_m5stack_papers3_support
branch
from
828e849 to
04e7f2f
Compare
|
Drat, that was a merge conflict. Fixed, and made most of the recommendations. CodeRabbit is awesome :) |
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (2)
Drivers/EPDiyDisplay/Source/EpdiyDisplay.h (1)
3-5: Expose Tactility include dir in the component to fix CI header resolution.CI reported Tactility/hal/display/DisplayDevice.h not found; ensure Drivers/EPDiyDisplay/CMakeLists.txt adds Tactility’s include path to INCLUDE_DIRS, or export it from the Tactility component. This is a build-system wiring issue, not a code issue.
Example:
idf_component_register( SRC_DIRS "Source" INCLUDE_DIRS "Source" "../../Tactility/Include" REQUIRES Tactility epdiy esp_lvgl_port PRIV_REQUIRES esp_timer )Drivers/EPDiyDisplay/Source/EpdiyDisplay.cpp (1)
367-372: Don’t OR MODE_PACKING_2PPB into the draw mode.MODE_PACKING_2PPB is a buffer format flag, not an EpdDrawMode. Pass the configured mode unchanged.
- epd_hl_update_area( - &highlevelState, - static_cast<EpdDrawMode>(configuration->defaultDrawMode | MODE_PACKING_2PPB), - configuration->defaultTemperature, - update_area - ); + epd_hl_update_area( + &highlevelState, + configuration->defaultDrawMode, + configuration->defaultTemperature, + update_area + );
🧹 Nitpick comments (6)
Boards/M5StackPaperS3/Source/Init.cpp (1)
1-5: Include logging headers explicitly.ESP_LOGI/TT_LOG_E appear without an explicit include. To avoid reliance on transitive includes, add LogEsp (and/or esp_log.h) here.
#include "Tactility/kernel/SystemEvents.h" #include <Tactility/TactilityCore.h> +#include <Tactility/LogEsp.h> // for TT_LOG_* +// #include <esp_log.h> // if ESP_LOG* is used directlyPlease confirm LOG_MESSAGE_POWER_ON_* macros resolve from included headers; otherwise replace with string literals.
Boards/M5StackPaperS3/Source/Configuration.cpp (1)
20-84: Avoid ‘extern’ on a definition (clarity/linkage).Use ‘extern’ in declarations (headers) and a plain definition in this .cpp. Leaving ‘extern’ here is unusual and can confuse readers.
-extern const Configuration hardwareConfiguration = { +const Configuration hardwareConfiguration = {Boards/M5StackPaperS3/Source/devices/Power.hpp (1)
58-58: Avoid global factory symbol collisions.createPower() lives in the global namespace and may collide if multiple boards are compiled. Put it in a board-specific namespace or give it a unique name.
Apply one of:
-std::shared_ptr<tt::hal::power::PowerDevice> createPower(); +namespace m5stack_papers3 { +std::shared_ptr<tt::hal::power::PowerDevice> createPower(); +} // namespace m5stack_papers3And update the definition/call sites accordingly.
Drivers/EPDiyDisplay/Source/EpdiyDisplay.h (1)
56-60: Add a reusable packed buffer to avoid per-flush allocations.Allocate once (prefer SPIRAM), grow as needed, free in dtor.
EpdiyHighlevelState highlevelState = {}; uint8_t* framebuffer = nullptr; uint8_t* lvglDrawBuffer = nullptr; + // Reusable 4bpp packed buffer for flush() + uint8_t* packedBuf = nullptr; + size_t packedBufCap = 0;Also free packedBuf in ~EpdiyDisplay().
Drivers/EPDiyDisplay/Source/EpdiyDisplay.cpp (2)
219-223: Lower flush timing log to DEBUG to reduce spam.INFO logs every flush can degrade performance on ESP-IDF targets.
- TT_LOG_I(TAG, "flush took %llu us", (unsigned long long)(esp_timer_get_time() - t0)); + TT_LOG_D(TAG, "flush took %llu us", (unsigned long long)(esp_timer_get_time() - t0));
317-373: Allocate and reuse a SPIRAM packed buffer instead of new[]/delete[] per flush.Avoid heap churn and latency; reuse a member buffer sized to rowStrideBytes*height; free in dtor.
- size_t rowStrideBytes = (width + 1) / 2; // bytes per row in 2ppb - size_t packedSize = rowStrideBytes * height; - auto* pixelMap4bit = new uint8_t[packedSize]; + size_t rowStrideBytes = (width + 1) / 2; + size_t packedSize = rowStrideBytes * height; + if (packedBufCap < packedSize) { + if (packedBuf) heap_caps_free(packedBuf); + packedBuf = static_cast<uint8_t*>(heap_caps_malloc(packedSize, MALLOC_CAP_SPIRAM | MALLOC_CAP_8BIT)); + if (!packedBuf) { + packedBuf = static_cast<uint8_t*>(heap_caps_malloc(packedSize, MALLOC_CAP_DMA | MALLOC_CAP_INTERNAL)); + } + tt_check(packedBuf != nullptr); + packedBufCap = packedSize; + } @@ - uint8_t* dst = pixelMap4bit + row * rowStrideBytes; + uint8_t* dst = packedBuf + row * rowStrideBytes; @@ - epd_draw_rotated_image( + epd_draw_rotated_image( update_area, - pixelMap4bit, + packedBuf, framebuffer ); - - delete[] pixelMap4bit;Also add:
- EpdiyDisplay::~EpdiyDisplay() { + EpdiyDisplay::~EpdiyDisplay() { + if (packedBuf) { heap_caps_free(packedBuf); packedBuf = nullptr; packedBufCap = 0; } if (lvglDisplay != nullptr) { stopLvgl(); }And declare packedBuf/packedBufCap in the header (see earlier comment).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
.gitmodules(1 hunks)Boards/M5StackPaperS3/CMakeLists.txt(1 hunks)Boards/M5StackPaperS3/Source/Configuration.cpp(1 hunks)Boards/M5StackPaperS3/Source/Init.cpp(1 hunks)Boards/M5StackPaperS3/Source/devices/Display.cpp(1 hunks)Boards/M5StackPaperS3/Source/devices/Display.hpp(1 hunks)Boards/M5StackPaperS3/Source/devices/Power.cpp(1 hunks)Boards/M5StackPaperS3/Source/devices/Power.hpp(1 hunks)Boards/M5StackPaperS3/Source/devices/SdCard.cpp(1 hunks)Boards/M5StackPaperS3/Source/devices/SdCard.hpp(1 hunks)Buildscripts/board.cmake(1 hunks)CMakeLists.txt(1 hunks)Drivers/EPDiyDisplay/CMakeLists.txt(1 hunks)Drivers/EPDiyDisplay/README.md(1 hunks)Drivers/EPDiyDisplay/Source/EpdiyDisplay.cpp(1 hunks)Drivers/EPDiyDisplay/Source/EpdiyDisplay.h(1 hunks)Drivers/EPDiyDisplay/Source/EpdiyDisplayHelper.h(1 hunks)Drivers/EspLcdCompat/Source/EspLcdTouch.cpp(2 hunks)Drivers/EstimatedPower/Source/ChargeFromAdcVoltage.h(1 hunks)Firmware/Kconfig(1 hunks)Libraries/epdiy(1 hunks)sdkconfig.board.m5stack-papers3(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- Drivers/EPDiyDisplay/README.md
🚧 Files skipped from review as they are similar to previous changes (12)
- Drivers/EspLcdCompat/Source/EspLcdTouch.cpp
- Boards/M5StackPaperS3/Source/devices/SdCard.cpp
- Drivers/EPDiyDisplay/Source/EpdiyDisplayHelper.h
- Boards/M5StackPaperS3/Source/devices/Display.cpp
- Boards/M5StackPaperS3/CMakeLists.txt
- Firmware/Kconfig
- .gitmodules
- sdkconfig.board.m5stack-papers3
- Drivers/EstimatedPower/Source/ChargeFromAdcVoltage.h
- Libraries/epdiy
- CMakeLists.txt
- Buildscripts/board.cmake
🧰 Additional context used
🧬 Code graph analysis (6)
Boards/M5StackPaperS3/Source/Configuration.cpp (4)
Boards/M5StackPaperS3/Source/devices/Power.cpp (2)
createPower(219-258)createPower(219-219)Boards/M5StackPaperS3/Source/devices/Display.cpp (2)
createDisplay(23-26)createDisplay(23-23)Boards/M5StackPaperS3/Source/devices/SdCard.cpp (2)
createSdCard(9-21)createSdCard(9-9)Boards/M5StackPaperS3/Source/Init.cpp (2)
initBoot(12-20)initBoot(12-12)
Boards/M5StackPaperS3/Source/devices/SdCard.hpp (1)
Tactility/Include/Tactility/hal/sdcard/SdCardDevice.h (1)
SdCardDevice(14-67)
Boards/M5StackPaperS3/Source/devices/Power.cpp (2)
Boards/M5StackPaperS3/Source/devices/Power.hpp (4)
PaperS3Power(31-36)PaperS3Power(37-37)type(43-43)type(44-44)Drivers/EstimatedPower/Source/ChargeFromAdcVoltage.h (1)
ChargeFromAdcVoltage(6-52)
Drivers/EPDiyDisplay/Source/EpdiyDisplay.h (3)
Drivers/EPDiyDisplay/Source/EpdiyDisplay.cpp (34)
EpdiyDisplay(11-18)EpdiyDisplay(20-27)flushCallback(216-224)flushCallback(216-216)flushInternal(318-373)flushInternal(318-318)rotationEventCallback(413-426)rotationEventCallback(413-413)handleRotationChange(428-442)handleRotationChange(428-428)epdRotationToLvgl(375-393)epdRotationToLvgl(375-375)lvglRotationToEpd(395-411)lvglRotationToEpd(395-395)start(29-67)start(29-29)stop(69-94)stop(69-69)setPowerOn(96-110)setPowerOn(96-96)startLvgl(113-186)startLvgl(113-113)stopLvgl(188-214)stopLvgl(188-188)clearScreen(228-244)clearScreen(228-228)clearArea(246-259)clearArea(246-246)updateScreen(261-282)updateScreen(261-261)updateArea(284-305)updateArea(284-284)setAllWhite(307-315)setAllWhite(307-307)Tactility/Include/Tactility/hal/display/DisplayDevice.h (1)
DisplayDevice(15-47)Tactility/Include/Tactility/hal/touch/TouchDevice.h (1)
TouchDevice(12-30)
Drivers/EPDiyDisplay/Source/EpdiyDisplay.cpp (1)
Drivers/EPDiyDisplay/Source/EpdiyDisplay.h (1)
start(84-89)
Boards/M5StackPaperS3/Source/devices/Power.hpp (1)
Boards/M5StackPaperS3/Source/devices/Power.cpp (2)
PaperS3Power(36-51)PaperS3Power(53-56)
🪛 Clang (14.0.6)
Boards/M5StackPaperS3/Source/Init.cpp
[error] 1-1: 'Tactility/kernel/SystemEvents.h' file not found
(clang-diagnostic-error)
Boards/M5StackPaperS3/Source/Configuration.cpp
[error] 1-1: 'Tactility/hal/Configuration.h' file not found
(clang-diagnostic-error)
Boards/M5StackPaperS3/Source/devices/SdCard.hpp
[error] 3-3: 'Tactility/hal/sdcard/SdCardDevice.h' file not found
(clang-diagnostic-error)
Boards/M5StackPaperS3/Source/devices/Display.hpp
[error] 3-3: 'Tactility/hal/display/DisplayDevice.h' file not found
(clang-diagnostic-error)
Drivers/EPDiyDisplay/Source/EpdiyDisplay.h
[error] 3-3: 'Tactility/hal/display/DisplayDevice.h' file not found
(clang-diagnostic-error)
Boards/M5StackPaperS3/Source/devices/Power.hpp
[error] 3-3: 'memory' file not found
(clang-diagnostic-error)
🔇 Additional comments (3)
Boards/M5StackPaperS3/Source/Init.cpp (1)
12-20: Boot init flow looks fine.
Straight‑through happy path with clear error handling; OK as a baseline.If future power work needs delays (PMIC settle, ADC warm‑up), insert non‑blocking waits or yields to avoid WDT on slow peripherals.
Boards/M5StackPaperS3/Source/Configuration.cpp (1)
59-84: Fix designated initializer: missing '=' for .spi (compile error).C++20 designated initializer requires ‘=’. Current form won’t compile.
- .spi { + .spi = { spi::Configuration { .device = SPI2_HOST, .dma = SPI_DMA_CH_AUTO, .config = {⛔ Skipped due to learnings
Learnt from: KenVanHoeylandt PR: ByteWelder/Tactility#394 File: Boards/WaveshareEsp32C6Lcd147/Source/WaveshareEsp32C6Lcd147.cpp:49-50 Timestamp: 2025-10-27T22:33:23.792Z 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.Drivers/EPDiyDisplay/Source/EpdiyDisplay.cpp (1)
216-224: Timing logs are in place; measure actual flush duration under typical UI before refactoring.The timing instrumentation at line 222 is correct and will capture flush duration. The flushInternal function (lines 318+) is synchronous with proper mutex locking—appropriate for LVGL callbacks. Whether async offload is needed depends on actual measurements showing consistent flushes exceeding 10–20ms, which only runtime observation under typical UI can reveal. The codebase already has patterns (FreeRTOS tasks, dispatcher threads) to support async work if measurements justify it.
Run the timing log monitoring during normal operation to establish actual performance characteristics before considering async refactoring.
| std::shared_ptr<PowerDevice> createPower() { | ||
| // Configure ChargeFromAdcVoltage for battery voltage monitoring | ||
| ChargeFromAdcVoltage::Configuration config = { | ||
| .adcMultiplier = VOLTAGE_DIVIDER_MULTIPLIER, | ||
| .adcRefVoltage = 3.3f, // ESP32-S3 reference voltage | ||
| .adcChannel = VBAT_ADC_CHANNEL, | ||
| .adcConfig = { | ||
| .unit_id = ADC_UNIT_1, | ||
| .clk_src = ADC_RTC_CLK_SRC_DEFAULT, | ||
| .ulp_mode = ADC_ULP_MODE_DISABLE, | ||
| }, | ||
| .adcChannelConfig = { | ||
| .atten = ADC_ATTEN_DB_12, | ||
| .bitwidth = ADC_BITWIDTH_DEFAULT, | ||
| }, | ||
| }; | ||
|
|
||
| // Create ChargeFromAdcVoltage with battery voltage range | ||
| auto chargeFromAdcVoltage = std::make_unique<ChargeFromAdcVoltage>( | ||
| config, | ||
| MIN_BATTERY_VOLTAGE, | ||
| MAX_BATTERY_VOLTAGE | ||
| ); | ||
|
|
||
| // Get ADC handle to share with charge status monitoring | ||
| adc_oneshot_unit_handle_t adcHandle = chargeFromAdcVoltage->getAdcHandle(); | ||
|
|
||
| if (adcHandle == nullptr) { | ||
| TT_LOG_E(TAG, "Failed to get ADC handle"); | ||
| return nullptr; | ||
| } | ||
|
|
||
| // Create and return PaperS3Power instance | ||
| return std::make_shared<PaperS3Power>( | ||
| std::move(chargeFromAdcVoltage), | ||
| adcHandle, | ||
| CHARGE_STATUS_ADC_CHANNEL, | ||
| POWER_OFF_PIN | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Good guard on ADC handle; consider logging guidance for callers.
Returning nullptr when ADC init fails is correct; ensure the caller handles a null power device and logs a single clear error to avoid duplicate noise.
If desired, verify all call sites treat nullptr as “no power device” and skip registration.
🏁 Script executed:
#!/bin/bash
rg -nP 'createPower\s*\(' -C2Length of output: 2142
I'll verify the call sites to check if they properly handle a nullptr return from createPower().
🏁 Script executed:
#!/bin/bash
# Check the full implementation of createDevices() to see how it handles createPower() result
rg -nP -A20 'static std::vector<std::shared_ptr<Device>> createDevices' Boards/M5StackPaperS3/Source/Configuration.cppLength of output: 688
🏁 Script executed:
#!/bin/bash
# Also check the Device type and vector usage to understand null handling downstream
rg -nP 'std::vector<std::shared_ptr<Device>>' -C3 Boards/M5StackPaperS3/Source/Configuration.cppLength of output: 169
🏁 Script executed:
#!/bin/bash
# Search for any nullptr checks or validation of devices in the vector
rg -nP 'for.*Device|\.empty\(\)|nullptr' Boards/M5StackPaperS3/Source/Configuration.cpp -C2Length of output: 143
🏁 Script executed:
#!/bin/bash
# Check what happens with the device vector after creation (iteration, usage, etc.)
rg -nP -B5 -A5 'createDevices\(\)' Boards/M5StackPaperS3/Source/Configuration.cpp | head -40Length of output: 227
Now let me check where hardwareConfiguration is used and how the devices vector is consumed:
🏁 Script executed:
#!/bin/bash
# Find where hardwareConfiguration is used and the device vector is accessed
rg -nP 'hardwareConfiguration\.|\.createDevices' -C3Length of output: 18127
🏁 Script executed:
#!/bin/bash
# Search for device vector iteration to see if nullptr is checked
rg -nP 'for.*device|devices.*\||register.*Device' --type=cpp -C2 | head -60Length of output: 3352
Let me check the registerDevice() implementation:
🏁 Script executed:
#!/bin/bash
# Find registerDevice implementation to see if it validates nullptr
rg -nP 'void\s+registerDevice|registerDevice\s*\(' -C5 Tactility/Source/hal/Hal.cppLength of output: 754
Now let me find the registerDevice() function definition:
🏁 Script executed:
#!/bin/bash
# Find the registerDevice() function definition
rg -nP 'void\s+registerDevice\s*\(' -C10 Tactility/Source/hal/Hal.cppLength of output: 0
🏁 Script executed:
#!/bin/bash
# Search across all Hal files for registerDevice implementation
rg -nP 'void\s+registerDevice' --type=cpp -C10Length of output: 4524
Let me verify there's no nullptr guard in registerDevices:
🏁 Script executed:
#!/bin/bash
# Double-check registerDevices to confirm no nullptr filter before registerDevice call
rg -nP -A15 'void registerDevices' Tactility/Source/hal/Hal.cppLength of output: 669
Add nullptr guard in registerDevices() before dereferencing device
The call site in Tactility/Source/hal/Hal.cpp at line 24 passes devices directly to registerDevice() without checking for nullptr. Since createPower() can return nullptr on ADC init failure, this will crash at Tactility/Source/hal/Device.cpp:25 when calling device->getId() on a null pointer. Guard the loop with a nullptr check, similar to the pattern already used for touch devices at line 31.
🤖 Prompt for AI Agents
In Boards/M5StackPaperS3/Source/devices/Power.cpp around lines 219-258,
createPower() can return nullptr on ADC init failure; update
Tactility/Source/hal/Hal.cpp registerDevices() to check each device pointer for
nullptr before calling registerDevice()/dereferencing (same pattern used for
touch devices at line 31) — skip null entries (or log and continue) so
device->getId() is never called on a null pointer.
Just keep in mind that its suggestions aren't always correct! 😅 |
fonix232
force-pushed
the
feat/add_m5stack_papers3_support
branch
from
04e7f2f to
a212682
Compare
Yep, what CR missed, Claude fixed 😅 |
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (4)
Drivers/EPDiyDisplay/Source/EpdiyDisplay.cpp (2)
336-349: Packing bug: bytes must not cross row boundaries (root cause of slanted rows).The current linear packing pairs the last pixel of a row with the first of the next row when width is odd, corrupting rows. This is the root cause of the slanted/shortening artifacts reported in the PR discussion. Pack per row with stride (width+1)/2 and pad the last nibble.
Apply this diff to fix the packing:
- // Convert 8-bit grayscale to 4-bit and pack 2 pixels per byte - size_t pixelCount = width * height; - size_t packedSize = (pixelCount + 1) / 2; // Round up for odd pixel counts - auto* pixelMap4bit = new uint8_t[packedSize]; - - // Pack pixels: convert 8-bit to 4-bit and pack 2 per byte - for (size_t i = 0; i < pixelCount; i += 2) { - // Map 8-bit (0-255) to 4-bit (0-15) by dividing by 17 - uint8_t pixel1 = pixelMap[i] / 17; - uint8_t pixel2 = (i + 1 < pixelCount) ? pixelMap[i + 1] / 17 : 0; - - // Pack two 4-bit pixels into one byte: pixel2 in upper nibble, pixel1 in lower nibble - pixelMap4bit[i / 2] = (pixel2 << 4) | pixel1; - } + // Convert 8-bit grayscale to 4-bit and pack 2 pixels per byte + // Pack per row to avoid crossing row boundaries (EPDiy MODE_PACKING_2PPB requirement) + size_t rowStrideBytes = (width + 1) / 2; + size_t packedSize = rowStrideBytes * height; + auto* pixelMap4bit = new uint8_t[packedSize]; + + for (int row = 0; row < height; ++row) { + const uint8_t* src = pixelMap + row * width; + uint8_t* dst = pixelMap4bit + row * rowStrideBytes; + int col = 0; + for (; col + 1 < width; col += 2) { + uint8_t p1 = src[col] / 17; + uint8_t p2 = src[col + 1] / 17; + dst[col / 2] = (p2 << 4) | p1; + } + if (col < width) { // pad last byte if odd width + uint8_t p1 = src[col] / 17; + dst[col / 2] = (p1 << 4) | 0; + } + }
368-373: Draw mode misuse: don't OR MODE_PACKING_2PPB into epd_hl_update_area().MODE_PACKING_2PPB is a framebuffer packing format flag describing how pixels are stored (4 bits-per-pixel), not a waveform draw mode. The mode parameter for
epd_hl_update_area()expects anEpdDrawModeenum (MODE_GC16, MODE_GL16, etc.). OR-ing a packing flag into the draw mode creates an invalid mode value.Use the configured draw mode as-is (this pattern is already correct at line 305 in
updateArea):epd_hl_update_area( &highlevelState, - static_cast<EpdDrawMode>(configuration->defaultDrawMode | MODE_PACKING_2PPB), + configuration->defaultDrawMode, configuration->defaultTemperature, update_area );Drivers/EPDiyDisplay/CMakeLists.txt (1)
1-6: Add esp_heap_caps to PRIV_REQUIRES.
EpdiyDisplay.cppdirectly includes<esp_heap_caps.h>(line 7) and usesheap_caps_malloc()andheap_caps_free(), but the ESP-IDF componentesp_heap_capsis not declared in CMakeLists.txt. This creates a fragile build that relies on transitive includes.Apply this diff:
idf_component_register( SRC_DIRS "Source" INCLUDE_DIRS "Source" REQUIRES Tactility epdiy esp_lvgl_port - PRIV_REQUIRES esp_timer + PRIV_REQUIRES esp_timer esp_heap_caps )Drivers/EPDiyDisplay/Source/EpdiyDisplay.h (1)
3-11: CI include path for Tactility headers (repeat from earlier reviews)Ensure the ESP‑IDF component exposes
Tactility/Includeviaidf_component_register(INCLUDE_DIRS ...)so this header resolves in CI.Suggested in component CMake:
idf_component_register( SRC_DIRS "Source" INCLUDE_DIRS "Source" "../../Tactility/Include" REQUIRES Tactility epdiy esp_lvgl_port PRIV_REQUIRES esp_timer )
🧹 Nitpick comments (7)
Drivers/EPDiyDisplay/Source/EpdiyDisplay.cpp (1)
217-225: Lower flush timing log level to DEBUG.The flush callback now logs timing at INFO level (line 223). This will spam logs on every LVGL refresh. Use
TT_LOG_Dinstead ofTT_LOG_Ito keep logs clean during normal operation.Apply this diff:
- TT_LOG_I(TAG, "flush took %llu us", (unsigned long long)(esp_timer_get_time() - t0)); + TT_LOG_D(TAG, "flush took %llu us", (unsigned long long)(esp_timer_get_time() - t0));Boards/M5StackPaperS3/Source/devices/SdCard.hpp (1)
6-8: Removeusingdirective from header; fully qualify return type.Exporting
usingdirectives from headers pollutes the global namespace for all includers. Fully qualify the return type instead.Apply this diff:
-using tt::hal::sdcard::SdCardDevice; - -std::shared_ptr<SdCardDevice> createSdCard(); +std::shared_ptr<tt::hal::sdcard::SdCardDevice> createSdCard();Boards/M5StackPaperS3/Source/Configuration.cpp (2)
3-5: Minor: drop unused include
<Tactility/lvgl/LvglSync.h>isn’t used here; remove to speed up builds a bit.-#include <Tactility/lvgl/LvglSync.h>
22-86: Style: avoidexternon a definition
extern const Configuration hardwareConfiguration = { ... };is unconventional for a definition. Prefer removingexternhere and keepexternonly in a header declaration.-extern const Configuration hardwareConfiguration = { +const Configuration hardwareConfiguration = {Boards/M5StackPaperS3/Source/devices/Power.hpp (1)
3-8: Reduce header coupling: forward‑declareChargeFromAdcVoltageYou only store a
unique_ptrand never need the full type here; forward‑declare and include the header in Power.cpp to shorten rebuilds and avoid transitive deps.-#include <memory> -#include <ChargeFromAdcVoltage.h> +#include <memory> +class ChargeFromAdcVoltage; // forward declarationAnd ensure Power.cpp includes the real header (it already does via your include order).
Boards/M5StackPaperS3/Source/devices/Power.cpp (1)
87-94: Bitmask cast to avoid UB if an invalid GPIO sneaks inMake the shift operand unsigned. It’s cheap safety.
- gpio_config_t io_conf = { - .pin_bit_mask = (1ULL << powerOffPin), + gpio_config_t io_conf = { + .pin_bit_mask = (1ULL << static_cast<uint32_t>(powerOffPin)),Drivers/EPDiyDisplay/Source/EpdiyDisplay.h (1)
83-101: Consider async flush to avoid LVGL task WDT (behind a switch)If flush still blocks too long, offload
flushInternalwork to a worker/task and calllv_display_flush_ready()immediately; guard with a config flag so you can A/B test sync vs async.Minimal pattern (pseudo):
static void flushCallback(lv_display_t* display, const lv_area_t* area, uint8_t* pixelMap) { - auto* instance = static_cast<EpdiyDisplay*>(lv_display_get_user_data(display)); - instance->flushInternal(area, pixelMap); - lv_display_flush_ready(display); + auto* self = static_cast<EpdiyDisplay*>(lv_display_get_user_data(display)); + self->enqueue_flush(area, pixelMap); // copy area + buffer or take ownership + lv_display_flush_ready(display); }Please measure and share timing: current code logs
flush took ... us; gather a few samples with typical UI screens.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
.gitmodules(1 hunks)Boards/M5StackPaperS3/CMakeLists.txt(1 hunks)Boards/M5StackPaperS3/Source/Configuration.cpp(1 hunks)Boards/M5StackPaperS3/Source/Init.cpp(1 hunks)Boards/M5StackPaperS3/Source/devices/Display.cpp(1 hunks)Boards/M5StackPaperS3/Source/devices/Display.hpp(1 hunks)Boards/M5StackPaperS3/Source/devices/Power.cpp(1 hunks)Boards/M5StackPaperS3/Source/devices/Power.hpp(1 hunks)Boards/M5StackPaperS3/Source/devices/SdCard.cpp(1 hunks)Boards/M5StackPaperS3/Source/devices/SdCard.hpp(1 hunks)Buildscripts/board.cmake(1 hunks)CMakeLists.txt(1 hunks)Drivers/EPDiyDisplay/CMakeLists.txt(1 hunks)Drivers/EPDiyDisplay/README.md(1 hunks)Drivers/EPDiyDisplay/Source/EpdiyDisplay.cpp(1 hunks)Drivers/EPDiyDisplay/Source/EpdiyDisplay.h(1 hunks)Drivers/EPDiyDisplay/Source/EpdiyDisplayHelper.h(1 hunks)Drivers/EstimatedPower/Source/ChargeFromAdcVoltage.h(1 hunks)Firmware/Kconfig(1 hunks)Libraries/epdiy(1 hunks)sdkconfig.board.m5stack-papers3(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- Libraries/epdiy
- Buildscripts/board.cmake
- Drivers/EstimatedPower/Source/ChargeFromAdcVoltage.h
- Boards/M5StackPaperS3/Source/devices/Display.cpp
- Drivers/EPDiyDisplay/Source/EpdiyDisplayHelper.h
- Drivers/EPDiyDisplay/README.md
- .gitmodules
- Boards/M5StackPaperS3/CMakeLists.txt
- CMakeLists.txt
🧰 Additional context used
🧬 Code graph analysis (6)
Boards/M5StackPaperS3/Source/Configuration.cpp (4)
Boards/M5StackPaperS3/Source/devices/Power.cpp (2)
createPower(222-261)createPower(222-222)Boards/M5StackPaperS3/Source/devices/Display.cpp (2)
createDisplay(23-26)createDisplay(23-23)Boards/M5StackPaperS3/Source/devices/SdCard.cpp (2)
createSdCard(9-21)createSdCard(9-9)Boards/M5StackPaperS3/Source/Init.cpp (2)
initBoot(12-20)initBoot(12-12)
Boards/M5StackPaperS3/Source/devices/Power.cpp (2)
Boards/M5StackPaperS3/Source/devices/Power.hpp (4)
PaperS3Power(31-36)PaperS3Power(37-37)type(43-43)type(44-44)Drivers/EstimatedPower/Source/ChargeFromAdcVoltage.h (1)
ChargeFromAdcVoltage(6-52)
Boards/M5StackPaperS3/Source/devices/Power.hpp (1)
Boards/M5StackPaperS3/Source/devices/Power.cpp (2)
PaperS3Power(38-53)PaperS3Power(55-58)
Drivers/EPDiyDisplay/Source/EpdiyDisplay.h (4)
Drivers/EPDiyDisplay/Source/EpdiyDisplay.cpp (34)
EpdiyDisplay(12-19)EpdiyDisplay(21-28)flushCallback(217-225)flushCallback(217-217)flushInternal(319-374)flushInternal(319-319)rotationEventCallback(414-427)rotationEventCallback(414-414)handleRotationChange(429-443)handleRotationChange(429-429)epdRotationToLvgl(376-394)epdRotationToLvgl(376-376)lvglRotationToEpd(396-412)lvglRotationToEpd(396-396)start(30-68)start(30-30)stop(70-95)stop(70-70)setPowerOn(97-111)setPowerOn(97-97)startLvgl(114-187)startLvgl(114-114)stopLvgl(189-215)stopLvgl(189-189)clearScreen(229-245)clearScreen(229-229)clearArea(247-260)clearArea(247-247)updateScreen(262-283)updateScreen(262-262)updateArea(285-306)updateArea(285-285)setAllWhite(308-316)setAllWhite(308-308)Tactility/Include/Tactility/hal/display/DisplayDevice.h (1)
DisplayDevice(15-47)Tactility/Include/Tactility/hal/touch/TouchDevice.h (1)
TouchDevice(12-30)Tactility/Include/Tactility/hal/display/DisplayDriver.h (1)
DisplayDriver(17-28)
Drivers/EPDiyDisplay/Source/EpdiyDisplay.cpp (1)
Drivers/EPDiyDisplay/Source/EpdiyDisplay.h (1)
start(84-89)
Boards/M5StackPaperS3/Source/devices/SdCard.hpp (1)
Tactility/Include/Tactility/hal/sdcard/SdCardDevice.h (1)
SdCardDevice(14-67)
🪛 Clang (14.0.6)
Boards/M5StackPaperS3/Source/Configuration.cpp
[error] 1-1: 'vector' file not found
(clang-diagnostic-error)
Boards/M5StackPaperS3/Source/devices/Power.hpp
[error] 3-3: 'memory' file not found
(clang-diagnostic-error)
Drivers/EPDiyDisplay/Source/EpdiyDisplay.h
[error] 3-3: 'Tactility/hal/display/DisplayDevice.h' file not found
(clang-diagnostic-error)
Boards/M5StackPaperS3/Source/Init.cpp
[error] 1-1: 'Tactility/kernel/SystemEvents.h' file not found
(clang-diagnostic-error)
Boards/M5StackPaperS3/Source/devices/Display.hpp
[error] 3-3: 'memory' file not found
(clang-diagnostic-error)
Boards/M5StackPaperS3/Source/devices/SdCard.hpp
[error] 3-3: 'memory' file not found
(clang-diagnostic-error)
🔇 Additional comments (6)
Firmware/Kconfig (1)
56-57: LGTM!The board configuration addition is correctly placed and follows the existing pattern.
Boards/M5StackPaperS3/Source/Init.cpp (1)
7-10: Verify that no power-on sequence is required for PaperS3.The
powerOn()stub returnstruewithout performing any initialization. Per the PR objectives, power management features remain untested. Confirm this is correct for the M5Stack PaperS3 hardware, or add a TODO comment if power sequencing will be implemented later.Boards/M5StackPaperS3/Source/devices/Display.hpp (1)
1-11: LGTM!The header is clean with proper includes and clear constant definitions. The resolution macros are acceptable and widely understood.
sdkconfig.board.m5stack-papers3 (1)
1-76: LGTM!The board configuration is comprehensive and includes appropriate settings for the M5Stack PaperS3 (ESP32-S3 with 16MB flash, SPIRAM, LVGL, FATFS, TinyUSB MSC, etc.). The configuration aligns with the hardware capabilities described in the PR objectives.
Boards/M5StackPaperS3/Source/devices/SdCard.cpp (1)
10-16: Based on my verification, the review comment is incorrect. GPIO_NUM_NC is defined as -1 to signal that a GPIO is not connected to software. This is intentional and valid.When all SPI signals are either routed through the IO_MUX or are -1, an exception is made, allowing the driver to use IOMUX directly rather than the GPIO matrix. Your codebase demonstrates this pattern consistently across 25+ board implementations—all use GPIO_NUM_NC for MISO, MOSI, and CLK pins, with only the CS pin specified. This is the correct and documented approach for SD card SPI communication on ESP32.
The M5StackPaperS3 configuration is valid and consistent with all other boards in the codebase.
Drivers/EPDiyDisplay/Source/EpdiyDisplay.h (1)
64-66: Event used for rotation callback may miss changes; verify LVGL v9 eventYou register on
LV_EVENT_RESOLUTION_CHANGEDfor rotation; confirm this is fired onlv_display_set_rotation()in your LVGL version. If not, considerLV_EVENT_REFR_STARTor re‑apply rotation mapping directly afterset_rotation.
fonix232
force-pushed
the
feat/add_m5stack_papers3_support
branch
from
a212682 to
590f3d3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
Drivers/EPDiyDisplay/CMakeLists.txt (1)
1-6: Add esp_heap_caps to PRIV_REQUIRES.As flagged in the previous review, EpdiyDisplay.cpp uses
<esp_heap_caps.h>but this ESP-IDF component is not declared in PRIV_REQUIRES. Add the missing dependency to avoid relying on transitive includes.Apply this diff:
idf_component_register( SRC_DIRS "Source" INCLUDE_DIRS "Source" REQUIRES Tactility epdiy esp_lvgl_port - PRIV_REQUIRES esp_timer + PRIV_REQUIRES esp_timer heap )Boards/M5StackPaperS3/Source/devices/Power.cpp (1)
128-133: Use the instance's ADC channel for calibration.The calibration config hardcodes
CHARGE_STATUS_ADC_CHANNELat line 130; use the instance memberchargeStatusAdcChannelso calibration remains correct if the instance is constructed with a different channel.Based on past review comments.
Drivers/EPDiyDisplay/Source/EpdiyDisplay.cpp (1)
368-373: Remove MODE_PACKING_2PPB from draw mode parameter.Line 370 ORs
MODE_PACKING_2PPB(a framebuffer format flag) into theEpdDrawModeparameter forepd_hl_update_area(). This creates an invalid draw mode value. Use the configured draw mode directly, as done correctly in theupdateArea()method at line 305.Apply this diff:
epd_hl_update_area( &highlevelState, - static_cast<EpdDrawMode>(configuration->defaultDrawMode | MODE_PACKING_2PPB), + configuration->defaultDrawMode, configuration->defaultTemperature, update_area );Based on past review comments.
🧹 Nitpick comments (2)
Boards/M5StackPaperS3/Source/devices/Display.hpp (1)
8-9: Consider constexpr over macros for type safety.While the macros work correctly, using
constexprprovides type safety and better scoping. This is an optional improvement that can be deferred.Apply this diff if you choose to refactor:
-#define PAPERS3_EPD_HORIZONTAL_RESOLUTION 540 -#define PAPERS3_EPD_VERTICAL_RESOLUTION 960 +inline constexpr int PAPERS3_EPD_HORIZONTAL_RESOLUTION = 540; +inline constexpr int PAPERS3_EPD_VERTICAL_RESOLUTION = 960;Boards/M5StackPaperS3/Source/devices/SdCard.hpp (1)
6-6: Consider removing using directive from header.The
usingdirective (line 6) exportsSdCardDeviceinto the global namespace for all files that include this header. While not critical, prefer forward declaration or fully qualified names to avoid namespace pollution.Apply this diff to use forward declaration:
-#include "Tactility/hal/sdcard/SdCardDevice.h" - -using tt::hal::sdcard::SdCardDevice; +// Forward declaration reduces namespace pollution +namespace tt::hal::sdcard { class SdCardDevice; } -std::shared_ptr<SdCardDevice> createSdCard(); +std::shared_ptr<tt::hal::sdcard::SdCardDevice> createSdCard();Then include the full header in SdCard.cpp where the implementation needs it.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
.gitmodules(1 hunks)Boards/M5StackPaperS3/CMakeLists.txt(1 hunks)Boards/M5StackPaperS3/Source/Configuration.cpp(1 hunks)Boards/M5StackPaperS3/Source/Init.cpp(1 hunks)Boards/M5StackPaperS3/Source/devices/Display.cpp(1 hunks)Boards/M5StackPaperS3/Source/devices/Display.hpp(1 hunks)Boards/M5StackPaperS3/Source/devices/Power.cpp(1 hunks)Boards/M5StackPaperS3/Source/devices/Power.hpp(1 hunks)Boards/M5StackPaperS3/Source/devices/SdCard.cpp(1 hunks)Boards/M5StackPaperS3/Source/devices/SdCard.hpp(1 hunks)Buildscripts/board.cmake(1 hunks)CMakeLists.txt(1 hunks)Drivers/EPDiyDisplay/CMakeLists.txt(1 hunks)Drivers/EPDiyDisplay/README.md(1 hunks)Drivers/EPDiyDisplay/Source/EpdiyDisplay.cpp(1 hunks)Drivers/EPDiyDisplay/Source/EpdiyDisplay.h(1 hunks)Drivers/EPDiyDisplay/Source/EpdiyDisplayHelper.h(1 hunks)Drivers/EstimatedPower/Source/ChargeFromAdcVoltage.h(1 hunks)Firmware/Kconfig(1 hunks)Libraries/epdiy(1 hunks)sdkconfig.board.m5stack-papers3(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- Drivers/EPDiyDisplay/README.md
- Libraries/epdiy
🚧 Files skipped from review as they are similar to previous changes (8)
- CMakeLists.txt
- Boards/M5StackPaperS3/CMakeLists.txt
- .gitmodules
- Firmware/Kconfig
- Drivers/EstimatedPower/Source/ChargeFromAdcVoltage.h
- sdkconfig.board.m5stack-papers3
- Boards/M5StackPaperS3/Source/devices/Display.cpp
- Buildscripts/board.cmake
🧰 Additional context used
🧬 Code graph analysis (7)
Drivers/EPDiyDisplay/Source/EpdiyDisplayHelper.h (3)
Drivers/EPDiyDisplay/Source/EpdiyDisplay.cpp (2)
EpdiyDisplay(12-19)EpdiyDisplay(21-28)Tactility/Include/Tactility/hal/touch/TouchDevice.h (1)
TouchDevice(12-30)Drivers/RgbDisplay/Source/RgbDisplay.h (1)
Configuration(22-58)
Boards/M5StackPaperS3/Source/devices/SdCard.hpp (1)
Tactility/Include/Tactility/hal/sdcard/SdCardDevice.h (1)
SdCardDevice(14-67)
Boards/M5StackPaperS3/Source/devices/Power.hpp (1)
Boards/M5StackPaperS3/Source/devices/Power.cpp (2)
PaperS3Power(38-53)PaperS3Power(55-58)
Drivers/EPDiyDisplay/Source/EpdiyDisplay.h (3)
Drivers/EPDiyDisplay/Source/EpdiyDisplay.cpp (34)
EpdiyDisplay(12-19)EpdiyDisplay(21-28)flushCallback(217-225)flushCallback(217-217)flushInternal(319-374)flushInternal(319-319)rotationEventCallback(414-427)rotationEventCallback(414-414)handleRotationChange(429-443)handleRotationChange(429-429)epdRotationToLvgl(376-394)epdRotationToLvgl(376-376)lvglRotationToEpd(396-412)lvglRotationToEpd(396-396)start(30-68)start(30-30)stop(70-95)stop(70-70)setPowerOn(97-111)setPowerOn(97-97)startLvgl(114-187)startLvgl(114-114)stopLvgl(189-215)stopLvgl(189-189)clearScreen(229-245)clearScreen(229-229)clearArea(247-260)clearArea(247-247)updateScreen(262-283)updateScreen(262-262)updateArea(285-306)updateArea(285-285)setAllWhite(308-316)setAllWhite(308-308)Tactility/Include/Tactility/hal/display/DisplayDevice.h (1)
DisplayDevice(15-47)Tactility/Include/Tactility/hal/touch/TouchDevice.h (1)
TouchDevice(12-30)
Boards/M5StackPaperS3/Source/Configuration.cpp (4)
Boards/M5StackPaperS3/Source/devices/Power.cpp (2)
createPower(222-261)createPower(222-222)Boards/M5StackPaperS3/Source/devices/Display.cpp (2)
createDisplay(23-26)createDisplay(23-23)Boards/M5StackPaperS3/Source/devices/SdCard.cpp (2)
createSdCard(9-21)createSdCard(9-9)Boards/M5StackPaperS3/Source/Init.cpp (2)
initBoot(12-20)initBoot(12-12)
Boards/M5StackPaperS3/Source/devices/Power.cpp (2)
Boards/M5StackPaperS3/Source/devices/Power.hpp (4)
PaperS3Power(31-36)PaperS3Power(37-37)type(43-43)type(44-44)Drivers/EstimatedPower/Source/ChargeFromAdcVoltage.h (1)
ChargeFromAdcVoltage(6-52)
Drivers/EPDiyDisplay/Source/EpdiyDisplay.cpp (1)
Drivers/EPDiyDisplay/Source/EpdiyDisplay.h (1)
start(84-89)
🪛 Clang (14.0.6)
Boards/M5StackPaperS3/Source/devices/SdCard.hpp
[error] 3-3: 'memory' file not found
(clang-diagnostic-error)
Boards/M5StackPaperS3/Source/devices/Power.hpp
[error] 3-3: 'memory' file not found
(clang-diagnostic-error)
Drivers/EPDiyDisplay/Source/EpdiyDisplay.h
[error] 3-3: 'Tactility/hal/display/DisplayDevice.h' file not found
(clang-diagnostic-error)
Boards/M5StackPaperS3/Source/Configuration.cpp
[error] 1-1: 'vector' file not found
(clang-diagnostic-error)
Boards/M5StackPaperS3/Source/Init.cpp
[error] 1-1: 'Tactility/kernel/SystemEvents.h' file not found
(clang-diagnostic-error)
Boards/M5StackPaperS3/Source/devices/Display.hpp
[error] 3-3: 'memory' file not found
(clang-diagnostic-error)
🔇 Additional comments (8)
Boards/M5StackPaperS3/Source/Init.cpp (1)
7-20: LGTM!The initialization logic is straightforward and appropriate for the M5Stack PaperS3, which requires no special power-on sequence. The stub
powerOn()function and error handling ininitBoot()provide a clean hook for future extensions if needed.Boards/M5StackPaperS3/Source/Configuration.cpp (1)
1-86: LGTM!The hardware configuration is well-structured and all previously flagged issues have been addressed. The I2C and SPI configurations are detailed and appropriate for the M5Stack PaperS3 board.
Boards/M5StackPaperS3/Source/devices/Power.hpp (1)
1-58: LGTM!The power management interface is well-designed with clear documentation of the hardware configuration. The class properly encapsulates battery voltage monitoring, charge detection, and power-off functionality for the M5Stack PaperS3 board.
Boards/M5StackPaperS3/Source/devices/SdCard.cpp (1)
9-21: Verification complete—code is correct, no changes needed.The SPI bus configuration is properly shared and correctly implemented:
- SPI2_HOST is pre-configured in Configuration.cpp with MOSI/MISO/SCK pins (GPIO_NUM_38/40/39)
SpiSdCardDevice::Configdefaults tospiHost = SPI2_HOSTwhen not explicitly providedSpiSdCardDevicecorrectly usesslot_config.host_id = config->spiHostto attach to the pre-configured bus- CS pin GPIO_NUM_47 is properly used via
slot_config.gpio_cs- The
GPIO_NUM_NCvalues passed are for optional card detect/write-protect/interrupt pins, not MISO/MOSI/SCKThe implementation follows the correct ESP-IDF pattern: the bus is initialized once with physical pins, then devices attach to it specifying only their CS pin.
Drivers/EPDiyDisplay/Source/EpdiyDisplay.cpp (4)
1-28: LGTM!Includes are complete (esp_timer.h added), and lifecycle management is correct. The destructor properly cleans up LVGL before stopping the display.
114-215: LGTM!LVGL initialization and cleanup are correct. The draw buffer is now properly stored as a member variable and explicitly freed in
stopLvgl()before deleting the LVGL display, resolving the memory leak flagged in past reviews.
336-351: LGTM!The pixel packing now correctly operates row-by-row with proper stride calculation, avoiding the cross-row nibble merge that caused slanted row artifacts. Odd widths are properly padded.
376-443: LGTM!Rotation mapping between EPDiy and LVGL coordinate systems is well-designed with static lookup tables, bounds validation, and proper fallbacks. The event callback safely synchronizes rotation changes.
fonix232
force-pushed
the
feat/add_m5stack_papers3_support
branch
from
590f3d3 to
e8907be
Compare
fonix232
force-pushed
the
feat/add_m5stack_papers3_support
branch
from
f18d5d9 to
29e61c5
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (5)
CMakeLists.txt (1)
42-42: Verify epdiy submodule is properly initialized with component metadata.As noted in the previous review, the
Libraries/epdiydirectory must contain ESP-IDF component metadata (component.cmakeoridf_component.yml) for the build to succeed. TheEPDiyDisplaydriver declaresREQUIRES epdiy, so ESP-IDF will fail during component resolution if the submodule is not properly populated.Verify the submodule is initialized and contains the required build files:
#!/bin/bash # Verify epdiy submodule has required ESP-IDF component files echo "=== Checking epdiy submodule status ===" git submodule status Libraries/epdiy echo "" echo "=== Checking for ESP-IDF component metadata ===" ls -la Libraries/epdiy/ | head -20 echo "" echo "=== Looking for component configuration files ===" find Libraries/epdiy -maxdepth 2 -type f \( -name "CMakeLists.txt" -o -name "component.cmake" -o -name "idf_component.yml" \)Boards/m5stack-papers3/Source/devices/Power.cpp (1)
128-133: Use instance ADC channel for calibration (not the constant).The calibration config hard‑codes CHARGE_STATUS_ADC_CHANNEL. Use the instance field to keep calibration correct for any constructed channel.
- adc_cali_curve_fitting_config_t cali_config = { - .unit_id = ADC_UNIT_1, - .chan = CHARGE_STATUS_ADC_CHANNEL, - .atten = ADC_ATTEN_DB_12, - .bitwidth = ADC_BITWIDTH_DEFAULT, - }; + adc_cali_curve_fitting_config_t cali_config = { + .unit_id = ADC_UNIT_1, + .chan = chargeStatusAdcChannel, + .atten = ADC_ATTEN_DB_12, + .bitwidth = ADC_BITWIDTH_DEFAULT, + };Drivers/EPDiyDisplay/Source/EpdiyDisplay.cpp (2)
217-225: Synchronous flush still risks WDT; consider async queue or at least throttle logs.flushCallback runs conversion+EPD update inline before lv_display_flush_ready(), blocking the LVGL task. Prior WDTs point here. Either:
- Offload: copy the area to a small PSRAM buffer and enqueue to a worker, then call lv_display_flush_ready() immediately; or
- Keep sync but constrain: narrow the lock (see separate comment), and throttle logging.
Minimal change for log noise:
- TT_LOG_I(TAG, "flush took %llu us", (unsigned long long)(esp_timer_get_time() - t0)); + TT_LOG_D(TAG, "flush took %llu us", (unsigned long long)(esp_timer_get_time() - t0));If you want, I can sketch an async worker + queue (copy-of-area) behind a config flag.
368-373: epd_hl_update_area(): don’t OR packing flags into draw mode (invalid enum value).MODE_PACKING_2PPB is a framebuffer packing flag, not a waveform update mode. Passing it via bitwise-OR yields an undefined mode.
Apply this fix:
- epd_hl_update_area( - &highlevelState, - static_cast<EpdDrawMode>(configuration->defaultDrawMode | MODE_PACKING_2PPB), - configuration->defaultTemperature, - update_area - ); + epd_hl_update_area( + &highlevelState, + configuration->defaultDrawMode, + configuration->defaultTemperature, + update_area + );Drivers/EPDiyDisplay/Source/EpdiyDisplay.h (1)
56-57: Add nullability annotations for consistency.These pointers can be null; align with lvglDisplay’s _Nullable annotation.
- uint8_t* framebuffer = nullptr; - uint8_t* lvglDrawBuffer = nullptr; + uint8_t* _Nullable framebuffer = nullptr; + uint8_t* _Nullable lvglDrawBuffer = nullptr;
🧹 Nitpick comments (9)
sdkconfig.board.m5stack-papers3 (1)
66-67: Add more context to the ESP-IDF bug comment.The comment mentions reverting to 80MHz "after ESP-IDF bug is resolved" but doesn't provide enough context for future maintainers to know when it's safe to change this setting.
Consider updating the comment with more specific information:
-# SPI Flash (can set back to 80MHz after ESP-IDF bug is resolved) +# SPI Flash: Set to 120MHz (workaround for ESP-IDF issue with 80MHz on ESP32-S3) +# TODO: Test reverting to 80MHz after ESP-IDF 5.6+ (see issue #XXX or espressif/esp-idf#XXXXX) CONFIG_ESPTOOLPY_FLASHFREQ_120M=yIf there's a specific ESP-IDF issue number, include it for reference.
Boards/m5stack-papers3/Source/Init.cpp (1)
7-20: Consider simplifying the redundant power-on logic.The
powerOn()function always returnstrue, making the error handling ininitBoot()unreachable dead code. This structure may be intentional scaffolding for future power management implementation or to match patterns from other boards.If no power-on sequence is planned, simplify to:
-static bool powerOn() { - // No power on sequence needed for M5Stack PaperS3 - return true; -} - bool initBoot() { ESP_LOGI(TAG, LOG_MESSAGE_POWER_ON_START); - if (!powerOn()) { - TT_LOG_E(TAG, LOG_MESSAGE_POWER_ON_FAILED); - return false; - } - + // No power on sequence needed for M5Stack PaperS3 return true; }Alternatively, if this structure is intentional to allow future power-on logic, add a comment explaining the scaffolding:
static bool powerOn() { - // No power on sequence needed for M5Stack PaperS3 + // Placeholder: No power on sequence currently needed for M5Stack PaperS3 + // Keep this structure for future power management implementation return true; }Boards/m5stack-papers3/Source/devices/Display.cpp (1)
3-5: Include GPIO header explicitly.#include <Gt911Touch.h> #include <EpdiyDisplayHelper.h> +#include <driver/gpio.h>Boards/m5stack-papers3/Source/Configuration.cpp (1)
1-5: Add explicit driver headers for I2C/SPI/GPIO types.This TU uses i2c_config_t, SPI2_HOST/SPI_DMA_CH_AUTO, and GPIO_NUM_* but does not include their defining headers here. Add them to avoid transitive‑include coupling.
#include <vector> #include <memory> #include <Tactility/hal/Configuration.h> #include <Tactility/lvgl/LvglSync.h> +#include <driver/i2c.h> +#include <driver/spi_master.h> +#include <driver/gpio.h>Boards/m5stack-papers3/Source/devices/Power.hpp (1)
3-6: Include for std::string.#include <memory> +#include <string> #include <ChargeFromAdcVoltage.h> #include <Tactility/hal/power/PowerDevice.h>Drivers/EPDiyDisplay/Source/EpdiyDisplay.cpp (3)
325-327: Reduce critical section: do conversion outside the mutex.Packing can be expensive; keep the mutex only for EPDiy calls to lower WDT risk and contention with other tasks.
- // Lock for thread safety - std::lock_guard<tt::Mutex> guard(*lock); + // Do conversion outside the mutex; guard only around EPDiy calls below @@ - epd_draw_rotated_image( - update_area, - pixelMap4bit.data(), - framebuffer - ); - - epd_hl_update_area( - &highlevelState, - static_cast<EpdDrawMode>(configuration->defaultDrawMode | MODE_PACKING_2PPB), - configuration->defaultTemperature, - update_area - ); + { + // Guard EPDiy operations + std::lock_guard<tt::Mutex> guard(*lock); + epd_draw_rotated_image(update_area, pixelMap4bit.data(), framebuffer); + epd_hl_update_area(&highlevelState, configuration->defaultDrawMode, configuration->defaultTemperature, update_area); + }Also applies to: 362-373
202-211: Comment incorrect: LVGL won’t free user buffers; you already free it above.You correctly free lvglDrawBuffer. The comment saying “this will automatically free the buffers” is misleading; lv_display_delete does not free user-provided buffers.
- // Delete display (this will automatically free the buffers) + // Delete display (user-provided buffers are already freed above) lv_display_delete(lvglDisplay);
336-351: Allocation per flush: reuse a scratch buffer to reduce PSRAM churn.std::vector per flush can fragment PSRAM and add latency. Keep a reusable member buffer sized to the max area seen or to the full width-row stride, and reuse it.
Drivers/EPDiyDisplay/Source/EpdiyDisplay.h (1)
129-136: Document return codes for callers.Briefly note common EpdDrawError outcomes (e.g., SUCCESS, FAILED_ALLOC) to guide callers’ handling.
Also applies to: 144-148
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
.gitmodules(1 hunks)Boards/m5stack-papers3/CMakeLists.txt(1 hunks)Boards/m5stack-papers3/Source/Configuration.cpp(1 hunks)Boards/m5stack-papers3/Source/Init.cpp(1 hunks)Boards/m5stack-papers3/Source/devices/Display.cpp(1 hunks)Boards/m5stack-papers3/Source/devices/Display.hpp(1 hunks)Boards/m5stack-papers3/Source/devices/Power.cpp(1 hunks)Boards/m5stack-papers3/Source/devices/Power.hpp(1 hunks)Boards/m5stack-papers3/Source/devices/SdCard.cpp(1 hunks)Boards/m5stack-papers3/Source/devices/SdCard.hpp(1 hunks)CMakeLists.txt(1 hunks)Drivers/EPDiyDisplay/CMakeLists.txt(1 hunks)Drivers/EPDiyDisplay/README.md(1 hunks)Drivers/EPDiyDisplay/Source/EpdiyDisplay.cpp(1 hunks)Drivers/EPDiyDisplay/Source/EpdiyDisplay.h(1 hunks)Drivers/EPDiyDisplay/Source/EpdiyDisplayHelper.h(1 hunks)Drivers/EstimatedPower/Source/ChargeFromAdcVoltage.h(1 hunks)Firmware/Kconfig(1 hunks)Libraries/epdiy(1 hunks)sdkconfig.board.m5stack-papers3(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- Drivers/EPDiyDisplay/README.md
- Firmware/Kconfig
- .gitmodules
- Drivers/EPDiyDisplay/CMakeLists.txt
- Libraries/epdiy
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-28T05:42:03.372Z
Learnt from: NellowTCS
PR: ByteWelder/Tactility#398
File: Boards/LilygoTdisplayS3/Source/devices/Display.cpp:183-199
Timestamp: 2025-10-28T05:42:03.372Z
Learning: The LilyGo T-Display S3 board (Boards/LilygoTdisplayS3) requires a non-standard LVGL flush pattern in its ST7789 i8080 display driver. In st7789_send_color_cb, lv_display_flush_ready() must be called immediately after esp_lcd_panel_io_tx_color() with a vTaskDelay(1), rather than using standard DMA completion callbacks via esp_lcd_panel_io_register_event_callbacks. Standard DMA callback approaches cause display corruption on this specific board. This pattern was tested against multiple alternatives (callback registration, buffer tuning, clock reduction) and is the only stable solution for T-Display S3 hardware.
Applied to files:
Drivers/EPDiyDisplay/Source/EpdiyDisplay.cpp
🧬 Code graph analysis (7)
Drivers/EPDiyDisplay/Source/EpdiyDisplayHelper.h (2)
Drivers/EPDiyDisplay/Source/EpdiyDisplay.cpp (2)
EpdiyDisplay(12-19)EpdiyDisplay(21-28)Tactility/Include/Tactility/hal/touch/TouchDevice.h (1)
TouchDevice(12-30)
Boards/m5stack-papers3/Source/Configuration.cpp (4)
Boards/m5stack-papers3/Source/devices/Power.cpp (2)
createPower(222-261)createPower(222-222)Boards/m5stack-papers3/Source/devices/Display.cpp (2)
createDisplay(23-26)createDisplay(23-23)Boards/m5stack-papers3/Source/devices/SdCard.cpp (2)
createSdCard(9-21)createSdCard(9-9)Boards/m5stack-papers3/Source/Init.cpp (2)
initBoot(12-20)initBoot(12-12)
Boards/m5stack-papers3/Source/devices/Power.cpp (2)
Boards/m5stack-papers3/Source/devices/Power.hpp (4)
PaperS3Power(31-36)PaperS3Power(37-37)type(43-43)type(44-44)Drivers/EstimatedPower/Source/ChargeFromAdcVoltage.h (1)
ChargeFromAdcVoltage(6-52)
Drivers/EPDiyDisplay/Source/EpdiyDisplay.cpp (1)
Drivers/EPDiyDisplay/Source/EpdiyDisplay.h (1)
start(84-89)
Boards/m5stack-papers3/Source/devices/Power.hpp (1)
Boards/m5stack-papers3/Source/devices/Power.cpp (2)
PaperS3Power(38-53)PaperS3Power(55-58)
Drivers/EPDiyDisplay/Source/EpdiyDisplay.h (3)
Drivers/EPDiyDisplay/Source/EpdiyDisplay.cpp (34)
EpdiyDisplay(12-19)EpdiyDisplay(21-28)flushCallback(217-225)flushCallback(217-217)flushInternal(319-374)flushInternal(319-319)rotationEventCallback(414-427)rotationEventCallback(414-414)handleRotationChange(429-443)handleRotationChange(429-429)epdRotationToLvgl(376-394)epdRotationToLvgl(376-376)lvglRotationToEpd(396-412)lvglRotationToEpd(396-396)start(30-68)start(30-30)stop(70-95)stop(70-70)setPowerOn(97-111)setPowerOn(97-97)startLvgl(114-187)startLvgl(114-114)stopLvgl(189-215)stopLvgl(189-189)clearScreen(229-245)clearScreen(229-229)clearArea(247-260)clearArea(247-247)updateScreen(262-283)updateScreen(262-262)updateArea(285-306)updateArea(285-285)setAllWhite(308-316)setAllWhite(308-308)Tactility/Include/Tactility/hal/display/DisplayDevice.h (1)
DisplayDevice(15-47)Tactility/Include/Tactility/hal/touch/TouchDevice.h (1)
TouchDevice(12-30)
Boards/m5stack-papers3/Source/devices/SdCard.hpp (1)
Tactility/Include/Tactility/hal/sdcard/SdCardDevice.h (1)
SdCardDevice(14-67)
🪛 Clang (14.0.6)
Boards/m5stack-papers3/Source/Init.cpp
[error] 1-1: 'Tactility/kernel/SystemEvents.h' file not found
(clang-diagnostic-error)
Boards/m5stack-papers3/Source/devices/Display.hpp
[error] 3-3: 'memory' file not found
(clang-diagnostic-error)
Boards/m5stack-papers3/Source/Configuration.cpp
[error] 1-1: 'vector' file not found
(clang-diagnostic-error)
Boards/m5stack-papers3/Source/devices/Power.hpp
[error] 3-3: 'memory' file not found
(clang-diagnostic-error)
Drivers/EPDiyDisplay/Source/EpdiyDisplay.h
[error] 3-3: 'Tactility/hal/display/DisplayDevice.h' file not found
(clang-diagnostic-error)
Boards/m5stack-papers3/Source/devices/SdCard.hpp
[error] 3-3: 'memory' file not found
(clang-diagnostic-error)
🔇 Additional comments (9)
Boards/m5stack-papers3/CMakeLists.txt (1)
1-7: LGTM! Standard ESP-IDF component registration.The component registration follows ESP-IDF conventions correctly. The use of
GLOB_RECURSEis appropriate for board-specific sources, and the declared dependencies align with the M5Stack PaperS3 hardware requirements (display, touch, ADC, SD card).sdkconfig.board.m5stack-papers3 (1)
1-76: Configuration looks comprehensive for M5Stack PaperS3.The SDK configuration appropriately covers:
- Increased stack sizes for WiFi stability
- LVGL setup with appropriate fonts and features
- FreeRTOS tuning for the EPD display workload
- SPI RAM/Flash configuration matching hardware capabilities
- Proper partition table and board identifiers
Drivers/EPDiyDisplay/Source/EpdiyDisplayHelper.h (1)
21-41: LGTM! Factory pattern and memory management are correct.The factory method properly encapsulates M5Paper S3 display configuration:
- Appropriate default parameters (20°C, MODE_GL16 for grayscale, landscape orientation)
- Correct use of
std::make_uniquefor configuration followed bystd::moveinto the display constructor- Proper initialization options combining EPD_LUT_1K and EPD_FEED_QUEUE_32
Boards/m5stack-papers3/Source/devices/SdCard.hpp (1)
1-8: LGTM! Standard factory header pattern.The header correctly declares the SD card factory function and includes necessary dependencies. The static analysis error about
<memory>not being found is a false positive from the analyzer's environment—<memory>is a standard C++ header.Boards/m5stack-papers3/Source/devices/Display.hpp (1)
1-11: LGTM! Display factory header is well-structured.The header properly declares the display factory function and defines resolution constants (540×960) that match the M5Stack PaperS3 hardware specifications. The static analysis error about
<memory>is a false positive.Boards/m5stack-papers3/Source/devices/SdCard.cpp (1)
3-6: MISO/MOSI/SCLK parameters do not exist in SpiSdCardDevice::Config.The review's core concern is factually incorrect. The
SpiSdCardDevice::Configconstructor accepts only five parameters:spiPinCs,spiPinCd(Card Detect),spiPinWp(Write Protect),spiPinInt(Interrupt), andMountBehaviour. The code correctly passesGPIO_NUM_NCfor the optional CD/WP/Int pins, and there are no MISO/MOSI/SCLK pin parameters to worry about.While the suggestion to add explicit
#include <driver/gpio.h>and#include <memory>is a reasonable hygiene practice to avoid relying on transitive includes, it is not required here since the necessary symbols are already available through the included headers (SdCard.hppincludes<memory>, andSpiSdCardDevice.hprovides GPIO definitions).Likely an incorrect or invalid review comment.
Drivers/EPDiyDisplay/Source/EpdiyDisplay.h (1)
3-5: Review comment is incorrect—no fix required.The Tactility component already exports its headers via
INCLUDE_DIRS "Include/"in itsidf_component_registercall (Tactility/CMakeLists.txt:33). In ESP-IDF, components listed in theREQUIRESlist automatically provide their include directories to dependent components. Since Drivers/EPDiyDisplay/CMakeLists.txt already includes Tactility inREQUIRES, the Tactility headers will be found without manually adding the path toINCLUDE_DIRS.Likely an incorrect or invalid review comment.
Drivers/EPDiyDisplay/Source/EpdiyDisplay.cpp (2)
172-175: No issues found — event type is correct.The code correctly uses
LV_EVENT_RESOLUTION_CHANGED, which LVGL v9 sends whenlv_display_set_rotation()is called.LV_EVENT_REFR_START/LV_EVENT_REFR_READYare refresh-cycle events, not rotation notifications. The subscription is appropriate as written.
84-91: Code cleanup is correct; no changes needed.The
epd_hl_init()function allocates framebuffers in PSRAM, and the proper cleanup is to callepd_deinit(), which releases driver resources. The code correctly callsepd_deinit()at line 84, and there is no separateepd_hl_deinit()function available in the EPDiy API. The PSRAM buffers will be freed when the system shuts down. The explanatory comment in the code accurately reflects this design.
| auto configuration = std::make_unique<Gt911Touch::Configuration>( | ||
| I2C_NUM_0, | ||
| PAPERS3_EPD_HORIZONTAL_RESOLUTION,// yMax - PaperS3 is rotated | ||
| PAPERS3_EPD_VERTICAL_RESOLUTION, // xMax - PaperS3 is rotated | ||
| true, // swapXY | ||
| true, // mirrorX | ||
| false, // mirrorY | ||
| GPIO_NUM_NC, | ||
| GPIO_NUM_48 | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
Verify GT911 configuration parameter order vs. swap/mirror.
You set swapXY=true and also pass PAPERS3_EPD_HORIZONTAL_RESOLUTION as the “yMax” argument. If the constructor order is (xMax, yMax, …), this combination could double‑swap or mis‑scale. Please confirm the signature and adjust either the max arguments or swapXY accordingly.
Tip: prefer initializing via a named Configuration aggregate (if supported) to avoid positional mistakes.
🏁 Script executed:
#!/bin/bash
# Show the Gt911Touch::Configuration fields / ctor signature used here.
fd -a 'Gt911Touch' | xargs -I {} rg -nP -C3 'class\s+Gt911Touch|struct\s+Configuration|Configuration\s*\(' {}Length of output: 265
🏁 Script executed:
#!/bin/bash
# Extract the complete Gt911Touch::Configuration constructor definition
rg -nP -A 15 'class\s+Configuration\s*\{' | head -30Length of output: 2473
🏁 Script executed:
#!/bin/bash
# Find Gt911Touch header file and show complete constructor
fd -i 'gt911|gt.*911' --type f | head -20Length of output: 69
🏁 Script executed:
#!/bin/bash
# Extract complete Gt911Touch::Configuration constructor from header
rg -nP -A 20 'class\s+Configuration' Drivers/GT911/Source/Gt911Touch.hLength of output: 689
🏁 Script executed:
#!/bin/bash
# Find PAPERS3_EPD resolution definitions
rg -nP 'PAPERS3_EPD_HORIZONTAL_RESOLUTION|PAPERS3_EPD_VERTICAL_RESOLUTION' --type-add='h:*.h' --type=hLength of output: 193
Fix inconsistent GT911 configuration: conflicting parameter order and swapXy flag.
The comments state dimensions are swapped for rotation, yet you pass HORIZONTAL_RESOLUTION as xMax and VERTICAL_RESOLUTION as yMax (unrotated order) while also setting swapXy=true. This creates conflicting swap logic.
Either swap the parameter positions (540→yMax, 960→xMax) and set swapXy=false, or keep the current parameter order and set swapXy=false. Clarify the intended display orientation and correct accordingly.
🤖 Prompt for AI Agents
In Boards/m5stack-papers3/Source/devices/Display.cpp around lines 9-18, the
GT911 constructor is passed display dimensions and swapXY in a conflicting way
(comments imply rotated dimensions but swapXY is true). Decide the intended
orientation and fix the params: if the display is meant to be rotated, swap the
resolution arguments (use VERTICAL for yMax and HORIZONTAL for xMax) and set
swapXY=false; if not rotated, keep the current argument order and set
swapXY=false so the axis mapping is consistent.
There was a problem hiding this comment.
Ignore this.
The suggested edits result in badly resolved touch positions. This is the only correct configuration that actually works.
There was a problem hiding this comment.
@fonix232, understood! I'll dismiss this review comment. Empirical testing always trumps theoretical parameter analysis – if this configuration produces correct touch positions and the suggested changes don't, then this is the right setup for the M5Stack PaperS3 hardware.
✏️ Learnings added
Learnt from: fonix232
PR: ByteWelder/Tactility#380
File: Boards/m5stack-papers3/Source/devices/Display.cpp:9-18
Timestamp: 2025-10-29T14:45:02.882Z
Learning: For M5Stack PaperS3 in Boards/m5stack-papers3/Source/devices/Display.cpp, the GT911 touch configuration must use PAPERS3_EPD_HORIZONTAL_RESOLUTION (960) as yMax, PAPERS3_EPD_VERTICAL_RESOLUTION (540) as xMax, with swapXY=true and mirrorX=true. Alternative configurations (swapping the resolution parameters or setting swapXY=false) result in badly resolved touch positions. This specific configuration is empirically verified to work correctly.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
The suggested edits result in badly resolved touch positions. This is the only correct configuration that actually works.
I'm pretty confident this is solveable and that coderabbitai is correct in this scenario. The Gt911Touch::Configuration has properties for swapXY, mirrorX and mirrorY.
So setting swapXY to false and mirrorX to false for the display should work when you also adjust the touch driver configuration.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
@KenVanHoeylandt I have tried every single possible configuration, alas to no avail. Setting them up otherwise, in configurations that should work in theory, offsets the zero point to an offscreen position.
| /** | ||
| * @brief Get the ADC unit handle | ||
| * | ||
| * Exposes the ADC unit handle to allow configuring additional channels | ||
| * on the same ADC unit. Use with caution. | ||
| * | ||
| * @return The ADC unit handle, or nullptr if not initialized | ||
| */ | ||
| adc_oneshot_unit_handle_t getAdcHandle() const { return adcHandle; } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Consider providing a safer API instead of exposing the raw ADC handle.
Exposing the internal adc_oneshot_unit_handle_t breaks encapsulation and allows external code to modify ADC configuration in potentially unsafe ways. While the documentation warns "Use with caution," this leaky abstraction could lead to maintenance issues.
Consider adding a method to configure additional channels while maintaining encapsulation:
+ /**
+ * @brief Configure an additional channel on the same ADC unit
+ * @param channel The ADC channel to configure
+ * @param config Channel configuration
+ * @return true if successful, false otherwise
+ */
+ bool configureChannel(adc_channel_t channel, const adc_oneshot_chan_cfg_t& config) {
+ if (adcHandle == nullptr) {
+ return false;
+ }
+ return adc_oneshot_config_channel(adcHandle, channel, &config) == ESP_OK;
+ }This would allow the PaperS3Power implementation to configure additional channels without direct handle access.
Committable suggestion skipped: line range outside the PR's diff.
|
Considering how well it seems to work from the video, I might get this device too. |
|
Random but do the launcher icons render for you? |
| } | ||
|
|
||
| int adc_raw = 0; | ||
| esp_err_t ret = adc_oneshot_read(adcHandle, chargeStatusAdcChannel, &adc_raw); |
There was a problem hiding this comment.
Can we use ChargeFromAdcVoltage::readBatteryVoltageSampled() instead of exposing its ADC handle and doing these calls?
There was a problem hiding this comment.
No, I don't think so. If you check the code you'll notice that there's two ADC handles being utilised:
- one for the battery voltage readout, on GPIO3
- one for detecting the USB connection (thus charging status), on GPIO4 (this should trigger above 0.2V, I set it as 0.1V to be safe, since it floats around 0.02V)
So two handles are needed. Of course any ideas to refactor all this junk into something more usable, is most welcome, so if you have any specific ideas how to handle this scenario, do tell!
|
|
||
| // If we're still running after a delay, log it | ||
| vTaskDelay(pdMS_TO_TICKS(1000)); | ||
| TT_LOG_E(TAG, "Device did not power off as expected"); |
There was a problem hiding this comment.
Should we reboot at this point?
That way the battery won't drain as the user thinks their device is off.
There was a problem hiding this comment.
Possibly yes, although I am yet to encounter a case where my units didn't shut down after 2-3 tries of the above loop.
It just "seems" to work well - WiFi still can't stay active without getting major watchdog timer issues thanks to how EPDiy works (launching long-blocking tasks on each core, with screen updates taking up to 1500ms. Unfortunately I'm yet to find a way around this. Beyond scaling, e-ink displays will also need their own LVGL theme to increase contrast, and a way to generate more contrasting (monochrome or 4-bit grayscale) icons for the apps (maybe even a separate way to serve SVG icons directly that can be re-coloured during runtime - LVGL can support SVG with a simple feature toggle).
Yes, but not with just this branch. I was wary pushing changes to the core LVGL config, among other things, and stuck to core board support only (especially since further tinkering with LVGL for proper theming and scaling is required, which should be a feature and PR on its own). Although IIRC I put the right config bits in the sdkconfig of the board itself, so the icons, while not too contrastful, should show up. |
We'd need to have the watchdog issue fixed before merging though. Long-blocking tasks generally indicate a software design error.
Not sure we need SVG support - but I'm open to giving it a try. |
Unfortunately that's a downside of EPD displays - no matter if you're using a standard LUT or an on-the-fly matrix, unless you know the exact details of the display, you can't really issue truly quick updates (and even then the quick updates depend on drawing only the differential image from a double buffer). Since the specifics of even this widely used display aren't that well known, there are no open source algos that would speed things up. I've tried many things to resolve it, including yielding in tight loops (EPDiy has three-four of these, within the code running as a pair of pinned tasks), reducing task priority to 1, and so on, but unfortunately it still ends up in a situation where redrawing the screen (re-generating the waveforms and bitbanging it out, even using DMA) simply just takes too long. I guess I could go over to the EPDiy git repo and ask around for help about this, but I don't expect much more advancement on this front either.
Not sure if it will be slower, but it could potentially reduce app sizes AND make e-ink support much easier (no need to include a third, e-ink compatible variant of the app icon). |
I'd also suspect an issue with EPDiy specifically: The WDT only triggers after 5 seconds, if interpreted the sdkconfig correctly. Does the WDT timeout result in a crash? Or does Tactility continue working? e.g. Try App Hub and see if it is able to load the list of apps.
It definitely could reduce app size. App icon variants currently are avoided by recolouring the icons (e.g. in the launcher). |
The update time actually depends on how much of the display is updated and how fast. A full screen render requires anywhere between 900 and 1500ms (measured as per the recommendations of CodeRabbit), whereas small updates can be as quick as 150-200ms.
The device becomes unresponsive, and spams the WDT message in console output. I have managed, to some extent, to get around this, and even got to start entering WiFi details (though the keyboard rendering on this screen layout/size in portrait mode is awful + contrast is super low thanks to the theme being geared for LCD displays), but the crash/lockup at that point is inevitable and can only be resolved by resetting the device (via the power button). |
|
Perhaps we can merge it and mark the board as "incubating" for now in the properties file. I'll review it soon. |
|
Let me do the changes you've recommended, first, over the weekend 🙂 |
|
Regarding the new merge conflicts: the board-specific In the very near future, I'll also be renaming the |
fonix232
force-pushed
the
feat/add_m5stack_papers3_support
branch
from
29e61c5 to
510e9f4
Compare
|
@KenVanHoeylandt apologies for the delay, had some personal life stuff to take care of. Updated to the latest changes + added the UART port, however with these changes, I can't seem to launch apps anymore (app icon flashes as if it was updated after tap, but the app, let it be the app list, file browser or settings app, won't actually launch). Logs also don't show any launch attempt (or turn-off attempt if one were to press the power button). Does the new |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
Devices/m5stack-papers3/Source/devices/Power.cpp (1)
123-143: Use the instance ADC channel for calibration instead of the hard‑coded constant
adc_cali_curve_fitting_config_tcurrently sets.chan = CHARGE_STATUS_ADC_CHANNEL, while reads use the per‑instancechargeStatusAdcChannel. Using the member keeps the calibration consistent with the actual channel and makes the class safer to reuse with other channels.- adc_cali_curve_fitting_config_t cali_config = { - .unit_id = ADC_UNIT_1, - .chan = CHARGE_STATUS_ADC_CHANNEL, - .atten = ADC_ATTEN_DB_12, - .bitwidth = ADC_BITWIDTH_DEFAULT, - }; + adc_cali_curve_fitting_config_t cali_config = { + .unit_id = ADC_UNIT_1, + .chan = chargeStatusAdcChannel, + .atten = ADC_ATTEN_DB_12, + .bitwidth = ADC_BITWIDTH_DEFAULT, + };
🧹 Nitpick comments (3)
Devices/m5stack-papers3/Source/devices/SdCard.hpp (1)
6-6: Removeusingdeclaration from header file.The
usingdeclaration in a header file pollutes the namespace of all files that include this header. This is considered bad practice in C++.Apply this diff to use the fully qualified type name instead:
-using tt::hal::sdcard::SdCardDevice; - -std::shared_ptr<SdCardDevice> createSdCard(); +std::shared_ptr<tt::hal::sdcard::SdCardDevice> createSdCard();Devices/m5stack-papers3/Source/devices/SdCard.cpp (1)
10-20: Consider simplifying the configuration construction.The current implementation creates a
unique_ptronly to immediately move it intomake_shared. This intermediate step is unnecessary and can be simplified.Apply this diff to streamline the construction:
std::shared_ptr<SdCardDevice> createSdCard() { - auto configuration = std::make_unique<SpiSdCardDevice::Config>( + return std::make_shared<SpiSdCardDevice>( + std::make_unique<SpiSdCardDevice::Config>( - PAPERS3_SDCARD_PIN_CS, - GPIO_NUM_NC, - GPIO_NUM_NC, - GPIO_NUM_NC, - SdCardDevice::MountBehaviour::AtBoot + PAPERS3_SDCARD_PIN_CS, + GPIO_NUM_NC, + GPIO_NUM_NC, + GPIO_NUM_NC, + SdCardDevice::MountBehaviour::AtBoot + ) ); - - return std::make_shared<SpiSdCardDevice>( - std::move(configuration) - ); }Devices/m5stack-papers3/Source/devices/Power.hpp (1)
3-7: Make the header self‑contained forstd::string
PaperS3Powerexposesstd::stringin its public interface, but this header doesn’t include<string>directly. IfTactility/hal/power/PowerDevice.hever stops including<string>, this header will stop compiling. Consider adding an explicit#include <string>here to keep the header standalone.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
.gitmodules(1 hunks)CMakeLists.txt(1 hunks)Devices/m5stack-papers3/CMakeLists.txt(1 hunks)Devices/m5stack-papers3/Source/Configuration.cpp(1 hunks)Devices/m5stack-papers3/Source/Init.cpp(1 hunks)Devices/m5stack-papers3/Source/devices/Display.cpp(1 hunks)Devices/m5stack-papers3/Source/devices/Display.hpp(1 hunks)Devices/m5stack-papers3/Source/devices/Power.cpp(1 hunks)Devices/m5stack-papers3/Source/devices/Power.hpp(1 hunks)Devices/m5stack-papers3/Source/devices/SdCard.cpp(1 hunks)Devices/m5stack-papers3/Source/devices/SdCard.hpp(1 hunks)Devices/m5stack-papers3/device.properties(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- Devices/m5stack-papers3/device.properties
🚧 Files skipped from review as they are similar to previous changes (1)
- CMakeLists.txt
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: NellowTCS
Repo: ByteWelder/Tactility PR: 398
File: Boards/LilygoTdisplayS3/Source/devices/Display.cpp:183-199
Timestamp: 2025-10-28T05:42:03.394Z
Learning: The LilyGo T-Display S3 board (Boards/LilygoTdisplayS3) requires a non-standard LVGL flush pattern in its ST7789 i8080 display driver. In st7789_send_color_cb, lv_display_flush_ready() must be called immediately after esp_lcd_panel_io_tx_color() with a vTaskDelay(1), rather than using standard DMA completion callbacks via esp_lcd_panel_io_register_event_callbacks. Standard DMA callback approaches cause display corruption on this specific board. This pattern was tested against multiple alternatives (callback registration, buffer tuning, clock reduction) and is the only stable solution for T-Display S3 hardware.
Learnt from: fonix232
Repo: ByteWelder/Tactility PR: 380
File: Boards/m5stack-papers3/Source/devices/Display.cpp:9-18
Timestamp: 2025-10-29T14:45:02.914Z
Learning: For M5Stack PaperS3 in Boards/m5stack-papers3/Source/devices/Display.cpp, the GT911 touch configuration must use PAPERS3_EPD_HORIZONTAL_RESOLUTION (960) as yMax, PAPERS3_EPD_VERTICAL_RESOLUTION (540) as xMax, with swapXY=true and mirrorX=true. Alternative configurations (swapping the resolution parameters or setting swapXY=false) result in badly resolved touch positions. This specific configuration is empirically verified to work correctly.
📚 Learning: 2025-10-29T14:45:02.914Z
Learnt from: fonix232
Repo: ByteWelder/Tactility PR: 380
File: Boards/m5stack-papers3/Source/devices/Display.cpp:9-18
Timestamp: 2025-10-29T14:45:02.914Z
Learning: For M5Stack PaperS3 in Boards/m5stack-papers3/Source/devices/Display.cpp, the GT911 touch configuration must use PAPERS3_EPD_HORIZONTAL_RESOLUTION (960) as yMax, PAPERS3_EPD_VERTICAL_RESOLUTION (540) as xMax, with swapXY=true and mirrorX=true. Alternative configurations (swapping the resolution parameters or setting swapXY=false) result in badly resolved touch positions. This specific configuration is empirically verified to work correctly.
Applied to files:
Devices/m5stack-papers3/Source/devices/Display.cppDevices/m5stack-papers3/Source/devices/Display.hpp
📚 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-papers3/Source/Configuration.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-papers3/Source/Configuration.cpp
🧬 Code graph analysis (4)
Devices/m5stack-papers3/Source/devices/Power.cpp (2)
Devices/m5stack-papers3/Source/devices/Power.hpp (4)
PaperS3Power(31-36)PaperS3Power(37-37)type(43-43)type(44-44)Drivers/EstimatedPower/Source/ChargeFromAdcVoltage.h (1)
ChargeFromAdcVoltage(6-52)
Devices/m5stack-papers3/Source/devices/Power.hpp (1)
Devices/m5stack-papers3/Source/devices/Power.cpp (2)
PaperS3Power(38-53)PaperS3Power(55-58)
Devices/m5stack-papers3/Source/Configuration.cpp (4)
Devices/m5stack-papers3/Source/devices/Power.cpp (2)
createPower(222-261)createPower(222-222)Devices/m5stack-papers3/Source/devices/Display.cpp (2)
createDisplay(23-26)createDisplay(23-23)Devices/m5stack-papers3/Source/devices/SdCard.cpp (2)
createSdCard(9-21)createSdCard(9-9)Devices/m5stack-papers3/Source/Init.cpp (2)
initBoot(12-20)initBoot(12-12)
Devices/m5stack-papers3/Source/devices/SdCard.hpp (1)
Tactility/Include/Tactility/hal/sdcard/SdCardDevice.h (1)
SdCardDevice(14-67)
🪛 Clang (14.0.6)
Devices/m5stack-papers3/Source/Init.cpp
[error] 1-1: 'Tactility/kernel/SystemEvents.h' file not found
(clang-diagnostic-error)
Devices/m5stack-papers3/Source/devices/Power.hpp
[error] 3-3: 'memory' file not found
(clang-diagnostic-error)
Devices/m5stack-papers3/Source/Configuration.cpp
[error] 1-1: 'vector' file not found
(clang-diagnostic-error)
Devices/m5stack-papers3/Source/devices/SdCard.hpp
[error] 3-3: 'memory' file not found
(clang-diagnostic-error)
Devices/m5stack-papers3/Source/devices/Display.hpp
[error] 3-3: 'memory' file not found
(clang-diagnostic-error)
🔇 Additional comments (20)
.gitmodules (1)
19-21: Verify maintenance plan for the epdiy fork.The submodule points to a personal fork rather than the upstream
vroland/epdiyrepository. While this is acceptable during development (especially since you've merged fixes from vroland/epdiy#392), consider documenting the plan for either upstreaming your changes or switching to the official repository once the required fixes are available upstream.What is the long-term maintenance plan for this fork? Are there changes that need to be upstreamed, or can this switch to the official repository once certain PRs are merged?
Devices/m5stack-papers3/Source/Init.cpp (2)
7-10: LGTM!The empty power-on sequence is appropriate for this hardware, and the comment clearly documents the intent.
12-20: LGTM!The boot initialization logic is correct with appropriate logging and error handling.
Devices/m5stack-papers3/Source/devices/Display.hpp (1)
1-11: LGTM!The resolution constants and display factory declaration are correct for the M5Stack PaperS3 hardware.
Devices/m5stack-papers3/Source/devices/Display.cpp (2)
7-21: LGTM!The GT911 touch configuration correctly uses the empirically verified settings (HORIZONTAL as yMax with swapXY=true and mirrorX=true) that produce accurate touch positions on the M5Stack PaperS3 hardware.
Based on learnings.
23-26: LGTM!The display factory correctly composes the touch device and EPDiy display helper following clean factory patterns.
Devices/m5stack-papers3/CMakeLists.txt (1)
1-7: LGTM!The component registration follows ESP-IDF conventions with appropriate dependencies for the M5Stack PaperS3 hardware (EPDiyDisplay for e-ink, GT911 for touch, vfs/fatfs for SD card).
Devices/m5stack-papers3/Source/Configuration.cpp (4)
14-20: LGTM!The device factory composition is clean and correctly wires power, display, and SD card devices for the M5Stack PaperS3.
25-60: LGTM!The I2C configuration correctly sets up the internal bus for touch and peripherals, and provides the Grove port as a mutable External bus (disabled by default to allow UART usage).
61-85: LGTM!The SPI configuration correctly sets up the bus for SD card access with standard ESP-IDF parameters. The
lock=nullptris appropriate here since this SPI bus is for the SD card, not for display operations requiring LVGL synchronization.
86-110: LGTM!The UART configuration correctly enables the Grove port for UART peripherals (such as GPS) with standard 115200 8N1 settings, addressing the previous review feedback.
Devices/m5stack-papers3/Source/devices/SdCard.cpp (1)
9-16: Configuration is correct; no SD card function issues identified.The GPIO_NUM_NC pins (CD/WP/INT) are intentional. They mean "use the default pins from the SPI bus configuration," not missing pins. The code correctly uses the SPI2_HOST bus, which is properly configured in
Devices/m5stack-papers3/Source/Configuration.cppwith MOSI (GPIO_NUM_38), MISO (GPIO_NUM_40), and SCLK (GPIO_NUM_39). This shared-bus pattern is consistent across similar devices in the codebase.Devices/m5stack-papers3/Source/devices/Power.cpp (6)
38-58: Constructor/destructor wiring and logging look solidClean dependency injection (unique_ptr + explicit ADC handle) and detailed boot/shutdown logs should make hardware wiring issues easy to diagnose. No issues here.
60-79: Charge‑status ADC initialization is clear and guardedIdempotent setup of the ADC channel with error logging and an early return if already initialized is straightforward and appropriate for this use.
81-105: Power‑off GPIO configuration is straightforwardThe one‑time GPIO config with explicit pull settings, interrupts disabled, and an initial low level is a good fit for a dedicated power‑off line.
190-220: Power‑off pulse sequence is reasonable for this hardwareThe configurable pulse train followed by a final high level, plus explicit logging before and after, is a clear implementation for asserting the PaperS3 shutdown line. The blocking delays here are acceptable given this only runs during power‑off.
222-261: Factory wiring betweenChargeFromAdcVoltageandPaperS3Powerlooks correctThe configuration applies the 2× divider multiplier, LiPo voltage range, and ADC1 channel for VBAT, then shares the resulting ADC unit with charge‑status monitoring. Returning
nullptrwhengetAdcHandle()fails is a sensible failure path for this board‑specific factory.
153-188: No C++ standard concerns—project is already standardized on C++20The widespread use of
using enumthroughout the codebase (33 occurrences across framework, drivers, and device implementations) confirms the project is already configured for C++20. Your code change does not introduce any new standard requirements.Devices/m5stack-papers3/Source/devices/Power.hpp (2)
21-54: Class design cleanly encapsulates PaperS3 power managementThe class surfaces exactly the expected capabilities (battery metrics, charge detection, power‑off) and holds the right hardware resources (ADC handle, charge‑status channel, power‑off GPIO). The declaration matches the implementation in
Power.cppand fits naturally into thePowerDeviceabstraction.
58-58:createPower()declaration matches the implementation patternThe global
createPower()factory signature here aligns with the definition inPower.cppand the way other device factories are exposed in this codebase.
Check logs, if there is a |
I have - logs show a few screen refreshes but that's it. |
No worries, there's no rush in this project! :)
I agree with NellowTCS that it's likely a Mutex issue. Nothing of that sort changed recently, though.
No, |
|
@KenVanHoeylandt did a diff on the sdkconfigs, there isn't much: To list them:
I don't really see any reason here that would affect the app launch. I also double checked in the logs and am not getting any Mutex locks. |
|
It seems that you diffed the entire sdkconfig after you ran It's best to compare:
The colour depth difference could be problematic if the graphics driver can't manage it. The TinyUSB mount path could be problematic if TinyUSB is enabled. |
|
@Shadowtrance just found the related bug in the sdkconfig generation ( |
|
@KenVanHoeylandt I don't see how that PR relates to this one at all? I had no trouble with the IDF target name. |
|
@fonix232 There was a bug fixed that related to this:
... and some other performance-related settings not being properly set by |
|
@KenVanHoeylandt I'm fairly certain I've set |
Sure, but that shouldn't be necessary anymore as it's now added automatically (or should be).
It does. I just forgot to document it here 🙈 |
3 participants
This PR is very much WIP. Closes off #379 once ready.
I've confirmed touch is working. However, the display driver is giving me a hard time, so any kind of help is most welcome.
Decided to use FastEPD as it's just smaller and neater than EPDiy.
The basic init seems to work and I can even get the screen to clean, however everything down the line within the
lvglTaskseems to hang, no matter what I do:Power management, USB, etc. needs testing but can't do that until I have UI access...