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

Update color.py to prevent ZeroDivisionError #15305

Closed
wants to merge 1 commit into from
Closed

Update color.py to prevent ZeroDivisionError #15305

wants to merge 1 commit into from

Conversation

bartnaaijkens
Copy link

@bartnaaijkens bartnaaijkens commented Jul 5, 2018

Description:

To prevent a ZeroDivisionError I suggest this change to check if the value is bigger then 0 before making a division in line 469 and 476.

Only thing I'm not sure about is wether the functions are expected to return something so maybe an 'else' clause should be included returning null or 0 ?

Related issue (if applicable): fixes #
#13381

Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.github.io#<home-assistant.github.io PR number goes here>

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

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

If the code communicates with devices, web services, or third-party tools:

  • New dependencies have been added to the REQUIREMENTS variable (example).
  • New dependencies are only imported inside functions that use them (example).
  • New or updated dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
  • New files were added to .coveragerc.

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.

To prevent the ZeroDivisionError listed #13381 I suggest this change to check if the value is bigger then 0 before making a division in line 469 and 476.

Only thing I'm not sure about is wether the functions are expected to return something so maybe an 'else' clause should be included returning null or 0 ?
@bartnaaijkens bartnaaijkens requested a review from a team as a code owner July 5, 2018 08:44
@homeassistant homeassistant added core small-pr PRs with less than 30 lines. labels Jul 5, 2018
@homeassistant
Copy link
Contributor

Hi @bartnaaijkens,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

@ghost ghost added the in progress label Jul 5, 2018
return math.floor(1000000 / mired_temperature)
if mired_temperature > 0:
return math.floor(1000000 / mired_temperature)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

blank line contains whitespace

Copy link
Member

@pvizeli pvizeli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is only a workaround and don't fix the problem:
https://github.com/home-assistant/home-assistant/blob/dev/homeassistant/components/google_assistant/trait.py#L314-L325

Please fix the problem on root.

@bartnaaijkens
Copy link
Author

Diving a bit deeper into this issue I found that the root cause is that the Deconz Component is setting the color_temp to 0 for groups exposed as lights.

In Deconz there is a concept of a group of lights (https://dresden-elektronik.github.io/deconz-rest-doc/groups/#getattr) that will be exposed in HA as a light (https://github.com/home-assistant/home-assistant/blob/dev/homeassistant/components/light/deconz.py). If 1 or more lights in a group have a color temperature and are switched on, the exposed group light will also get this attribute but it will be 0. (it does however allow you to set a color temperature to a group that will be forwarded to all lights in the group that support a color temperature.

I am unsure what would be the right place to fix this. In Deconz component? In trait.py? Or still in color_util.py? If 0 mired would translate to 0 kelvin and HA should be able to deal with 0 color_temperature values in general I believe this could be the right place to fix it but then return 0 in case the input is 0. Or should the component never set the color_temp to 0?

Hope above makes sense. I'm new to Github and HA so be patient with me :)

@balloob
Copy link
Member

balloob commented Jul 7, 2018

This should be fixed in Deconz. 0 is not a valid color temp. CC @Kane610

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla-needed core small-pr PRs with less than 30 lines.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants