Magic Home LED Controller: white channel not independent anymore

[hyteoo]

Using a RGBW controller
As of 1.13.4 1.13.3, the white channel (channel #3) seems to be connected to the brightness setting.
Therefore the RGB and white channel can't be controlled independently anymore.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

[mcspr]

Hey,

I think I am missing something, but how exactly can you reproduce it? Yesterday I built a test image (i do not have magichome controller hw), but did not see anything unusual when doing brightness N or channel 3 N via terminal. I will also try with some leds attached asap, to actually see the change

sorry for the delay 🙉

[hyteoo]

You can't see anything unusual in the interface, you have to connect a controller.
What happens is now the white channel is changed both by brightness and channel3.
Before brightness controlled RGB and channel3...the white.

[mcspr]

OK. So does that mean it happens when "Use white channel" setting is off? I see one change that removed check for channels >= 3 when applying brightness:
0180274#diff-1e2d5ef62603513c39da08ec25f60755L150
But that was already in 1.13.3

Only other change to lights after that was 19d0c5d, but that was merely renaming 'shadow' into 'target' for transitions

[hyteoo]

Yes, "Use white channel" setting is off.
The issue is old, since 1.13.3

[mcspr]

Encoder thing was for the Xiaomi lamp with only WW channels without the RGB part, so to check both color and RGB channel number did not make sense there.

IDK what is the right thing to do here. Change useWhite meaning to support ignoring white channel value (aka brightness) when controlling global brightness? Kind-of complicated.

[mcspr]

Also, maybe related to #1292. MQTT brightness is not exposed unless we have RGB channels

[copyrights]

@mcspr wrote in #1730

Thanks! Brightness should be there.
Next question is what to do in cases like #1575

Good question. I think in most chases the current behaviour will fit for most users need. But I also understand the need @hyteoo has.

The change would be quit simple. All you need to do is to reduce the loop in _generateBrightness().

espurna/code/espurna/light.ino

Lines 149 to 152 in 868d4d5

	// Apply brightness equally to all channels
	for (unsigned char i=0; i < _light_channel.size(); i++) {
	_light_channel[i].value = _light_channel[i].inputValue * brightness;
	}

But what would be the criteria?

[0x3333]

Same problem here. I'm using an H801, RGB+2W. I use the White channels independently from the RGB. But if I set the brightness on RGB, it set on the W too. Also, I need to turn on RGB to turn on the W.

[copyrights]

So something like configurable relay groups could be a useful feature.

e.g.:
Relay group 0: channel 0-2, use color
Relay group 1: channel 3
or in case of @0x3333
Relay group 1: channel 3-4, use white channel, use white color temperature

[0x3333]

That's what I thought initially. How do you believe it should be implemented?

[copyrights]

Start point would be here

espurna/code/espurna/relay.ino

Lines 105 to 122 in 9ceca44

	// If the number of dummy relays matches the number of light channels
	// assume each relay controls one channel.
	// If the number of dummy relays is the number of channels plus 1
	// assume the first one controls all the channels and
	// the rest one channel each.
	// Otherwise every dummy relay controls all channels.
	if (DUMMY_RELAY_COUNT == lightChannels()) {
	lightState(id-physical, status);
	lightState(true);
	} else if (DUMMY_RELAY_COUNT == (lightChannels() + 1u)) {
	if (id == physical) {
	lightState(status);
	} else {
	lightState(id-1-physical, status);
	}
	} else {
	lightState(status);
	}

add a case where dummy relay groups are configured.

And from there all the way up till UI implementation and documentation.

[0x3333]

@copyrights, this I already know... 🤣🤣🤣, I'm talking about the way it will work. If it will be configurable or not, would use defines, etc.

I'm thinking about creating a relay group define, like:

#define DUMMY_RELAY_GROUP     "311"

This will group the first 3 channels and the last 2 individually. This would replace the default DUMMY_RELAY_COUNT, the sum of the groups is equal to the old DUMMY_RELAY_COUNT.

What do you think about this approach? I could create a PR.

[copyrights]

@0x3333 ok, sorry for the misunderstanding 😁.

I'm definitively no design authority here, but IMHO it should be configurable. There won't be any need for custom builds.

[0x3333]

Sure it should be configurable, but at compile time, as the other features are, including relays.

@xoseperez, what do you think? Worth working on it?

[mcspr]

@copyrights The initial idea was to somehow tweak useWhite setting / maybe have another one, but relay grouping should be easier to understand. Otherwise it would be restoring old behaviour (pre 1.13.3, no brightness for white channels). Some light settings already overlap in a surprising ways, but so are use-cases and light setups.

@0x3333 it would be appreciated. it would be nice to convert that relay setup part into something more directly configurable, but I am not quite sure what your initial idea was with 311 number. can we use the same numbered definitions approach, LIGHT_RELAY_GROUP1, ...GROUP2, etc. and directly specify required values? (and for easier kv naming, lightRelayGroup0 or something like that)
Also, nothing stops us from using some sub-format inside string value. Human-readable or otherwise, to safe flash space by using smaller strings (or byte values directly, but @xoseperez might disagree about that:)).

Despite v2 looming over the horizon. it shouldn't be a save-all. Some runtime config improvements can be useful right now, despite build-time settings being the main ones.

[mcspr]

Any update on this?
I would try to implement relay grouping as best I can then:), with the proposed mapping
(or maybe a code snippet like MY92XX_MAPPING / comma-separated string for v2)

[hyteoo]

I would propose the following simple behavior:

• brightness impacts only the RGB channels
• any extra (white) channels already have their own intensity setting, nothing else should influence that

[mcspr]

#1826 does the proposed fix when "Use white channel" setting is off
f422391 was the crucial part, since the actual channel value would wrong

[0x3333]

This weekend I'll give it a shot, and let you know if it solves my problem. Thanks.