bluekitchen / btstack

Dual-mode Bluetooth stack, with small memory footprint.
http://bluekitchen-gmbh.com
Other
1.65k stars 595 forks source link

Multiple PBAP/GOEP instances #620

Open amorniroli opened 3 weeks ago

amorniroli commented 3 weeks ago

Is your feature request related to a problem? Please describe. From my understanding of the source codes, PBAP and GOEP (partially) implementations are "singleton", meaning that it would not possible to connect to multiple devices.

Describe the solution you'd like The same behavior as, for example, a2dp/hfp/etc, where connections are taken from mempools (e.g. MAX_NR_PBAP_CONNECTIONS).

Describe alternatives you've considered /

Additional context In the current implementation, it may happend that pbab/goep client is never released if putting stack to power down.

For instance, consider the following scenario:

    pbap_connect (packet_handler, address, &cid);
    hci_power_control (HCI_POWER_OFF); // called before pbab connection is established
    ...
    hci_power_control (HCI_POWER_ON);
    ...

    pbap_connect (packet_handler, address, &cid); // will fail for BTSTACK_MEMORY_ALLOC_FAILED

The same would happend in the following scenario

    pbap_connect (packet_handler, address, &cid);
    hci_power_control (HCI_POWER_OFF); // called before pbab connection is established
    ...
    hci_power_control (HCI_POWER_ON);
    ...
    pbap_client_deinit ();
    pbap_client_init ();
    pbap_connect (packet_handler, address, &cid); // will fail for BTSTACK_MEMORY_ALLOC_FAILED caused by goep_client_create_connection)
mringwal commented 3 weeks ago

What functionality do you need to use from PBAP Client? Could you serialize the requests, e.g. read phonebook from first device, disconnect, then read phonebook from second device? Or do you need to use PBAP client dynamically, e.g. on incoming call?

amorniroli commented 3 weeks ago

What functionality do you need to use from PBAP Client? Could you serialize the requests, e.g. read phonebook from first device, disconnect, then read phonebook from second device? Or do you need to use PBAP client dynamically, e.g. on incoming call?

Dynamically (e.g. on incoming call, to retrieve extra info by phonebook).

I'll try to serialize requests: it should work (except for the issue reported in "Additional context" of the first message: I was updating it while you replied).

amorniroli commented 3 weeks ago

In addition, it would be useful to extend supported application parameters, at least for max_list_count and list_start_offset.

For the moment, I've resolved with the following patch (see attached patch.txt, created from 6d3263ec491c8c502d2f33782fd7ae497db4ee9d).

patch.txt

Alessandro

mringwal commented 3 weeks ago

Hi there. There's nothing wrong with extending the implementation to support multiple connections, the main change is the memory management. We've started to require the client to provide memory for its requests instead of relying on fixed pools. Just need to come up with a good function name for pbap_client_connect_xxx.

Thanks for pointing out the issue with the power cycle.

Also thanks for the app param setters. I will apply that right away.

mringwal commented 3 weeks ago

set start offset / max list count -> 04f86d0

mringwal commented 2 weeks ago

Re: power cycle. I've tried to call pbap_connect(), power off, wait for state == off, power on, and call pbap_connect(), this works. Did you wait for state == off before calling power control on again?

Update: I'm running on mac, which uses malloc, so there won't be an memory issue. Let me double-check...

Double-check: found an issue in goep_client, but it did not cause a problem with pbap_connect after power cycle.

mringwal commented 2 weeks ago

Hi @amorniroli - we've reworked pbap client to support multiple parallel connections with the new pbap_client_connect function. A quick test with the pbap_client_demo looked good. Please let me know if it works as expected with two or more mobile phones.

Memory needs to be provided by the application - a scheme we've now prefer over configurable memory pools (as they need configuration)

amorniroli commented 1 week ago

Hi @amorniroli - we've reworked pbap client to support multiple parallel connections with the new pbap_client_connect function. A quick test with the pbap_client_demo looked good. Please let me know if it works as expected with two or more mobile phones.

Memory needs to be provided by the application - a scheme we've now prefer over configurable memory pools (as they need configuration)

Hi @mringwal ,

today I've finally found some time to test the feature.

I think a fix is still needed inside pbap_client_connect: it's calling goep_client_create_connection, which use singleton, so no multiple connections are allowed.

E.g. consider the following consecutive calls, with different clients

pbap_client_connect (&client0, ...) ... pbap_client_connect (&client1, ...) -> will faill because of singleton

image

Should be implemented using goep_client_connect instead of goep_client_create_connection?

mringwal commented 1 week ago

Hi @amorniroli oh! you're totally right. For this to work, the pbap_client_connect also needs the L2CAP ERTM configuration + memory to pass on (and another singleton to support the old pbap_connect call). We'll look into this in the next days