M5Stack PaperS3 Improvements#512
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR replaces the FastEpdDisplay driver with a new EpdiyDisplay implementation and helper, removes FastEpdDisplay sources/headers, and updates build files/manifests to use epdiy and the EPDiyDisplay component. It adds a PaperS3 power management subsystem (PaperS3Power, createPower(), ADC-based charge reading, buzzer, power-off) and an initBoot() implementation. Display creation was refactored so createDisplay() constructs touch internally. Other changes include LVGL symbol exports, Wi‑Fi default-driver cleanup, minor UI/file-operation fixes, device property tweak, and related CMake/README updates. 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
device.py (1)
225-230:⚠️ Potential issue | 🟠 MajorUse valid LVGL symbols for
DefaultLighttheme selection.Line 228 writes
CONFIG_LV_THEME_DEFAULT_LIGHT=y, but that symbol does not exist in LVGL's lv_conf.h. The correct approach is to setLV_THEME_DEFAULT_DARK=0for light mode andLV_THEME_DEFAULT_DARK=1for dark mode.Proposed fix
if theme == "DefaultDark": + output_file.write("CONFIG_LV_USE_THEME_DEFAULT=y\n") output_file.write("CONFIG_LV_THEME_DEFAULT_DARK=y\n") elif theme == "DefaultLight": - output_file.write("CONFIG_LV_THEME_DEFAULT_LIGHT=y\n") + output_file.write("CONFIG_LV_USE_THEME_DEFAULT=y\n") + output_file.write("CONFIG_LV_THEME_DEFAULT_DARK=n\n") elif theme == "Mono": output_file.write("CONFIG_LV_USE_THEME_MONO=y\n")
🧹 Nitpick comments (2)
Drivers/EPDiyDisplay/README.md (1)
1-3: Consider expanding this README with setup/compatibility details.A short section for supported panels, required dependencies, and init expectations would make integration/debugging easier.
Firmware/idf_component.yml (1)
59-61: Pin the forkedepdiydependency to a commit hash instead of a version tag for stronger reproducibility.While the current version tag (
2.0.1) is acceptable for ESP-IDF components, pinning directly to a commit hash is the preferred approach for git-sourced dependencies. This prevents unexpected changes if the tag is reused or moved in the fork. Additionally, consider committing adependencies.lockfile to lock all resolved versions across the dependency tree, ensuring consistent builds across machines and CI environments.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ddb2f3c8-9f1b-4598-b379-2a779c964d38
📒 Files selected for processing (26)
Devices/m5stack-papers3/CMakeLists.txtDevices/m5stack-papers3/Source/Configuration.cppDevices/m5stack-papers3/Source/Init.cppDevices/m5stack-papers3/Source/devices/Display.cppDevices/m5stack-papers3/Source/devices/Display.hDevices/m5stack-papers3/Source/devices/Power.cppDevices/m5stack-papers3/Source/devices/Power.hDevices/m5stack-papers3/device.propertiesDrivers/EPDiyDisplay/CMakeLists.txtDrivers/EPDiyDisplay/README.mdDrivers/EPDiyDisplay/Source/EpdiyDisplay.cppDrivers/EPDiyDisplay/Source/EpdiyDisplay.hDrivers/EPDiyDisplay/Source/EpdiyDisplayHelper.hDrivers/FastEpdDisplay/Source/FastEpdDisplay.cppDrivers/FastEpdDisplay/Source/FastEpdDisplay.hFirmware/idf_component.ymlModules/lvgl-module/source/symbols.cTactility/Source/app/apphubdetails/AppHubDetailsApp.cppTactility/Source/app/files/View.cppTactility/Source/app/launcher/Launcher.cppTactility/Source/lvgl/Statusbar.cppTactility/Source/service/webserver/WebServerService.cppTactility/Source/service/wifi/WifiEsp.cppTactilityC/Source/symbols/freertos.cppTactilityC/Source/tt_init.cppdevice.py
💤 Files with no reviewable changes (2)
- Drivers/FastEpdDisplay/Source/FastEpdDisplay.cpp
- Drivers/FastEpdDisplay/Source/FastEpdDisplay.h
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c76e9eab-3680-4f57-97f0-cdb6ac277dad
📒 Files selected for processing (7)
Devices/m5stack-papers3/Source/devices/Power.cppDrivers/EPDiyDisplay/Source/EpdiyDisplay.cppDrivers/EPDiyDisplay/Source/EpdiyDisplay.hTactility/Include/Tactility/hal/display/DisplayDevice.hTactility/Include/Tactility/hal/touch/TouchDevice.hTactility/Source/service/wifi/WifiEsp.cppTactilityCore/Include/Tactility/CoreDefines.h
🚧 Files skipped from review as they are similar to previous changes (1)
- Tactility/Source/service/wifi/WifiEsp.cpp
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Devices/m5stack-papers3/Source/devices/Power.cpp (1)
111-116: Update the USB detect comment to match the current boot setup.The implementation is fine, but the comment says
initBoot()enables a pull-down. The current boot path only resets the pin and sets it to input, so this note will mislead the next person who revisits USB detection. Based on learnings: In Devices/m5stack-papers3/Source/Init.cpp, the USB_DETECT_PIN (GPIO_NUM_5) and CHARGE_STATUS_PIN (GPIO_NUM_4) are initialized with gpio_reset_pin() + gpio_set_direction() only, intentionally matching the M5Stack HAL user demo pattern. No explicit pull-mode configuration is set because USB detection is still a work in progress. Do not flag the missing pull-mode call as a defect until USB detection is resolved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cf4109d7-84ba-456e-ad22-f6225c29f4a8
📒 Files selected for processing (3)
Devices/m5stack-papers3/Source/devices/Power.cppDrivers/EstimatedPower/Source/ChargeFromAdcVoltage.cppDrivers/EstimatedPower/Source/ChargeFromAdcVoltage.h
|
Looks great, thanks a lot! |
2 participants
M5Stack PaperS3
Other Additions
Still more styling to be fixed for paper displays like invisible checkbox's until turned on, borders around dropdown menus etc etc, just general visibility styling needs work.
The EpdiyDisplay driver and Power is mostly based on @fonix232's work with a fair few changes to it by me.
Summary by CodeRabbit
New Features
Bug Fixes
Improvements