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

Check-in command support #446

Merged
merged 3 commits into from
Nov 7, 2021
Merged

Check-in command support #446

merged 3 commits into from
Nov 7, 2021

Conversation

kennylevinsen
Copy link
Contributor

This implements support for receiving and responding to check-in commands. If there are any pending messages, fast poll is requested and the queue is emptied. Otherwise, fast poll is declined.

Notes:

  • The device configure is still responsible for binding the cluster, as the messages will otherwise never arrive.
  • Implicit check-in is disabled if we receive a check-in message, but maybe it would be nicer if it was controlled together with the binding setup (device.enablePollClusters())?
  • The check-in command still bubbles up as a message, so if the device lacks a genPollCtrl commandCheckIn converter, zigbee2mqtt will log an error. We could let the message be consumed, but the "checkin_presence" converter currently relies on it.

Comments welcome.

Related to #445

@Koenkk
Copy link
Owner

Koenkk commented Nov 3, 2021

Implicit check-in is disabled if we receive a check-in message, but maybe it would be nicer if it was controlled together with the binding setup (device.enablePollClusters())?

Good point, we can do this in the configure() and save this in the database, now useImplicitCheckin is not persisted across restarts.

The check-in command still bubbles up as a message, so if the device lacks a genPollCtrl commandCheckIn converter, zigbee2mqtt will log an error. We could let the message be consumed, but the "checkin_presence" converter currently relies on it.

Can be solved easily by adding genPollCtrl to https://github.com/Koenkk/zigbee2mqtt/blob/b3e9afaf3ee55e2d62f6775dc0c62e8550b8fef1/lib/extension/receive.ts#L117

I see the tests are currently failing, can you update these and make sure the new code is covered? Use npm tst and npm run test-with-coverage.

@kennylevinsen
Copy link
Contributor Author

Good point, we can do this in the configure() and save this in the database, now useImplicitCheckin is not persisted across restarts.

Take a look at configurePollControl and see what you think. I find the interface slightly awkward, but on the other hand I feel like it would be very manual to use poll control if people had to bind and flip implicitCheckin themselves.

I see the tests are currently failing, can you update these and make sure the new code is covered? Use npm tst and npm run test-with-coverage.

Done, although tests fail on master right now.

@Koenkk
Copy link
Owner

Koenkk commented Nov 4, 2021

Sorry for the failing tests, I've fixed this now in master.
Reviewed the code, looks good to me so if you rebase and CI checks pass I will merge this.

@kennylevinsen
Copy link
Contributor Author

Sorry for the failing tests, I've fixed this now in master.

It happens - I was going to submit a fix, but you beat me to it. :)

Rebased. Now we're just failing a lint on a file that hasn't been changed.

@Koenkk
Copy link
Owner

Koenkk commented Nov 5, 2021

Rebased. Now we're just failing a lint on a file that hasn't been changed.

It fails in src/zcl/zclFrame.ts which has been changed

The genPollCtrl checkin command is used by end-devices to occasionally
give their clients control over their poll mode in order to communicate
with them reliably.

Utilize this to reliably sync pending messages.
@kennylevinsen
Copy link
Contributor Author

It fails in src/zcl/zclFrame.ts which has been changed

... Apologies, I must have forgotten to put my eyeballs back in. You're right, and fixed.

@Koenkk
Copy link
Owner

Koenkk commented Nov 7, 2021

Haha 👀 , thanks!

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.

None yet

2 participants