HAP: wait for pairing event #551
Conversation
| await self._preset_changed_operation(connection) | ||
| # Update the active preset index if needed | ||
| await self.notify_active_preset_for_connection(connection) | ||
| @connection.on('pairing') # type: ignore |
There was a problem hiding this comment.
One issue here is that this only remembers paired connections when the pairing happens while the service is running. But what about connections from devices that were previously paired and bonded?
There was a problem hiding this comment.
I assume that setting up HAS is done before we connect to a remote device.
If an already paired device reconnect, it should go in this if:
if connection.peer_resolvable_address:
self.on_incoming_paired_connection(connection)directly
There was a problem hiding this comment.
A problem here is that peer_resolvable_address may not be set if identity information is not exchanged (though Android almost always does that). I didn't find a clear identity requirement in HAP (besides IAC https://www.bluetooth.com/specifications/specs/hap-1-0/ ), so I am afraid checking RPA is not very precise here.
Since what HAP requires is an LTK, maybe we can make on_connection an async function (using utils.AsyncRunner) and check whether LTK is available via device.get_long_term_key(...)?
There was a problem hiding this comment.
Example:
def __init__(
self, device: Device, features: HearingAidFeatures, presets: List[PresetRecord]
) -> None:
...
device.on('connection', self.on_connection)
@AsyncRunner.run_in_task()
async def on_connection(self, connection: Connection) -> None:
def on_disconnection(reason: int) -> None:
del reason # unused
# BTW, reference of connection might change here, so we had better freeze it with functool.partial
self.currently_connected_clients.remove(connection)
connection.on('disconnection', on_disconnection)
# TODO Should we filter on device bonded && device is HAP ?
self.currently_connected_clients.add(connection)
if (
connection.peer_address
not in self.preset_changed_operations_history_per_device
):
self.preset_changed_operations_history_per_device[
connection.peer_address
] = []
return
# Send all the PresetChangedOperation that occur when not connected
await self._preset_changed_operation(connection)
# Update the active preset index if needed
await self.notify_active_preset_for_connection(connection)
There was a problem hiding this comment.
But what about connections from devices that were previously paired and bonded?
In what scenario do we have previously paired and bonded devices without HAS knowing it ? I believe HAS is present since the device is "alive".
Could we open a bug to track this specific behavior instead and implement it later ? We would need this PR to land to merge some HAP Avatar test within Android.
|
My opinion is that Bumble is not designed for persistent usage, so it's better to do this kind of operations at the caller side, which has more (well-controlled) context of persistent states. |
|
@zxzxwu, the service need to notify every currently connected devices. The only persistent code in has is to update the device after re connection. I haven't implemented any persistent storage that would works between multiples bumble restart. They are only lasting as long as the current bumble is running. |
|
Is it possible to merge this PR ? |
|
I believe that the PR is ready and just need approval. I have a bumble related test that exercise the logic added here and require it in order to pass: |
|
If it's urgent, you may implement your services in Android or other
codebase like Avatar. As there isn't low level change involving the
implementation, change here is more like a contribution instead of a
necessary part of path.
Slvr ***@***.***> 於 2024年10月5日 週六 05:30 寫道：
… ***@***.**** commented on this pull request.
------------------------------
In bumble/profiles/hap.py
<#551 (comment)>:
> - self.currently_connected_clients.add(connection)
- if (
- connection.peer_address
- not in self.preset_changed_operations_history_per_device
- ):
- self.preset_changed_operations_history_per_device[
- connection.peer_address
- ] = []
- return
-
- async def on_connection_async() -> None:
- # Send all the PresetChangedOperation that occur when not connected
- await self._preset_changed_operation(connection)
- # Update the active preset index if needed
- await self.notify_active_preset_for_connection(connection)
+ @connection.on('pairing') # type: ignore
But what about connections from devices that were previously paired and
bonded?
In what scenario do we have previously paired and bonded devices without
HAS knowing it ? I believe HAS is present since the device is "alive".
Could we open a bug to track this specific behavior instead and implement
it later ? We would need this PR to land to merge some HAP Avatar test
within Android.
—
Reply to this email directly, view it on GitHub
<#551 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AWBGOLAL6TFH32S26BEXTF3ZZ4CIXAVCNFSM6AAAAABN7Z4LF2VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDGNBZGI4TANJVGY>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
I think it's Ok to merge the PR as is, even if the detection of previously bonded devices across a restart of the device/app isn't going to work. Support for persistence that way can be added later in a separate PR. |
|
I don't have much opinion on this PR besides #551 (comment). Checking pairing record with identity address is not a good approach. |
I agree that checking for |
HAP is supposed to work only when device are bonded.
By listening to the
on_pairingevent of the connection, we can start add the remote device to thepreset_changed_operations_history_per_deviceonly after the bonding (and thus the public address) has been done.This missing logic was found thanks to https://r.android.com/3236429, an avatar test that try to delete a preset and then wait for the notification / indication to be sent to the device