nguyer / homeassistant-bond-home

38 stars 15 forks source link

Fix #1 - Added fireplace support and much more #13

Closed niemyjski closed 4 years ago

niemyjski commented 4 years ago

Please note: This is my first time really writing python so any feedback or pointers are very very welcome! I've been a professional programmer for a long time and felt it was pretty simple to pick up but I'm still learning.

Remaining Items

Completed Items

niemyjski commented 4 years ago

@bcb2000, @marciogranzotto, @nguyer, @a200462790, @Timp103 Can you please review this pr!

nguyer commented 4 years ago

Thanks for opening this! I’ll try to review it this weekend!

niemyjski commented 4 years ago

@nguyer I've been running this in my HA Prod instance for 3 days now. The only bug I've really ran into is with the fireplace (I haven't been able to test the other types but they do not through errors during setup). Here's the issue and it's going to have to be solved at some point but I'm not sure how...

The HA has a brightness value of 0-255 and the bond controller has a brightness of 0-100. Problem is all devices seem to operate differently like I saw the comment for the hard coded fan having 3 levels. I can definitely see more than that but at the same time, every fan I've ever owned has had 3. For example, my fireplace has a flame up and flame down and calling SetFlame doesn't do anything useful it just triggers the RF call with the converted brightness level. BUT how my fireplace actually works is it moves the flame valve up and down while HOLDING the RF signal... So I'm really really not sure how to get brightness to work reliably as well as get the max flame level as bond doesn't know that and I don't know how they would.

TLDR; Brightness is there on the fireplace but probably isn't going to work the greatest...

nguyer commented 4 years ago

The HA has a brightness value of 0-255 and the bond controller has a brightness of 0-100.

Okay great. Makes sense. We probably just need to divide the HA brightness by 2.55 and round, to map it to the bond controller brightness. I would assume that brightness is always in the range 0 - 100 in the Bond API. This is true of my dimmable fan lights. Though maybe that's not a safe assumption. I'm trying to look it up in the docs, but it appears the doc site is down right now.

Problem is all devices seem to operate differently like I saw the comment for the hard coded fan having 3 levels. I can definitely see more than that but at the same time, every fan I've ever owned has had 3.

I think this is actually a separate thing, and I think it's already addressed by the fact that fan devices report their maximum speed as an attribute.

socbrian commented 4 years ago

@nguyer are you going to merge this branch into the main one or keep this separate?

niemyjski commented 4 years ago

I actually did the math to convert it, several other integrations do it already as part of the core repo

nguyer commented 4 years ago

@nguyer are you going to merge this branch into the main one or keep this separate?

I have a couple more things I want to test and possibly tweak in the feature/fan-speed branch, and then I'll merge it to master. Still can't change the speed of my fans from the dropdown in lovelace. It's not an issue with this PR though.

niemyjski commented 4 years ago

Also if someone wants to cache the get devices and get device info on init and store it on the domain object, we could save a boatload of time on startup 4-10 seconds

Sent from my iPhone

On Jan 20, 2020, at 6:48 PM, Nicko Guyer notifications@github.com wrote:

 @nguyer are you going to merge this branch into the main one or keep this separate?

I have a couple more things I want to test and possibly tweak in the feature/fan-speed branch, and then I'll merge it to master. Still can't change the speed of my fans from the dropdown in lovelace. It's not an issue with this PR though.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or unsubscribe.

niemyjski commented 4 years ago

I've been running these changes for a few weeks without any issues. We should get this merged