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
Improve MQTT json light brightness scaling #104510
Conversation
Hey there @emontnemery, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original issue is that it's not possible to scale between ranges 1..255 and 1..254, that issue is not fixed by this PR.
The only change is that the PR moves the missing code when translating integer range 1..254 to 1..255; without this PR 254 is skipped, with this PR 127 is skipped, and it's not obvious that's an improvement (although rounding seems more reasonable).
@jbouwh could you explain more clearly the motivation for the changes in this PR?
* 255 | ||
), | ||
255, | ||
255, round(brightness * 255 / scale) # type: ignore[operator] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, so this change replaces truncating with rounding, that makes sense
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is based on de fan percentage scaling code we use for fan. In fact the logic is not changed. Therefor the tests did not need an update.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This way the value we send will closer result if the same value is sent and translated back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not perfect, but may be a better approach then #104422
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @jbouwh 👍
This doesn't fix anything, apart in one scale few values, other scales are still not working as expected... I am not sure how it's fine that set/get returns two different values. If command sets value X and then reads it should be X. Not X + 1 or X - 10 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 round(device_brightness * 255 / brightness_scale)
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() Result: Details
Testing with brightness_scale: 255 |
Proposed change
Improve MQTT json light brightness scaling.
Fixes rounding issues.
Alternative solution for #104422
Type of change
Additional information
Checklist
black --fast homeassistant tests
)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest
.requirements_all.txt
.Updated by running
python3 -m script.gen_requirements_all
..coveragerc
.To help with the load of incoming pull requests: