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 additional segment options when controlling over e1.31 #3616

Merged
merged 4 commits into from
Mar 1, 2024

Conversation

demophoon
Copy link

Before this commit it was only possible to control mirror and reverse on a 1d segment. All of the other options for 2d effects could not be set and thus they would be kept disabled.

This commit replaces the Effect Option dmx channel with a bitfield which allows for each segment option to be individually toggled depending on which bit is set in the field. Backwards compatibility has been maintained with existing 1d segment options.

pull bot pushed a commit to AmirulAndalib/WLED that referenced this pull request Dec 26, 2023
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.

I am not proficient wit E1.31 but if I remember correctly @mxklb is.
@mxklb can you comment, please.

wled00/e131.cpp Outdated Show resolved Hide resolved
@demophoon
Copy link
Author

I've also not added a docs PR yet, if that would be helpful for reviewing this PR let me know and I can expand upon the additional effect options in the effect options table.

@mxklb
Copy link
Contributor

mxklb commented Dec 26, 2023

I've also not added a docs PR yet, if that would be helpful for reviewing this PR let me know and I can expand upon the additional effect options in the effect options table.

That would be nice. And helps to understand and must be provided nevertheless. I don't have 2d matrix to test :-( For real user testing, post into wled discourse, hopefully someone is able to test it, good luck ..

@demophoon
Copy link
Author

@mxklb I've also added documentation for this feature over at Aircoookie/WLED-Docs#179. I'll see if I can get someone else to verify this build. It only requires WLED be configured with a 2D strip to enable those options in the UI to verify.

image

@demophoon
Copy link
Author

Discourse thread can be found at https://wled.discourse.group/t/validation-requested-for-expanded-segment-effects-over-e1-31/10522.

Also for those catching up, I'm also available in the WLED Discord as @Demophoon. If you find this and have further questions about the implementation feel free to ping me here, on Discourse, or Discord.

@blazoncek
Copy link
Collaborator

I'm sorry but I still have trouble understanding why the complexity of the code.

IMO if your intention is to enhance E1.31 data received by WLED (in particular field 5) to include every segment option then why not simply use field 5 as a segment option bitfield? Well, not directly but something like:

seg.reverse = (bool)(e131_data[dataOffset+5] & 0b01000000);

There is no need for ifs. Those are the remains of a time when segment options were inaccessible directly.

@mxklb
Copy link
Contributor

mxklb commented Dec 27, 2023

As far as I know, macros in standard DMX mostly utilize the full channel range of 255 possible values. Dividing the full range into X possible macros is a best practice. I think this is due to most DMX hardware consoles use sliders to adjust channel values - single value steps are mostly hard to adjust, depending on the DMX console used. Why not use equidistant range for all possible options|states? Actually you say in documentation you have 128 marcos (=possible options|states) but I can only see 9 in the documentation table you provided. Isn't it uncommon for DMX to use bit fields? I don't know exactly, but I've never seen DMX fixtures that use them, maybe I'm wrong.. @demophoon do you know commercial DMX hardware that uses such approach for controlling macros, can you give an example? If so, please post link to data sheet/manual, as an example.. DMX fixture manuals in general mostly show simple value ranges (like 0..50,51..100,..) to adjust macros - at least all manuals I looked into..

In current code (mainline) the option channel is implemented to switch all possible combinations of 2 options (X: mirror & reverse) = 2*2 = 4 states/possibilities. When adding additional 2 more options (Y: mirror & reverse) + another transpose, aren't there 5*5 = 25 possible combinations? I think there shall be 25 marcos to be able to control all possibilities? So, due to 255 channel values, why not simply stick to 10 values per macro? Actually for me it's really complicated and hard to understand, how to address one of these 25 possible options|states, when reviewing your code & documentation.

I think the more clear/easy you get it, the more uncomplicated UX you can expect (also true for code).

@mxklb
Copy link
Contributor

mxklb commented Dec 27, 2023

P.S. Using first 4 macro ranges for 1D and following ranges for 2D options would also gain UX.

zanhecht pushed a commit to zanhecht/WLED that referenced this pull request Dec 27, 2023
@mxklb
Copy link
Contributor

mxklb commented Dec 27, 2023

There is no need for ifs. Those are the remains of a time when segment options were inaccessible directly.

IfS are there to explicitly not set a state if it is already present. Otherwise, realtime performance wasn't that good. Maybe this is subject of change, but shall be tested vs ifS. Some of the setter functions are slower than such if checks.

@demophoon
Copy link
Author

