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

Overhaul Emulated Hue #28317

Merged
merged 4 commits into from Dec 2, 2019
Merged

Overhaul Emulated Hue #28317

merged 4 commits into from Dec 2, 2019

Conversation

NobleKangaroo
Copy link
Contributor

@NobleKangaroo NobleKangaroo commented Oct 29, 2019

Breaking Change:

  • As previous entityid were not Hue compliant, emulated_hue_ids.json will need to be cleared out and devices re-discovered and configured in Alexa / Google Home. This is a necessary growing pain to move the component forward in a Hue-compliant fashion.

Description:

  • Emulated Hue should not support brightness values 0 or 255, as the Hue API states that brightness should range from 1 to 254. This is causing Alexa to produce "Device doesn't support requested value" errors. Clamping brightness, hue, saturation and color temperature to Hue API-compatible values resolves this issue.

  • Adjusted response codes for errors and unauthorized request responses:

    • Converted all responses to self.json_message format and removed aiohttp import
    • Changed some HTTP_NOT_FOUND to HTTP_UNAUTHORIZED where appropriate
  • Changed "uniqueid" to match Hue bridge format:

    • Format: 00:%s:%s:%s:%s:%s:%s:%s-%s
    • Noticed that when using some hex prefixes, Alexa would not detect some devices; prefixing everything with 00: seems to resolve that issue. A new store-bought Hue bridge also contains MACs beginning with 00: so I believe this is the right way to go about this.
  • Added support for all light types:

    • On/off light: Supports groups, scenes and on/off control
    • Dimmable light: Supports groups, scenes, on/off and dimming
    • Color temperature light: Supports groups, scenes, on/off, dimming, and setting of a color temperature
    • Color light: Supports on/off, dimming and color control (hue/saturation, enhanced hue, color loop and XY)
    • Extended Color light: Same as Color light, but which supports additional setting of color temperature
  • General code cleanups; examples:

    • Removed parse_hue_api_put_light_body() as after code refactoring and cleanup, I found that HueOneLightChangeView was the only place that references it. It could always be broken out again if a call is needed elsewhere in the code, but I don't believe that's the case.
    • Within HueOneLightStateView() - success response creation routine
    • Within get_entity_state() - clamping routine
  • To Do:

    • Several spots throughout, the code sets brightness to 0 or 255; this is technically outside of the limits of the Hue API, but with clamping there is little or no effect; that is, Alexa never sees the true value, so leaving this as-is is of no consequence. To correct this, we need to discuss how to best handle this as - at present time - other apps such as the LIFX app can set the value of lights to 0 or 255. Generally speaking, these devices weren't ever designed to be Hue bridge compliant, so perhaps the clamping that's being done is sufficient. The only negative effect of this is if you use Alexa to set a light to 100%, the other app (LIFX in this example) sees the brightness as 99% (as floor(254/255) * 100 = 99)

Related issue (if applicable):

Pull request with documentation for home-assistant.io (if applicable): home-assistant/home-assistant.io#<home-assistant.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
  • 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 communicates with devices, web services, or third-party tools:

  • (N/A) The manifest file has all fields filled out correctly. Update and include derived files by running python3 -m script.hassfest.
  • (N/A) New or updated dependencies have been added to requirements_all.txt by running python3 -m script.gen_requirements_all.
  • (N/A) Untested files have been added to .coveragerc.

If the code does not interact with devices:

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

@@ -393,141 +459,66 @@ def __init__(self, config):
hass.services.async_call(domain, service, data, blocking=True)
Copy link
Member

Choose a reason for hiding this comment

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

Why is this a create_task with blocking=True. In that case you could also just do async_call with blocking=False.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why is this a create_task with blocking=True. In that case you could also just do async_call with blocking=False.

Not sure ( I didn't write that :) ). I would ponder it's to give the device time to update before returning a response to Alexa / Google. Will test with blocking=False and report in later with the result.

Copy link
Member

@balloob balloob left a comment

Choose a reason for hiding this comment

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

This looks great! Can you remove the commented line and please add yourself as a codeowner in the manifest.

Dev automation moved this from Needs review to Reviewer approved Nov 26, 2019
}
return {
unique_id = hashlib.md5(entity.entity_id.encode()).hexdigest()
unique_id = "00:%s:%s:%s:%s:%s:%s:%s-%s" % (
Copy link
Member

Choose a reason for hiding this comment

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

Please use f-strings instead of old style string formatting.

Copy link
Member

Choose a reason for hiding this comment

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

Here I would actually just do a ":".join(("00", val1, val2, …))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here I would actually just do a ":".join(("00", val1, val2, …))

While that would work to generate most of the unique_id, there's actually a hyphen before the last two pairs of hex characters which would have to be added post-join(). As such, there's not a "clean" way to represent it with join():

unique_id = ":".join((
    "00",
    unique_id[0:2],
    unique_id[2:4],
    unique_id[4:6],
    unique_id[6:8],
    unique_id[8:10],
    unique_id[10:12],
    unique_id[12:14],
)) + "-" + unique_id[14:16]

Pro: Much easier to read and modify
Con: Concatenation on top of a join
 

With f-strings that "problem" goes away, but the string overall is more difficult to read and perhaps could cause confusion for editors in the future:

# "Plain" - don't care about string length
unique_id_fstring = f"00:{unique_id[0:2]}:{unique_id[2:4]}:{unique_id[4:6]}:{unique_id[6:8]}:{unique_id[8:10]}:{unique_id[10:12]}:{unique_id[12:14]}-{unique_id[14:16]}"

# "Tidy" - don't use lines longer than 80 characters results in:
unique_id_fstring2 = f"00:{unique_id[0:2]}:{unique_id[2:4]}:{unique_id[4:6]}:" \
                     f"{unique_id[6:8]}:{unique_id[8:10]}:{unique_id[10:12]}:" \
                     f"{unique_id[12:14]}-{unique_id[14:16]}"

Pro: Uses f-strings (ooohhh, shiny)
Con: A bit harder to read
 

I don't currently have the ability to test with Alexa if changing the hyphen to a colon would cause issues (personally I found issue with Alexa not discovering devices without a leading 00:) and won't have the ability to do so for at least a few days to weeks, but I would think that it actually shouldn't be removed as the hyphen does in fact exist in the Hue API's official documentation, as well as the output from a Hue bridge.

Either of you guys care to chime in? I didn't make any change yet and to be honest, I'm not entirely impressed with either solution. If I had to make a pick, though, I'd go with the join() for code readability. That said, Home Assistant code style would be to use f-strings so with that in mind, I am absolutely fine with following suit and calling it a day. Appreciate your thoughts!

Copy link
Member

Choose a reason for hiding this comment

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

okay, in that case let's use

Suggested change
unique_id = "00:%s:%s:%s:%s:%s:%s:%s-%s" % (
unique_id = "00:%s:%s:%s:%s:%s:%s:%s-%s".format(

Anything but % :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@balloob Done, committed. Thanks!

@balloob balloob merged commit 3f2b6bf into home-assistant:dev Dec 2, 2019
Dev automation moved this from Reviewer approved to Done Dec 2, 2019
@balloob
Copy link
Member

balloob commented Dec 2, 2019

🎉

@NobleKangaroo NobleKangaroo mentioned this pull request Dec 2, 2019
9 tasks
@lock lock bot locked and limited conversation to collaborators Dec 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Dev
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants