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

White Light Bulb LB1 (0x60c7) support #341

Closed
wants to merge 7 commits into from
Closed

Conversation

santoxyz
Copy link

@santoxyz santoxyz commented Apr 17, 2020

White Light Bulb LB1 (0x60c7) support

broadlink/__init__.py Outdated Show resolved Hide resolved
broadlink/__init__.py Outdated Show resolved Hide resolved
Comment on lines +1070 to +1072
def set_power(self, state):
self.set_brightness(100)
self.set_state(state)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this method is necessary.

Copy link
Author

Choose a reason for hiding this comment

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

I wrote this as a hook to broadlink_cli . Am I wrong?

Copy link
Collaborator

@felipediel felipediel Oct 29, 2020

Choose a reason for hiding this comment

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

This class needs a rework. set_state() should be renamed to set_power(). And the new set_state() should get a dictionary as input. I intend to rework this class when I find some time, but I am working on a lot of things, so feel free to address this issue if you want to. I think we need a better interface for this class.

Ah, we don't want to send two packets with the same function. Our communication must be atomic, so client applications can handle exceptions.

Comment on lines +1074 to +1075
def check_power(self):
return self.get_state()
Copy link
Collaborator

Choose a reason for hiding this comment

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

The same for this one. Why a method just to call another method that already exists?

Copy link
Author

Choose a reason for hiding this comment

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

I wrote this as a hook to broadlink_cli . Am I wrong?

Comment on lines +1066 to +1068
def set_brightness(self, state):
cmd = '{"brightness":%d}' % state
self.send_command(cmd)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is good. We definitely need this method.

@felipediel
Copy link
Collaborator

This class was refactored. Now we use set_state() and get_state() to control the attributes. We can adapt the CLI in the future. Thank you!

@felipediel felipediel closed this Sep 11, 2021
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.

3 participants