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

[wiz] Initial contribution #17681

Open
wants to merge 21 commits into
base: main
Choose a base branch
from
Open

[wiz] Initial contribution #17681

wants to merge 21 commits into from

Conversation

ccutrer
Copy link
Contributor

@ccutrer ccutrer commented Oct 31, 2024

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:

  • Renamed binding to just wiz, instead of wizlighting, since they offer non-lighting smart devices as well
  • 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
  • Renamed bulbMacAddress and bulbIpAddress configuration parameters and properties to just macAddress and ipAddress
  • Added temperature-abs channel
  • Added fan speed, reverse, and mode channels
  • Added fan-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.
  • Fixed INCREASE/DECREASE to speed channel changing temperature
  • Fixed caching of fan state channel after a command
  • 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
  • Changed lots of references from "bulb" to "device"
  • Address several compilation warnings

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]>
@ccutrer ccutrer added the new binding If someone has started to work on a binding. For a new binding PR. label Oct 31, 2024
@ccutrer ccutrer requested a review from a team as a code owner October 31, 2024 21:42
@ccutrer
Copy link
Contributor Author

ccutrer commented Oct 31, 2024

@frejos: thoughts on renaming this to wizconnected as part of this, instead of wizlighting, since it supports smart plugs and fans, not just lights?

@lsiepel
Copy link
Contributor

lsiepel commented Nov 1, 2024

+1 for the renaming.

@jlaur
Copy link
Contributor

jlaur commented Nov 1, 2024

thoughts on renaming this to wizconnected

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]>
@ccutrer ccutrer changed the title [wizlighting] Initial contribution [wiz] Initial contribution Nov 1, 2024
Copy link
Contributor

@jlaur jlaur left a 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/README.md Outdated Show resolved Hide resolved
Copy link
Contributor

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. 🙂

Copy link
Contributor Author

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.

@ccutrer
Copy link
Contributor Author

ccutrer commented Nov 4, 2024

@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 fan# channel group. But if your device only has a fan, then as this PR currently is is reasonable.

@sfussenegger
Copy link

@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]>
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]>
@ccutrer
Copy link
Contributor Author

ccutrer commented Nov 4, 2024

@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 fan-with-dimmable-bulb, but you can still manually create a fan (only) thing. It would be good for you to try both (even though you don't have a light with it - I manually created a fan-with-dimmable-bulb against my light, and tested those channels). Also, I set the range of the fan's speed channel as 0-6, but perhaps it's supposed to be 1-6? It would also be nice if you could turn on DEBUG logging for org.openhab.binding.wiz.internal.utils.WizPacketConverter, and let me know the getModelConfig response. I suspect available fan speeds are in there, and then I can dynamically set the min/max based on the actual device.

Copy link
Contributor

@jlaur jlaur left a 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.

Signed-off-by: Cody Cutrer <[email protected]>
@sfussenegger
Copy link

@ccutrer I've tested today, and found that fanState is a boolean but the fan returns it as an int (0 or 1 I believe), leading to a gson exception. I've tried fixing it, but for some reason I can't get Eclipse to update the dependency. I'll have to look into this with a little more time.

@ccutrer
Copy link
Contributor Author

ccutrer commented Nov 11, 2024

@ccutrer I've tested today, and found that fanState is a boolean but the fan returns it as an int (0 or 1 I believe), leading to a gson exception. I've tried fixing it, but for some reason I can't get Eclipse to update the dependency. I'll have to look into this with a little more time.

Oh, interesting. I specifically changed to a boolean because I saw that StateRequestParam was using a boolean for state, and assumed the serialization must be auto-translating 0/1 to that. When I have some time (hopefully this evening) I'll see if I can find some attributes to control the serialization like that. If not, I'll revert it back to an integer.

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]>
@ccutrer
Copy link
Contributor Author

ccutrer commented Nov 14, 2024

@sfussenegger: I reverted the serialization to ints.

@sfussenegger
Copy link

@ccutrer I can verify this works now. A few things I've noticed:

Channels

The new binding offers the following channels when installed via inbox as fan:

  • Power (fanState)
  • Fan Speed (fanSpeed)
  • Reverse (fanReverse)
  • Mode (fanMode)

Missing:

  • Signal Strength
  • Last Update

When installling as fan-with-dimmabel-bulb these channels are available. It's probably best to use the same channel-groups for <thing-type id="fan"> minus light.

