playback of .tpm2#2876
Conversation
|
@constant-flow: I have a question/suggestion concerning the 2D mode coming up with V0.14: So my question: |
|
It uses |
|
I just can tell you what I noticed: |
|
Is the 2d setup something that prohibits this user mod from being merged? I would really like to use it. Especially the segments feature is important for my use case. |
|
I've just tested your mod with my matrix and faced an issue: So I wonder if this a limitation of the ESP32 or WLED or if there is a code issue. (My tpm2 records are fine; I also habe tested it with another tpm2 player where everything is running properly.) I tested it with 0.13.3 and 0.14. |
|
So far the implementation is rather naïve and doesn't do any kind of double/triple buffering. Loading data for so many LEDs is something I didn't test and cannot test (no hardware at hand). All data is read just-in-time from the file. Did you load the data from SD or from flash, I bet SD is slower than the Flash as well. I think if required the performance can be boosted, but at some point there will be definitely a limit by the hardware itself to playback that fast, latest when the animation doesn't fit into memory or when it's too many LEDs to get the info for. |
|
@ElToberino concerning the 2D setup. I located the origin of the issue. My code relies on the |
|
@constant-flow: Thanks for investigating the 2D setup! |
|
Try adjusting WLED FPS in LED settings (advanced part). Start with 25FPS. |
|
@blazoncek: |
|
@ElToberino you may want to try the development branch of this PR I just commited a change that hopefully fixes the 2D Segments issue. More than welcome to get feedback if it works for you. I will start to look into buffering and timing today. |
|
@constant-flow: |
|
@constant-flow: |
|
Hi, @constant-flow! The code of this draft in here also works fine with the current 0.14 beta (of course showing the issue of far too slow speed with my 858 leds.) Best regards! |
|
Hey! This issue 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. |
stale
Bot
added
the
stale
|
It's not stale. |
stale
Bot
removed
the
stale
|
@ElToberino do you remember what you meant with your post. From what i read it is (referencing this commit graph), you mean commit Can you recall what specifically didn't work? Not compiling, No playback, Wrong speed, Wrong display, or simply still too slow? 
Looking at the graph, it seems like I branched off to far off from the working state. Merging the buffering part into the functional version creates way to many conflicts. So probably a rewrite is required. Hooray 😞 |
|
@blazoncek @Aircoookie @ElToberino OK, I was able to manually merge the changes of my buffering code (7ab6cc3) and the code of this PR (0df38e6). I literally didn't do any changes to the code other than the changes from the branches. 🫵 How to test (please do 🙇 )Get my code from here and make sure you are on the Build and DeploySelect and build the You have three option (you probably know 🤓 but I add it for completeness)
Store playback fileSo please test this first you need to upload a TPM2 (e.g. ... after uploading it should look similar to this: 
Create presetThen setup a preset, according to your filename:
Hit Save 
Play recordingClick the preset, to play it Please replyif this works for you, only then, i'd check how much work FSEQ would be to implement. |
@constant-flow can you provide a sample TPM2 file? I have none. |
|
Here is a simple file living on the discord. Please try with a segment defined, as it relies on the segments |
|
@constant-flow I've checked your fork, tpm2 branch and although the code is clean and well written I would recommend different approach. Please exercise a new possibility in 0.14 to create custom effects and use that to:
I would also like to see in the code:
I do not know anything about TPM2 but was wondering if it does not have FPS field? Does it? |
|
Also tested the usermod and more often than not I get a crash. Sometimes it will play something. :) |
|
Hi, @constant-flow, |
|
Having compiled and tested your code I have to report some problems unfortunately: Additional setup information: Tested with two different files recorded with Jinx! (size: 508 kb and 86 kb). WLED configured as a 1-D-strip, matrix patching was done in Jinx!). As it was before in your first PR (before buffering was added) there seem to be problems with a larger number of leds, though 768 leds should be no problem for an esp32 (with tpm2.net via Wifi everything works fine.) |
|
@coderabbitai please review. Additionally we need a few summary lists:
|
🧩 Analysis chainI'll gather the PR code, review the implementation, and search for related PRs before generating my full response. 🏁 Script executed: Length of output: 894 🏁 Script executed: Length of output: 6317 🏁 Script executed: Length of output: 19950 🏁 Script executed: Length of output: 11768 🏁 Script executed: Length of output: 1548 🏁 Script executed: Length of output: 1752 🏁 Script executed: Length of output: 5467 🏁 Script executed: Length of output: 7 🏁 Script executed: Length of output: 5042
PR
|
| # | Title | State | Notes |
|---|---|---|---|
#2292 |
TPM2 Playback from LittleFS and SD | CLOSED | Predecessor to this PR |
#2413 |
FSEQ Playback from LittleFS and SD | OPEN | Direct FSEQ counterpart, open since 2021 |
#3551 |
FSEQ Playback from LittleFS and SD | CLOSED | Rework of #2413, closed 2023 |
#4570 |
added support for fseq files | OPEN | Another FSEQ attempt, open since 2025 |
#5395 |
Add FSEQ usermod (SD playback + FPP) | OPEN | Most recent FSEQ usermod, includes FPP support |
#5417 |
FSEQ Usermod: integrate sd_card usermod + compressed xLights | CLOSED | Extension of #5395, closed 2026 |
#3835 |
Image effect (GIF support) | MERGED | Solved a closely analogous problem (file → LEDs) via the custom effects API |
#5040 |
Improved 1D support for GIF images | MERGED | Follow-up to #3835 |
The merged GIF effect PRs (#3835, #5040) are architecturally the most relevant reference — they solved the same "file → per-frame LED data" problem using the custom effects API (with SEGENV.call == 0 init and segment name as filename), which @blazoncek also recommended for this PR.
🔧 Major Changes Required for Compatibility with Current main
-
Fix
is<JsonInteger>()/is<JsonFloat>()→is<int>()/is<double>()— compile blocker. -
Move all file-scope globals into
PlaybackRecordingsprivate members — the class currently has no state of its own; everything is file-scope globals which will cause ODR violations and make the usermod non-reusable. -
Fix
playbackFile.available()as open-check — replace with abool _isPlayingmember. -
Fix
Segmentcopy +int→uint8_tnarrowing — useSegment& sg = strip.getSegment(segId)with explicit main-segment fallback. -
2D support — iterate
sg.startY..sg.stopYrows andsg.start..sg.stopcolumns, mapping the linear TPM2 pixel index to(x, y)via the segment geometry. TheXY()helper inFX.hcan be used for this. -
Buffered file reads — replace per-pixel
read()calls withplaybackFile.read(buf, count)to reduce I/O overhead for large setups. -
Consider implementing as a custom effect (as
@blazonceksuggested) — this would automatically give segment support, UI integration viaaddToConfig/addToJsonInfo, and correct integration with the effects pipeline, following the pattern established by the GIF effect (#3835). The realtime API approach means playback fights with the normal effects pipeline and requires manualstrip.show()calls.
🧠 Learnings used
Learnt from: softhack007
Repo: wled/WLED PR: 5443
File: wled00/FX_fcn.cpp:1277-1277
Timestamp: 2026-03-24T12:10:32.630Z
Learning: In WLED's `WS2812FX::service()` (wled00/FX_fcn.cpp), the old condition `|| (doShow && seg.mode == FX_MODE_STATIC)` was an **inclusion** guard — it caused FX_MODE_STATIC to render only when another segment had already set doShow=true. It did NOT skip or protect FX_MODE_STATIC from rendering. The PR `#5443` simplification removes this condition, meaning FX_MODE_STATIC now renders on every `timeToShow` tick uniformly. This is intentional and not a regression. Do not flag FX_MODE_STATIC special-casing as missing in future reviews of this function.
Learnt from: willmmiles
Repo: wled/WLED PR: 5462
File: wled00/json.cpp:1189-1198
Timestamp: 2026-03-30T15:32:08.847Z
Learning: In WLED's `respondModeData()` (wled00/json.cpp), the 256-character `lineBuffer` limit for effect descriptor strings (getModeData) is an intentional constraint that matches the same 256-char limit used ~6 other places in the codebase. It is not new to this PR and should not be flagged as a regression. Lifting it requires a future refactor (better type system or dynamic buffer).
Learnt from: KrX3D
Repo: wled/WLED PR: 4585
File: usermods/seven_segment_display_reloaded_v2/seven_segment_display_reloaded_v2.cpp:3-5
Timestamp: 2026-02-18T00:11:02.403Z
Learning: In PR `#4585`, the seven_segment_display_reloaded_v2 usermod intentionally uses `#error` to require MQTT (`#ifdef WLED_DISABLE_MQTT `#error` "This user mod requires MQTT to be enabled." `#endif``) because the integration flow has not been tested or validated without MQTT, and failing fast at compile time is preferred over allowing potentially broken builds until a full MQTT-optional refactor is completed.
Learnt from: BobLoeffler68
Repo: wled/WLED PR: 5440
File: usermods/user_fx/user_fx.cpp:1304-1313
Timestamp: 2026-03-25T07:03:35.475Z
Learning: In WLED `mode_dissolveplus` (usermods/user_fx/user_fx.cpp), using `hw_random16(SEGLEN)` to select the survivor pixel index is correct and safe for this 1D-only effect. The 0xFFFF unmapped-entry concern from the physical bus mapping does not apply to 1D segments because virtual indices 0..SEGLEN-1 always map to valid physical LEDs without gaps. Do not flag this as a bug in future reviews of 1D effects.
Learnt from: DedeHai
Repo: wled/WLED PR: 4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR `#4798`, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-12-28T14:06:48.772Z
Learning: In WLED post-commit ee9ac947, the rendering pipeline uses per-segment buffers and per-pixel bus updates. Unmapped (0xFFFF) mapping entries are now skipped in WS2812FX::show() (no “clear to black”), which can leave physical gap LEDs with stale/random colors unless they are explicitly cleared. This is a behavior change from pre-0.16 where a full physical buffer was effectively refreshed each frame.
Learnt from: softhack007
Repo: wled/WLED PR: 5443
File: wled00/FX_fcn.cpp:1277-1277
Timestamp: 2026-03-24T12:13:21.670Z
Learning: In WLED's `WS2812FX::service()` (wled00/FX_fcn.cpp), `seg.freeze` means "do not run the effect function (_mode[seg.mode]())" — it does NOT mean "skip show()". A frozen segment's pixel buffer can still be updated externally (e.g., realtime control or single-pixel JSON API). `strip.trigger()` is the primary mechanism to flush those externally written pixels to the LEDs on the next service tick. Therefore, frozen segments must remain part of the `doShow`/`show()` path, and it is architecturally wrong to exclude frozen segments from `doShow`. Do not suggest skipping frozen segments from the show path in future reviews.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-03-25T12:25:11.610Z
Learning: In WLED's `Segment::startTransition()` (wled00/FX_fcn.cpp:284), when `isInTransition()` is true and `_t->_oldSegment` already exists, the function silently returns without updating `_t->_start` or `_t->_bri`. This causes a bug where rapid successive on/off toggles during a non-FADE blending transition (e.g., fairy dust) leave the transition clock stale: by the time of the second re-trigger, elapsed time may already exceed `_dur`, so `updateTransitionProgress()` sets `_progress = 0xFFFF` immediately on the next service tick and `stopTransition()` fires — the blending effect never plays. The fix is to always reset `_t->_start = millis()`, `_t->_dur = dur`, and `_t->_bri = currentBri()` (current visible brightness) in the `isInTransition()` branch, regardless of whether `_oldSegment` exists.
Learnt from: DedeHai
Repo: wled/WLED PR: 5464
File: wled00/FX_fcn.cpp:1699-1701
Timestamp: 2026-04-09T07:26:14.510Z
Learning: In WLED (wled00/util.cpp), `allocate_buffer()` processes `BFRALLOC_NOBYTEACCESS` in an `if` with an `else` that covers all other flags. On classic ESP32, this means `BFRALLOC_ENFORCE_PSRAM` and `BFRALLOC_PREFER_PSRAM` are silently ignored when `BFRALLOC_NOBYTEACCESS` is also set — IRAM (MALLOC_CAP_32BIT) is always tried first. Combining `BFRALLOC_CLEAR | BFRALLOC_NOBYTEACCESS` is unsafe: the allocator's internal `memset` at util.cpp:901 will perform byte-level access on a potentially IRAM-resident buffer. This affects `Segment::pixels` in `setGeometry()` (FX.h) and `_pixels` in `finalizeInit()` (FX_fcn.cpp). Additionally, `memcpy` on `Segment::pixels` in the copy constructor and copy assignment (FX_fcn.cpp lines ~73 and ~116) is also a byte-level violation when the buffer is in IRAM. The safe pattern is element-wise 32-bit access (for loops over `uint32_t*`) rather than `memset`/`memcpy` for all `BFRALLOC_NOBYTEACCESS` buffers.
Learnt from: softhack007
Repo: wled/WLED PR: 4838
File: platformio.ini:149-150
Timestamp: 2026-03-29T16:47:56.452Z
Learning: In WLED PR `#4838` (ESP-IDF V5 branch, platformio.ini): The original FastLED library has been replaced with a custom `fastled-slim` fork. As a result, the `[v5_pioarduino_workaround]` section (containing `lib_archive = yes`) and the commented-out `post:pio-scripts/fastled_cxx_workaround.py` extra script are intentional dead code pending removal after back-to-back testing. Do NOT flag `lib_archive = yes` in `[v5_pioarduino_workaround]` as unreferenced or suggest moving it to active env blocks — the entire section is slated for removal.
✅ Actions performed
Review triggered.
Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.
WalkthroughThis pull request introduces a new "Playback recordings" usermod for WLED that enables playback of LED animation recordings stored on LittleFS and optionally SD cards. The feature includes documentation, the usermod implementation with file handling and frame decoding logic, and build system integration. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client/API
participant Preset as Preset System
participant Usermod as PlaybackRecordings<br/>Usermod
participant FileSystem as LittleFS/SD
participant Strip as LED Strip
Client->>Preset: Set preset with playback config
Preset->>Usermod: readFromJsonState()
Usermod->>Usermod: Parse JSON (file, repeat, fps, seg)
Usermod->>FileSystem: Open recording file
loop Playback Loop (realtimeMode = PLAYBACK)
Usermod->>FileSystem: Read TPM2 frame packet
FileSystem-->>Usermod: Frame data (RGB bytes)
Usermod->>Usermod: Decode frame & write pixels
Usermod->>Strip: setRealtimePixel() + show()
alt Frame delay elapsed
Usermod->>Usermod: Advance to next frame
end
alt End of file
alt Repeat enabled
Usermod->>FileSystem: seek(0)
else Repeat exhausted/disabled
Usermod->>FileSystem: Close file
Usermod->>Usermod: Exit realtime mode
Usermod->>Strip: Clear segment
end
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches⚔️ Resolve merge conflicts
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
added
the
documentation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
usermods/playback_recordings/usermod_playback_recordings.h (1)
77-98: Move constant JSON keys and format names out of heapStrings.These are fixed literals, but each instance allocates RAM for them anyway. In a usermod, this is easy to avoid with
static const char[] PROGMEM, and it matches the project convention.As per coding guidelines,
usermods/**/*.{cpp,h,hpp}: Store repeated strings as static const char[] PROGMEM in usermods.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usermods/playback_recordings/usermod_playback_recordings.h` around lines 77 - 98, Replace the heap-allocated String members (jsonKeyPlayback, jsonKeyFilePath, jsonKeyPlaybackSegment, jsonKeyPlaybackSegmentId, jsonKeyPlaybackRepeats, jsonKeyFramesPerSecond, formatTpm2) and the playback_formats array with project-convention flash-stored literals: declare them as static const char[] PROGMEM (and for the array use static const char* PROGMEM playback_formats[]). Update all usages in PlaybackRecordings methods to read from PROGMEM (e.g., use FPSTR/strcmp_P or construct a runtime String only when needed) so no RAM is consumed by these constant keys/format names. Ensure names remain the same for easy locating (jsonKeyPlayback, jsonKeyFilePath, jsonKeyPlaybackSegment, jsonKeyPlaybackSegmentId, jsonKeyPlaybackRepeats, jsonKeyFramesPerSecond, formatTpm2, playback_formats).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@usermods/playback_recordings/readme.md`:
- Around line 6-10: Change the README instructions to tell contributors to
update platformio_override.ini (not platformio.ini) and to add the USERMOD flag
to the configuration's build_flags there; update the example fenced snippet to
be labeled as ini and show adding -D USERMOD_PLAYBACK_RECORDINGS under
build_flags so readers and markdownlint are satisfied and WLED's rule about not
editing the main platformio.ini is followed.
In `@usermods/playback_recordings/usermod_playback_recordings.h`:
- Around line 66-77: Reset currentPlaybackFormat to
PLAYBACK_FORMAT::FORMAT_UNKNOWN before scanning a new filename and ensure the
format-lookup loop doesn't read past playback_formats; either make
COUNT_PLAYBACK_FORMATS match the actual number of elements in playback_formats
or, better, iterate using the array's real size (e.g.,
sizeof(playback_formats)/sizeof(playback_formats[0])) when checking entries.
Update the code paths that detect formats (references: currentPlaybackFormat,
playback_formats, PLAYBACK_FORMAT, COUNT_PLAYBACK_FORMATS) so detection always
starts from FORMAT_UNKNOWN and loop bounds are derived from the actual array
length to avoid out-of-bounds access.
- Around line 63-64: Define a distinct realtime mode constant for playback
instead of aliasing REALTIME_MODE_PLAYBACK to REALTIME_MODE_GENERIC; add a new
unique enum/define (e.g., REALTIME_MODE_PLAYBACK) in the same place where
realtime modes are declared and replace all usages that currently rely on the
alias—specifically update the definition (remove the `#define`
REALTIME_MODE_PLAYBACK REALTIME_MODE_GENERIC) and change callers like
file_handlePlayPlayback() and any realtimeLock(..., REALTIME_MODE_PLAYBACK)
invocations so they reference the new dedicated mode constant so playback
sessions are isolated from GENERIC realtime sessions.
- Around line 171-179: The code reads fps from jsonPlaybackFps and computes
newMsDelay without validating its value, which allows fps==0 or negative values
to cause divide-by-zero or huge wrap-around; update the jsonPlaybackFps handling
in the block that sets msFrameDelay (using symbols jsonPlaybackEntry,
jsonKeyFramesPerSecond, jsonPlaybackFps, fps, newMsDelay, PLAYBACK_FRAME_DELAY,
msFrameDelay) to reject non-positive fps (e.g. if fps <= 0) before computing
round(1000.f / fps), log a warning/debug message about the invalid fps, and
leave msFrameDelay at PLAYBACK_FRAME_DELAY (or previously set value) when the
value is invalid.
- Around line 405-416: The code treats packetLength as a pixel count but TPM2
packetLength is bytes; change tpm2_processFrameData to compute pixels =
packetLength / channels (where channels = 3 for RGB TPM2 frames), validate that
packetLength % channels == 0 (handle/log/ignore trailing bytes if not
divisible), then set lastLed = min(playbackLedStop, uint16_t(playbackLedStart +
pixels)) and loop pixels reading one pixel's worth of bytes per LED via
tpm2_GetNextColorData and calling setRealtimePixel; keep the final
tpm2_SkipUntilEndOfPacket() call. Ensure you reference packetLength, channels
(use a constant or literal 3), tpm2_GetNextColorData, setRealtimePixel,
playbackLedStart and playbackLedStop when making the change.
---
Nitpick comments:
In `@usermods/playback_recordings/usermod_playback_recordings.h`:
- Around line 77-98: Replace the heap-allocated String members (jsonKeyPlayback,
jsonKeyFilePath, jsonKeyPlaybackSegment, jsonKeyPlaybackSegmentId,
jsonKeyPlaybackRepeats, jsonKeyFramesPerSecond, formatTpm2) and the
playback_formats array with project-convention flash-stored literals: declare
them as static const char[] PROGMEM (and for the array use static const char*
PROGMEM playback_formats[]). Update all usages in PlaybackRecordings methods to
read from PROGMEM (e.g., use FPSTR/strcmp_P or construct a runtime String only
when needed) so no RAM is consumed by these constant keys/format names. Ensure
names remain the same for easy locating (jsonKeyPlayback, jsonKeyFilePath,
jsonKeyPlaybackSegment, jsonKeyPlaybackSegmentId, jsonKeyPlaybackRepeats,
jsonKeyFramesPerSecond, formatTpm2, playback_formats).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e7cdbf5c-ce74-432a-9c23-bdc417eb083e
📒 Files selected for processing (4)
usermods/playback_recordings/readme.mdusermods/playback_recordings/usermod_playback_recordings.hwled00/const.hwled00/usermods_list.cpp
| - modify `platformio.ini` and add to the `build_flags` of your configuration the following | ||
|
|
||
| ``` | ||
| -D USERMOD_PLAYBACK_RECORDINGS | ||
| ``` |
There was a problem hiding this comment.
Point builders to platformio_override.ini and label the snippet as ini.
For a regular usermod, documenting edits to the main platformio.ini sends contributors down a path WLED generally does not want, and the unlabeled fence here is already tripping markdownlint. Please switch the instructions to platformio_override.ini (or an equivalent override file) and mark the block as ini.
Based on learnings, in WLED, the general rule is that usermod PRs must NOT modify the main platformio.ini and should use platformio_override.ini instead.
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 8-8: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@usermods/playback_recordings/readme.md` around lines 6 - 10, Change the
README instructions to tell contributors to update platformio_override.ini (not
platformio.ini) and to add the USERMOD flag to the configuration's build_flags
there; update the example fenced snippet to be labeled as ini and show adding -D
USERMOD_PLAYBACK_RECORDINGS under build_flags so readers and markdownlint are
satisfied and WLED's rule about not editing the main platformio.ini is followed.
| //TODO: maybe add as custom RT_MODE in `const.h` and `json.cpp` | ||
| #define REALTIME_MODE_PLAYBACK REALTIME_MODE_GENERIC |
There was a problem hiding this comment.
Give playback its own realtime mode.
Aliasing playback to REALTIME_MODE_GENERIC means file_handlePlayPlayback() will also run during unrelated generic realtime sessions, and realtimeLock(..., REALTIME_MODE_PLAYBACK) cannot distinguish playback from those sources. That makes cross-feature interference unavoidable here.
Also applies to: 335-342, 418-420
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@usermods/playback_recordings/usermod_playback_recordings.h` around lines 63 -
64, Define a distinct realtime mode constant for playback instead of aliasing
REALTIME_MODE_PLAYBACK to REALTIME_MODE_GENERIC; add a new unique enum/define
(e.g., REALTIME_MODE_PLAYBACK) in the same place where realtime modes are
declared and replace all usages that currently rely on the alias—specifically
update the definition (remove the `#define` REALTIME_MODE_PLAYBACK
REALTIME_MODE_GENERIC) and change callers like file_handlePlayPlayback() and any
realtimeLock(..., REALTIME_MODE_PLAYBACK) invocations so they reference the new
dedicated mode constant so playback sessions are isolated from GENERIC realtime
sessions.
| enum PLAYBACK_FORMAT { | ||
| TPM2=0, | ||
| FSEQ, | ||
| FORMAT_UNKNOWN, | ||
| COUNT_PLAYBACK_FORMATS | ||
| }; | ||
|
|
||
| enum PLAYBACK_FORMAT currentPlaybackFormat = PLAYBACK_FORMAT::FORMAT_UNKNOWN; | ||
|
|
||
| int32_t playbackRepeats = PLAYBACK_REPEAT_LOOP; | ||
| uint32_t msFrameDelay = PLAYBACK_FRAME_DELAY; // time between frames | ||
| static const String playback_formats[] = {"tpm2",/*, "fseq"*/" "}; |
There was a problem hiding this comment.
Reset format detection and stop iterating past playback_formats.
currentPlaybackFormat is never reset before scanning a new file name, so an unsupported extension after a previous TPM2 playback will keep the old format. On top of that, COUNT_PLAYBACK_FORMATS is 3 while playback_formats only has 2 elements, so the last iteration reads past the array.
🐛 Proposed fix
- for(int i=0; i<PLAYBACK_FORMAT::COUNT_PLAYBACK_FORMATS; i++){
- if(pathToPlayback.endsWith(playback_formats[i])) {
- currentPlaybackFormat = (PLAYBACK_FORMAT) i;
+ currentPlaybackFormat = PLAYBACK_FORMAT::FORMAT_UNKNOWN;
+ for (uint8_t i = 0; i < (sizeof(playback_formats) / sizeof(playback_formats[0])); i++) {
+ if (pathToPlayback.endsWith(playback_formats[i])) {
+ currentPlaybackFormat = static_cast<PLAYBACK_FORMAT>(i);
break;
}
}Also applies to: 146-152
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@usermods/playback_recordings/usermod_playback_recordings.h` around lines 66 -
77, Reset currentPlaybackFormat to PLAYBACK_FORMAT::FORMAT_UNKNOWN before
scanning a new filename and ensure the format-lookup loop doesn't read past
playback_formats; either make COUNT_PLAYBACK_FORMATS match the actual number of
elements in playback_formats or, better, iterate using the array's real size
(e.g., sizeof(playback_formats)/sizeof(playback_formats[0])) when checking
entries. Update the code paths that detect formats (references:
currentPlaybackFormat, playback_formats, PLAYBACK_FORMAT,
COUNT_PLAYBACK_FORMATS) so detection always starts from FORMAT_UNKNOWN and loop
bounds are derived from the actual array length to avoid out-of-bounds access.
| //adjust the framerate if defined | ||
| msFrameDelay = PLAYBACK_FRAME_DELAY; | ||
| JsonVariant jsonPlaybackFps = jsonPlaybackEntry[jsonKeyFramesPerSecond]; | ||
| if (jsonPlaybackFps) { | ||
| if(jsonPlaybackFps.is<JsonInteger>() || jsonPlaybackFps.is<JsonFloat>()) { | ||
| float fps = jsonPlaybackFps; | ||
| uint32_t newMsDelay = round(1000.f / fps); | ||
| DEBUG_PRINTF("[%s] framerate %d -> delay between frames: %d\n", _name, fps, newMsDelay); | ||
| msFrameDelay = newMsDelay; |
There was a problem hiding this comment.
Reject non-positive fps values before computing the delay.
fps = 0 divides by zero here, and negative values wrap into a huge uint32_t delay after the cast. A malformed preset can wedge playback timing instead of falling back to the default.
🛡️ Proposed fix
msFrameDelay = PLAYBACK_FRAME_DELAY;
JsonVariant jsonPlaybackFps = jsonPlaybackEntry[jsonKeyFramesPerSecond];
if (jsonPlaybackFps) {
- if(jsonPlaybackFps.is<JsonInteger>() || jsonPlaybackFps.is<JsonFloat>()) {
- float fps = jsonPlaybackFps;
- uint32_t newMsDelay = round(1000.f / fps);
- DEBUG_PRINTF("[%s] framerate %d -> delay between frames: %d\n", _name, fps, newMsDelay);
- msFrameDelay = newMsDelay;
+ if (jsonPlaybackFps.is<JsonInteger>() || jsonPlaybackFps.is<JsonFloat>()) {
+ float fps = jsonPlaybackFps;
+ if (fps > 0.0f) {
+ uint32_t newMsDelay = round(1000.0f / fps);
+ msFrameDelay = newMsDelay;
+ } else {
+ DEBUG_PRINTF("[%s] '%s' must be > 0\n", _name, jsonKeyFramesPerSecond.c_str());
+ }
} else {
DEBUG_PRINTF("[%s] %s either as integer or float, though delay will be in ms and integer always\n", _name, jsonKeyFramesPerSecond.c_str());
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@usermods/playback_recordings/usermod_playback_recordings.h` around lines 171
- 179, The code reads fps from jsonPlaybackFps and computes newMsDelay without
validating its value, which allows fps==0 or negative values to cause
divide-by-zero or huge wrap-around; update the jsonPlaybackFps handling in the
block that sets msFrameDelay (using symbols jsonPlaybackEntry,
jsonKeyFramesPerSecond, jsonPlaybackFps, fps, newMsDelay, PLAYBACK_FRAME_DELAY,
msFrameDelay) to reject non-positive fps (e.g. if fps <= 0) before computing
round(1000.f / fps), log a warning/debug message about the invalid fps, and
leave msFrameDelay at PLAYBACK_FRAME_DELAY (or previously set value) when the
value is invalid.
| void tpm2_processFrameData() | ||
| { | ||
| uint16_t packetLength = tpm2_getNextPacketLength(); // opt-TODO maybe stretch recording to available leds | ||
| uint16_t lastLed = min(playbackLedStop, uint16_t(playbackLedStart + packetLength)); | ||
|
|
||
| for (uint16_t i = playbackLedStart; i < lastLed; i++) | ||
| { | ||
| tpm2_GetNextColorData(colorData); | ||
| setRealtimePixel(i, colorData[0], colorData[1], colorData[2], colorData[3]); | ||
| } | ||
|
|
||
| tpm2_SkipUntilEndOfPacket(); |
There was a problem hiding this comment.
Treat the TPM2 length field as bytes, not LED count.
The TPM2 frame size is the number of payload bytes, and RGB TPM2 frames use 3 data bytes per LED. This loop uses packetLength directly as a pixel count, so a 150-byte frame gets decoded as 150 LEDs / 450 color bytes and desynchronizes the stream. Decode packetLength / colorChannels pixels after validating divisibility. (gist.github.com)
🐛 Proposed fix
void tpm2_processFrameData()
{
- uint16_t packetLength = tpm2_getNextPacketLength(); // opt-TODO maybe stretch recording to available leds
- uint16_t lastLed = min(playbackLedStop, uint16_t(playbackLedStart + packetLength));
+ uint16_t packetLength = tpm2_getNextPacketLength();
+ if ((packetLength % colorChannels) != 0) {
+ DEBUG_PRINTF("[%s] Invalid TPM2 payload length: %u\n", PlaybackRecordings::_name, packetLength);
+ tpm2_SkipUntilEndOfPacket();
+ return;
+ }
+
+ uint16_t pixelCount = packetLength / colorChannels;
+ uint16_t lastLed = min(playbackLedStop, uint16_t(playbackLedStart + pixelCount));
for (uint16_t i = playbackLedStart; i < lastLed; i++)
{
tpm2_GetNextColorData(colorData);
setRealtimePixel(i, colorData[0], colorData[1], colorData[2], colorData[3]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@usermods/playback_recordings/usermod_playback_recordings.h` around lines 405
- 416, The code treats packetLength as a pixel count but TPM2 packetLength is
bytes; change tpm2_processFrameData to compute pixels = packetLength / channels
(where channels = 3 for RGB TPM2 frames), validate that packetLength % channels
== 0 (handle/log/ignore trailing bytes if not divisible), then set lastLed =
min(playbackLedStop, uint16_t(playbackLedStart + pixels)) and loop pixels
reading one pixel's worth of bytes per LED via tpm2_GetNextColorData and calling
setRealtimePixel; keep the final tpm2_SkipUntilEndOfPacket() call. Ensure you
reference packetLength, channels (use a constant or literal 3),
tpm2_GetNextColorData, setRealtimePixel, playbackLedStart and playbackLedStop
when making the change.
softhack007
added
the
needs_rework
6 participants
Enable playback of animation recordings via presets (tpm2 right now, extensible to other formats).
Animations can be stored in flash
/editor on SD card (see other upcoming PR)Summary by CodeRabbit
New Features
Documentation