Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add number of UDP retries #2830

Merged
merged 11 commits into from
Oct 18, 2022
Merged

Add number of UDP retries #2830

merged 11 commits into from
Oct 18, 2022

Conversation

Squall-DA
Copy link
Contributor

Add sync option for number of UDP messages to improve WLED sync reliability. Value can be configured from 1 - 255 messages. Anywhere between 5 and 10 messages seems to create a reliable, well synced system on my network. This takes the place of the notify twice option.

This should help mitigate Issue #2112

@blazoncek
Copy link
Collaborator

Hi, thakns for the PR. I do have a few questions and general comment, though.

Have you tried/tested this with 60FPS and 32 segments (with segment options syncing enabled, possibly with 4096 LEDs defined)? How much does it add to FPS drop (if any) or other performance hit?

A side note: UDP is inherently unreliable, especially with WiFi. Its intention is low latency. Adding multiple retransmissions will only add to the latency and may not solve bad WiFi coverage/signal strength issues. I do agree that multiple retransmissions will increase the chance of transmitting a packet successfully but it may still not be 100% successful.

If you want 100% successful delivery utilise websockets (you will need to implement client on WLED side yourself). Be mindful that websockets consume precious RAM and WLED currently only supports up to 8 clients on ESP32 (3 on ESP8266).

Please re-base your PR for 0_14 branch to prevent any conflicts with upcoming version.

Aircoookie and others added 6 commits October 16, 2022 08:50
* Update

* update readme file

* readme update

* Update readme.md

* Update readme.md

* Update readme.md

* Update readme.md

* Update platformio.ini
Add a configurable number of retries to the UDP WLED sync function.
@Squall-DA Squall-DA changed the base branch from main to 0_14 October 16, 2022 13:13
@Squall-DA
Copy link
Contributor Author

Squall-DA commented Oct 16, 2022

@blazoncek I went through and rebased my branch and pull request and performed those tests you suggested. I do not see performance hit between 1 UDP message or 200 UDP sync messages. I would expect the performance hit to be minimal as the sync messages are only set when changing color and effect related settings and not a constant sync (at least to my understanding of the code). The only minor issue I noticed is that separate WLED devices may not be perfectly synced. I believe for most use cases of WLED sync not perfectly synced is still better than not displaying the correct color/effect (at least it is in mine).

EDIT:
I do agree that this is more of a band aid. I was going to look into using web sockets or some alternative method in the future.

Copy link
Collaborator

@blazoncek blazoncek left a comment

Choose a reason for hiding this comment

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

Other than what I commented I agree with the PR.
@Aircoookie I propose to merge it into 0_14 after the changes are made.

Change default number of retries
Fix migration from old settings
Copy link
Collaborator

@blazoncek blazoncek left a comment

Choose a reason for hiding this comment

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

Please change to t>=0 and it is approved.

@Squall-DA
Copy link
Contributor Author

@blazoncek / @Aircoookie Do I need to do anything to resolve the conflicting autogenerated file?

@blazoncek
Copy link
Collaborator

pull updated 0_14 branch or remove generated .h file

@Aircoookie Aircoookie merged commit b3a2918 into wled:0_14 Oct 18, 2022
@blazoncek blazoncek mentioned this pull request Jan 25, 2023
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.

5 participants