fix timing issue when changing 1D <-> 2D credits to @blazoncek

[DedeHai]

this fixes #4911 and #4800

Summary by CodeRabbit

Release Notes

• Bug Fixes
  • Enhanced synchronization during LED matrix geometry reconfiguration to prevent race conditions
  • Improved stability of LED effect drawing during matrix setup and configuration changes

[coderabbitai]

Walkthrough

The changes introduce synchronization mechanisms to prevent race conditions during LED matrix geometry updates. Modified waitForIt timeout, added early-return guards to block segment mutations during active frame servicing, and wrapped geometry changes with suspend/waitForIt/resume sequences in configuration handling.

Changes

Cohort / File(s)	Summary
2D Matrix Setup Refactoring wled00/FX_2Dfcn.cpp	Removed suspend() and resume() calls from setUpMatrix(); added explanatory comments indicating effect drawing suspension must occur externally or from loop context.
Segment Operations Synchronization wled00/FX_fcn.cpp	Increased waitForIt() timeout to 2 × getFrameTime() + 100; added early-return guards (isServicing() checks) in resetSegments, makeAutoSegments, and fixInvalidSegments; removed suspend/waitForIt from deserializeMap with added warning comments.
Configuration Synchronization wled00/set.cpp	Added suspend/waitForIt/resume synchronization around map deserialization and auto-segmentation during 2D matrix setup and general settings changes to prevent concurrent effect execution during geometry reinitialization.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

The changes span three files with related but distinct synchronization modifications targeting race condition prevention. Logic density is moderate—understanding concurrency control, frame servicing state checks, and timeout behavior adjustments is required. Changes are somewhat heterogeneous, requiring separate reasoning for each file's modifications.

Possibly related PRs

• PR Bugfix for brightness factor upon save: fixes #4824 #4827: Both PRs modify WS2812FX segment and matrix setup synchronization code paths, including waitForIt() logic, resetSegments/makeAutoSegments/fixInvalidSegments guards, and setUpMatrix()/deserializeMap handling.

Suggested reviewers

• blazoncek
• willmmiles

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check	✅ Passed	The pull request title "fix timing issue when changing 1D <-> 2D credits to @blazoncek" is directly related to the main change in the changeset. The PR addresses race condition and synchronization problems when switching between 1D and 2D matrix modes by adding guards and suspending/resuming effects at critical points. The title accurately captures the core issue (timing) and the specific context (1D to 2D conversion), making it clear and specific enough for teammates reviewing the history to understand the primary objective.
Linked Issues Check	✅ Passed	The code changes align with the requirements from issue #4911. The PR addresses the crash that occurs when saving a 2D configuration with panel size exceeding configured LED count by introducing proper synchronization mechanisms. In set.cpp, synchronization is added around LED matrix/ledmap geometry changes during both 2D matrix setup and general settings changes, ensuring effects are suspended before geometry reinitialization. Additional guards in FX_fcn.cpp (isServicing() checks) prevent segment mutations during frame servicing, and timeout handling is improved to better manage edge cases. These changes collectively prevent the race conditions that previously caused crashes during 1D to 2D transitions.
Out of Scope Changes Check	✅ Passed	All code changes in the pull request are directly in scope and related to fixing the timing/race condition issue. The modifications in FX_2Dfcn.cpp, FX_fcn.cpp, and set.cpp are focused on preventing concurrent access to critical sections during geometry reinitialization and improving synchronization during 1D to 2D transitions. No extraneous refactoring, unrelated bug fixes, or off-topic changes are present. The addition of explanatory comments and timing guard mechanisms are all part of the coherent effort to resolve the crash issue.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

wled00/set.cpp (1)

818-824: Synchronization pattern correctly applied for matrix geometry changes.

The suspend/waitForIt/resume sequence properly prevents effects from running during matrix reinitialization, aligning with the fix for issue #4911. The new code (lines 818-824) is correct.

