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

Allow enable/disable CM in App mode and disconnect/reconnect on failed commands #77

Merged

Conversation

iranl
Copy link
Contributor

@iranl iranl commented Nov 1, 2024

  • CM
    According to the API Continuous mode can only be disabled/enabled when paired as a Bridge using the LockAction.
    When paired as app CM can still be enabled/disabled using the separate command available for this (0x0057), although this does require a valid PIN to be set.

  • Disconnect on failed commands
    NUKI_ALT_CONNECT mode tries to reuse connections as much as possible. This can lead to stuck connections in some cases. This PR disconnects (and reconnects) when commands fail instead of trying to reuse the failing connection (when NUKI_ALT_CONNECT is defined).

@technyon
Copy link
Collaborator

technyon commented Nov 2, 2024

"Disconnect on failed commands" seems a very good idea. Shouldn't we just make it the default bahavior and remove the #ifdef ?

@technyon
Copy link
Collaborator

technyon commented Nov 2, 2024

Did you check if CM is a problem like this in bridge mode? I had paired and bridge mode and never had a problem settting CM. Maybe it's not enforced as of now, but sticking to the API is a good thing anyway.

@iranl
Copy link
Contributor Author

iranl commented Nov 2, 2024

API only says this is only not allowed in App mode. So this PR also only changes this behaviour when not in Bridge mode.

Disconnect on failed is mainly an issue in the alt connect mode, which leans more heavily on trying to reuse connections.

Changes (and alt connect mode) could be made default if no issues arise when used for a while in NukiHub.

@technyon
Copy link
Collaborator

technyon commented Nov 2, 2024

Lets enable alternative mode for the upcoming test. Please remove the cast to int as suggested, then we can merge.

@iranl
Copy link
Contributor Author

iranl commented Nov 2, 2024

Cast to int has been removed.

Alt connect mode has been enabled in NukiHub for a couple of months.

@technyon technyon merged commit 0b4e590 into I-Connect:main Nov 2, 2024
3 checks passed
@iranl iranl deleted the fix-cm-and-reconnect-on-failed-commands branch November 3, 2024 12:02
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