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

Fixes #310 "Other" IR Devices send commandOn and commandOff at reversed times #311

Merged
merged 4 commits into from
Apr 12, 2022

Conversation

jonzhan
Copy link

@jonzhan jonzhan commented Apr 12, 2022

As far as I can tell this fix makes sense but I have no idea how to test it on my local setup, sorry.

donavanbecker and others added 3 commits March 19, 2022 07:16
## [Version 1.12.8](https://github.com/OpenWonderLabs/homebridge-switchbot/releases/tag/v1.12.8) (2022-03-19)

## What's Changed

- Housekeeping and updated dependencies.

**Full Changelog**: OpenWonderLabs/homebridge-switchbot@v1.12.7...v1.12.8
Swapped calls to pushOffChanges and pushOnChanges and inverted downstream checks for this.Active
Fix for "Other" IR devices sending reversed On/Off commands
@donavanbecker donavanbecker merged commit c5072b9 into OpenWonderLabs:beta Apr 12, 2022
@donavanbecker
Copy link
Collaborator

I have pushed this to beta. you can test this on the latest beta version and let me know if it works for you?

@jonzhan
Copy link
Author

jonzhan commented Apr 12, 2022

Thank you, I can confirm that this works on v1.12.9-beta.23!

@donavanbecker
Copy link
Collaborator

@jonzhan awesome! I will release and update soon

@jonzhan
Copy link
Author

jonzhan commented Apr 12, 2022

Great. Thank you!

@jonzhan
Copy link
Author

jonzhan commented Apr 13, 2022

@donavanbecker I found another related bug (close enough that I don't think it needs another issue) - right now the code assumes that any instruction is a toggle of the device's state, so it works fine with simple switches but may send the wrong signal when using scenes.

I can cook up a fix, but I would like to ask for your input on what the desired behavior should be.

If, for example, a device is already "off" and a scene tries to turn the device off again, would you prefer that:

  1. an "off" IR signal should be sent anyway
  2. nothing should be done

@donavanbecker
Copy link
Collaborator

I guess we could make a config that would say do you want 1 or do you want 2 then users can choose.

@jonzhan
Copy link
Author

jonzhan commented Oct 13, 2022 via email

@donavanbecker
Copy link
Collaborator

Yes I have. I actually have a config called allow push for one of the other devices. But it's a SwitchBot device not Ir, so might as well add it. So we do not send, and only send when allowPush is yes in config.

@donavanbecker
Copy link
Collaborator

@jonzhan if you wanna put a PR together. I will ad the allowPush config

@jonzhan
Copy link
Author

jonzhan commented Oct 13, 2022

@donavanbecker Sounds good. I'll get a PR over in a day or so!

@donavanbecker
Copy link
Collaborator

No rush. Also have you upgraded to v2.0.0?

@jonzhan
Copy link
Author

jonzhan commented Oct 13, 2022 via email

@donavanbecker
Copy link
Collaborator

that was always there before but kinda changed things up on it.

@jonzhan
Copy link
Author

jonzhan commented Oct 14, 2022

I upgraded to 2.0 but unfortunately it has broken my IR devices, whoops! I'll make a separate ticket and circle back to this after that's figured out.

@donavanbecker
Copy link
Collaborator

@jonzhan V2.1.0 should have fixed that?

@jonzhan
Copy link
Author

jonzhan commented Oct 14, 2022 via email

@donavanbecker
Copy link
Collaborator

@jonzhan sorry. Okay once you open the ticket I will take a look.

@jonzhan
Copy link
Author

jonzhan commented Oct 14, 2022

No worries, wonder what it is! Created #520

@jonzhan
Copy link
Author

jonzhan commented Oct 15, 2022

I've made a bunch of changes to support allowPush in this PR - I inferred the necessary properties and logic based on the allowPush implementation in bot.ts, but I think it's correct.

@donavanbecker
Copy link
Collaborator

Okay latest beta should have the right change... I think.

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.

2 participants