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

Large ledmap support #4262

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Large ledmap support #4262

wants to merge 4 commits into from

Conversation

blazoncek
Copy link
Collaborator

Adds support for loading large ledmaps (>1200 pixels).
Also adds filtering option to readObjectFromFile().

- add filtering support for readObjectFromFile()
@dosipod
Copy link
Contributor

dosipod commented Nov 10, 2024

Thank you , I was in the process to test the large ledmaps and I could only test using DDP .

The issue is that there is a bug when using DDP listed here
#3917 ( This was closed quickly )

The solution to use Respect LED Maps assume the slave units is on new builds to use that option but that is not the case with real setups like what we all have . Otherwise I can not test this in an accurate way and I could not find any of my contacts with large fixture ( we still did not know how large can we go but only >1200 )

The unit I flashed with this seems stable and used it with DDP on 16x16 slave and the custom maps works fine .
I hope you understand that it takes a lot of time to test in an accurate way and I can not risk my production units ( some are with modified code or complex setup ) hence it is important that DDP issue is fixed from master and not slave not only for this PR but for all future testing .

I am also sure with time someone else would test and raise an issue if any so might be not everything needs to be beta tested as in this case and this is not really a critical change so might be merge for others to do so . Cheers

@blazoncek
Copy link
Collaborator Author

Ledmap is used on the strip level. strip contains physical and virtual LEDs in one set. If you have ledmap on master and slave you can disable slave's ledmap in settings when receiving DDP but master's ledmap will remain in effect.

But I am not sure if this is what you want to hear as I have difficulty understanding what you write.

@dosipod
Copy link
Contributor

dosipod commented Nov 10, 2024

If that is the case I wounder we did you close that issue we raised in the first place but no big deal , you would only need to test the same way below with 32x16 ( this is two 16x16) . If you match the 2D page setup on master and slave then you will see that it does not work as expected ( This was raised multiple time from the guys using xlights and jinx ) but do you agree that if you match the 2D setup then the correct behavior for the slave and master to work with 2D effects the same way ?

image

The solution we had even before " Respect LED Maps" option is just go to slave and disable Serpentine .

@willmmiles
Copy link
Member

If you match the 2D page setup on master and slave then you will see that it does not work as expected ( This was raised multiple time from the guys using xlights and jinx ) but do you agree that if you match the 2D setup then the correct behavior for the slave and master to work with 2D effects the same way?

No, I don't think that's the correct behaviour? The master shouldn't need to know what the slave's physical mapping is. If the slave is taking care of the pixel mapping, the master should target it as if it's a flat device. This way I can swap out different physical targets without changing the settings on the master. I'm a bit confused -- why do you want to have to enter the same information in multiple places?

@dosipod
Copy link
Contributor

dosipod commented Nov 10, 2024

I'm a bit confused -- why do you want to have to enter the same information in multiple places?

If you mean why not setup the slave in 1D ( This is the workaround other guys use from xlights to simplify the mapping ) then the reason is I do not want to change the slave config in any way as they are production units running in 2D
like below and to reach good size to test this PR i am adding about 9 units with DDP
image

@blazoncek
Copy link
Collaborator Author

To clarify: Master will always treat strip in a non-serpentine left-to-right, top-to-bottom fashion (when dealing with 2D but ledmaps are one-dimesional). This is how effects operate (for simplicity use one segment without transpose, mirror or reverse). When this is sent to LEDs (physical or virtual) it is transformed using ledmap in the same non-serpentine, left-to-right, top-to-bottom fashion.

When using ledmap the panel set-up is irrelevant and means nothing. You could easily set up just one panel in any orientation you wish. Ledmap takes precedence.
Disregard your panel set up.

But that is irrelevant for this PR. This PR deals with the ability to load a ledmap that will exceed memory constrains. The point for testing this PR is to create a ledmap with more than 2000 pixels and have it load. If you are wondering how to test it if you do not have a fixture of such size, is by examining debug dump of in-memory created ledmap which should correspond to ledmap file.

@dosipod
Copy link
Contributor

dosipod commented Nov 10, 2024

Unfortunately from past experience I would only like to confirm on a fixture but I might have a quick workaround as I am testing with hub75 64X64 but on MM so I will use that as a slave and will not focus on none ledmap related for now .

@dosipod
Copy link
Contributor

dosipod commented Nov 10, 2024

For now basic maps for 64x64 did not work , peek seems okay but fixture is not taken that .
Some error related to FX_fcn.cpp when trying to use debug build flags prevented further investigation .

@netmindz netmindz changed the base branch from 0_15 to main December 16, 2024 13:26
@blazoncek
Copy link
Collaborator Author

@softhack007 I'd recommend this PR is merged before #4463 as it modifies some of the same files.

@dosipod
Copy link
Contributor

dosipod commented Jan 9, 2025

Since you and I tried in discord and privately to forward the guys with large fixture to test but did not get feedback so might be yes the best way is to merge .

For the debug I thought to try again to use it as you mentioned but I still get the error you see below , do you spot anything wrong in the env ( This is your branch directly in code spaces ) .If that's working for you then okay.

image

@blazoncek
Copy link
Collaborator Author

@dosipod why don't you use CI builds from this PR? If you are using my fork then make sure you get everything. My fork has diverged substantially.

@dosipod
Copy link
Contributor

dosipod commented Jan 9, 2025

@blazoncek Yeah I did since day one but since you do not have a debug CI build I wanted to add that to test what you mentioned about examining debug dump of in-memory created ledmap to bypass that I do not have large fixture in place , if this works then I could verify a lot of test cases . The error is apparently only for debug but in FX_fcn.cpp

@blazoncek
Copy link
Collaborator Author

@dosipod there is something wrong with your environment or repository then. DEBUGFX_PRINTLN() is defined in FX.h.
You activate its output using -D WLED_DEBUG_FX otherwise it doe nothing. Saves on flash size.

@DedeHai
Copy link
Collaborator

DedeHai commented Jan 20, 2025

is this ready to merge?

@netmindz netmindz added this to the 0.16.0 candidate milestone Jan 22, 2025
@blazoncek
Copy link
Collaborator Author

I think this is ok to be merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants