-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
[wiz] Initial contribution #17681
base: main
Are you sure you want to change the base?
[wiz] Initial contribution #17681
Conversation
This binding integrates [WiZ Connected](https://www.wizconnected.com/en-US/) smart devices. This is a successor to the community marketplace version of this binding, most recently maintained by @frejos, bringing it up to date with openHAB guidelines and standards, to be part of the main distribution. User visible changes: * Renamed thing type, channel type, and channel ids to be `lower-case-hyphen` * Removed `wiz-` prefix from thing type ids * Removed `dimming` and `state` channels from `color-bulb` (use `color` channel) * Removed `state` channel from `tunable-bulb` (use `brightness` channel) * Removed `state` channel from `dimmable-bulb` (use `brightness` channel) * Renamed `fan-state` channel to just `state` * Renamed `dimming` channel to `brightness` * Renamed `light-mode` channel to just `mode` * Added `temperature-abs` channel * Fixed INCREASE/DECREASE to `speed` channel changing temperature * Set color temperature to UNDEF when not in color temp mode * Calculate the effective color when in color temp mode Internal changes: * Removed recreation of thing types based on binding version (openHAB core supports update instructions to handle this automatically) * Use PercentType.ZERO, PercentType.HUNDRED, DecimalType.ZERO as appropriate * Splits updates for fan and light apart, to reduce confusion * Remove lots of debugging-style logging * Address several compilation warnings Co-authored-by: Joshua Freeman <[email protected]> Co-authored-by: Stefan Fussenegger <[email protected]> Co-authored-by: Sara Damiano <[email protected]> Signed-off-by: Cody Cutrer <[email protected]>
Signed-off-by: Cody Cutrer <[email protected]>
Signed-off-by: Cody Cutrer <[email protected]>
@frejos: thoughts on renaming this to wizconnected as part of this, instead of wizlighting, since it supports smart plugs and fans, not just lights? |
+1 for the renaming. |
Or perhaps simply wiz? Disclaimer: just did a quick search, see for example https://faq.wizconnected.com/hc/en/3-wiz-legacy/faq/35-what-is-wiz-and-how-does-it-work/. And in HA also seems to be just WiZ. |
including renaming files and packages Signed-off-by: Cody Cutrer <[email protected]>
Signed-off-by: Cody Cutrer <[email protected]>
Signed-off-by: Cody Cutrer <[email protected]>
Signed-off-by: Cody Cutrer <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Many thanks for your contribution! I just had a very quick look and already made a few minor comments. The code already looks nice and clean. 👍 I don't know when/if I would personally have time to review the full binding, so decided to push these comments now.
bundles/org.openhab.binding.wiz/src/main/resources/OH-INF/thing/thing-types.xml
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think it would be possible to support suggestion finder? See for example #16277. This is obviously not a requirement, just a thought. 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good idea. I'll look into it as a separate PR. It might take me a bit to test it, though. Discovery doesn't work for me at all at the moment, because my WiZ bulbs are on a private VLAN, and right now the binding will only listen on the first IP address on the machine. I need to address that as well.
....openhab.binding.wiz/src/main/java/org/openhab/binding/wiz/internal/WizBindingConstants.java
Outdated
Show resolved
Hide resolved
....openhab.binding.wiz/src/main/java/org/openhab/binding/wiz/internal/WizBindingConstants.java
Outdated
Show resolved
Hide resolved
...g.openhab.binding.wiz/src/main/java/org/openhab/binding/wiz/internal/handler/WizHandler.java
Outdated
Show resolved
Hide resolved
...inding.wiz/src/main/java/org/openhab/binding/wiz/internal/config/WizDeviceConfiguration.java
Outdated
Show resolved
Hide resolved
@sfussenegger: what kind of fan do you have? I was browsing through the pywizlighting repo, and noticed their not-yet-merged-but-seemingly-abandoned PR seems to imply that a fan also has a light. If that's the case, we should put the fan-related channels into a separate |
@ccutrer the model I have doesn't have a light, though the controller seems to be supporting it (including an unused connector). So the answer regarding my model is yes and no I guess. The WiZ app discovered it as a fan with light too, but it was possible to change the auto-discovered type to fan-only, suggesting both types are supported by WiZ and should probably be supported by the addon. Great to see this is going into the core modules. Just let me know if you need any help with the fan stuff, I'm ready to help. My previous PR may contain some valuable information too. |
Signed-off-by: Cody Cutrer <[email protected]>
by putting light and fan channels into groups. default discovery will detect fans as a fan-with-light, just like the WiZ app. a fan-only device will have to be manually created Signed-off-by: Cody Cutrer <[email protected]>
Signed-off-by: Cody Cutrer <[email protected]>
replace it with "device" where relevant, or just remove it completely as a prefix to the config options macAddress and ipAddress Signed-off-by: Cody Cutrer <[email protected]>
and remove extraneous debugging logging Signed-off-by: Cody Cutrer <[email protected]>
Signed-off-by: Cody Cutrer <[email protected]>
@sfussenegger: I've updated the fan support significantly, but don't have a fan to test with. I'd appreciate if you could pull this down and try it out. Note that you'll have to re-create your things, and re-link your items. If you use discovery, it should detect it as a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm caught up with a few other things, so decided to push some pending comments. I hope you are okay with these small iterations. I now have only 8 files left to check a bit deeper.
....openhab.binding.wiz/src/main/java/org/openhab/binding/wiz/internal/WizBindingConstants.java
Outdated
Show resolved
Hide resolved
...inding.wiz/src/main/java/org/openhab/binding/wiz/internal/config/WizDeviceConfiguration.java
Outdated
Show resolved
Hide resolved
...inding.wiz/src/main/java/org/openhab/binding/wiz/internal/discovery/WizDiscoveryService.java
Outdated
Show resolved
Hide resolved
...nhab.binding.wiz/src/main/java/org/openhab/binding/wiz/internal/handler/WizMediatorImpl.java
Outdated
Show resolved
Hide resolved
...g.wiz/src/main/java/org/openhab/binding/wiz/internal/runnable/WizUpdateReceiverRunnable.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Cody Cutrer <[email protected]>
@ccutrer I've tested today, and found that |
Oh, interesting. I specifically changed to a boolean because I saw that |
Signed-off-by: Cody Cutrer <[email protected]>
Signed-off-by: Cody Cutrer <[email protected]>
so don't bother stating them. still leave the falses (even the double false, which could technically just not have an @expose, to make it clear they're not part of the JSON) Signed-off-by: Cody Cutrer <[email protected]>
@sfussenegger: I reverted the serialization to ints. |
@ccutrer I can verify this works now. A few things I've noticed: ChannelsThe new binding offers the following channels when installed via inbox as
Missing:
When installling as I you want to follow the behavior of the Wiz app you probably want to suggest creating a
|
One more thing: I'm using the demo app in |
Signed-off-by: Cody Cutrer <[email protected]>
so a fan-only thing would need to be manually created Signed-off-by: Cody Cutrer <[email protected]>
since it's 1/2, not 0/1, and theoretically more could be added Signed-off-by: Cody Cutrer <[email protected]>
I'm not familiar with the demo app. I just use the OSGi console (using It should be noted that anytime I change the structure of the thing within this PR (adding, modifying, or removing channels), you'll have to delete and re-create it. Once the binding has been released as part of an official openHAB release, any further changes will require me to do "update instructions" that will automatically make changes to existing things to bring them up to date with the new structure. So don't expect the signal-strength channel to instantly show up on your fan-only thing! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fan works well, only a small error in WizHandler
left.
Possible enhancement: Would there be a way to tell items created for fanSpeed
channel to use stepper widgets by default?
...g.openhab.binding.wiz/src/main/java/org/openhab/binding/wiz/internal/handler/WizHandler.java
Outdated
Show resolved
Hide resolved
<channels> | ||
<channel id="brightness" typeId="system.brightness"/> | ||
<channel id="mode" typeId="light-mode"/> | ||
<channel id="speed" typeId="light-mode-speed"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe "dimmable-light" group is missing a state
channel (it's independent of fanState
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is quite intentional. According to openHAB guidelines, when a light supports brightness, it should only have the dimmer channel, not a switch+dimmer channel. If you somehow have a non-dimmable thing hooked up to a device that normally supports a dimmable thing, you can always just link a switch item to a dimmer channel. Dimmer items can still also accept ON/OFF commands, and some UIs have a way to present that (I'm more familiar with BasicUI rather than MainUI; for BasicUI it's the switchSupport
option on a Slider
widget; Colorpicker
widgets also present an "on" and "off" button of some form when trying to send an ON or OFF command to a Color item). You can also just connect a switch widget to a dimmer item.
Signed-off-by: Cody Cutrer <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe I'm through all files now. I have provided a few additional comments from this walk-through.
|
||
private void handleIncreaseDecreaseCommand(boolean isIncrease) { | ||
int oldDimming = mostRecentState.dimming; | ||
int newDimming = 50; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This value seems unused, thus misleading?
int newDimming = 50; | |
int newDimming; |
|
||
private void handleIncreaseDecreaseTemperatureCommand(boolean isIncrease) { | ||
float oldTempPct = colorTempToPercent(mostRecentState.getTemperature()).floatValue(); | ||
float newTempPct = 50; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here:
float newTempPct = 50; | |
float newTempPct; |
|
||
private void handleIncreaseDecreaseSpeedCommand(boolean isIncrease) { | ||
int oldSpeed = mostRecentState.speed; | ||
int newSpeed = 50; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And here:
int newSpeed = 50; | |
int newSpeed; |
updateStatus(ThingStatus.UNKNOWN); | ||
if (ValidationUtils.isMacNotValid(config.macAddress)) { | ||
updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.CONFIGURATION_ERROR, "MAC address is not valid"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To avoid first updating to UNKNOWN, then immediately after that to OFFLINE when having invalid MAC address configured:
updateStatus(ThingStatus.UNKNOWN); | |
if (ValidationUtils.isMacNotValid(config.macAddress)) { | |
updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.CONFIGURATION_ERROR, "MAC address is not valid"); | |
} | |
if (ValidationUtils.isMacNotValid(config.macAddress)) { | |
updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.CONFIGURATION_ERROR, "MAC address is not valid"); | |
} else | |
updateStatus(ThingStatus.UNKNOWN); | |
} |
Also, should we return immediately after setting status OFFLINE, or should we call the next two methods in this scenario?
Thread newThread = new Thread(newReceiver); | ||
newThread.setDaemon(true); | ||
newThread.start(); | ||
newThread.setName("wizReceiverThread"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yers, allmost. Just a dash between the binding id and the optional part and then it's perfect. I would also prefer to have it set as part of the Thread constructor overload at line 178.
Something like:
Thread newThread = new Thread(newReceiver, "OH-binding-"+bindingconstants.BINDING_ID+"-ReceiverThread");
method = WizMethodType.valueOf(properCaseMethod); | ||
deserializedResponse.setMethod(method); | ||
} catch (IllegalArgumentException e) { | ||
logger.warn("Bulb returned an invalid method: {}", jobject.get("method")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and here.
<channel id="temperature-abs" typeId="system.color-temperature-abs"/> | ||
<channel id="mode" typeId="light-mode"/> | ||
<channel id="speed" typeId="light-mode-speed"/> | ||
<channel id="signal-strength" typeId="system.signal-strength"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think it's worth adding the RSSI as an advanced channel, since it's available and the signal strength is actually derived from that?
- WiZ Smart Plugs | ||
- Smart fans (with or without a dimmable light) | ||
|
||
_Note_ This binding was created for and tested on the full color with tunable white bulbs, however, users have reported success with other bulb types and plugs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if colon is missing?
_Note_ This binding was created for and tested on the full color with tunable white bulbs, however, users have reported success with other bulb types and plugs. | |
_Note:_ This binding was created for and tested on the full color with tunable white bulbs, however, users have reported success with other bulb types and plugs. |
Or perhaps like in line 54:
_Note_ This binding was created for and tested on the full color with tunable white bulbs, however, users have reported success with other bulb types and plugs. | |
**NOTE:** This binding was created for and tested on the full color with tunable white bulbs, however, users have reported success with other bulb types and plugs. |
| speed | Dimmer | Speed of the color changes in dynamic light modes | R/W | | ||
| signal-strength | Number | Quality of the bulb's WiFi connection | R | | ||
| last-update | Time | The last time an an update was received from the bulb | R | | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think some fan channels are missing here, e.g. mode
and reverse
. Also some channels seem to have same name, but different meaning, so maybe needs to be provided per Thing type? For example speed
is different for fan.
- Fade in/out times are configured in the app. | ||
- Sending too many commands to the bulbs too quickly can cause them to stop responding for a period of time. | ||
|
||
## Example item linked to a channel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
## Example item linked to a channel | |
## Example Item Linked To a Channel |
This binding integrates WiZ Connected smart devices.
This is a successor to the community marketplace version of this binding, most recently maintained by @frejos, bringing it up to date with openHAB guidelines and standards, to be part of the main distribution.
User visible changes:
wiz
, instead ofwizlighting
, since they offer non-lighting smart devices as welllower-case-hyphen
wiz-
prefix from thing type idsdimming
andstate
channels fromcolor-bulb
(usecolor
channel)state
channel fromtunable-bulb
(usebrightness
channel)state
channel fromdimmable-bulb
(usebrightness
channel)fan-state
channel to juststate
dimming
channel tobrightness
light-mode
channel to justmode
bulbMacAddress
andbulbIpAddress
configuration parameters and properties to justmacAddress
andipAddress
temperature-abs
channelspeed
,reverse
, andmode
channelsfan-with-dimmable-bulb
type, for a device that has a fan and a bulb; this is how new fan devices are detected. Fan-only devices cannot be reliably detected, so if that's what you have, you'll need to create it manually.speed
channel changing temperatureUNDEF
when not in color temp modeInternal changes: