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

En-/disable LED maps when receiving realtime data #3554

Merged
merged 8 commits into from Jan 10, 2024

Conversation

ezcGman
Copy link
Contributor

@ezcGman ezcGman commented Nov 27, 2023

Hey there,

after some discussions in #learning on the WLED Discord server (https://discord.com/channels/473448917040758787/757254961640898622/1174867799991140392), I wanted to introduce a new setting to the sync / realtime settings page: Enabling or disabling of LED maps when receiving realtime data.

The current behavior is that LED maps are always applied, which I think is highly debatable if that makes sense / is desired every time. Hence I decided to make a setting that allows to tell WLED to not respect LED maps for all realtime modes:
image
Default setting is to respect LED maps, to not introduce a breaking change.

Future improvements could be to specifically select realtime modes u want that setting to be on or off, but that felt to be overkill for now.

I saw two solutions how disabling of LED maps can be done:

  1. Wipe the WS2812FX::customMappingTable whenever realtime mode is entered and fill it again when realtime mode is exited.
    • Pros: Saving some memory when wiping the (not used) mapping table
    • Cons: Depending on the size of the LED map could be costly to reload it. Also it's not easily possible for LED maps set to presets, as when the preset is loaded, the loadLedmap variable is wiped again, so we don't know which map was loaded before, requiring more (unnecessary) changes
  2. Centralize the checking for a mapped pixel in a new function and in that function check if realtime mode is enabled and the setting is set to not respecting LED maps.
    • Pros: Less code, less complicated
    • Cons: "Mixing logic": The function for checking a mapped pixel now checks for a "random realtime setting".

For the HTML generation I simply ran node run build, which worked flawless. However, I was told to also run a compression, but I couldn't find that. I think that node task is the compression. Maybe I got confused.

On the two failing build tasks: Yes, they also fail locally, but I don't know really what to do about it. There is a note in platformio.ini about this error, but that's not helpful for fixing the pipeline here in GitHub :/ So some advice would be appreciated.

Happy for feedback on how to maybe make this PR better :)
Greetings,

Andy!

@jazakrzewski
Copy link

Great function idea here, many times the additional software sending real time data already has it's own mapping for the pixels having the WLED mapping in place will cause issues

@blazoncek
Copy link
Collaborator

The easiest and already working solution (without any coding) is to not use/create ledmap.json but only have ledmap1.json, ledmap2.json, etc.
That way, when you select "Default" ledmap in UI, no mapping will occur but with each preset, you can use whatever ledmap you want.

@ezcGman
Copy link
Contributor Author

ezcGman commented Nov 28, 2023

Hey Blaz,

no, this doesn't help, sadly. Because the usecase is that you have a preset active while you don't send realtime data (you don't have the show running) that does some nice effects (and of course has all the segments configured to properly segment the props (arms, bodies, heads, etc of the figures)) in the meantime. And for that, you need the LED map, otherwise the Snowman and other props you need to map look pretty ugly ;) And here it doesn't matter if it's the default map or a map selected on the preset: The map is applied/active when WLED enters realtime mode.

And it's in my (and others) opinion that this is not a fix that needs a workaround, but plainly wrong how it is implemented right now, and/or should be configurable. As again: Software that send the livedata usually have their own mapping, so WLEDs mapping simply interferes with it.

Greetings,

Andy!

@blazoncek
Copy link
Collaborator

Please re-base this for 0_15 branch.

@ezcGman
Copy link
Contributor Author

ezcGman commented Nov 28, 2023

Will do later today, thanks!! 🙇‍♂️

@ezcGman ezcGman changed the base branch from main to 0_15 November 28, 2023 10:41
@ezcGman
Copy link
Contributor Author

ezcGman commented Nov 28, 2023

Rebased, still need to double check functionality after re-base, so hold on :)

@ezcGman
Copy link
Contributor Author

ezcGman commented Nov 28, 2023

Confirmed still working as intended :)

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.

Apart from those minor things I'm ok with the PR.
Let's see what @softhack007 say.

wled00/FX_fcn.cpp Outdated Show resolved Hide resolved
wled00/FX_fcn.cpp Outdated Show resolved Hide resolved
wled00/set.cpp Outdated Show resolved Hide resolved
@ezcGman
Copy link
Contributor Author

ezcGman commented Nov 30, 2023

@softhack007 Would love to request review from you here, thanks!!

@softhack007 softhack007 self-requested a review November 30, 2023 14:56
@blazoncek blazoncek linked an issue Dec 9, 2023 that may be closed by this pull request
@spblat
Copy link

spblat commented Dec 9, 2023

Say a preset with a ledmap is active, and then we receive DDP and switch to a “live” ledmap as described above, and then DDP stops so the running preset resumes. Will this commit re-apply the original ledmap so the resuming preset is correct?

@ezcGman
Copy link
Contributor Author

ezcGman commented Dec 9, 2023

yes, because it never switches out the LED map, it simply doesn't respect it anymore. See my explanation on how it's implemented. To be even more specific: This commit doesn't even touch the LED map that is loaded, it simply adds a "switch" infront to respect the loaded LED map, or not

@ezcGman
Copy link
Contributor Author

ezcGman commented Dec 9, 2023

@softhack007 Do you still wanna review? :) Or are sou good?

@softhack007
Copy link
Collaborator

@softhack007 Do you still wanna review? :) Or are sou good?

Yes, I'd like to review before merging. Give me some days to recover, as I'm currently sick with covid (that breast is not dead, it just lost some of its teeth ...).

@ezcGman
Copy link
Contributor Author

ezcGman commented Dec 9, 2023

Oh no :( GET WELL SOON!!! And ofc, take your time <3

@softhack007
Copy link
Collaborator

Thanks 🤒 I'm doing my best
👍 already better than two days ago.

@blazoncek
Copy link
Collaborator

@softhack007 I hope you are better. Did you have any chance to look into this? IMO it is ok.

@blazoncek
Copy link
Collaborator

@softhack007 please.

@softhack007
Copy link
Collaborator

@softhack007 please.

Hi, its about Christmas time, so grandpa has high priority duties 🎅 and all interrupts (except reset) are masked off ;-)
but i'll look into the PR "between the days" and as soon as I can.

@blazoncek
Copy link
Collaborator

@ezcGman can you please update PR? (resolve conflicts). I am sorry this is taking so long. 😉

@ezcGman
Copy link
Contributor Author

ezcGman commented Jan 10, 2024

@ezcGman can you please update PR? (resolve conflicts). I am sorry this is taking so long. 😉

No worries, should be done :)

@blazoncek blazoncek merged commit 5fc0084 into Aircoookie:0_15 Jan 10, 2024
12 checks passed
@spblat
Copy link

spblat commented Mar 31, 2024

Looks good in 0.15.0b1. Test case:

  1. WLED running locally on a preset that includes a specific ledmap
  2. “Respect LED Maps” off
  3. Send UDP from another WLED, using a preset that uses a ledmap on the WLED which is sending UDP. This ledmap assumes the destination WLED has no ledmap
  4. Target WLED displays received signals correctly
  5. Turn off sending WLED
  6. Target WLED resumes its local preset, with ledmap active as required

Nice job and thank you :-)

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.

Lock in specific ledmap upon receiving DDP
5 participants