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

Bug - Fix_multiple bugs incontroller blueprint for philips_324131092621 on Zigbee2MQTT #597

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

Conversation

nomike
Copy link

@nomike nomike commented Feb 3, 2024

Thank you for taking the time to work on a Pull Request. Your contribution is really appreciated! 🎉
Please don't delete any part of the template, since keeping the provided structure will help maintainers to review your work more rapidly.

Sections marked as * are required and need to be filled in.

Proposed change*

There are several bugs in the blueprint which make it unsuable when using Zigbee2MQTT.
I debugged the blueprint on my system and prepared this MR. Before submitting, I stumbled upon a few other PRs which address some of the issues exactly like I did, but not all of them.

Thus I still decided to go forward, so you can fix all issues with one single PR.

Merging this PR will solve the following issues:

#216
#406
#425
#457
#496
#541
#558
#576

and will make the following PRs obsolete:

#406
#542
#577

The fixes in more detail

Event mapping for zigbee2mqtt is wrong

There are two issues with that mapping. First of all, the events are sent with underscores, whereas the blueprint expects dashes. This prevents the script from working at all.

Secondly the event to listen for on short presses should be "on_press_release" and not "on_press". Listening to "on_press" triggers the script too early and too often. This causes all sorts of weird behaviour and a lot of race conditions. Listening to "on_press_release" solves this issue.

trigger_delta calculation is not working due to the regex not matching the helper value

The helper value looks like this:

{"a":"on_press_release","t":1706926380.358825}

The blueprint however, expects this:

{"a" :"on_press_release", "t" :1706926380.358825}

I've modified the regular expression to make whitespaces optional and also accept other types of whitespace (tab, newline) and multiple consecutive whitespaces. This should also make it future proof.

Automation should not run on "on_press" events

As the event mapping is now using "on_press_release" events, the whole automation should not run for "on_press" events anymore. This also causes race conditions and weird unpredictable behaviour where double presses are not recognized for example.

I've changed the trigger condition to be false if such an event is received.

Checklist*

  • I followed sections of the Contribution Guidelines relevant to changes I'm proposing.
  • I properly tested proposed changes on my system and confirm that they are working as expected.
  • I formatted files with Prettier using the command npm run format before submitting my Pull Request.

nomike added 4 commits February 3, 2024 03:33
- event names are with underscores (e.g. "on_hold" instead of "on-hold")
- short presses should happen on the "_press_release" event and not on "_press"
- Accept no whitspace behind json keys.
- Add sample value as comment
Copy link
Contributor

github-actions bot commented Feb 3, 2024

Hey @nomike, thank you so much for your contribution! 🚀

🔄 We're currently running a few checks to make sure that everything is great with your contribution.
If further actions need to be performed before your contribution can be reviewed, additional guidance will be provided to you in the next comment.

Results are coming soon, stay tuned!

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