As far as I know, macros in standard DMX mostly utilize the full channel range of 255 possible values. Dividing the full range into X possible macros is a best practice. I think this is due to most DMX hardware consoles use sliders to adjust channel values - single value steps are mostly hard to adjust, depending on the DMX console used.

Is this not the case for the Effect ID and Palette ID channels? They appear to be different at individual steps with a swath of the dmx range being unused. I was treating the segment options channel similarly where it would be assumed that a user would put an id in and get the options that they wanted out.

Why not use equidistant range for all possible options|states? Actually you say in documentation you have 128 marcos (=possible options|states) but I can only see 9 in the documentation table you provided. Isn't it uncommon for DMX to use bit fields? I don't know exactly, but I've never seen DMX fixtures that use them, maybe I'm wrong..

The current implementation in mainline can be implemented as a bitfield, you just use 0b11000000 as your mask which gives you 4 states across a 0..255 range (0..63 = None, 64..127 = Reverse, 128..191 = Mirror, 192..255 = Mirror and Reverse). Starting at 0 if a user wanted to enable Reverse they add 64. If they only wanted Mirroring they would only add 128. Wanting both Reverse and mirroring they would add both 64 + 128 = 192 which happen to line up exactly with the current approach on mainline. So while it may not be called a bitfield in user manuals, they are definitely hidden in there.

@demophoon do you know commercial DMX hardware that uses such approach for controlling macros, can you give an example? If so, please post link to data sheet/manual, as an example.. DMX fixture manuals in general mostly show simple value ranges (like 0..50,51..100,..) to adjust macros - at least all manuals I looked into..

Admittedly I am very new to commercial DMX hardware and only have experience with lighting software and controlling them with the MIDI boards I have laying around the house.

In current code (mainline) the option channel is implemented to switch all possible combinations of 2 options (X: mirror & reverse) = 2*2 = 4 states/possibilities. When adding additional 2 more options (Y: mirror & reverse) + another transpose, aren't there 5*5 = 25 possible combinations? I think there shall be 25 marcos to be able to control all possibilities? So, due to 255 channel values, why not simply stick to 10 values per macro? Actually for me it's really complicated and hard to understand, how to address one of these 25 possible options|states, when reviewing your code & documentation.

In 1D the only options that are applicable are Mirror and Reverse. Each option can occupy 2 states and thus the total options is 2 * 2 = 4. In 2D, in addition to the 1D options which map to the X-axis Mirror and X-axis Reverse you have the additional options Mirror-Y, Reverse-Y, Transpose, and 1D-to-2D map. The only option with more than 2 possible states (on and off) conveniently only has 4 states that it can be in (Pixel, Bar, Arc, and Corner) which gives us 2 * 2 * 2 * 2 * 2 * 4 = 128 possible combinations that account for all possible states that 2D segments may occupy.

Perhaps this feature is more appropriate as a separate DMX mode which includes an additional 3 channels. 1 channel that copies the structure of main's X-axis options, 1 channel for 1D to 2D mapping (also split into 4 ranges across 0..255), and a transpose channel which is either off or on (0..127 = off, and 128..255 = on). Thoughts?

Also thanks to both of you for taking the time to discuss this with me.

@demophoon
Copy link
Author

IfS are there to explicitly not set a state if it is already present. Otherwise, realtime performance wasn't that good. Maybe this is subject of change, but shall be tested vs ifS. Some of the setter functions are slower than such if checks.

I have not attempted this without the if statements to see if there was a performance hit or not. I left it as is because I assumed it was relatively costly to always set these options. I also don't think I'm setup to performance test qualitatively unless y'all have suggestions.

@blazoncek
Copy link
Collaborator

As I don't have any experience with DMX or E1.31 I cannot say anything contextual.
It is true that segment options are a C/C++ bitfield so there may be a penalty accessing them. But I would not expect much.
It was just a suggestion to possibly make code more transparent.

@mxklb
Copy link
Contributor

mxklb commented Dec 27, 2023

@demophoon

Is this not the case for the Effect ID and Palette ID channels? They appear to be different at individual steps with a swath of the dmx range being unused. I was treating the segment options channel similarly where it would be assumed that a user would put an id in and get the options that they wanted out.