The existing code at lines 911-913 uses only suspend/resume without waitForIt for single segment geometry changes. While this remains pre-existing code, applying the same waitForIt pattern to all geometry operations would improve consistency. However, this is an optional improvement since setGeometry calls affect individual segments rather than global matrix structure.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b60313e and 50c0f41.

📒 Files selected for processing (3)

• wled00/FX_2Dfcn.cpp (2 hunks)
• wled00/FX_fcn.cpp (4 hunks)
• wled00/set.cpp (1 hunks)

🧰 Additional context used

📓 Path-based instructions (1)

wled00/**/*.cpp

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Use 2-space indentation for C++ source files (.cpp)

Files:

• wled00/set.cpp
• wled00/FX_fcn.cpp
• wled00/FX_2Dfcn.cpp

🧠 Learnings (1)

📚 Learning: 2025-08-26T11:51:21.817Z

Learnt from: DedeHai
PR: wled/WLED#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.

Applied to files:

• wled00/FX_fcn.cpp
• wled00/FX_2Dfcn.cpp

🧬 Code graph analysis (1)

wled00/FX_fcn.cpp (2)

usermods/Analog_Clock/Analog_Clock.cpp (1)

• millis (151-155)

usermods/word-clock-matrix/word-clock-matrix.cpp (1)

• millis (294-321)

⏰ 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). (15)

• GitHub Check: wled_build / Build Environments (usermods)
• GitHub Check: wled_build / Build Environments (lolin_s2_mini)
• GitHub Check: wled_build / Build Environments (esp32_wrover)
• GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
• GitHub Check: wled_build / Build Environments (esp32c3dev)
• GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
• GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
• GitHub Check: wled_build / Build Environments (esp8266_2m_160)
• GitHub Check: wled_build / Build Environments (esp32_eth)
• GitHub Check: wled_build / Build Environments (nodemcuv2_compat)
• GitHub Check: wled_build / Build Environments (esp32dev)
• GitHub Check: wled_build / Build Environments (esp8266_2m_compat)
• GitHub Check: wled_build / Build Environments (esp01_1m_full_160)
• GitHub Check: wled_build / Build Environments (esp01_1m_full_compat)
• GitHub Check: wled_build / Build Environments (nodemcuv2)

🔇 Additional comments (7)

wled00/FX_2Dfcn.cpp (2)

20-20: Clear warning about concurrency requirements.

The WARNING comment appropriately documents that setUpMatrix() must either be called from loop context or with explicit suspension, preventing misuse that could trigger the race condition addressed in #4911.

---

52-54: LGTM: Helpful documentation.

The comment clarifies the relationship between panel layout dimensions and actual LED count, explaining the custom mapping and LED counting behavior.

wled00/FX_fcn.cpp (5)

1684-1691: Timeout increased to handle edge cases, but acknowledged as incomplete.

Doubling the wait window from 1*getFrameTime() to 2*getFrameTime() improves tolerance for slow effects at high FPS settings. However, the inline TODO referencing #4779 indicates this remains a partial fix rather than a complete solution to the timeout calculation.

The expanded timeout reduces false-positive timeouts when effects exceed a single frame budget, which aligns with the broader race-condition fixes in this PR.

---

1817-1821: Defensive guard prevents mutations during servicing.

The isServicing() check provides defense-in-depth: even if a caller forgets to suspend()/waitForIt() before invoking resetSegments(), this guard prevents segment corruption.

---

1828-1829: Guard in makeAutoSegments mirrors resetSegments pattern.

Consistent with the guard added to resetSegments() at line 1821, preventing segment mutation while the strip is actively rendering.

---

1900-1901: Guard in fixInvalidSegments completes the defensive trio.

All three segment-mutation helpers (resetSegments, makeAutoSegments, fixInvalidSegments) now reject calls during active servicing, preventing the race conditions reported in #4911 and #4800.

---

1964-1964: Warning documents suspension requirement for deserializeMap.

Matches the warning added to setUpMatrix() in FX_2Dfcn.cpp, line 20. Both functions modify strip geometry and require proper synchronization.