I you want to follow the behavior of the Wiz app you probably want to suggest creating a fan-with-dimmable-bulb by default. Currently it's fan. My model doesn't have a light but it doesn't seem to know that with state and dimming available options. Therefore it probably should have been discovered with having a light.

fanMode

Value must be either 1 or 2, 0 is invalid:

11:38:15.515 [WARN ] [ternal.utils.WizResponseDeserializer] - Bulb returned an error on method "setPilot":  -32600, Invalid Request

Raw response:

echo '{"method":"setPilot","params":{"fanMode":0}}' | nc -u -w 1 192.168.0.167 38899
{"method":"setPilot","env":"pro","error":{"code":-32600,"message":"Invalid Request"}}

Anything higher > 2 is treated as 2. So while this is implemented as a switch in the Wiz app it look more like there could be more modes added, just like light modes. Not sure how you'd like to proceed there, but in my opinion using multi-values modes for fanMode just like mode would make sense.

Therefore adapt thing-types.xml to make <channel-type id="fan-mode"> look more like <channel-type id="light-mode"> with <option value="2">Breeze</option>

@sfussenegger
Copy link

One more thing: I'm using the demo app in openhab-distro/launch/app to test after adding org.openhab.binding.wiz as a dependency. However, whenever I restart the app the binding doesn't load with previously created things show as NOT_YET_READY. It feels like I'm the one doing something wrong, not the binding. Or am I? Maybe you can tell me what I'm doing wrong. I am a Java developer and use Eclipse every day, but the OSGI stuff is new to me and just can't figure out what the issue is and it's driving me nuts. Thanks!

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]>
@ccutrer
Copy link
Contributor Author

ccutrer commented Nov 17, 2024

One more thing: I'm using the demo app in openhab-distro/launch/app to test after adding org.openhab.binding.wiz as a dependency. However, whenever I restart the app the binding doesn't load with previously created things show as NOT_YET_READY. It feels like I'm the one doing something wrong, not the binding. Or am I? Maybe you can tell me what I'm doing wrong. I am a Java developer and use Eclipse every day, but the OSGI stuff is new to me and just can't figure out what the issue is and it's driving me nuts. Thanks!

I'm not familiar with the demo app. I just use the OSGi console (using install -s <full file URI to JAR> for the initial install, and then update org.openhab.binding.wiz <full file URI to JAR> each time I recompile. And my things go offline momentarily, then immediately back online.

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!

Copy link

@sfussenegger sfussenegger left a 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?

<channels>
<channel id="brightness" typeId="system.brightness"/>
<channel id="mode" typeId="light-mode"/>
<channel id="speed" typeId="light-mode-speed"/>

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)

Copy link
Contributor Author

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.

Copy link
Contributor

@jlaur jlaur left a 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;
Copy link
Contributor

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?

Suggested change
int newDimming = 50;
int newDimming;


private void handleIncreaseDecreaseTemperatureCommand(boolean isIncrease) {
float oldTempPct = colorTempToPercent(mostRecentState.getTemperature()).floatValue();
float newTempPct = 50;
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here:

Suggested change
float newTempPct = 50;
float newTempPct;


private void handleIncreaseDecreaseSpeedCommand(boolean isIncrease) {
int oldSpeed = mostRecentState.speed;
int newSpeed = 50;
Copy link
Contributor

Choose a reason for hiding this comment

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

And here:

Suggested change
int newSpeed = 50;
int newSpeed;

Comment on lines +456 to +459
updateStatus(ThingStatus.UNKNOWN);
if (ValidationUtils.isMacNotValid(config.macAddress)) {
updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.CONFIGURATION_ERROR, "MAC address is not valid");
}
Copy link
Contributor

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:

Suggested change
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");
Copy link
Contributor

Choose a reason for hiding this comment

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

@lsiepel - I see you are working on #17804. Should this be:

Suggested change
newThread.setName("wizReceiverThread");
newThread.setName("OH-binding-wizReceiverThread");

?

Copy link
Contributor

@lsiepel lsiepel Nov 27, 2024

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"));
Copy link
Contributor

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"/>
Copy link
Contributor

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.
Copy link
Contributor

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?

Suggested change
_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:

Suggested change
_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 |

Copy link
Contributor

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
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
## Example item linked to a channel
## Example Item Linked To a Channel

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new binding If someone has started to work on a binding. For a new binding PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants