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

Support lights and the temperature sensor of the Echo device #1244

Merged
merged 3 commits into from
Apr 17, 2021

Conversation

blm126
Copy link
Contributor

@blm126 blm126 commented Apr 14, 2021

Closes #1237 and #1202.

This moves all of the phoenix calls being done by the alarm platform into the data update coordinator so that sensor/light platform can share. This involved refactoring of the guard code.

Device support intentionally excludes all Skill-based devices Alexa has access to as there is likely another Home Assistant integration that can better address those items. The light support is quite limited supporting only brightness and power state. Temperature sensor support is complete, but is restricted specifically to the sensor built-in to an Echo.

With the exception of the new guard code, everything should be hidden by a new option. Enabling this feature requires turning that option on and reloading the integration.

I assume there are likely changes needed here. In particular, I wasn't sure on the process for translations. I saw the other languages, but I don't speak them. Should I run my text through Google translate? I'm also completely new to coding for home assistant and struggled with the appropriate way to hide this behind an option. Its definitely hidden, but the "reload the component" step feels like I'm missing some obvious way to do this better.

This moves all of the phoenix calls being done by the alarm platform into the date update coordinator so that sensor/light can share. This involved significant refactoring of the guard code.

Device support intentionally excludes all Skill-based devices Alexa has access to as there is likely another Home Assistant integration that can better address those items. The light support still remains quite limited supporting only brightness and power state. Temperature sensor support is complete, but is restricted specifically to the sensor built-in to an Echo.

With the exception of the new guard code, everything should be hidden by an new option. Enabling this feature requires turning that option and reloading the integration.
Copy link
Owner

@alandtse alandtse left a comment

Choose a reason for hiding this comment

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

Thanks. As for translations, you just need to do English. We use lokalize and once you modify the strings.json it will appear as something for our translators to work on. You will need to modify strings.json though.

custom_components/alexa_media/__init__.py Outdated Show resolved Hide resolved
custom_components/alexa_media/alexa_entity.py Outdated Show resolved Hide resolved
custom_components/alexa_media/__init__.py Outdated Show resolved Hide resolved
custom_components/alexa_media/__init__.py Show resolved Hide resolved
custom_components/alexa_media/alarm_control_panel.py Outdated Show resolved Hide resolved
custom_components/alexa_media/alarm_control_panel.py Outdated Show resolved Hide resolved
custom_components/alexa_media/alexa_entity.py Show resolved Hide resolved
@alandtse
Copy link
Owner

Also, please plan to add a wiki entry explaining this new functionality plus the fact you have to enable it. I'll add the link into the release notes for this added feature.

…ted ids in logs, and better assumed_state checks.
@alandtse
Copy link
Owner

Feel free to resolve conversations that you believe are addressed and let me know when you think it's ready to merging.

…rge device json more fully into the existing structure, and to make strides towards integrating temperature just like the other sensor types.

Changes after code review to merge the alexa_entity property into the existing devices property.
@blm126
Copy link
Contributor Author

blm126 commented Apr 17, 2021

I have added some initial documentation to the Wiki here.

I believe everything is working and I have addressed all the issues you brought up and this is ready to merge from my side. Let me know if you see anything else you think should change.

Also, thanks for the prompt code reviews and helping this move forward! This integration was lots of fun already and getting this feature in is going to let me do a bunch of new things with HA.

@alandtse
Copy link
Owner

Thanks for putting this together. This was a long requested feature that I actually couldn't test myself. We'll leave this in dev for a week and then I'll do a new feature release. You may want to ask people on the forum to test it out.

@alandtse alandtse merged commit 26b4b51 into alandtse:dev Apr 17, 2021
@alandtse
Copy link
Owner

FYI, opened an issue on dev because the properties key probably doesn't exist somewhere even though options has not been enabled
#1249 please take a look

@alandtse
Copy link
Owner

alandtse commented Apr 17, 2021

Oh, I fixed 1249 pretty easily. I'll put in a PR for that.

However, I got this warning:

2021-04-17 22:29:36 WARNING (MainThread) [homeassistant.components.light] Light is deprecated, modify AlexaLight to extend LightEntity

Please convert.

Also, with the options, you can actually programmatically force a reload if you have the entry_id. Consider doing that.

     await self.hass.config_entries.async_reload(self.config_entry.entry_id)

This will expose emulated_hue lights as lights #1251. We should warn about that or figure out if we can identify emulated_hue lights and ignore them. HA will allow you to look at other domains so we should check the code to see if emulated_hue is enabled.

Lastly, I got many warnings about unique ids. We'll want to eventually resolve that if we can figure out what is causing it. Also note the warning about no data. We should see if that can be ignored after the first time it happens.

2021-04-17 22:31:00 INFO (MainThread) [homeassistant.helpers.entity_registry] Registered new light.alexa_media entity: light.dining_room_lights
2021-04-17 22:31:00 DEBUG (MainThread) [custom_components.alexa_media.alexa_entity] Coordinator has no data for 6********************************04d
2021-04-17 22:31:00 DEBUG (MainThread) [custom_components.alexa_media.alexa_entity] Coordinator has no data for 6********************************04d
2021-04-17 22:31:00 ERROR (MainThread) [homeassistant.components.light] Platform alexa_media does not generate unique IDs. ID 2ee676ce-34f4-4367-a959-39d38a024c21 already exists - ignoring light.kitchen_lights
2021-04-17 22:31:00 ERROR (MainThread) [homeassistant.components.light] Platform alexa_media does not generate unique IDs. ID af23f902-1169-4b20-836e-22b59fa8646d already exists - ignoring light.guest_room_light
2021-04-17 22:31:00 INFO (MainThread) [homeassistant.helpers.entity_registry] Registered new light.alexa_media entity: light.master_closet_light
2021-04-17 22:31:00 DEBUG (MainThread) [custom_components.alexa_media.alexa_entity] Coordinator has no data for f********************************94e
2021-04-17 22:31:00 DEBUG (MainThread) [custom_components.alexa_media.alexa_entity] Coordinator has no data for f********************************94e
2021-04-17 22:31:00 INFO (MainThread) [homeassistant.helpers.entity_registry] Registered new light.alexa_media entity: light.playstation
2021-04-17 22:31:00 DEBUG (MainThread) [custom_components.alexa_media.alexa_entity] Coordinator has no data for b********************************c39
2021-04-17 22:31:00 DEBUG (MainThread) [custom_components.alexa_media.alexa_entity] Coordinator has no data for b********************************c39
2021-04-17 22:31:00 ERROR (MainThread) [homeassistant.components.light] Platform alexa_media does not generate unique IDs. ID 6380364a-7bb2-4283-91c8-c701568980d4 already exists - ignoring light.stairs_switch
2021-04-17 22:31:00 INFO (MainThread) [homeassistant.helpers.entity_registry] Registered new light.alexa_media entity: light.raspberry_pi

@blm126 blm126 deleted the Lights branch April 18, 2021 00:46
alandtse added a commit that referenced this pull request Apr 24, 2021
* feat: add lights and the temperature sensors (#1244)

Closes #1237 and #1202. Since this is based on polling it is disabled by default and is enabled in options. Please check the instructions on the wiki.
https://github.com/custom-components/alexa_media_player/wiki#discover-and-control-devices-connected-to-an-echo

* fix: check for existence of properties key
closes #1249

* fix: TypeError: _typeddict_new() missing typename

* style: fix lint errors

* fix: auto reload when extended entity discovery is enabled (#1254)

* Automatically reload the integration when extended entity discovery is enabled.

* fix: reload only after all values processed

Co-authored-by: Alan Tse <[email protected]>

* fix: detect and ignore lights created by emulated_hue (#1253)

* Detect and ignore lights created by emulated_hue plus some general cleanup.

* Adjust naming and comments to be more accurate.

Co-authored-by: Brady Mulhollem <[email protected]>
@curt7000
Copy link

Closes #1237 and #1202.

This moves all of the phoenix calls being done by the alarm platform into the data update coordinator so that sensor/light platform can share. This involved refactoring of the guard code.

Device support intentionally excludes all Skill-based devices Alexa has access to as there is likely another Home Assistant integration that can better address those items. The light support is quite limited supporting only brightness and power state. Temperature sensor support is complete, but is restricted specifically to the sensor built-in to an Echo.

With the exception of the new guard code, everything should be hidden by a new option. Enabling this feature requires turning that option on and reloading the integration.

I assume there are likely changes needed here. In particular, I wasn't sure on the process for translations. I saw the other languages, but I don't speak them. Should I run my text through Google translate? I'm also completely new to coding for home assistant and struggled with the appropriate way to hide this behind an option. Its definitely hidden, but the "reload the component" step feels like I'm missing some obvious way to do this better.

Is there a way to selectively support an Alexa Skill here? I have one Alexa Skill with no HA integration (Frigidaire Dehumidifier) that would be amazing to turn off with an automation when our TV is on.

I've got a workaround using your integration where I am sending a voice command to Alexa, but actual status of the unit would be helpful in other ways.

@blm126
Copy link
Contributor Author

blm126 commented Apr 24, 2021

The limitation around Alexa Skills is hard coded in the current implementation. There isn't anything in the Alexa API stopping support for skill based devices. However, I don't have any that I could test. Especially for a dehumidifier, I have no idea how Alexa would report that. I don't imagine it would be much different than lights, but its probably different enough that it would need someone with access to the actual device to implement it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support lights(and other devices?) integrated into Alexa
3 participants