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

Fix notification events order #964

Merged

Conversation

ashway83
Copy link
Contributor

♻️ Current situation

Empirically proven that Apple backend processes multiple characteristics submitted in the same request in reverse order. In the current version, the characteristics array is populated in chronological order. In some situations, the incorrect order of characteristics can lead to the setting of an obsolete characteristic value.

For example, some Zigbee devices report the current state before the change which causes consequent updates within a very short time window. In this case, HAP-NodeJS sends multiple characteristics in the same request.

2022-08-26T21:04:30.554Z HAP-NodeJS:EventedHTTPServer:Connection [::ffff:10.10.10.139] HTTP request: /characteristics
2022-08-26T21:04:30.554Z HAP-NodeJS:HAPServer [0E:3B:79:C7:EE:E4] HAP Request: PUT /characteristics
2022-08-26T21:04:30.555Z HAP-NodeJS:Accessory [Homebridge EEE4 3B31] Processing characteristic set: {"characteristics":[{"aid":25,"iid":11,"value":1}]}
[8/27/2022, 12:04:30 AM] [zigbee2mqtt] Pending data: {"state_left":"ON"}
2022-08-26T21:04:30.556Z HAP-NodeJS:EventedHTTPServer [::ffff:10.10.10.139] Muting event '25.11' notification for this connection since it originated here.
[8/27/2022, 12:04:30 AM] [zigbee2mqtt] Publish to 'zigbee2mqtt/0x00124b001802eebd/set': '{"state_left":"ON"}'
2022-08-26T21:04:30.557Z HAP-NodeJS:Accessory [Homebridge EEE4 3B31] Setting Characteristic "On" to value 1
2022-08-26T21:04:30.559Z HAP-NodeJS:EventedHTTPServer:Connection [::ffff:10.10.10.139] HTTP Response is finished
[8/27/2022, 12:04:30 AM] [zigbee2mqtt] Handled device update for sw-living-room-2: {"linkquality":98,"state_bottom_right":"ON","state_left":"OFF","state_right":"ON"}
[8/27/2022, 12:04:31 AM] [zigbee2mqtt] Handled device update for sw-living-room-2: {"linkquality":94,"state_bottom_right":"ON","state_left":"ON","state_right":"ON"}
2022-08-26T21:04:31.098Z HAP-NodeJS:EventedHTTPServer:Connection [::ffff:10.10.10.139] Sending HAP event notifications [ { aid: 25, iid: 11, value: 0 }, { aid: 25, iid: 11, value: 1 } ]

The characteristics are submitted in chronological order: 0 (Off), 1(On)

[
   {
      "aid":25,
      "iid":11,
      "value":0
   },
   {
      "aid":25,
      "iid":11,
      "value":1
   }
]

The accessory after switching On flips to Off since HomeKit backend processes characteristics in reverse order.

💡 Proposed solution

The trivial fix is to reverse the characteristics array before submission in writeEventNotification function.

notification.characteristics.reverse();

⚙️ Release Notes

➕ Additional Information

Testing

Conducted several tests locally.

Reviewer Nudging

Apple backend processes events in reverse order, so we need to reverse the array so that events are processed in chronological order.
@ashway83
Copy link
Contributor Author

ashway83 commented Sep 8, 2022

Hi, @Supereg @KhaosT as top contributors, could you please check this PR? It remains unattended for a while already.

@Supereg
Copy link
Member

Supereg commented Sep 8, 2022

Sorry for the delay. Will have a look at it later 👍🏼

@coveralls
Copy link

coveralls commented Sep 8, 2022

Pull Request Test Coverage Report for Build 3043870799

  • 0 of 1 (0.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.004%) to 51.753%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/lib/util/eventedhttp.ts 0 1 0.0%
Totals Coverage Status
Change from base Build 3043869701: -0.004%
Covered Lines: 5718
Relevant Lines: 10091

💛 - Coveralls

@Supereg Supereg changed the base branch from master to beta-0.10.3 September 13, 2022 09:15
@Supereg Supereg merged commit d256005 into homebridge:beta-0.10.3 Sep 13, 2022
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