This is true. It was done because already more than 128 effect IDs existed during implementation, so it didn't make sense to combine some into value ranges and some not. It made more sense to keep upper values unused, to be used by future upcomming effects IDs. Think of MIDI slider, it can only handle 128 states, so it will never be able to select more than these states. It is already an "issue" of the current implementation. With grandMA hardware, we had trouble selecting effects and palettes precisely via slider. As solution we made a fixture with each effect and palette accessible directly as a macro. Configuring custom lights for DMX hardware is one time setup, i.g. dependent on the vendor, if not open sourced - I think QLC+ can use OFL.

Finally: Of cause it's okay to do single steps, but should be avoided if just some small amount of macro states shall be triggered. Combining ranges helps to be more accessible by (also cheap) DMX hardware, so it should be best practice.

The current implementation in mainline can be implemented as a bitfield, you just use 0b11000000 as your mask which gives you 4 states across a 0..255 range (0..63 = None, 64..127 = Reverse, 128..191 = Mirror, 192..255 = Mirror and Reverse). Starting at 0 if a user wanted to enable Reverse they add 64. If they only wanted Mirroring they would only add 128. Wanting both Reverse and mirroring they would add both 64 + 128 = 192 which happen to line up exactly with the current approach on mainline. So while it may not be called a bitfield in user manuals, they are definitely hidden in there.

Implementation detail shall have no effect to the end user. So if bitfield usage does the same, this is totally fine.

Admittedly I am very new to commercial DMX hardware and only have experience with lighting software and controlling them with the MIDI boards I have laying around the house.

As mentioned above, with MIDI 7bit values transformed to 255 DMX values, it's not possible to perform single steps precisely. Of cause in MIDI world combining two 7bit CC could do so, I think it's call fine control or something like that ..

In 1D the only options that are applicable are Mirror and Reverse. Each option can occupy 2 states and thus the total options is 2 * 2 = 4. In 2D, in addition to the 1D options which map to the X-axis Mirror and X-axis Reverse you have the additional options Mirror-Y, Reverse-Y, Transpose, and 1D-to-2D map. The only option with more than 2 possible states (on and off) conveniently only has 4 states that it can be in (Pixel, Bar, Arc, and Corner) which gives us 2 * 2 * 2 * 2 * 2 * 4 = 128 possible combinations that account for all possible states that 2D segments may occupy.

This I did not know, due to lack of 2D matrix usage. So there really are 128 options = macros. All right!

Perhaps this feature is more appropriate as a separate DMX mode which includes an additional 3 channels. 1 channel that copies the structure of main's X-axis options, 1 channel for 1D to 2D mapping (also split into 4 ranges across 0..255), and a transpose channel which is either off or on (0..127 = off, and 128..255 = on). Thoughts?

Because the effect channel already combines 1D and 2D effects, I don't think it makes sense to introduce special 2D modes with more channels. And of cause more channels per segment means less segments to be controlled (due to max 512 channels per DMX universe).

