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

Merged
merged 26 commits into from
Dec 4, 2024
Merged

[wiz] Initial contribution #17681

merged 26 commits into from
Dec 4, 2024

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

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

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

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

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

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

@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!

@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?

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.

@lsiepel
Copy link
Contributor

lsiepel commented Dec 2, 2024

Just a little reminder if we want this in 4.3.0 we have about a week left as the feature freeze for 4.3.0 is in about a week.

@ccutrer
Copy link
Contributor Author

ccutrer commented Dec 2, 2024

I'm hoping to have time tomorrow to look at the rest of the comments.

@ccutrer
Copy link
Contributor Author

ccutrer commented Dec 3, 2024

@sfussenegger : what happens when you send 0 to the speed channel for a fan? Is it ignored? Is zero the lowest (but still moving) speed? If it's ignored, we should handle it as an OFF command.

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.

LGTM. Thanks for providing the PR and addressing the review comments, @ccutrer.

Should I add a sign-off (Also-by) for @frejos as well (in which case I'd need e-mail address and name)?

Anything more to test related to fans, or ready to be merged?

@ccutrer
Copy link
Contributor Author

ccutrer commented Dec 3, 2024

Should I add a sign-off (Also-by) for @frejos as well (in which case I'd need e-mail address and name)?

I added as many of the prior authors as I could identify (including @frejos) as Co-authored-by in the first commit. Do they need to be listed elsewhere as well?

Anything more to test related to fans, or ready to be merged?

Let's give @sfussenegger 24 hours to respond, and then merge if no response. If anything else comes up, it can be handled as a separate bug fix PR.

@jlaur
Copy link
Contributor

jlaur commented Dec 3, 2024

I added as many of the prior authors as I could identify (including @frejos) as Co-authored-by in the first commit. Do they need to be listed elsewhere as well?

No, that's perfectly fine, I just missed/forgot about that. Thanks!

@sfussenegger
Copy link

what happens when you send 0 to the speed channel for a fan? Is it ignored? Is zero the lowest (but still moving) speed? If it's ignored, we should handle it as an OFF command.

As far as I remember - and I'll re-check this evening - the command is simply ignored as 0 isn't a valid state (valid range is 1 to 6, at least for my model).

Anything else you would need me to check or test?

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]>
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]>
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]>
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]>
Number:Dimensionless with unit dB

also mark last-update as advanced - most people
don't care about it

Signed-off-by: Cody Cutrer <[email protected]>
Signed-off-by: Cody Cutrer <[email protected]>
mostly adjusting error handling, especially wrt not being able to find
openHAB's IP address or MAC address

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

ccutrer commented Dec 4, 2024

okay, I've added handling that command fan speed 0 sends an OFF command instead.

and rebased to resolve a conflict in CODEOWNERS.

I think we're ready to submit @jlaur

@sfussenegger
Copy link

just to confirm:

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

going through the comments here I've also noticed that I've never provided the getModelConfig output:

$ echo '{"method":"getModelConfig","params":{}}' | nc -u -w 3 192.168.0.167 38899 | jshon
{
 "method": "getModelConfig",
 "env": "pro",
 "result": {
  "ps": 1,
  "pwmFreq": 200,
  "pwmRange": [
   0,
   100
  ],
  "wcr": 20,
  "nowc": 1,
  "cctRange": [
   2700,
   2700,
   2700,
   2700
  ],
  "renderFactor": [
   255,
   0,
   255,
   255,
   0,
   0,
   0,
   0,
   0,
   0
  ],
  "fanSpeed": 6,
  "wizc1": {
   "mode": [
    0,
    0,
    0,
    0,
    0,
    0,
    0
   ]
  },
  "wizc2": {
   "mode": [
    0,
    0,
    0,
    0,
    0,
    0,
    0
   ]
  }
 }
}

@jlaur jlaur merged commit 044e9a3 into openhab:main Dec 4, 2024
5 checks passed
@jlaur jlaur added this to the 4.3 milestone Dec 4, 2024
@jlaur
Copy link
Contributor

jlaur commented Dec 4, 2024

I think we're ready to submit @jlaur

Thanks again. Now, you could add the binding's logo to the openHAB website. See https://www.openhab.org/docs/developer/addons/#add-your-add-on-s-logo-to-the-openhab-website

@openhab-bot
Copy link
Collaborator

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/wizlighting-binding-4-0-0-0-4-3-0-0/148164/23

@ccutrer ccutrer deleted the wizlighting branch December 5, 2024 21:52
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.

5 participants