fix POV Display usermod #4427
fix POV Display usermod #4427
Conversation
|
did not try to compile, but generally looks ok to me. |
Liliputech
force-pushed
the
pov_display
branch
from
3402a05 to
001b882
Compare
|
@Liliputech please do not force-push while you have a PR open for the the main WLED repo. It causes lots of crazy side-effects like lost review comments, auto-closed bug reports and broken links to code authors.
|
softhack007
added
the
in progress
|
@Liliputech I think the usermod needs some more fixing and improvements.
Btw, do you know who is "Arthur Suzuki"? he appears as the author of initial commit, but does not have a github account. He has directly committed the usermod into the Aircoookie repo, so he must be a maintainer? or maybe just a hacker who abused a security flaw in github? |
Hi @softhack007 Thank you for your feedback :)
Maybe I should try to fix that by updating my code to fit the new version of PNGDEC instead then.
Sure! And a couple pictures!
Glad you liked it :)
Got to fix this indeed.
It is intended for 1D strip, the idea is to have an imaged displayed on a rotating strip.
Unfortunately there is not much to be done here if you want to properly decode PNG.
I am Arthur :) you can find the information elsewhere by searching for my nickname or profile picture ;) |
|
@Liliputech Since you are fixing things might be consider adding a readme as it is strange the usermod even got merged without one |
|
Actually I'm thinking about re-using the routines provided by the "image_loader.cpp" file provided by the GIF decoder effect. That would make a lot more sense :) |
Liliputech
force-pushed
the
pov_display
branch
from
001b882 to
ce44827
Compare
|
I can't reuse the routines from image_loader.cpp because the functions signatures are too different... I will just rename my methods in a more explicit and non-conflicting way and provide a README. |
|
just to mention, the functions will only be compiled if the USERMOD_POV_DISPLAY flag is set, which for most wled user won't be the case. I don't think this usermod will have that much impact on other things in wled codebase. |
|
Hey! This pull request has been open for quite some time without any new comments now. It will be closed automatically in a week if no further activity occurs. |
github-actions
bot
added
the
stale
WalkthroughAdds a 24‑bit uncompressed BMP loader and shared ~64KB buffer, a POV renderer that maps BMP rows to an LED strip, integrates BMP‑based POV image effect into the pov_display usermod (replacing PNG decoding), adds usage docs, and removes the PNGdec dependency. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
usermods/pov_display/README.md (1)
3-3: Minor writing improvements and URL formatting.Consider these improvements for better readability:
-The whole goal of the project is to use images like the one found on this website : https://visualpoi.zone/ +The whole goal of the project is to use images like the one found on this website: [https://visualpoi.zone/](https://visualpoi.zone/)-The usermod also handles mirroring, which means you can "fold" a led strip in two to get the same image displayed on both side. +The usermod also handles mirroring, which means you can "fold" an LED strip in two to get the same image displayed on both sides.-The effect will first draw the first line of your image, then very rapidly display the second line, etc, until the last line, and then it will loop to the first line. +The effect will first draw the first line of your image, then very rapidly display the second line, etc., until the last line, and then it will loop to the first line.-To help you to use this project you could also use the GifPlayer from https://github.com/Manut38/WLED-GIFPlayer-html -you would have to update the effect ID in the html file to reflect the effect ID of the "POV Image" effect though. +To help you to use this project, you could also use the GifPlayer from [https://github.com/Manut38/WLED-GIFPlayer-html](https://github.com/Manut38/WLED-GIFPlayer-html). +You would have to update the effect ID in the HTML file to reflect the effect ID of the "POV Image" effect, though.Also applies to: 9-9, 10-10, 17-18
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
3-3: Bare URL used
null(MD034, no-bare-urls)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
usermods/pov_display/README.md(1 hunks)usermods/pov_display/usermod_pov_display.h(4 hunks)wled00/usermods_list.cpp(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
wled00/usermods_list.cpp (1)
wled00/um_manager.cpp (2)
add(65-70)add(65-65)
🪛 LanguageTool
usermods/pov_display/README.md
[grammar] ~9-~9: Did you mean “LED” (= diode, light)?
Context: ...mirroring, which means you can "fold" a led strip in two to get the same image disp...
(LED)
[style] ~10-~10: In American English, abbreviations like “etc.” require a period.
Context: ...n very rapidly display the second line, etc, until the last line, and then it will ...
(ETC_PERIOD)
[typographical] ~18-~18: It seems that a comma is missing.
Context: ... the effect ID of the "POV Image" effect though.
(THOUGH_COMMA)
🪛 markdownlint-cli2 (0.17.2)
usermods/pov_display/README.md
3-3: Bare URL used
null
(MD034, no-bare-urls)
17-17: Bare URL used
null
(MD034, no-bare-urls)
🔇 Additional comments (7)
wled00/usermods_list.cpp (1)
476-476: Constructor parameters align with usermod changes.The updated constructor call correctly matches the new
PovDisplayUsermodconstructor signature that now accepts a name and enabled flag.usermods/pov_display/README.md (1)
1-18: Great addition of missing documentation!This README provides essential documentation for the POV Display usermod that was previously missing. The explanation of the persistence of vision effect and usage instructions will be very helpful for users.
🧰 Tools
🪛 LanguageTool
[grammar] ~9-~9: Did you mean “LED” (= diode, light)?
Context: ...mirroring, which means you can "fold" a led strip in two to get the same image disp...(LED)
[style] ~10-~10: In American English, abbreviations like “etc.” require a period.
Context: ...n very rapidly display the second line, etc, until the last line, and then it will ...(ETC_PERIOD)
[typographical] ~18-~18: It seems that a comma is missing.
Context: ... the effect ID of the "POV Image" effect though.(THOUGH_COMMA)
🪛 markdownlint-cli2 (0.17.2)
3-3: Bare URL used
null(MD034, no-bare-urls)
17-17: Bare URL used
null(MD034, no-bare-urls)
usermods/pov_display/usermod_pov_display.h (5)
5-8: Good refactoring to avoid naming conflicts.The renaming of global variables (
png→png_decoder,f→pov_image) and the "Pov" prefix for functions addresses the feedback about potential naming conflicts with generic names.
9-16: Consistent function renaming implementation.The file operation functions are consistently renamed with the "Pov" prefix and properly use the renamed global variables.
58-60: Proper file path construction.The file path construction correctly prepends a "/" to the segment name to create a valid filesystem path.
69-85: Constructor follows WLED usermod patterns.The new constructor accepting name and enabled parameters follows WLED usermod conventions and addresses the feedback about usermod initialization. The protected members also follow appropriate naming patterns.
7-7: Effect name maintains uppercase convention.The effect name "POV Image" correctly maintains the uppercase convention used by other effects in WLED, consistent with previous feedback.
|
@Liliputech can you please update it to latest main? |
github-actions
bot
removed
the
stale
this usermod adds a new effect called "POV Image". To get it to work: - read the README :) - upload a bmp image to the ESP filesystem using "/edit" url. - select "POV Image" effect. - set the filename (ie: "/myimage.bmp") as segment name. - rotate the segment at approximately 20 RPM. - enjoy the show!
Liliputech
force-pushed
the
pov_display
branch
from
ebd0000 to
41f31b8
Compare
|
Hi sorry for the delay, I've reworked the usermod to make it better, and also rebased on current main. |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (11)
usermods/pov_display/pov.h (2)
10-15: Improve const-correctness of showLineThe line buffer is not modified; accept it as const to prevent accidental writes and allow passing const sources.
Apply this diff (paired with pov.cpp):
- void showLine(byte * line, uint16_t size); + void showLine(const byte * line, uint16_t size);
16-18: Fix typos in comments and tighten signatures
- Spelling: “Returns”, “returns”.
- Prefer const char* for filename access; returning non-const exposes internal buffer to mutation.
Apply this diff:
- /* Reads from file an image and making it current image */ - bool loadImage(char * filename); + /* Reads an image from file and makes it the current image */ + bool loadImage(const char * filename); /* Show next line of active image - Retunrs the index of next line to be shown (not yet shown!) - If it retunrs 0, it means we have completed showing the image and + Returns the index of the next line to be shown (not yet shown!) + If it returns 0, it means we have completed showing the image and next call will start again */ int16_t showNextLine(); ... - char * getFilename() {return image.getFilename();} + const char * getFilename() {return image.getFilename();}Also applies to: 32-32, 20-22
usermods/pov_display/bmpimage.h (3)
6-23: Minor doc typos and clarityPolish comments: fix “passign/usign”, clarify single shared buffer usage.
Apply this diff:
- * To initialize, call init(), passign to it name of a bitmap file + * To initialize, call init() passing it the name of a bitmap file ... - * For performance reasons, before actually usign the image, you need to load + * For performance reasons, before actually using the image, you need to load ... - * All load() operations use the same reserved buffer in RAM, so you can only + * All load() operations use a single shared buffer in RAM, so you can only
36-36: Make filename accessor const-correctAvoid exposing a mutable pointer to internal storage.
Apply this diff:
- char * getFilename() {return filename;}; + const char * getFilename() const {return filename;};
48-49: Avoid exporting the global image bufferLeaking _buffer via header couples unrelated code to this implementation detail and risks name collisions. Keep the buffer internal to the translation unit.
Apply this diff (and make the definition in bmpimage.cpp ‘static’):
-extern byte _buffer[];And in bmpimage.cpp:
-byte _buffer[BUF_SIZE]; +static byte _buffer[BUF_SIZE];usermods/pov_display/bmpimage.cpp (1)
108-114: Bounds-check line index before returning a row pointerDefensive check avoids returning pointers past the buffer if misused.
Apply this diff:
-byte* BMPimage::line(uint16_t n){ - if (_loaded) { - return (_buffer+n*_rowSize); - } else { - return NULL; - } -} +byte* BMPimage::line(uint16_t n){ + if (!_loaded) return NULL; + if (n >= _height) return NULL; + return (_buffer + n * _rowSize); +}usermods/pov_display/pov_display.cpp (5)
2-2: Consider compile-gating the usermod include to avoid accidental builds when the usermod is disabledIf pov_display isn’t enabled, including "pov.h" can still trigger compilation in some setups. Wrapping the file (or at least the include) in a USERMOD_POV_DISPLAY guard makes the build more robust across different PlatformIO configs.
Example:
+#ifdef USERMOD_POV_DISPLAY #include "pov.h"And add at the very end of the file:
#endif // USERMOD_POV_DISPLAY
4-4: Verify effect metadata string flags are correct for 1D-only usage"POV Image@!;;;;" differs from the commonly used metadata patterns. If the previous variant ended with “;1” to indicate 1D or other flags, make sure removing it doesn’t regress UI controls or capability flags in WLED. If 1D-only is intended, double-check the last field semantics.
Would you like me to cross-check the effect string format against the current WLED effect metadata spec and propose the exact flags?
6-6: Limit the symbol scope of the globalpovto avoid ODR/linkage collisionsMake
povinternal to this translation unit. This avoids accidental external linkage and name collisions with other usermods.Apply this diff:
-POV pov; +static POV pov;
29-33: Use an initializer list for the constructorCleaner and avoids double assignments.
Apply this diff:
- PovDisplayUsermod(const char *name, bool enabled) { - this->_name=name; - this->enabled = enabled; - } + PovDisplayUsermod(const char *name, bool enabled) + : enabled(enabled), _name(name) {}
41-50: Usermod enabled flag doesn’t gate the effect functionThe effect is registered regardless of enabled state, and
mode_pov_imageruns independently ofenabled. If “enabled=false” is intended to fully disable behavior, either:
- Gate registration in setup() behind enabled, or
- Have
mode_pov_imagecheck a shared flag (e.g., expose a function or static in this TU) and early-return when disabled.I can wire
mode_pov_imageto a TU-scopedstatic volatile bool s_enabledthat mirrors the usermod state if you want a minimal change.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
usermods/pov_display/README.md(1 hunks)usermods/pov_display/bmpimage.cpp(1 hunks)usermods/pov_display/bmpimage.h(1 hunks)usermods/pov_display/library.json(0 hunks)usermods/pov_display/pov.cpp(1 hunks)usermods/pov_display/pov.h(1 hunks)usermods/pov_display/pov_display.cpp(1 hunks)
💤 Files with no reviewable changes (1)
- usermods/pov_display/library.json
🧰 Additional context used
🧬 Code Graph Analysis (3)
usermods/pov_display/bmpimage.h (2)
usermods/pov_display/bmpimage.cpp (10)
load(91-106)load(91-91)line(108-114)line(108-108)pixelColor(116-131)pixelColor(116-116)init(18-79)init(18-18)clear(81-89)clear(81-81)usermods/pov_display/pov.h (1)
getFilename(32-32)
usermods/pov_display/pov.h (3)
usermods/pov_display/pov.cpp (7)
POV(3-3)showLine(5-22)showLine(5-5)loadImage(24-29)loadImage(24-24)showNextLine(31-38)showNextLine(31-31)usermods/pov_display/bmpimage.cpp (2)
line(108-114)line(108-108)usermods/pov_display/bmpimage.h (1)
getFilename(36-36)
usermods/pov_display/pov.cpp (3)
usermods/pov_display/pov.h (1)
POV(6-38)usermods/pov_display/bmpimage.cpp (2)
line(108-114)line(108-108)usermods/pov_display/pov_display.cpp (1)
strip(34-37)
🪛 LanguageTool
usermods/pov_display/README.md
[grammar] ~6-~6: There might be a mistake here.
Context: ... - resize your image, the height should be same number of pixels as your led strip...
(QB_NEW_EN)
[grammar] ~6-~6: There might be a mistake here.
Context: ...same number of pixels as your led strip. - rotate your image 90 degrees clockwise (...
(QB_NEW_EN)
[grammar] ~8-~8: There might be a mistake here.
Context: ...to the ESP filesystem using "/edit" url. - select "POV Image" effect. - set the seg...
(QB_NEW_EN)
[grammar] ~9-~9: There might be a mistake here.
Context: .../edit" url. - select "POV Image" effect. - set the segment name with the absolute p...
(QB_NEW_EN)
[grammar] ~10-~10: There might be a mistake here.
Context: ... path of the image (ie: "/myimage.bmp"). - rotate the segment at approximately 20 R...
(QB_NEW_EN)
[grammar] ~11-~11: There might be a mistake here.
Context: ...ate the segment at approximately 20 RPM. - enjoy the show!
(QB_NEW_EN)
🪛 markdownlint-cli2 (0.17.2)
usermods/pov_display/README.md
1-1: No space after hash on atx style heading
(MD018, no-missing-space-atx)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: wled_build / Build Environments (esp32_wrover)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (esp32dev)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
🔇 Additional comments (7)
usermods/pov_display/pov.cpp (1)
31-38: Line-stepping logic is soundshowNextLine correctly wraps at image.height(), returns the next line index, and delegates size/row padding handling to showLine. Looks good.
usermods/pov_display/bmpimage.cpp (1)
18-61: Header parsing is reasonable for 24-bit uncompressed BMPsLittle-endian helpers and checks for signature, planes, depth (24), compression (0), and 4-byte row padding are correct for the targeted format.
usermods/pov_display/pov_display.cpp (5)
34-37: Confirm effect ID stability and duplicates when registering with 255Hard-coding 255 can collide with other custom effects or future core additions. If the API supports auto-assignment or symbolic constants, prefer that. Also ensure setup() isn’t called multiple times (e.g., after a soft re-init) to avoid duplicate registration.
Would you like me to scan the tree to verify there are no other addEffect(255, ...) registrations and that 255 is free in current main?
52-54: LGTM: clean no-op on ESP-NOW messagesReturning false is appropriate if no handling is required.
56-58: LGTM: stable usermod IDUsing USERMOD_ID_POV_DISPLAY keeps it consistent with the core registry.
61-62: LGTM: static instance and registrationStatic construction and REGISTER_USERMOD usage are consistent with typical WLED usermod patterns.
2-2: PNGdec dependency removed; no conflicting addEffect(255) usages
- PNGdec only appears as a commented entry in platformio_override.sample.ini; it’s not in the active platformio.ini or anywhere else.
- All calls to strip.addEffect(255, …) across usermods use 255 as the “auto-assign” sentinel for custom effects—this is by design and does not create conflicts.
No further action required.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
usermods/pov_display/README.md (1)
1-3: README added and core usage clarified — nice improvementThis addresses earlier feedback to document the usermod, clarifies BMP format constraints, and outlines step-by-step usage. Good job landing this.
🧹 Nitpick comments (4)
usermods/pov_display/README.md (4)
10-10: Clarify absolute path and case sensitivity for the filenameExplicitly call out that the path must start with “/” and is case-sensitive to prevent common misconfigurations.
-- Set the segment name to the absolute filesystem path of the image (e.g., “/myimage.bmp”). +- Set the segment name to the absolute filesystem path of the image (e.g., “/myimage.bmp”). + The path is case-sensitive and must start with “/”.
11-12: Provide quick tuning guidance for RPMA short note on tuning rotation speed helps users get from “it shows something” to “it looks correct”.
-- Rotate the segment at approximately 20 RPM. +- Rotate the segment at approximately 20 RPM. + Tune as needed so that one full revolution maps to the image width (if the image appears stretched or compressed, adjust RPM slightly).
14-17: Quantify the 64 KB image limit with concrete examples and a mirroring tipConcrete numbers reduce guesswork. Also add a quick fix if users see mirrored text.
Notes: - Only 24-bit uncompressed BMP files are supported. -- The image must fit into ~64 KB of RAM (width × height × 3 bytes, plus row padding to a 4-byte boundary). +- The image must fit into ~64 KB of RAM (width × height × 3 bytes, plus row padding to a 4‑byte boundary). +- Examples (approximate, excluding row padding): + - 128×128 (49,152 bytes) fits. + - 160×160 (76,800 bytes) does NOT fit. + - 96×192 (55,296 bytes) fits; padding may add a small overhead. - The path must be absolute. + - If the rendered image appears mirrored or upside‑down, rotate 90° the other way or flip horizontally in your editor and try again.
1-17: Add “Requirements”, “Troubleshooting”, and “Safety” sectionsA few concise sections make first‑time setup smoother and nudge users toward safe operation of rotating rigs.
## POV Display usermod @@ - Enjoy the show! Notes: @@ - The path must be absolute. + +### Requirements +- 1D rotating LED strip/segment (POV setup). Ensure the segment length equals the number of physical LEDs. +- BMP image saved as 24‑bit, uncompressed (no alpha, no palette). +- Sufficient free RAM (~64 KB) for the image buffer. + +### Troubleshooting +- Nothing displays: verify the file exists at the exact absolute path (case‑sensitive) and is a 24‑bit uncompressed BMP. +- Garbled colors or wrong orientation: re‑export as 24‑bit BMP and retry the rotation/flip guidance above. +- Image too large: reduce width and/or height until it fits within ~64 KB (see examples). +- Path issues: confirm you uploaded the file via the “/edit” URL and can see it in the filesystem browser. + +### Safety +- Secure the rotating assembly and keep clear of moving parts. +- Balance the strip/hub to minimize vibration before running at speed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
usermods/pov_display/README.md(1 hunks)usermods/pov_display/bmpimage.cpp(1 hunks)usermods/pov_display/pov.cpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- usermods/pov_display/bmpimage.cpp
- usermods/pov_display/pov.cpp
🧰 Additional context used
🪛 LanguageTool
usermods/pov_display/README.md
[grammar] ~3-~3: Ensure spelling is correct
Context: ## POV Display usermod This usermod adds a new effect called “POV Image”. ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: wled_build / Build Environments (usermods)
Liliputech
force-pushed
the
pov_display
branch
from
aeb19e8 to
0ce9ac6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
usermods/pov_display/pov_display.cpp (1)
10-24: Resolved: robust .bmp ends-with check and null-guard implementedNice fix. The ends-with, case-insensitive check and null-guard for mainseg.name address the fragility flagged earlier. Good adherence to the suggested approach.
🧹 Nitpick comments (6)
usermods/pov_display/pov_display.cpp (6)
6-6: Limit global symbol scope: make POV instance internal and less collision-pronePrior reviews called out collisions from generic globals. The identifier pov is short and global; recommend internal linkage or a more specific name.
Apply this to keep the symbol TU-local and avoid clashes:
-POV pov; +static POV s_pov;And update uses:
- const char* current = pov.getFilename(); + const char* current = s_pov.getFilename(); @@ - pov.showNextLine(); + s_pov.showNextLine(); @@ - pov.loadImage(segName); + s_pov.loadImage(segName);Also applies to: 26-30, 32-33
32-33: Throttle repeated load attempts to avoid tight FS loops on missing/corrupt filesIf loadImage fails (file missing/corrupt), this branch will be hit every frame, hammering the FS and starving other tasks. Throttle attempts.
Minimal in-function backoff:
- pov.loadImage(segName); + static unsigned long s_lastLoadAttemptMs = 0; + unsigned long nowMs = millis(); + // Retry at most twice per second if the image is not yet loaded. + if (nowMs - s_lastLoadAttemptMs < 500) return FRAMETIME; + s_lastLoadAttemptMs = nowMs; + pov.loadImage(segName);If you adopt the s_pov rename above, switch pov.* to s_pov.* here as well.
36-41: Trim or use unused members (_name, initDone) to reduce footprint
- _name is stored but not used anywhere.
- initDone is only set, never read.
Either use them or drop to keep the usermod lean.
Suggested diff (also touches setup at Line 51):
class PovDisplayUsermod : public Usermod { protected: bool enabled = false; //WLEDMM - const char *_name; //WLEDMM - bool initDone = false; //WLEDMM + // const char *_name; //WLEDMM (unused) + // bool initDone = false; //WLEDMM (unused) unsigned long lastTime = 0; //WLEDMM public:And in setup:
- initDone=true; + // initDone removed (unused)
44-47: Prefer member-initializer list for clarity and correct init orderCleaner and avoids temporary default-inits.
- PovDisplayUsermod(const char *name, bool enabled) { - this->_name=name; - this->enabled = enabled; - } + PovDisplayUsermod(const char *name, bool enabled) + : enabled(enabled) /*, _name(name)*/ {}If you keep _name, include it in the initializer list as shown in the comment.
55-65: Loop guard pattern is fine; consider removing dead debug to keep code tightThe isUpdating guard and heartbeat are fine. The commented Serial println can be dropped to reduce noise.
67-69: Avoid unused-parameter warnings in onEspNowMessageMark parameters as unused to keep builds warning-clean across toolchains.
- bool onEspNowMessage(uint8_t* sender, uint8_t* payload, uint8_t len) override { - return false; - } + bool onEspNowMessage(uint8_t* sender, uint8_t* payload, uint8_t len) override { + (void)sender; (void)payload; (void)len; + return false; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
usermods/pov_display/pov.cpp(1 hunks)usermods/pov_display/pov.h(1 hunks)usermods/pov_display/pov_display.cpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- usermods/pov_display/pov.h
- usermods/pov_display/pov.cpp
🧰 Additional context used
🧬 Code graph analysis (1)
usermods/pov_display/pov_display.cpp (2)
usermods/TetrisAI_v2/TetrisAI_v2.cpp (1)
strip(236-239)usermods/PWM_fan/PWM_fan.cpp (1)
enabled(215-223)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: wled_build / Build Environments (usermods)
🔇 Additional comments (4)
usermods/pov_display/pov_display.cpp (4)
4-4: Effect data string looks consistentThe PROGMEM data string for the effect follows existing usermod patterns. No issues.
49-52: Effect registration looks standardRegistering the effect in setup with id 255 mirrors other usermods (e.g., TetrisAI). No concerns.
71-73: getId implementation is standardNo issues.
76-77: Instantiation and registration look correct; default disabled is a sensible choiceStatic instance + registration pattern matches other usermods. No concerns.
|
Thanks for the update. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
usermods/pov_display/pov_display.cpp (1)
9-24: Nice hardening: null‑guard and case‑insensitive .bmp ends‑with checkThis addresses the previous fragility around strstr(".bmp") and potential null dereference of the current filename. Looks good.
Also applies to: 26-30
🧹 Nitpick comments (8)
usermods/pov_display/README.md (4)
7-16: Add a short “Build/Enable” section so users know how to compile this usermodRight now the README assumes the effect is present. Please add explicit build steps (PlatformIO build_flags, macro) so newcomers can enable it without digging into WLED docs.
Apply this patch near the top (right after the heading) to add a concise section:
@@ ## POV Display usermod +### Build/Enable +- Build WLED with this usermod enabled. If you use PlatformIO, add to your environment in `platformio_override.ini`: + - `build_flags = -D USERMOD_POV_DISPLAY` +- Recompile and flash. After enabling, the “POV Image” effect appears in the effects list. +- No external PNGdec library is required; BMP decoding is built-in. + To get it working:
18-27: Clarify extension handling, main‑segment usage, and absolute pathsThe code accepts “.bmp” case‑insensitively and currently reads the file path from the main segment’s name. Making this explicit prevents confusion in multi‑segment setups.
Apply:
Notes: -- Only 24-bit uncompressed BMP files are supported. +- Only 24-bit uncompressed BMP files are supported. Filenames must end with “.bmp” (case-insensitive; “.BMP” is fine). - The image must fit into ~64 KB of RAM (width × height × 3 bytes, plus row padding to a 4-byte boundary). @@ -- The path must be absolute. +- The path must be absolute. +- The POV Image effect currently reads the image path from the main segment’s name. Use the main segment for POV Image, or ensure other segments are disabled.
20-25: Add the exact BMP row-size formula to help users size images correctlyYou already mention 4‑byte row padding. Including the canonical formula removes guesswork.
-- The image must fit into ~64 KB of RAM (width × height × 3 bytes, plus row padding to a 4-byte boundary). +- The image must fit into ~64 KB of RAM (width × height × 3 bytes, plus row padding to a 4‑byte boundary; rowSize = ((24*width + 31) / 32) * 4).
33-37: Troubleshooting: add a tip for uppercase extensions and main‑segment requirementSmall clarity boost for common pitfalls.
- Nothing displays: verify the file exists at the exact absolute path (case‑sensitive) and is a 24‑bit uncompressed BMP. +- Nothing displays: verify the file exists at the exact absolute path (case‑sensitive), ends with “.bmp” or “.BMP”, and is a 24‑bit uncompressed BMP. Confirm the filename is set on the main segment.usermods/pov_display/pov_display.cpp (4)
9-14: Use the current segment instead of the main segment for better multi‑segment behaviorRight now the effect reads the path from the main segment’s name, which can be surprising if the effect is applied to a non‑main segment. Using the active rendering segment keeps behavior consistent with other WLED effects.
Apply:
- Segment& mainseg = strip.getMainSegment(); - const char* segName = mainseg.name; + Segment& seg = SEGMENT; // use the current segment being rendered + const char* segName = seg.name; if (!segName) { return FRAMETIME; }If you intentionally want “main segment only,” please keep this code and explicitly state that in the README (suggested in my README comment).
32-38: Skip repeated load attempts when the file is missing to reduce FS churnA lightweight existence check avoids hammering the filesystem for non‑existent paths between retries.
// Retry at most twice per second if the image is not yet loaded. if (nowMs - s_lastLoadAttemptMs < 500) return FRAMETIME; s_lastLoadAttemptMs = nowMs; + // Avoid spamming FS if the file doesn't exist + if (!WLED_FS.exists(segName)) return FRAMETIME; s_pov.loadImage(segName);
41-68: Trim unused usermod state (enabled/initDone/lastTime) or make it purposefulThe usermod’s loop is effectively a no‑op and the extra state adds noise. Either remove the unused members and the loop override, or wire “enabled” to a config flag/UM menu.
Minimal cleanup option:
class PovDisplayUsermod : public Usermod { protected: - bool enabled = false; //WLEDMM - const char *_name; //WLEDMM - bool initDone = false; //WLEDMM - unsigned long lastTime = 0; //WLEDMM + const char *_name; //WLEDMM public: @@ void loop() override { - // if usermod is disabled or called during strip updating just exit - // NOTE: on very long strips strip.isUpdating() may always return true so update accordingly - if (!enabled || strip.isUpdating()) return; - - // do your magic here - if (millis() - lastTime > 1000) { - lastTime = millis(); - } + if (strip.isUpdating()) return; }Or, if you prefer to keep the fields for MoonModules conventions, mark them as
[[maybe_unused]]to avoid warnings.
74-75: Constructor flag is misleading; the usermod registers the effect regardlessSince
setup()adds the effect unconditionally andloop()is a no‑op, passingfalseforenableddoesn’t change behavior. Consider setting ittrueor removing the flag to avoid confusion.-static PovDisplayUsermod pov_display("POV Display", false); +static PovDisplayUsermod pov_display("POV Display", true);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
usermods/pov_display/pov_display.gifis excluded by!**/*.gif
📒 Files selected for processing (3)
platformio_override.sample.ini(0 hunks)usermods/pov_display/README.md(1 hunks)usermods/pov_display/pov_display.cpp(1 hunks)
💤 Files with no reviewable changes (1)
- platformio_override.sample.ini
🧰 Additional context used
🧬 Code graph analysis (1)
usermods/pov_display/pov_display.cpp (2)
usermods/TetrisAI_v2/TetrisAI_v2.cpp (1)
strip(236-239)usermods/Animated_Staircase/Animated_Staircase.cpp (1)
enabled(345-352)
🪛 LanguageTool
usermods/pov_display/README.md
[grammar] ~25-~25: There might be a mistake here.
Context: ...rizontally in your editor and try again. - The path must be absolute. ### Requirem...
(QB_NEW_EN)
[grammar] ~30-~30: There might be a mistake here.
Context: ...it, uncompressed (no alpha, no palette). - Sufficient free RAM (~64 KB) for the ima...
(QB_NEW_EN)
[grammar] ~33-~33: There might be a mistake here.
Context: ...r the image buffer. ### Troubleshooting - Nothing displays: verify the file exists...
(QB_NEW_EN)
[grammar] ~34-~34: There might be a mistake here.
Context: ...itive) and is a 24‑bit uncompressed BMP. - Garbled colors or wrong orientation: re‑...
(QB_NEW_EN)
[grammar] ~35-~35: There might be a mistake here.
Context: ... retry the rotation/flip guidance above. - Image too large: reduce width and/or hei...
(QB_NEW_EN)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: wled_build / Build Environments (esp32_eth)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (esp32_wrover)
- GitHub Check: wled_build / Build Environments (esp32c3dev)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_compat)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Environments (nodemcuv2_compat)
- GitHub Check: wled_build / Build Environments (esp32dev)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_160)
I've addressed this comment, added a small gif image to show it working in motion, added some information to the readme, fixed all suggestions from coderabbit :) |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
usermods/pov_display/README.md (1)
7-7: Fix Markdown heading spacing (MD018).Add a space after the hashes.
Apply this diff:
-###How does it work? +### How does it work?
🧹 Nitpick comments (2)
usermods/pov_display/README.md (2)
20-21: Remove duplicate note about absolute path.Line 20 already states case sensitivity and leading “/”. The separate “The path must be absolute.” repeats the same rule.
Apply this diff:
-- The path must be absolute.Also applies to: 33-33
35-39: Call out board/memory support explicitly (ESP32 vs ESP8266).Given the ~64 KB image buffer requirement, clarify recommended/unsupported boards to prevent confusion.
Apply this diff:
### Requirements - 1D rotating LED strip/segment (POV setup). Ensure the segment length equals the number of physical LEDs. +- ESP32 build recommended; ESP8266 usually lacks sufficient free RAM for the ~64 KB image buffer. - BMP image saved as 24‑bit, uncompressed (no alpha, no palette). - Sufficient free RAM (~64 KB) for the image buffer.If ESP8266 can be supported with smaller images, adjust the wording to “ESP32 recommended; ESP8266 works only with very small images” and add the tested max dimensions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
usermods/pov_display/README.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
usermods/pov_display/README.md
[grammar] ~8-~8: There might be a mistake here.
Context: ...f pixel from an image stored on the ESP. It will display the image row after row ...
(QB_NEW_EN)
[grammar] ~10-~10: Ensure spelling is correct
Context: ...image floating in the air thanks to the persistance of vision. It only works with RGB leds,...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~10-~10: There might be a mistake here.
Context: ...air thanks to the persistance of vision. It only works with RGB leds, without gro...
(QB_NEW_EN)
[grammar] ~11-~11: Ensure spelling is correct
Context: ...y works with RGB leds, without grouping nor spacing. The best results are obtained ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~11-~11: There might be a mistake here.
Context: ... RGB leds, without grouping nor spacing. The best results are obtained with high-...
(QB_NEW_EN)
[grammar] ~32-~32: There might be a mistake here.
Context: ...rizontally in your editor and try again. - The path must be absolute. ### Requirem...
(QB_NEW_EN)
[grammar] ~37-~37: There might be a mistake here.
Context: ...it, uncompressed (no alpha, no palette). - Sufficient free RAM (~64 KB) for the ima...
(QB_NEW_EN)
[grammar] ~40-~40: There might be a mistake here.
Context: ...r the image buffer. ### Troubleshooting - Nothing displays: verify the file exists...
(QB_NEW_EN)
[grammar] ~41-~41: There might be a mistake here.
Context: ...itive) and is a 24‑bit uncompressed BMP. - Garbled colors or wrong orientation: re‑...
(QB_NEW_EN)
[grammar] ~42-~42: There might be a mistake here.
Context: ... retry the rotation/flip guidance above. - Image too large: reduce width and/or hei...
(QB_NEW_EN)
🪛 markdownlint-cli2 (0.17.2)
usermods/pov_display/README.md
7-7: No space after hash on atx style heading
(MD018, no-missing-space-atx)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
🔇 Additional comments (1)
usermods/pov_display/README.md (1)
1-6: Nice doc pass; aligns with BMP-only POV flow.Good job documenting the workflow (rotate, upload via /edit, absolute path, 24‑bit BMP) and adding visuals. This addresses prior feedback about PNGdec and clarifies constraints.
|
looks good. |
I'd prefer instead to specify the supported platform(s) in |
I've restrained to esp32, maybe it could work on esp8266 but I don't have the hardware at hand. |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
usermods/pov_display/README.md (2)
7-7: Fix MD018: add a space after the heading hashesMarkdownlint flags “###How does it work?” (missing space). Please insert a space.
-###How does it work? +### How does it work?
10-12: Use consistent terminology: “LED strip” (not “pixel segment”)Earlier feedback clarified we rotate the LED strip, not a segment. Line 10 still says “pixel segment.” Suggest aligning wording.
-If you move the pixel segment at the right speed, you will see the full image floating in the air thanks to the persistence of vision. +If you move the LED strip at the right speed, you will see the full image floating in the air thanks to the persistence of vision.
🧹 Nitpick comments (5)
usermods/pov_display/README.md (5)
17-23: Tighten upload/path bullets and mention the Speed slider for tuningCombine the absolute-path and case-sensitive notes into one bullet to reduce repetition, and mention the Speed slider for practical RPM tuning.
-- Upload a BMP image (24-bit, uncompressed) to the ESP filesystem using the “/edit” URL. +- Upload a BMP image (24‑bit, uncompressed) to the device filesystem via “/edit”. - Select the “POV Image” effect. -- Set the segment name to the absolute filesystem path of the image (e.g., “/myimage.bmp”). -- The path is case-sensitive and must start with “/”. +- Set the segment name to the absolute, case‑sensitive filesystem path (must start with “/”), e.g., “/myimage.bmp”. - Rotate the pixel strip at approximately 20 RPM. -- Tune as needed so that one full revolution maps to the image width (if the image appears stretched or compressed, adjust RPM slightly). +- Tune as needed so that one full revolution maps to the image width. If the image appears stretched or compressed, adjust RPM slightly using the Speed slider.
25-34: Clarify memory math and remove duplicate “absolute path” note
- Provide the exact row-stride formula for BMP padding.
- Drop the duplicate absolute-path reminder already covered above.
Notes: - Only 24-bit uncompressed BMP files are supported. -- The image must fit into ~64 KB of RAM (width × height × 3 bytes, plus row padding to a 4-byte boundary). +- Memory usage ≈ rowStride × height, where rowStride = ((width × 3 + 3) & ~3) bytes (rows are padded to a 4‑byte boundary). Aim to keep total ≤ ~64 KB. - Examples (approximate, excluding row padding): - 128×128 (49,152 bytes) fits. - 160×160 (76,800 bytes) does NOT fit. - 96×192 (55,296 bytes) fits; padding may add a small overhead. -- If the rendered image appears mirrored or upside‑down, rotate 90° the other way or flip horizontally in your editor and try again. -- The path must be absolute. +- If the rendered image appears mirrored or upside‑down, rotate 90° counter‑clockwise or flip horizontally in your editor and try again.
35-39: State supported platforms explicitly (matches PR discussion)Since the usermod targets ESP32 and ESP8266 is not supported/tested, call this out in Requirements to set expectations. Also aligns with reviewer requests.
### Requirements - 1D rotating LED strip/segment (POV setup). Ensure the segment length equals the number of physical LEDs. +- Platform: ESP32 (tested). ESP8266 currently not supported. - BMP image saved as 24‑bit, uncompressed (no alpha, no palette). - Sufficient free RAM (~64 KB) for the image buffer.If this is inaccurate, adjust to reflect the exact set of supported boards.
40-45: Add two common troubleshooting tips (2D mode and wrong file type)Users often have 2D mapping enabled or accidentally upload PNG/JPG. Adding these bullets will prevent confusion.
### Troubleshooting - Nothing displays: verify the file exists at the exact absolute path (case‑sensitive) and is a 24‑bit uncompressed BMP. - Garbled colors or wrong orientation: re‑export as 24‑bit BMP and retry the rotation/flip guidance above. - Image too large: reduce width and/or height until it fits within ~64 KB (see examples). - Path issues: confirm you uploaded the file via the “/edit” URL and can see it in the filesystem browser. + - No output when using 2D: disable 2D matrix mapping; use a 1D segment with grouping=1 and spacing=0. + - Wrong file type: ensure the filename ends with .bmp (PNG/JPG are not supported).
46-48: Optional: add one more safety bulletMinor but helpful.
### Safety - Secure the rotating assembly and keep clear of moving parts. - Balance the strip/hub to minimize vibration before running at speed. + - Wear eye protection and keep cables and clothing clear of the rotating path.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
usermods/pov_display/README.md(1 hunks)usermods/pov_display/bmpimage.cpp(1 hunks)usermods/pov_display/bmpimage.h(1 hunks)usermods/pov_display/library.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- usermods/pov_display/bmpimage.h
- usermods/pov_display/library.json
- usermods/pov_display/bmpimage.cpp
🧰 Additional context used
🪛 LanguageTool
usermods/pov_display/README.md
[grammar] ~8-~8: There might be a mistake here.
Context: ... pixels from an image stored on the ESP. It displays the image row by row at a hi...
(QB_NEW_EN)
[grammar] ~32-~32: There might be a mistake here.
Context: ...rizontally in your editor and try again. - The path must be absolute. ### Requirem...
(QB_NEW_EN)
[grammar] ~37-~37: There might be a mistake here.
Context: ...it, uncompressed (no alpha, no palette). - Sufficient free RAM (~64 KB) for the ima...
(QB_NEW_EN)
[grammar] ~40-~40: There might be a mistake here.
Context: ...r the image buffer. ### Troubleshooting - Nothing displays: verify the file exists...
(QB_NEW_EN)
[grammar] ~41-~41: There might be a mistake here.
Context: ...itive) and is a 24‑bit uncompressed BMP. - Garbled colors or wrong orientation: re‑...
(QB_NEW_EN)
[grammar] ~42-~42: There might be a mistake here.
Context: ... retry the rotation/flip guidance above. - Image too large: reduce width and/or hei...
(QB_NEW_EN)
🪛 markdownlint-cli2 (0.17.2)
usermods/pov_display/README.md
7-7: No space after hash on atx style heading
(MD018, no-missing-space-atx)
🔇 Additional comments (1)
usermods/pov_display/README.md (1)
1-48: Overall: Clear, concise README that matches the BMP-based POV refactorGood job documenting the workflow, constraints (24‑bit BMP, ~64KB), and safety. The examples and step list are practical and align with the PR’s move away from PNGdec. A few small nits below will polish it further.
|
a buffer of 64k is off limits for ESP8266, it has less than 20k of free heap, so restriction is ok. |
|
ok, applied all suggested changes, if everything is fine this is ready to merge for me! |
|
thank you very much for your approval! |
5 participants
There was a compile issue when trying to enable POV Image effect.
This small PR fixes the problem.
Summary by CodeRabbit
New Features
Documentation
Chores / Refactor