Actually (I haven't tested) it should behave the same as in current implementation:

  • when sending option channel value 0 -> 1D shall be none (= no mirror, no reverse)

Doing so makes it backwards compatible, at least for 1D mode none. When looking to the docs table you provided, I see e1.31 values for mirror and reverse for 1D are already backwards compatible. That is really good! So DMX scenes saved for current 1D options shall be unchanged by your updated option channel value usage. Good work!

Thank you for your contribution.

P.S. In documentation it would be nice to write down all 128 combinations explicitly. This helps users to understand how to trigger wanted options. Otherwise everybody has to figure it out by them self. This must be done regularly, each time you have to configure your WLED DMX fixture for your desired DMX hard/software. I know it is a pain to do so, but may be useful for others to later copy & paste into DMX fixture definitions. This is what I think, nice to have, but not a must .. tbd

By the way, the docs you provided are already self-explanatory.

@demophoon
Copy link
Author

@mxklb

Finally: Of cause it's okay to do single steps, but should be avoided if just some small amount of macro states shall be triggered. Combining ranges helps to be more accessible by (also cheap) DMX hardware, so it should be best practice.

Entirely reasonable. I think one thing that I could do with this information is to make it so that the steps between each option are at least 2 values by shifting the options from 0b11011111 to 0b11111110.

This would make it so that the following values would be needed for each segment option.

Value Mod Option
+2 Reverse Y
+4 Mirror Y
+8 Transpose
+0 1D2D Map Pixels
+16 1D2D Map Bar
+32 1D2D Map Arc
+48 1D2D Map Corner
+64 Reverse X
+128 Mirror X

As mentioned above, with MIDI 7bit values transformed to 255 DMX values, it's not possible to perform single steps precisely. Of cause in MIDI world combining two 7bit CC could do so, I think it's call fine control or something like that ..

That is context that I definitely missed and appreciate the callout.

P.S. In documentation it would be nice to write down all 128 combinations explicitly. This helps users to understand how to trigger wanted options. Otherwise everybody has to figure it out by them self. This must be done regularly, each time you have to configure your WLED DMX fixture for your desired DMX hard/software. I know it is a pain to do so, but may be useful for others to later copy & paste into DMX fixture definitions. This is what I think, nice to have, but not a must .. tbd

I'm happy to do that for clarity. I actually already had that work done because I needed it for my custom fixture definition in QLC+ (Also happy to provide if desired). A python script took care of it for me so it won't take long at all. I mainly didn't want to muddy the documentation with features that I don't imagine many folks will be using considering there is many more options compared to a single 1D strip.

I've updated the docs PR to list out all the effects in the table.

@demophoon
Copy link
Author

@mxklb I went ahead and added the options shift to this PR which guarantees two values between every option step as documented in the new docs PR. This change can be found in 023eab3

softhack007 pushed a commit to MoonModules/WLED that referenced this pull request Dec 29, 2023
@blazoncek
Copy link
Collaborator

@mxklb as you are my reference for DMX stuff, can you take the time and test this or validate that the change will not break existing stuff? Thank you.

@demophoon please re-base this for 0_15 branch or wait until 0.15 hits beta.

Before this commit it was only possible to control mirror and reverse on
a 1d segment. All of the other options for 2d effects could not be set
and thus they would be kept disabled.

This commit replaces the Effect Option dmx channel with a bitfield which
allows for each segment option to be individually toggled depending on
which bit is set in the field. Backwards compatibility has been
maintained with existing 1d segment options.
Due to the midi interface being difficult/impossible to increment on
values of 1 because it has 7-bits of granularity, this commit moves all
the bitfields for configuring segment options left by one which
guarantees that every option has 2 values next to each other.

This allows midi controllers to more easily select an individual segment
option for 2D arrays.
@demophoon demophoon changed the base branch from main to 0_15 January 8, 2024 19:18
@demophoon
Copy link
Author

@blazoncek Rebased to 0_15. I'll get some testing with this new build later today and update you here

@blazoncek
Copy link
Collaborator

Thank you.

@mxklb
Copy link
Contributor

mxklb commented Jan 8, 2024

@mxklb as you are my reference for DMX stuff, can you take the time and test this or validate that the change will not break existing stuff? Thank you.

I'll test if it brakes my 1D strip DMX setup when I find time to.. But sadly I don't have 2D matrix to test new options.

Would be nice if anyone else could do some 2D matrix option DMX change tests as well ..

Djelibeybi pushed a commit to Djelibeybi/WLED-MM that referenced this pull request Jan 15, 2024
@mxklb
Copy link
Contributor

mxklb commented Jan 15, 2024

To compile the branch of this PR I had to manually npm install inliner to make it build (HTML & Binary). I Don't know if this is only on my system, or this is already addressed/fixed in upstream. Just wanted to notice..

I'll prepare a controller with this version and come back if I've done some (1D) tests with some of my already saved DMX scenes. I'll check if this version will break any of these scenes..

DedeHai pushed a commit to DedeHai/WLED that referenced this pull request Jan 27, 2024
@mxklb
Copy link
Contributor

mxklb commented Mar 1, 2024

I found time and tested @demophoon 's changes with my 1D E1.31 WLED setup. I can confirm, that it behaves like before. So at least I can say that the changes applied here don't have an effect on older (0.14.*) 1D WLED setups controlled by E1.31. Nevertheless I don't have a 2D WLED setup, so I can't test the E1.31 option changes directly. @blazoncek I think this is save to be merged. It looks implemented properly and as far as I could test, it is backwards compatible.

@mxklb
Copy link
Contributor

mxklb commented Mar 1, 2024

I'm happy to do that for clarity. I actually already had that work done because I needed it for my custom fixture definition in QLC+ (Also happy to provide if desired). A python script took care of it for me so it won't take long at all. I mainly didn't want to muddy the documentation with features that I don't imagine many folks will be using considering there is many more options compared to a single 1D strip.

@demophoon would you like to share your custom QLC+ fixture definition? That would be nice ..

@blazoncek blazoncek merged commit ec4afb2 into Aircoookie:0_15 Mar 1, 2024
softhack007 pushed a commit to MoonModules/WLED that referenced this pull request Apr 9, 2024
…-expanded

Add additional segment options when controlling over e1.31
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.

3 participants