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

Don't treat different broadcast msgs from same device as duplicate #256

Merged
merged 3 commits into from
Dec 18, 2020

Conversation

tstabrawa
Copy link
Contributor

@tstabrawa tstabrawa commented Dec 15, 2020

This fixes a problem that I noticed recently where when some broadcast messages aren't received by my PLM and the corresponding cleanup message is received, the cleanup message is being dropped by Insteon-MQTT. Upon digging in to the code, I noticed that the Broadcast handler had been written to attempt to avoid double-handling broadcasts and their corresponding cleanup messages by dropping any cleanup messages that follow a broadcast from the same device. In the case of my i2cs devices, this meant that even if a Success Report Broadcast message from a previous button press had been received, then a cleanup message for a subsequent button press would be treated as a duplicate of the Success Report Broadcast message.

To fix, I made the Broadcast handler's duplicate check also consider whether the broadcast messages have the same command and group number.

Tests performed: (see attached test log - cleanup-fix-testing.txt)

  • Confirmed that all new pytest tests fail before fix and pass after fix
  • Confirmed no new flake8 errors
  • Confirmed no new pylint errors
  • Confirmed all old & new pytest tests are passing
  • Reviewed test coverage report for substantial new gaps
  • Confirmed fixed code works in real life
    • Using an i2cs plug-in dimmer module, turned device off and on using physical buttons
    • Observed that Success Report Broadcast message was received for the off command, broadcast message for on command was not received, and cleanup message was processed by Insteon-MQTT.
  • Dog-food testing (~24 hours with Home Assistant 0.118.5, 24 hours with Home Assistant 2020.12.0, no problems observed)

Attachments:

cleanup-fix-testing.txt

Confirms that cleanup message is processed even if last broadcast received is from the same device (but not the same command/group).
Avoids problem where All-link cleanups for button presses (cmd1=0x11) are dropped if they come after (e.g.) a success report broadcast (cmd1=0x06) from a previous button press.
@krkeegan
Copy link
Collaborator

Good catch, that makes sense.

@krkeegan krkeegan merged commit 5c083fa into TD22057:dev Dec 18, 2020
@tstabrawa tstabrawa deleted the cleanup-fix branch December 20, 2020 00:44
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