color_temp should be bound to min/max range

[sjorge]

As mentioned here Koenkk/zigbee2mqtt#4926 (comment)

This is currently not the case, we should update the color_temp convertor to keep this in mind.

I'm not sure what the best way to do this is...

• option a
  • read min/max in device configure and store in meta
  • color_temp converter checks if meta data present, and caps the range according
    • throw error when out of bound (or warning?) || silently use min or max depending which bound we exceeded
• option b
  • color_temp converter read min/max, and caps the range according
    • throw error when out of bound (or warning?) || silently use min or max depending which bound we exceeded

Not sure if this will work and which one is best, I'm leaning more towards A because we do not have to read the range every time. But what if a firmware updates changes the range, we need to run configure again.

What for bulbs that no configure step? Do we add one to read the min/max?

Are there other values we might want to read? I believe we already do so for some of the power measurements, perhaps we need a 'generic' way to query such info similar to configure. Were you just have an extra property with manual configure ranges or a attribute to query to get it, which we then trigger at the end of the configure action.

That would make it easy to read other stuff in the future, but if we want this... is it worth the extra work right now?

[Koenkk]

I think the best way to implement this is by doing it similar to e.g.

zigbee-herdsman-converters/devices.js

Line 7773 in 7623a58

await readEletricalMeasurementPowerConverterAttributes(endpoint);

and then access the values via e.g.

zigbee-herdsman-converters/converters/fromZigbee.js

Line 933 in 7623a58

const multiplier = msg.endpoint.getClusterAttributeValue('haElectricalMeasurement', `${key}Multiplier`);

But what if a firmware updates changes the range, we need to run configure again.

My assumption is that the color temperature depends on the hardware so a firmware update should not change this

[sjorge]

    light_colortemp: {
        key: ['color_temp', 'color_temp_percent'],
        convertSet: async (entity, key, value, meta) => {
            let color_temp_min;
            let color_temp_max;
            if (entity.constructor.name === 'Group' && entity.members.length > 0) {
                for (const m of entity.members) {
                    let member_color_temp_min = m.getClusterAttributeValue('lightingColorCtrl', 'colorTempPhysicalMin');
                    let member_color_temp_max = m.getClusterAttributeValue('lightingColorCtrl', 'colorTempPhysicalMax');
                    if ((member_color_temp_min === undefined) || (member_color_temp_max === undefined)) {
                        // XXX: perhaps doing this once in configure() is good, but bulbs are the odd extend thing
                        // looks like we have no cached values, lets read query the device
                        await m.read('lightingColorCtrl', ['colorTempPhysicalMin', 'colorTempPhysicalMax']);
                        member_color_temp_min = m.getClusterAttributeValue('lightingColorCtrl', 'colorTempPhysicalMin');
                        member_color_temp_max = m.getClusterAttributeValue('lightingColorCtrl', 'colorTempPhysicalMax');
                    }
                    /*
                     * For groups we want the value to be in range for all devices
                     * So we grab the highest min and lowest max so we are OK in mixed groups.
                     */
                    if ((color_temp_min === undefined) || (member_color_temp_min > color_temp_min)) {
                        color_temp_min = member_color_temp_min;
                    }
                    if ((color_temp_max === undefined) || (member_color_temp_max < color_temp_max)) {
                        color_temp_max = member_color_temp_max;
                    }
                }
            } else {
                color_temp_min = entity.getClusterAttributeValue('lightingColorCtrl', 'colorTempPhysicalMin');
                color_temp_max = entity.getClusterAttributeValue('lightingColorCtrl', 'colorTempPhysicalMax');
                if ((color_temp_min === undefined) || (color_temp_max === undefined)) {
                    // XXX: perhaps doing this once in configure() is good, but bulbs are the odd extend thing
                    // looks like we have no cached values, lets read query the device
                    await entity.read('lightingColorCtrl', ['colorTempPhysicalMin', 'colorTempPhysicalMax']);
                    color_temp_min = entity.getClusterAttributeValue('lightingColorCtrl', 'colorTempPhysicalMin');
                    color_temp_max = entity.getClusterAttributeValue('lightingColorCtrl', 'colorTempPhysicalMax');
                }
            }

            if (key === 'color_temp_percent') {
                value = Number(value) * 3.46;
                value = Math.round(value + 154).toString();
            }

            // ensure value withing range
            if ((color_temp_min !== undefined) && (value < color_temp_min)) {
                meta.logger.warn(`Requested color_temp ${value} is lower than minimum supported ${color_temp_min}, using minimum!`);
                value = color_temp_min;
            }
            if ((color_temp_max !== undefined) && (value > color_temp_max)) {
                meta.logger.warn(`Requested color_temp ${value} is higher than maximum supported ${color_temp_max}, using maximum!`);
                value = color_temp_max;
            }

            value = Number(value);
            const payload = {colortemp: value, transtime: utils.getTransition(entity, key, meta).time};
            await entity.command('lightingColorCtrl', 'moveToColorTemp', payload, utils.getOptions(meta.mapped, entity));
            return {state: {color_temp: value}, readAfterWriteTime: payload.transtime * 100};
        },
        convertGet: async (entity, key, meta) => {
            await entity.read('lightingColorCtrl', ['colorTemperature']);
        },
    },

(first error message is gone, I had i t for debugging)

Zigbee2MQTT:error 2021-01-02 14:29:56: CT Min/Max => 250 / 454
Zigbee2MQTT:warn  2021-01-02 14:29:56: Requested color_temp 200 is lower than minimum supported 250, using minimum!

I got it working, but...

• couldn't get the initial read in configure() to work. (Possible because bulbs use the extend: thing and my test innr bulbs set their own meta for redfix/enhancedHue so it can't find the configure key?
• if we do go via configure() which I think would still be better than what we have now, we should replace read with a meta.logger.warn("Unable to find stored colorTempPhysicaMin and/or colorTempPhysicalmax, please reconfigure your bulb(s).?)
• we should also query colorCapabilities if we go via configure(), that would make the enhancedHue meta option obsolete! And we can make the color code more inteligent as we can check for xy, hs, enhanced hs support directly!
• groups are also handled gracefully, we pick the highest min and the lowest max, so the value has to be within range of all devices.

Edit... hmm there should be no reason for configure() not to work 🤔
Well aside from ZigBee 3.0 LED-controller, 4 channel 5A, RGBW LED as that is the only device with it's own.

[sjorge]

OK so the extend is the issue.
It does not do a deep merge... so configureKey gets dropped for innr and co

  meta: {
    enhancedHue: false,
    applyRedFix: true,
    turnsOffAtBrightness1: true
  },

Added some logging to where they get merged near the bottom.

[sjorge]

OK I got a fix, continueing with cleanup

[sjorge]

Have a draft PR #2010 open, as having code to look at helps for feedback