search

rgomezjnr / wizcon

Control Philips WiZ Connected smart light bulbs
https://pypi.org/project/wizcon/
MIT License
39 stars 7 forks source link

Add brightness functionality #1

Closed ariqfadlan closed 3 years ago

ariqfadlan commented 3 years ago

Hi!

First, thank you for creating this command line tools! Next, I'd like to submit this PR to add brightness functionality, since I've started using it and feel like missing this important feature.

Comments or reviews are appreciated!

rgomezjnr commented 3 years ago

Hello ariqfadlan,

I'm glad you like the tool! Thank you for the PR. I have also wanted to add brightness control. I will review it and get back to you soon.

Do you happen to know what your Philips smart bulb information is? Have you had any issues using wizcon with your bulb? Mine is as follows reported from the Wiz app:

Philips A19 Firmware version 1.22.0 Model ID 23007

ariqfadlan commented 3 years ago

Here's some information about my bulb

Description Value
Model AE27
Firmware Version 1.17.1
Model ID 23032

Most importantly, my bulb is the Tunnable White variant--which lacks of RGB color. Given that, I don't find any issues as long as I supplied wizcon with supported scene (as per mobile app).

rgomezjnr commented 3 years ago

Thanks for the bulb info. I did a quick test of your pull request and the brightness is working, very nice. I should have some additional commits and be able to merge this coming week.

rgomezjnr commented 3 years ago

Ariq I see you removed the colored scenes since you have a white bulb. What happens if you use wizcon to send a non-supported color scene ID to your bulb? Does wizcon fail gracefully? e.g. command returns with error, or command is ignored? Or does it lock up or become unresponsive?

To support both RGB and white bulbs, it would be easiest if we left the entire RGB scene ID numbers intact. Otherwise we might have to add a -w white bulb switch or convert between RGB and white scene ID numbers.

ariqfadlan commented 3 years ago

What happens if you use wizcon to send a non-supported color scene ID to your bulb?

If I were to use wizcon with unsupported color command, my bulb will ignore that particular command and no error occurs.

To reproduce I could run $ wizcon -si 8 <IP addr> ON then the command will successfully execute with no error.

However, on this branch, I could also run $ wizcon -si 8 -b 20 <IP adddr> ON After that, the command still exits cleanly and the bulb will respond to brightness change only.

This is actually expected as per this Wiz help center page.

Ariq I see you removed the colored scenes since you have a white bulb.

I maintained my own fork for my personal use. This PR doesn't correlate with that functionality change. Only brightness-related enhancement will brought by this PR.

Should you want to examine the code change, please refer to https://github.com/rgomezjnr/wizcon/pull/1/files

Edit: fix typo

rgomezjnr commented 3 years ago

I understand, thanks, I was looking at your fork.

I've already incorporated your commits and added a few more in https://github.com/rgomezjnr/wizcon/tree/brightness. When I try to change your PR to my base branch brightness GitHub says there are no new commits. Should I simply close this PR now?

ariqfadlan commented 3 years ago

Sure! Thank you!

P.S. you've got a typo at this line 😄

When I try to change your PR to my base branch brightness GitHub says there are no new commits

Edit: I am not sure what are you trying to do. If you want to include your change to this PR, you could just push to my f/brightness branch. However, if you want to merge manually, simply close this and merge by yourself.

rgomezjnr commented 3 years ago

I corrected your name, thank you for catching that!

First I cloned your repo, then I realized it wouldn't work for me since I have the RGB bulb. So I fetched your PR using

git fetch origin pull/1/head:brightness

I'll just close this PR since your commits are in my brightness branch. In the future, if I receive a pull request on master and I want to pull it into a feature branch instead, should I first create a new branch, then change the base branch to the new branch in GitHub?

ariqfadlan commented 3 years ago

In the future, if I receive a pull request on master and I want to pull it into a feature branch instead, should I first create a new branch?

Yes, according to docs, you'd have to create a new branch, push it to GitHub, and let the PR author know to update the base branch--from master to feature branch.

then change the base branch to the new branch in GitHub?

I think this should be accomplished by the PR author, not the repo owner (unless the owner is one that open the PR).

rgomezjnr commented 3 years ago

Thanks for explaining and for the contribution. I manually merged and pushed brightness branch to master and GitHub automatically updated and closed the PR.