robmarkcole / Hue-remotes-HASS

PLEASE READ THE README
Apache License 2.0
31 stars 9 forks source link

Simplification and better HA integration #7

Closed azogue closed 4 years ago

azogue commented 4 years ago

cc @robmarkcole, don't merge this one to master yet.

Description

Simplified to its minimal expression, and fully covered, with better HA integration. Instead of handling our data and listen to bridge changes, the platform is registered in the bridges, so it operates like the main platforms sensor and binary_sensor.

Now we do things like HA and rely on its data coordinator to handle the aiohue sensors.

Details

Notes about convergence with HA

From this point on, the next steps up to a PR for the official integration would require better integration, by using config_registry, and some minor changes inside the main hue, so IMO we stop here for the CC, as it is easier to do it on HA directly

(I'm already working on it on https://github.com/home-assistant/core/compare/dev...azogue:feature/add-remotes-to-hue)

Also, I have to thank @balloob for his guidance on the remotes refactor :)

BREAKING CHANGE

This PR relies on a small change on HA Core which is not included in the last HA release, so this one won't work on production systems, be advised!

azogue commented 4 years ago

Do not trust Travis on this one :)

Access to HA is mocked in tests. Under HA v0.106.6 it will miserably fail ;-)

robmarkcole commented 4 years ago

Ok will wait for HA 0.108 before merge

codecov-io commented 4 years ago

Codecov Report

Merging #7 into master will increase coverage by 9.61%. The diff coverage is 97.36%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master       #7      +/-   ##
==========================================
+ Coverage   87.91%   97.53%   +9.61%     
==========================================
  Files           4        3       -1     
  Lines         240       81     -159     
==========================================
- Hits          211       79     -132     
+ Misses         29        2      -27
Impacted Files Coverage Δ
custom_components/hueremote/implemented_remotes.py 100% <100%> (ø)
custom_components/hueremote/remote.py 95.45% <95%> (-4.55%) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 5bcc1c2...a3e41bb. Read the comment docs.

elmigbot commented 4 years ago

Hello, it looks like this will split the Lutron Aurora into two remotes, the button as ZLLSwitch, and the dial as ZLLRelativeRotary. Is there anyway to link them together? Otherwise it could be confusing when more than one is implemented.

klada commented 4 years ago

Since HASS 0.108 is out and has support for remotes (thanks @azogue) this could be merged, right?

robmarkcole commented 4 years ago

Appears the test failed

azogue commented 4 years ago

Now it is ready to work seamlessly with the latest HA Core, I think, @robmarkcole

BUT, now that remotes are recognized in main hue, this CC is not really necessary anymore (I think that is a good thing!). But this PR makes it work better with HA anyway.

Let's review the current status in the HA-Hue ecosystem:

IMO (but I cannot be objective here, obviously), the best combination is to use fasthue + eventsensor. And if you don't need to 'see' some entity, the latter is not required. And the former is just to set a faster refresh rate (or make it dynamic). If comfortable with the 5s of the main hue, then no CC is needed!

azogue commented 4 years ago

BTW, the 2 new CC's that I pointed are already in queue to be added to HACS:

but in the meanwhile, they can be used by adding the repos in the HACS config.

The fasthue one could even be installed and executed without any HA restart! (it was a beautiful surprise to me)

robmarkcole commented 4 years ago

OK great work @azogue! The hue CC ecosystem is getting quite busy, but you give a good summary above. Where do you think is the best location to document this?