also sync segment name through udp #4482
Conversation
|
Should this be an optional sync attribute, like the existing ones are? |
|
as of now its not optional but I can make it optional if needed. |
|
This is a breaking change. Old devices may crash or behave incorrectly due to message containing unexpected content. |
|
This is to be tested but since the "segment packet size" is sent in the UDP packet and read by the receiver it may be backward compatible. |
|
I'd suggest a new checkbox called Segment Name |
|
Ok, I'll work on this and will push some new commits to reflect this change |
|
one new commit is available with option to enable or disable segment name sync (disabled by default like Segment Options). |
|
What testing have you done so far with regards to @blazoncek 's comments about compatibility testing between systems using your new packet format and without? |
|
Unfortunately didn't had the opportunity to run such tests yet. I will do ASAP |
When you mean sync on both, do you mean making one the sender and one the receiver and then doing the same the other way round? We need to ensure we cover both "forward" and "backwards" compatibility |
|
Ok, as far as I can tell, this modification is both forward and backward compatible. |
this option is disabled by default. When enabled, the segment name is applied when received from another WLED instance.
Liliputech
force-pushed
the
udp_name_sync
branch
from
7f63eb2 to
12bbd20
Compare
|
just push a new commit to fix the issue previously mentioned + rebased on latest main |
|
Do not commit the html .h files as those are built on demand |
|
Please do not use force push |
|
arf, too late. but commits didn't changed, only rebased on main |
|
If you force push it means we have to start the code review process again. Are you able to take a look again @blazoncek ? |
There was a problem hiding this comment.
There are some things that need to be addressed before this is ready for merge.
I am not very fond of segment name syncing but if some users need that, so be it.
I should add that (even though not yet working as intended) ESP-NOW packet needs to be checked if segment name will overflow it.
| @@ -647,18 +647,20 @@ typedef class Receive { | |||
| bool Color : 1; | |||
| bool Effects : 1; | |||
| bool SegmentOptions : 1; | |||
| bool SegmentName : 1; | |||
There was a problem hiding this comment.
That's why there is reserved. Use that if possible.
There was a problem hiding this comment.
I'm sorry but i don't understand how I'm supposed to use reserved... to me it looks like it is a variable to hold the sum of all boolean define above, which is then not to be changed by hand?
There was a problem hiding this comment.
You should have not "resolved" this as you did not address my comment.
reservedis a placeholder for future enhancement as this PR so replace it with SegmentNameoption.
|
Looks like the wrong branch was merged into this branch
|
WalkthroughThe changes add support for handling segment names throughout the system. A new configuration parameter is introduced in the JSON deserialization and setting handler functions. The web interface now includes an additional checkbox to enable or disable the segment name functionality, and the corresponding JavaScript is updated. The UDP notification mechanism has been modified to accommodate the segment name by updating packet size definitions and integrating the name into UDP messages. Additionally, the Receive class has been updated to accept the new segment name parameter along with an associated macro. Changes
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (1)wled00/udp.cpp (4)🔇 Additional comments (2)
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
hi, just solved conflict after merge of #4484 :) hope this one can be merged! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
wled00/data/settings_sync.htm (1)
104-104: Consider improving UI layout for better readability.The current layout could be improved by grouping related options together.
Apply this diff to improve the UI layout:
-<input type="checkbox" name="SO"> Segment options, <input type="checkbox" name="SN"> name, <input type="checkbox" name="SG"> bounds +<input type="checkbox" name="SO"> Segment options (<input type="checkbox" name="SN"> name, <input type="checkbox" name="SG"> bounds)wled00/wled.h (2)
653-654: Consider usingreservedfield for future expansion.The
reservedfield could be used for future features instead of adding new boolean fields.Consider using bit fields from the
reservedfield for future features to maintain backward compatibility.
661-669: Improve parameter ordering in constructor.The parameter order could be more logical by grouping related parameters together.
Apply this diff to improve parameter ordering:
- Receive(bool b, bool c, bool e, bool sO, bool sN, bool sB, bool p) + Receive(bool b, bool c, bool e, bool p, bool sO, bool sN, bool sB) : Brightness(b) , Color(c) , Effects(e) + , Palette(p) , SegmentOptions(sO) , SegmentName(sN) , SegmentBounds(sB) - , Palette(p)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
wled00/cfg.cpp(2 hunks)wled00/data/settings_sync.htm(2 hunks)wled00/set.cpp(1 hunks)wled00/udp.cpp(6 hunks)wled00/wled.h(3 hunks)wled00/xml.cpp(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (18)
- GitHub Check: wled_build / Build Enviornments (esp32_wrover)
- GitHub Check: wled_build / Build Enviornments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Enviornments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Enviornments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Enviornments (esp32c3dev)
- GitHub Check: wled_build / Build Enviornments (lolin_s2_mini)
- GitHub Check: wled_build / Build Enviornments (esp32_eth)
- GitHub Check: wled_build / Build Enviornments (esp32dev_V4)
- GitHub Check: wled_build / Build Enviornments (esp32dev)
- GitHub Check: wled_build / Build Enviornments (esp01_1m_full_compat)
- GitHub Check: wled_build / Build Enviornments (esp8266_2m_compat)
- GitHub Check: wled_build / Build Enviornments (nodemcuv2_compat)
- GitHub Check: wled_build / Build Enviornments (esp01_1m_full_160)
- GitHub Check: wled_build / Build Enviornments (esp8266_2m_160)
- GitHub Check: wled_build / Build Enviornments (nodemcuv2_160)
- GitHub Check: wled_build / Build Enviornments (esp01_1m_full)
- GitHub Check: wled_build / Build Enviornments (esp8266_2m)
- GitHub Check: wled_build / Build Enviornments (nodemcuv2)
🔇 Additional comments (4)
wled00/xml.cpp (1)
420-420: LGTM!The JavaScript setting for the segment name sync checkbox is correctly implemented.
wled00/udp.cpp (1)
7-9:❓ Verification inconclusive
Verify ESP-NOW packet size limit.
The increased UDP segment size could exceed ESP-NOW's 250-byte packet limit. This might cause issues with ESP-NOW communication.
Run this script to check the total packet size:
🏁 Script executed:
#!/bin/bash # Description: Calculate total packet size and verify against ESP-NOW limit echo "UDP segment size = 36 + $(rg -N "WLED_MAX_SEGNAME_LEN" | head -n1)" echo "Total packet size = 41 + (number_of_segments * UDP_SEG_SIZE)"Length of output: 415
Review: Validate computed ESP-NOW packet size remains below 250 bytes.
The current computation of the UDP segment size—
UDP_SEG_SIZE = 36 + WLED_MAX_SEGNAME_LEN—pulls the definition ofWLED_MAX_SEGNAME_LENfrom code inwled00/FX_fcn.cpprather than providing a clear numeric constant. As a result, the total packet size defined byWLEDPACKETSIZE = SEG_OFFSET + (MAX_NUM_SEGMENTS * UDP_SEG_SIZE)remains ambiguous relative to ESP-NOW’s 250-byte limit. The script output confirms that we’re capturing the reference line instead of an evaluated numeric value.
Please manually verify that:
- The actual numeric value of
WLED_MAX_SEGNAME_LENis correctly constrained.- The computed
UDP_SEG_SIZEand resultantWLEDPACKETSIZEdo not exceed ESP-NOW’s limit.If necessary, adjust these constants to ensure that the UDP packet size stays safely within the 250-byte cap.
wled00/cfg.cpp (1)
446-446: LGTM! The new configuration parameter is well-integrated.The addition of
receiveSegmentNamefollows the established pattern for sync receive settings and is correctly placed in the synchronization settings section.wled00/set.cpp (1)
382-382: LGTM! The new setting handler is well-integrated.The addition of the "SN" argument handler follows the established pattern for sync receive settings and is correctly placed in the synchronization settings section.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
I know there is a desire to sync segment names across devices (though I can't find justification) but this implementation is "wasteful" in terms of needed bytes to transmit a name (i.e. if I name segment "A" it will still transmit 32/64 bytes for name instead of only 2 - 'A' and '\0').
| @@ -647,18 +647,20 @@ typedef class Receive { | |||
| bool Color : 1; | |||
| bool Effects : 1; | |||
| bool SegmentOptions : 1; | |||
| bool SegmentName : 1; | |||
There was a problem hiding this comment.
You should have not "resolved" this as you did not address my comment.
reservedis a placeholder for future enhancement as this PR so replace it with SegmentNameoption.
| @@ -338,6 +340,13 @@ static void parseNotifyPacket(const uint8_t *udpIn) { | |||
| selseg.check1 = (udpIn[31+ofs]>>7) & 0x1; | |||
| } | |||
| } | |||
| if (receiveSegmentName) { | |||
| const char* name = (char *) &udpIn[36+ofs]; | |||
There was a problem hiding this comment.
You can pass udpIn[...]directly to setName() no need to check for length or content.
You also do not need to clearName() prior to setName().
Also, indentation is off.
| bool SegmentBounds : 1; | ||
| bool Direct : 1; | ||
| bool Palette : 1; | ||
| uint8_t reserved : 1; | ||
| }; | ||
| }; | ||
| Receive(int i) { Options = i; } | ||
| Receive(bool b, bool c, bool e, bool sO, bool sB, bool p) | ||
| Receive(bool b, bool c, bool e, bool sO, bool sN, bool sB, bool p) |
There was a problem hiding this comment.
Normally one would add new additions at the end to increase compatibility. Just a personal opinion.
| packetSize += UDP_SEG_SIZE; | ||
| s0++; | ||
| } | ||
| // since we sync the name, only one segment can fit in the first packet |
There was a problem hiding this comment.
This is a waste of packet space. Do not forget that the same packet is re-used in ESP-NOW sync.
ESP-NOW packet can only contain 250 bytes so "compression" of data is a must.
This commit solves this issue : #4468
Summary by CodeRabbit