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

Expose GE Z-Wave in wall fan switch as a fan #105

Merged
merged 1 commit into from
Jun 8, 2018
Merged

Expose GE Z-Wave in wall fan switch as a fan #105

merged 1 commit into from
Jun 8, 2018

Conversation

vickyg3
Copy link
Contributor

@vickyg3 vickyg3 commented Jun 6, 2018

Wink recognizes this as a dimmer switch for some reason. But in
reality it is just a fan switch with 3 speeds (translating to
brightness levels of 33%, 66% and 100% in the dimmer switch).

This patch extends WinkFan to specialize the case of GE Z-Wave
in wall fan switch and exposes it as a fan instead of a light
bulb. I've also added a sample json for the tests.

This way home assistant will expose this as a fan without any
changes on the user's part.

Example device: https://www.amazon.com/GE-Controls-Required-SmartThings-14287/dp/B06XTKQTTV/ref=sr_1_1_sspa

Wink recognizes this as a dimmer switch for some reason. But in
reality it is just a fan switch with 3 speeds (translating to
brightness levels of 33%, 66% and 100% in the dimmer switch).

This patch extends WinkFan to specialize the case of GE Z-Wave
in wall fan switch and exposes it as a fan instead of a light
bulb. I've also added a sample json for the tests.

This way home assistant will expose this as a fan without any
changes on the user's part.
@coveralls
Copy link

coveralls commented Jun 6, 2018

Coverage Status

Coverage increased (+0.03%) to 93.552% when pulling a3d9c41 on vickyg3:master into b69878a on python-wink:master.

@bradsk88 bradsk88 self-requested a review June 6, 2018 19:57
@bradsk88
Copy link
Collaborator

bradsk88 commented Jun 6, 2018

Will look this over this evening

@vickyg3
Copy link
Contributor Author

vickyg3 commented Jun 6, 2018

@bradsk88 sure, thanks!

Copy link
Collaborator

@bradsk88 bradsk88 left a comment

Choose a reason for hiding this comment

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

Looks good to me.

One thing that comes to mind is that when I originally took ownership of this project, my intention was to have it act as a thin-wrapper for the Wink API.

That is to say: not having special logic like this.

I wonder if you could move some of this logic to live in a dedicated module. Something like special_cases.py or something...

Either way. Feel free to merge.

@vickyg3
Copy link
Contributor Author

vickyg3 commented Jun 7, 2018

Thanks for the review!

I wonder if you could move some of this logic to live in a dedicated module. Something like special_cases.py or something...

The custom code has test coverage. So i didn't think it was necessary to put it in a custom module. There's also other custom logic (like exposing some cameras as sensors). So i think this is fine. :)

Either way. Feel free to merge.

I don't have write access to this repo to merge. Could you please merge it?

@bradsk88
Copy link
Collaborator

bradsk88 commented Jun 8, 2018

It was more of a stylistic request than any sort of acceptance criteria.

Just a heads up that I'll probably try to refactor this to the way I described later. Behaviour should remain unchanged.

@bradsk88 bradsk88 merged commit 09bbca2 into python-wink:master Jun 8, 2018
@vickyg3
Copy link
Contributor Author

vickyg3 commented Jun 8, 2018

Thanks!

Could you please also create a new release on pypi so that i can import this into home assistant?

@bradsk88
Copy link
Collaborator

bradsk88 commented Jun 9, 2018

@vickyg3 Sorry for the delay. Published!

@vickyg3
Copy link
Contributor Author

vickyg3 commented Jun 9, 2018

Thank you! :)

The home assistant PR is here: home-assistant/core#14894

@GaryOkie
Copy link

This is a very much appreciated HA improvement to Wink's silly grouping of this in-wall fan control as a light dimmer. However, for the two identical Wink "fans" I have using this GE/Jasco In-wall fan control, one is being recognized by Hassio perfectly as a fan and the other is not being properly recognized as either a light or fan and becomes an unusable entity.

I will submit a detailed issue on this separately.

@GaryOkie
Copy link

To add further to the OP's comment "Wink recognizes this as a dimmer switch for some reason", I had contacted Wink support about it.

Thank you for reaching out to Wink. I can absolutely put this forward as a feature request for you. The reason why it adds as a light is because the the GE fan switch is not officially supported, so when it pairs, it pairs typically as a dimmer.

@vickyg3
Copy link
Contributor Author

vickyg3 commented Jun 27, 2018

the other is not being properly recognized as either a light or fan and becomes an unusable entity.

Ah i am sorry about that. I thought this was a safe change as it picked up only fans with a specific manufacturer_device_model. May be there are multiple devices with the same model?

Could really help to have a sample device json to look into this further.

@GaryOkie
Copy link

GaryOkie commented Jun 28, 2018

It may just be an edge case of when a "fan speed" (aka light brightness) is zero that wink/fan.py is not expecting. Both fans are using the identical GE Zwave fan control. I'm not sure how to provide the device json, but here's the detail I see for each fan entity when they are set to either low, med, or high speed...

entity: fan.den_ceiling_fan
speed: high
speed_list: low,medium,high
manufacturer_device_model: ge_jasco_in_wall_fan
device_manufacturer: ge
model_name: In-Wall Fan Control
friendly_name: Den Ceiling Fan
supported_features: 5

entity: fan.bedroom_ceiling_fan
speed: low
speed_list: low,medium,high
manufacturer_device_model: ge_jasco_in_wall_fan
device_manufacturer: ge
model_name: In-Wall Fan Control
friendly_name: Bedroom Ceiling Fan
supported_features: 5

Both fans are working fine now - but as a test, I set each fan speed to as low as the slider will go via the Wink interface and restarted HASSIO. Yep, the wink/fan.py errors returned and now neither fan appears in the HASSIO interface.

I would expect that you will be able to duplicate this.

Thanks!

`

@vickyg3
Copy link
Contributor Author

vickyg3 commented Jun 28, 2018

Moving the discussion to #107.

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.

None yet

4 participants