nguyer / homeassistant-bond-home

38 stars 15 forks source link

Add 4-6 Fan Speed Support #37

Closed DanPatten closed 3 years ago

DanPatten commented 4 years ago
niemyjski commented 4 years ago

Thanks for the pr, Is there documentation on this speed levels that we can detect and add these speeds? Or is this something we made up? How do we know the fan supports this many levels? I don't really like the names medium, low, medium high etc.. it's not very clear and these if blocks need some love as it looks like a logic bug awaiting to happen.

Also can you do separate pr's for the other bug fixes. The name one scares me a bit for two reasons: Where is this device name coming from (I didn't see it before but it could be a newer feature) and secondly we used the location if available to help with duplicate devices like a fan in two different rooms.

DanPatten commented 4 years ago

I can split that off. For the fan speeds HA does not support more than 3 speeds and don't plan on it anytime soon, if you have ideas for better fan speed names or ideas I'm all open. I wanted to maintain the minimum of the 3 speed names they use as those funnel into other things like Google Assistant, etc. The logic I am using will technically work for more than 6 speed fans but won't be as nice as 6 or less speed fans. I tried to maintain the same logic being used for low, med and high and base the other speeds based off of that.

See: https://github.com/home-assistant/core/blob/dev/homeassistant/components/fan/init.py and https://github.com/home-assistant/architecture/issues/127

As for the name change that is only only switches that did not follow the naming rule, when testing this I noticed it was returned in the device's name field like fans do. I'm guessing bond made an update to their API.

niemyjski commented 4 years ago

Can we rework that if logic to be more straight forward and instead of multiple mediums, we add ones below low and above high? Thoughts? What if we add even more speeds later? We need something that scales

marciogranzotto commented 4 years ago

Can we rework that if logic to be more straight forward and instead of multiple mediums, we add ones below low and above high? Thoughts? What if we add even more speeds later? We need something that scales

Agreed. I think there are fans with up to 8 speeds, this should be generic considering only max_speed

DanPatten commented 4 years ago

So I think a better solution would be to use speed numbers or a percentage slider as some people suggested in the related thread. Since HA only supports low, med and high at the moment none of those solutions work until they change the architecture of fan entity.

niemyjski commented 4 years ago

Is it even possible to detect how many speeds are supported out of the box? I know for my fireplace it's time based duration for flame level and impossible to know what the current setting is or if you hit the limit via bond. I'm thinking this is probably the same for fans. My fan has 3 speeds but I've never seen anything returned about max levels but it would be really nice to expose multiple levels if we know they exist or a percentage based. Is it possible to store metadata on a bond device to where maybe you manually add a key to bond via the api and then we know how many levels are supported and out of the box it defaults to 3?

DanPatten commented 4 years ago

Yes the bond device properties returns a max speed variable so if it returns 6, then you know speeds 1-6 exist and so on.

niemyjski commented 4 years ago

Where are we at on this? It would be nice to get any solution merged in I just don't feel like medium prefixed names is the solution (but I'm not completely opposed). But it's kind of weird making up other names too.

chrismccracken commented 4 years ago

Would it be feasible to map the speeds to a dimmer control interface to allow the granularity and flexibility for different models? Ie for a 6-speed model: 0-16%=Speed 1, 17-33%=Speed 2, etc...

DanPatten commented 4 years ago

We could make it like a dimmer but HA Core does not support that meaning bond fans will not sync with other integrations that do not use the high, medium, low speeds. It appears there is discussion to change fans to dimmer fine tuning control. Not sure what we want to do in the meantime.

niemyjski commented 4 years ago

I'm almost wondering if we should bring this discussion up with the core project now that they officially have a bond integration that's pretty well done. A dimmer wouldn't seem that bad. I think most of us think of fans with the conceptual model of 3 modes.

niemyjski commented 3 years ago

What's the consensus on this? Should we just merge this or have you tried getting this in the HA Core version?

marciogranzotto commented 3 years ago

@niemyjski I think it would be nicer to just implement everything in the HA Core version since everyone will have that by default

cajuncoding commented 3 years ago

So I’m confused...

Are there actually two different components? I already switched to the built in integration with core already — because it seemed obvious that the integration had been finally adopted into core.

Why would two versions be maintained unless there is a concrete difference between the two? And if that’s the case then it should be explained clearly in the readme for this custom component.

But yeah I agree that we should continue improvements with the core integration :-)

niemyjski commented 3 years ago

I agree

Sent from my iPhone

On Sep 26, 2020, at 2:41 PM, Marcio Granzotto Rodrigues notifications@github.com wrote:

 @niemyjski I think it would be nicer to just implement everything in the HA Core version since everyone will have that by default

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

DanPatten commented 3 years ago

I agree since this exists in core I will abandon this request and move the conversation there