Skip to content

Conversation

@DedeHai
Copy link
Collaborator

@DedeHai DedeHai commented Dec 28, 2025

this fixes an ancient copy-paste bug that apparently went under the radar for years. The rabbit found it.
@softhack007 this bug is even present in MM.

Summary by CodeRabbit

  • Bug Fixes
    • Fixed per-segment slider flags parsing in UDP notification packets. Previously, flags were incorrectly overwritten; they are now properly distributed and processed, ensuring segment controls respond correctly to remote commands.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 28, 2025

Walkthrough

The UDP packet parsing function parseNotifyPacket was corrected to properly distribute per-segment slider flag bits across three separate fields (check1, check2, check3) instead of overwriting the same field multiple times.

Changes

Cohort / File(s) Summary
UDP packet parsing fix
wled00/udp.cpp
Corrected bit distribution in parseNotifyPacket: per-segment slider flags from bits 5, 6, 7 of udpIn[31+ofs] now assigned to selseg.check1, selseg.check2, and selseg.check3 respectively, instead of all overwriting selseg.check1.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

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 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'fix FX checkmark sync' accurately summarizes the main change: fixing a copy-paste bug in FX checkmark synchronization parsing that was identified in the UDP packet parsing logic.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1c2cacf and 3815503.

📒 Files selected for processing (1)
  • wled00/udp.cpp
🧰 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/udp.cpp
🧠 Learnings (4)
📓 Common learnings
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: blazoncek
Repo: wled/WLED PR: 5140
File: wled00/data/settings_time.htm:66-76
Timestamp: 2025-12-01T07:01:16.949Z
Learning: In WLED PR #5134, the fix for macros being initialized with the enable bit set only handles new configurations, not existing ones. If there is a bug in timer/macro handling code (e.g., in settings_time.htm), it must be fixed to work correctly for existing configurations as well.
Learnt from: DedeHai
Repo: wled/WLED PR: 4997
File: wled00/e131.cpp:33-44
Timestamp: 2025-10-10T18:34:06.550Z
Learning: In WLED's DDP packet handling (ws.cpp and e131.cpp), only prevent out-of-bounds memory access rather than enforcing DDP spec compliance. Don't check the 1440-byte spec limit—accept out-of-spec packets assuming correct encoding. The bounds check `maxDataIndex = c + numLeds * ddpChannelsPerLed; if (maxDataIndex > dataLen) reject` is sufficient and already covers the timecode flag case (when c=4) without needing separate validation.
📚 Learning: 2025-10-10T18:34:06.550Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4997
File: wled00/e131.cpp:33-44
Timestamp: 2025-10-10T18:34:06.550Z
Learning: In WLED's DDP packet handling (ws.cpp and e131.cpp), only prevent out-of-bounds memory access rather than enforcing DDP spec compliance. Don't check the 1440-byte spec limit—accept out-of-spec packets assuming correct encoding. The bounds check `maxDataIndex = c + numLeds * ddpChannelsPerLed; if (maxDataIndex > dataLen) reject` is sufficient and already covers the timecode flag case (when c=4) without needing separate validation.

Applied to files:

  • wled00/udp.cpp
📚 Learning: 2025-08-21T00:00:17.196Z
Learnt from: willmmiles
Repo: wled/WLED PR: 4859
File: usermods/udp_name_sync/udp_name_sync.cpp:62-70
Timestamp: 2025-08-21T00:00:17.196Z
Learning: In WLED codebase, UDP packet processing code must never rely on null termination being present on the wire and must always validate packet size first. The codebase already has some vulnerabilities where malformed data can crash the software, so avoiding introduction of new ones is a priority.

Applied to files:

  • wled00/udp.cpp
📚 Learning: 2025-05-09T18:43:15.355Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4682
File: wled00/FX.cpp:8997-9005
Timestamp: 2025-05-09T18:43:15.355Z
Learning: In the WLED codebase, SEGMENT.custom3 is always constrained to the range 0-31 and will not exceed this range.

Applied to files:

  • wled00/udp.cpp
🧬 Code graph analysis (1)
wled00/udp.cpp (1)
wled00/data/index.js (1)
  • ofs (2247-2247)
⏰ 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). (20)
  • GitHub Check: wled_build / Build Environments (esp32dev_debug)
  • 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 (esp32dev)
  • GitHub Check: wled_build / Build Environments (esp32_eth)
  • GitHub Check: wled_build / Build Environments (esp32_wrover)
  • GitHub Check: wled_build / Build Environments (esp8266_2m_compat)
  • GitHub Check: wled_build / Build Environments (esp32c3dev)
  • GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
  • GitHub Check: wled_build / Build Environments (nodemcuv2_compat)
  • GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
  • GitHub Check: wled_build / Build Environments (esp32S3_wroom2)
  • GitHub Check: wled_build / Build Environments (esp01_1m_full_160)
  • GitHub Check: wled_build / Build Environments (usermods)
  • GitHub Check: wled_build / Build Environments (esp8266_2m)
  • GitHub Check: wled_build / Build Environments (nodemcuv2_160)
  • GitHub Check: wled_build / Build Environments (esp01_1m_full)
  • GitHub Check: wled_build / Build Environments (esp32c3dev_qio)
  • GitHub Check: wled_build / Build Environments (esp8266_2m_160)
  • GitHub Check: wled_build / Build Environments (nodemcuv2)
🔇 Additional comments (1)
wled00/udp.cpp (1)

336-338: Excellent catch—this critical copy-paste bug is now fixed!

The previous code overwrote selseg.check1 three times (with bits 5, 6, and 7), leaving check2 and check3 uninitialized. This fix correctly distributes the three checkmark flags from the packed byte at udpIn[31+ofs] to their respective fields, matching the sending logic at line 141.

Impact: Without this fix, FX slider checkmarks (check2 and check3) were never synchronized via UDP/ESP-NOW, breaking the intended behavior for multi-device setups.


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

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

Copy link
Member

@softhack007 softhack007 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DedeHai thanks, well spotted :-) yes this looks like a copy&paste error that survived for years.

@DedeHai
Copy link
Collaborator Author

DedeHai commented Dec 28, 2025

Note to self: this also needs 0.15 backport.

@DedeHai DedeHai merged commit 787d8a7 into wled:main Dec 29, 2025
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants