Incorrect Scaling of Brightness Values in MQTT Schema JSON Light Entity

[aurimasniekis]

The problem

Description:

There's a discrepancy in the scaling of brightness values between Home Assistant (HASS) and MQTT for the MQTT Schema JSON light entity. The scaling conversion when sending brightness values from HASS to MQTT is correct. However, when receiving brightness values back from MQTT, the scaling conversion does not correctly map back to the expected range in HASS.

Steps to Reproduce:

• Set the brightness in HASS which triggers a scaling conversion for MQTT. For example, setting a brightness of 204 in HASS gets correctly scaled and sent as 203 to MQTT (using the conversion 204 / 255 * 254 = 203).
• When MQTT sends back a brightness value of 203, HASS incorrectly scales it, resulting in the same value of 203 instead of the expected 204.

Expected Behavior:

The brightness value received from MQTT should be correctly scaled back to its equivalent in the HASS range. In the given example, a value of 203 received from MQTT should be converted back to 204 in HASS.

Actual Behavior:

The scaling conversion in HASS incorrectly rounds down the value. The current conversion formula used is int(203 / float(254) * 255), which results in 203 instead of 204.

Suggested Fix:

Modify the scaling conversion formula to include a rounding step for more accurate results. For instance, using int(round(203 / float(254), 2) * 255) would correctly produce 204.

Code Reference:

MQTT to HASS:
https://github.com/home-assistant/core/blob/dev/homeassistant/components/mqtt/light/schema_json.py#L370-L374

HASS to MQTT:
https://github.com/home-assistant/core/blob/dev/homeassistant/components/mqtt/light/schema_json.py#L593-L601

What version of Home Assistant Core has the issue?

core-2023.11.0.dev0

What was the last working version of Home Assistant Core?

No response

What type of installation are you running?

Home Assistant OS

Integration causing the issue

mqtt

Link to integration documentation on our website

https://www.home-assistant.io/integrations/mqtt

Diagnostics information

No response

Example YAML snippet

No response

Anything in the logs that might be useful for us?

No response

Additional information

No response

[home-assistant]

Hey there @emontnemery, @jbouwh, mind taking a look at this issue as it has been labeled with an integration (mqtt) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of mqtt can trigger bot actions by commenting:

• @home-assistant close Closes the issue.
• @home-assistant rename Awesome new title Renames the issue.
• @home-assistant reopen Reopen the issue.
• @home-assistant unassign mqtt Removes the current integration label and assignees on the issue, add the integration domain after the command.

_{^{(message by CodeOwnersMention)}}

---

mqtt documentation
mqtt source
_{^{(message by IssueLinks)}}

[jbouwh]

Not sure I can follow you. The default brightness scale is 255, Did you change that to 254?

[aurimasniekis]

Sorry if it's confusing, but it picks up 254 scale from config and I guess it's from Z2M

[aurimasniekis]

Sorry the default scale is 255 the hass side, but there is also device scale which is 254 fro Z2M lights

[jbouwh]

So I need to set the brightness scale to 254 to be able to reproduce?

[aurimasniekis]

Yes, sorry I am already in bed so can only share this screenshot I had made when I was investigating issue.

this is in turn_on method and you can see how brightness is changed from 204 to 203 the same happens on parsing the received message

[jbouwh]

I see what you mean, but if the brightness_scale is 255, with your formula, it will also turn receiving 203 to 204. Not sure if that should happen.

[aurimasniekis]

Are you talking about hass to mqtt side or mqtt to hass side? If hass to mqtt 0.8 * 255 = 204? If mqtt to hass side I am thinking of using round with precision of 2 to round up 0.799..99 to 0.8 and then multiple by 255 scale.

[jbouwh]

I am talking about mqtt to hass using your formula. If 1 is received it would turn brightness into 0 with a 254 scale which is invalid.

[aurimasniekis]

Oh I see now... I would think something like shift by 1 because hass has no 0 brightness but the z2m has 0 brightness that's why range 0-254, but thats prob not great...

Some thoughts is that we have scale which is top side of value but no mention of bottom side, hass doesn't work with 0 brightness and for e.g. Z2M does. That way would be possible to map values between two ranges I guess, but that's just my evening ramblings

[jbouwh]

The current formula works well but it does not round. If Z2M works with a scale of 0..254 then may be it would be better to add 1 to the brightness and keep the brightness scale to 255,
The other way round could be done the same.

[jbouwh]

Going to close this issue as the current solution offered seems to be working okay, and an update is not planned.

[aurimasniekis]

I am sorry what solution? The set attribute to 204 and read back attribute returns 203 that would fail any unit test as incorrect. The end system on MQTT doesn't matter if it's Z2M or other service, the HASS MQTT side doesn't work correctly

[jbouwh]

As I suggested add one using the templates and keep the brightness scaling to 255. This is not an MQTT issue.

[jbouwh]

Alternative is you open en PR with a change request, if you think that the current solution should change.

[aurimasniekis]

But thats a not fix in any way, I can produce a unit test which will be failing, because the scaling is implemented incorrectly.

[jbouwh]

But thats a not fix in any way, I can produce a unit test which will be failing, because the scaling is implemented incorrectly.

Feel free to open a PR. I cannot conclude given the you case there is an issue with the current code. If it comes to rounding. to integers there is always an error. A unit test should prove your new code works with all factors in a correct way, that is:
Brightness 1 is the minimum brightness (not 0). Brightness 255 is max.
When other scaling logic is suggested it should work two ways.
E.g:
With scale: 1-254 (assuming 0 is not allowed), and brightness(value)
brightness(1) = 1
brightness(254) = 255
and value(brightness) :
value(1) = 0
value(255) = 254

And this should be consistent for any other brightness scale.

[aurimasniekis]

Here you go, you can see how badly scaling works:

import random

def hass_to_mqtt(input_brightness, brightness_scale):
    brightness_normalized = input_brightness / 255
    device_brightness = min(round(brightness_normalized * brightness_scale), brightness_scale)
    device_brightness = max(device_brightness, 1)
    return device_brightness

def mqtt_to_hass(device_brightness, brightness_scale):
    return int(device_brightness / float(brightness_scale) * 255)

def test_brightness_conversion():
    brightness_scales = [255, 128, 100, 50]  # Add more scales as needed
    for brightness_scale in brightness_scales:
        print(f"Testing with brightness_scale: {brightness_scale}")
        for input_brightness in range(1, 256):  # Brightness range from 1 to 255
            mqtt_brightness = hass_to_mqtt(input_brightness, brightness_scale)
            hass_brightness = mqtt_to_hass(mqtt_brightness, brightness_scale)

            if hass_brightness != input_brightness:
                print(f"Mismatch at input_brightness {input_brightness}: Converted brightness is {hass_brightness}")
            else:
                print(f"Input brightness {input_brightness} successfully converted to {mqtt_brightness} and back to {hass_brightness}")

test_brightness_conversion()

[jbouwh]

So are you opening a PR to improve the scaling?

[aurimasniekis]

So are you opening a PR to improve the scaling?

Sorry, took a bit a while to find and perfect scaling method 😅

[jbouwh]

Point is that you do not have to use the built in scaling. You can use templates. But it will never work to get a perfect conversion if al long as values are rounded.

[aurimasniekis]

By templates you mean light entity template? Because I have like >500 lights...

[jbouwh]

If you are are using the json_schema, that will not work.
You would need the basic schema.

[aurimasniekis]

I can't change that, because it's coming from Z2M

[jbouwh]

Then open a ticket on Z2M

[aurimasniekis]

I appreciate your suggestions and the time you have invested in this discussion. However, I feel compelled to express my concern regarding the approach being taken towards this issue. In open source projects, collaboration and addressing problems at their source are crucial. This issue is not just about finding workarounds or shifting responsibilities to third-party integrations. It's about a fundamental flaw within the Home Assistant's MQTT integration.

As members of the open source community, our goal should be to make the project more robust and user-friendly. While I respect your perspective, I believe it is essential for the health of the project to address such core issues directly. If there is reluctance or unavailability to tackle this issue now, perhaps it can be revisited when there is more bandwidth or interest from other contributors.

Let’s continue to work together towards making Home Assistant better for everyone...

[jbouwh]

I appreciate your suggestions and the time you have invested in this discussion. However, I feel compelled to express my concern regarding the approach being taken towards this issue. In open source projects, collaboration and addressing problems at their source are crucial. This issue is not just about finding workarounds or shifting responsibilities to third-party integrations. It's about a fundamental flaw within the Home Assistant's MQTT integration.

As members of the open source community, our goal should be to make the project more robust and user-friendly. While I respect your perspective, I believe it is essential for the health of the project to address such core issues directly. If there is reluctance or unavailability to tackle this issue now, perhaps it can be revisited when there is more bandwidth or interest from other contributors.

Let’s continue to work together towards making Home Assistant better for everyone...

True, but solutions should be easy to read and avoid complexity when this is possible. Also we should we should keep in mind what the performance impact will be. I asked @emontnemery to have al look too BTW.

[jbouwh]

@aurimasniekis I've also proposed a less complex solution to deal with the scaling issue that you experienced, may you can have a look. IOM the issue in the current situation is not a big deal, and we should not make it that.

[aurimasniekis]

Until Set X == Get X, I don't consider this is a fixed/completed. Any logic which depends on check value will fail, or go into loop because it set value X but when checking if value is X it doesn't receive value X