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

Allow MQTT json light floating point transition #27253

Merged
merged 2 commits into from Oct 13, 2019

Conversation

@starkillerOG
Copy link
Contributor

starkillerOG commented Oct 6, 2019

Breaking Change:

The MQTT light with JSON schema will now sent a float instead of an int with the transition key.
Lights that are based on the "ArduinoJson" module should not experience problems due to the change from int to float (the float value will be truncated to an int).

Description:

Allow to use floating point values for the transition time of the MQTT json light.
In this way transitions shorter than 1s can be used (0.5 seconds for instance) if the MQTT light supports it.

Related issue (if applicable): fixes #

Pull request with documentation for home-assistant.io (if applicable): home-assistant/home-assistant.io#10763

Example entry for configuration.yaml (if applicable):

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist

If user exposed functionality or configuration variables are added/changed:

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.
Allow to use floating point values for the transition time of the MQTT json light.
In this way transitions shorter than 1s can be used (0.5 seconds for instance) if the MQTT light supports it.
@@ -463,7 +463,10 @@ def supported_features(self):
message["flash"] = self._flash_times[CONF_FLASH_TIME_SHORT]

if ATTR_TRANSITION in kwargs:
message["transition"] = int(kwargs[ATTR_TRANSITION])
if kwargs[ATTR_TRANSITION].is_integer():

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Oct 6, 2019

Member

The light transition will be validated as a float, in the service schema preceding the actuator method call.

VALID_TRANSITION = vol.All(vol.Coerce(float), vol.Clamp(min=0, max=6553))

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Oct 6, 2019

Member

So the idea is to send an integer if the float has 0 as decimal and otherwise send a float?

This comment has been minimized.

Copy link
@starkillerOG

starkillerOG Oct 6, 2019

Author Contributor

@MartinHjelmare yes kwargs[ATTR_TRANSITION] is already a float.
However the current code typcasts that float to an int.
the .is_integer() works on a float to determine if that float is an "whole number" if that is the case it will convert the float to an int.
Therefore if you call a transition with value 2 for instance, this will ensure that the MQTT message contains "2" instead of "2.0".

I think in most cases this will not matter, but I thought there might be some light out there in the world that cannot cope with floating points and would get upset with a value of "2.0" instead of "2".

Therefore I wanted to make it backwords compatible.

This comment has been minimized.

Copy link
@starkillerOG

starkillerOG Oct 6, 2019

Author Contributor

@MartinHjelmare your second post is completly correct.
Sorry i did not read that before typing my replay

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Oct 6, 2019

Member

Would be nice if we can always send the same type of value, but I don't know the device landscape very well.

This comment has been minimized.

Copy link
@starkillerOG

starkillerOG Oct 10, 2019

Author Contributor

@emontnemery I actually wrote my own software and put it on github if other people might be instrested: https://github.com/starkillerOG/h801-mqtt-json
It is meant for a H801 LED controller (about €10,- on aliexpress, inside is an ESP8266 for the wifi) that supports 12V RGB+cold-white+warm-white LED strips.
I wrote my own code because I could not find any code at the time that supported MQTT json that implemented color-temperature to control the warm/cold white LED strip together with the RGB strip as a single light in HomeAssistant.

This comment has been minimized.

Copy link
@starkillerOG

starkillerOG Oct 10, 2019

Author Contributor

@emontnemery I also use ArduinoJson to parse the JSON by the way.
If you want I could put a link to my github project in the implementations section of the HomeAssistant docs that you refered to: https://www.home-assistant.io/integrations/light.mqtt/#implementations

@emontnemery, @MartinHjelmare, Schould I change the code to always sent a float or keep it like this to sent a int if it is a "whole number" and otherwise a float?

This comment has been minimized.

Copy link
@emontnemery

emontnemery Oct 10, 2019

Contributor

@starkillerOG I think it's fine to change the code to always send a float, but can you please do some testing on your side to verify ArduinoJson is working as expected when assigning a float value to an integer type, I mean something like:
int transitionTime = root["transition"];

This comment has been minimized.

Copy link
@starkillerOG

starkillerOG Oct 10, 2019

Author Contributor

will test that and report back soon.

This comment has been minimized.

Copy link
@starkillerOG

starkillerOG Oct 13, 2019

Author Contributor

@emontnemery I just tested the int transitionTime = root["transition"];.
It works fine if you sent a float, it just floors the float so 2.8 --> 2
Therefore I will update this PR to just always sent a float.

@MartinHjelmare MartinHjelmare changed the title MQTT json light: allow floating point transition Allow MQTT json light floating point transition Oct 8, 2019
@project-bot project-bot bot moved this from Needs review to Second opinion wanted in Dev Oct 8, 2019
@starkillerOG

This comment has been minimized.

Copy link
Contributor Author

starkillerOG commented Oct 13, 2019

Can this now be merged?

Dev automation moved this from Second opinion wanted to Reviewer approved Oct 13, 2019
Copy link
Member

MartinHjelmare left a comment

Looks good!

@MartinHjelmare

This comment has been minimized.

Copy link
Member

MartinHjelmare commented Oct 13, 2019

We should mention this as a breaking change just to be transparent even though we hope that it will not cause breakage.

Please add a breaking change paragraph in the PR description.

@starkillerOG

This comment has been minimized.

Copy link
Contributor Author

starkillerOG commented Oct 13, 2019

@MartinHjelmare, just added the breaking change paragraph.

@MartinHjelmare Schould I put a link to my github project in the implementations section of the HomeAssistant docs of the MQTT Light: https://www.home-assistant.io/integrations/light.mqtt/#implementations?
https://github.com/starkillerOG/h801-mqtt-json

starkillerOG added a commit to starkillerOG/home-assistant.github.io that referenced this pull request Oct 13, 2019
After this PR:home-assistant/home-assistant#27253, I thought it might be nice to include this firmware in the implementations section if other people want to use this firmware.
@starkillerOG starkillerOG referenced this pull request Oct 13, 2019
0 of 2 tasks complete
@emontnemery

This comment has been minimized.

Copy link
Contributor

emontnemery commented Oct 13, 2019

@starkillerOG Yeah, please add a link to your project!

@emontnemery emontnemery merged commit 2acd3f9 into home-assistant:dev Oct 13, 2019
12 checks passed
12 checks passed
CI Build #20191013.28 succeeded
Details
CI (FullCheck Mypy) FullCheck Mypy succeeded
Details
CI (FullCheck Pylint) FullCheck Pylint succeeded
Details
CI (Overview CheckFormat) Overview CheckFormat succeeded
Details
CI (Overview Lint) Overview Lint succeeded
Details
CI (Overview Validate) Overview Validate succeeded
Details
CI (Tests PyTest Python36) Tests PyTest Python36 succeeded
Details
CI (Tests PyTest Python37) Tests PyTest Python37 succeeded
Details
cla-bot Everyone involved has signed the CLA
codecov/patch 100% of diff hit (target 94.26%)
Details
codecov/project 94.35% (target 90%)
Details
docs-missing Documentation ok.
Dev automation moved this from Reviewer approved to Done Oct 13, 2019
@lock lock bot locked and limited conversation to collaborators Oct 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Dev
  
Done
4 participants
You can’t perform that action at this time.