-
Notifications
You must be signed in to change notification settings - Fork 112
HAP: wait for MTU to process reconnection event #773
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
Conversation
bumble/profiles/hap.py
Outdated
| @connection.on(connection.EVENT_CONNECTION_ATT_MTU_UPDATE) | ||
| def on_mtu_update(*_: Any) -> None: | ||
| if connection.peer_resolvable_address: | ||
| self.on_incoming_paired_connection(connection) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't this also wait for the connection to be encrypted?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, Spec mention that indication should be sent only when the encryption is done.
I am not sure what is the better way of doing this with bumble. I updated the patch to us on_encryption_change event.
Let me know if you see a better way
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thats looks fine.
But the on_pairing handler above should probably be removed, because when you're pairing, you'll also get an on_encryption_change callback. If you're connecting and were already previously paired, you'll also get just on_encryption_change, but no on_pairing. It's probably best to not end up calling on_incoming_paired_connection twice in the case where you are pairing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made a couple change to try to have a working scenario while implementing your comment.
I need on_pairing, because this event can be sent after the on_encryption_change, and we need to compare address between reconnection. When there is no resolvable address, it consider it's a new device and the history of preset changed is dropped.
We also should check the MTU as we don't want to truncate the content of the notification.
So I made multiples listener on all the event, and I made sure to make the method safe to be called multiples times.
Hopefully that should handle all the cases
cc52dd4 to
249da22
Compare
When HAP reconnect, it sends indication of all events that happen during the disconnection. But it should wait for the profile to be ready and for the MTU to have been negotiated or else the remote may not be ready yet. As a side effect of this, the current GattServer doesn't re-populate the handle of subscriber during a reconnection, we have to bypass this check to send the notification
|
Test failures are because you uploaded the commits when the git reference didn't pass. You can push new commits or close and re-open this PR to refresh it. |
|
@wescande Is it ready to merge? |
|
Yes it should be ready to merge |
When HAP reconnect, it sends indication of all events that happen during the disconnection.
But it should wait for the profile to be ready and for the MTU to have been negotiated or else the remote may not be ready yet.
As a side effect of this, the current GattServer doesn't re-populate the handle of subscriber during a reconnection, we have to bypass this check to send the notification