[PATCH BlueZ v0 0/4] AVRCP connection-tracking issues

[PATCH BlueZ v0 0/4] AVRCP connection-tracking issues

[Mikel Astiz]

From: Mikel Astiz <mikel.astiz@bmw-carit.de>

This patchset addresses the issues reported by Alex Deymo in the thread "audio: Connect doesn't return when audio device is off". Extracted from his message:

"There are two ways to hit this problem:
* One is to attempt a connection when the device is off,
* the other one is to attempt a connection from the host right after
you short press the button with the bluetooth logo on the speakers.
This button normally reconnects the speakers to the host, but if you
attempt a connection while the device is also doing that, you end up
in the same situation."

I have been able to reproduce the first issue, which should be fixed with patch 1/4. The second issue is addressed in patch 3/4 but I couldn't actually test it.

Patch 4/4 tries to improve the AVRCP role heuristic, which could alone fix Alex's issues, but I think the core cannot rely on this heuristic nevertheless.

Mikel Astiz (4):
  avrcp: Fix missing reply to profile connect
  control: Remove unused parameter
  avrcp: Fix service connections not reported to core
  avrcp: Don't require active sink in role heuristic

 profiles/audio/avrcp.c   | 17 ++++-------------
 profiles/audio/control.c | 37 +++++++++++++++++++++++++++----------
 profiles/audio/control.h |  7 +++----
 3 files changed, 34 insertions(+), 27 deletions(-)

-- 
1.8.1.4

[PATCH BlueZ v0 1/4] avrcp: Fix missing reply to profile connect

[Mikel Astiz]

From: Mikel Astiz <mikel.astiz@bmw-carit.de>

The way control.c and avrcp.c interact makes some assumptions that can
be error-prone, specially since AVRCP connection-tracking was split per
role (bfe5f617940081844430871438410f956bbff78d).

During locally-initiated profile connection, control.c requires an AVCTP
session which indirectly initiates an AVRCP session. This AVRCP session
tries to guess the profile role (target vs remote) based on context
information (see avrcp.c:session_create()).

The problem is this "guess" might not always match the role originally
triggering the connection, and therefore the core never receives the
expected replies (state updates).

The proposed solution is to remove the role information in the
avrcp.c->control.c reporting of disconnections. After all, if the AVRCP
session is destroyed, neither of the roles can remain connected.

The issue has been reported by Alex Deymo with the traces below. It can
be observed that session_tg_destroy() gets called but the service state
is not updated, which is presumably due to the fact that the assumed
AVRCP role is wrong.

bluetoothd[22474]: src/device.c:connect_profiles() /org/bluez/hci0/dev_00_0C_8A_XX_XX_XX (all), client :1.642
bluetoothd[22474]: src/service.c:change_state() 0x638e4b0: device 00:0C:8A:XX:XX:XX profile audio-sink state changed: disconnected -> connecting (0)
bluetoothd[22474]: profiles/audio/manager.c:a2dp_sink_connect() path /org/bluez/hci0/dev_00_0C_8A_XX_XX_XX
bluetoothd[22474]: profiles/audio/avdtp.c:avdtp_ref() 0x64b56a0: ref=1
bluetoothd[22474]: profiles/audio/sink.c:sink_set_state() State changed /org/bluez/hci0/dev_00_0C_8A_XX_XX_XX: SINK_STATE_DISCONNECTED -> SINK_STATE_CONNECTING
bluetoothd[22474]: profiles/audio/sink.c:sink_connect() stream creation in progress
bluetoothd[22474]: src/adapter.c:connect_failed_callback() hci0 00:0C:8A:XX:XX:XX status 4
bluetoothd[22474]: src/adapter.c:bonding_attempt_complete() hci0 bdaddr 00:0C:8A:XX:XX:XX type 0 status 0x4
bluetoothd[22474]: src/device.c:device_bonding_complete() bonding (nil) status 0x04
bluetoothd[22474]: src/device.c:device_bonding_failed() status 4
bluetoothd[22474]: src/adapter.c:resume_discovery()
bluetoothd[22474]: connect error: Host is down (112)
bluetoothd[22474]: profiles/audio/avdtp.c:connection_lost() Disconnected from 00:0C:8A:XX:XX:XX
bluetoothd[22474]: profiles/audio/avdtp.c:avdtp_unref() 0x64b56a0: ref=0
bluetoothd[22474]: src/service.c:change_state() 0x638e4b0: device 00:0C:8A:XX:XX:XX profile audio-sink state changed: connecting -> disconnected (-5)
bluetoothd[22474]: src/device.c:device_profile_connected() audio-sink Input/output error (5)
bluetoothd[22474]: src/service.c:change_state() 0x639e740: device 00:0C:8A:XX:XX:XX profile audio-avrcp-target state changed: disconnected -> connecting (0)
bluetoothd[22474]: profiles/audio/manager.c:avrcp_target_connect() path /org/bluez/hci0/dev_00_0C_8A_XX_XX_XX
bluetoothd[22474]: profiles/audio/avctp.c:avctp_set_state() AVCTP Connecting
bluetoothd[22474]: profiles/audio/sink.c:sink_set_state() State changed /org/bluez/hci0/dev_00_0C_8A_XX_XX_XX: SINK_STATE_CONNECTING -> SINK_STATE_DISCONNECTED
bluetoothd[22474]: profiles/audio/avrcp.c:session_tg_destroy() 0x64d7ae0
bluetoothd[22474]: profiles/audio/avctp.c:avctp_set_state() AVCTP Disconnected
bluetoothd[22474]: profiles/audio/avdtp.c:avdtp_free() 0x64b56a0
bluetoothd[22474]: src/adapter.c:connect_failed_callback() hci0 00:0C:8A:XX:XX:XX status 2
bluetoothd[22474]: src/adapter.c:bonding_attempt_complete() hci0 bdaddr 00:0C:8A:XX:XX:XX type 0 status 0x2
bluetoothd[22474]: src/device.c:device_bonding_complete() bonding (nil) status 0x02
bluetoothd[22474]: src/device.c:device_bonding_failed() status 2
bluetoothd[22474]: src/adapter.c:resume_discovery()
---
 profiles/audio/avrcp.c   | 11 +----------
 profiles/audio/control.c | 23 ++++++++++++++++-------
 profiles/audio/control.h |  3 +--
 3 files changed, 18 insertions(+), 19 deletions(-)

diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
index 4558407..65ce314 100644
--- a/profiles/audio/avrcp.c
+++ b/profiles/audio/avrcp.c
@@ -2826,11 +2826,6 @@ static void session_tg_destroy(struct avrcp *session)
 	if (player != NULL)
 		player->sessions = g_slist_remove(player->sessions, session);
 
-	if (session->control_id == 0)
-		control_remote_connected(session->dev->control, -EIO);
-	else
-		control_remote_disconnected(session->dev->control, 0);
-
 	session_destroy(session);
 }
 
@@ -2840,11 +2835,6 @@ static void session_ct_destroy(struct avrcp *session)
 
 	g_slist_free_full(session->players, player_destroy);
 
-	if (session->control_id == 0)
-		control_target_connected(session->dev->control, -EIO);
-	else
-		control_target_disconnected(session->dev->control, 0);
-
 	session_destroy(session);
 }
 
@@ -2922,6 +2912,7 @@ static void state_changed(struct audio_device *dev, avctp_state_t old_state,
 			break;
 
 		session->destroy(session);
+		control_disconnected(dev->control);
 
 		break;
 	case AVCTP_STATE_CONNECTING:
diff --git a/profiles/audio/control.c b/profiles/audio/control.c
index cdba385..2630850 100644
--- a/profiles/audio/control.c
+++ b/profiles/audio/control.c
@@ -73,19 +73,28 @@ void control_target_connected(struct control *control, int err)
 	btd_service_connecting_complete(control->target, err);
 }
 
-void control_target_disconnected(struct control *control, int err)
-{
-	btd_service_disconnecting_complete(control->target, err);
-}
-
 void control_remote_connected(struct control *control, int err)
 {
 	btd_service_connecting_complete(control->remote, err);
 }
 
-void control_remote_disconnected(struct control *control, int err)
+void control_disconnected(struct control *control)
 {
-	btd_service_disconnecting_complete(control->remote, err);
+	if (control->remote != NULL) {
+		if (btd_service_get_state(control->remote) ==
+						BTD_SERVICE_STATE_CONNECTING)
+			btd_service_connecting_complete(control->remote, -EIO);
+		else
+			btd_service_disconnecting_complete(control->remote, 0);
+	}
+
+	if (control->target != NULL) {
+		if (btd_service_get_state(control->target) ==
+						BTD_SERVICE_STATE_CONNECTING)
+			btd_service_connecting_complete(control->target, -EIO);
+		else
+			btd_service_disconnecting_complete(control->target, 0);
+	}
 }
 
 static void state_changed(struct audio_device *dev, avctp_state_t old_state,
diff --git a/profiles/audio/control.h b/profiles/audio/control.h
index 0a0f208..a3e44a3 100644
--- a/profiles/audio/control.h
+++ b/profiles/audio/control.h
@@ -36,6 +36,5 @@ int control_connect(struct audio_device *dev);
 int control_disconnect(struct audio_device *dev);
 
 void control_target_connected(struct control *control, int err);
-void control_target_disconnected(struct control *control, int err);
 void control_remote_connected(struct control *control, int err);
-void control_remote_disconnected(struct control *control, int err);
+void control_disconnected(struct control *control);
-- 
1.8.1.4

[PATCH BlueZ v0 2/4] control: Remove unused parameter

[Mikel Astiz]

From: Mikel Astiz <mikel.astiz@bmw-carit.de>

The functions control_xxx_connected() are just used to report successful
connections and therefore the second parameter can be removed.
---
 profiles/audio/avrcp.c   | 4 ++--
 profiles/audio/control.c | 8 ++++----
 profiles/audio/control.h | 4 ++--
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
index 65ce314..e9d352c 100644
--- a/profiles/audio/avrcp.c
+++ b/profiles/audio/avrcp.c
@@ -2761,7 +2761,7 @@ static void session_tg_init_control(struct avrcp *session)
 		avrcp_register_notification(session,
 						AVRCP_EVENT_VOLUME_CHANGED);
 
-	control_remote_connected(session->dev->control, 0);
+	control_remote_connected(session->dev->control);
 }
 
 static void session_ct_init_browsing(struct avrcp *session)
@@ -2787,7 +2787,7 @@ static void session_ct_init_control(struct avrcp *session)
 	if (session->version >= 0x0104)
 		session->supported_events = (1 << AVRCP_EVENT_VOLUME_CHANGED);
 
-	control_target_connected(session->dev->control, 0);
+	control_target_connected(session->dev->control);
 
 	player = create_ct_player(session, 0);
 	if (player == NULL)
diff --git a/profiles/audio/control.c b/profiles/audio/control.c
index 2630850..f17e271 100644
--- a/profiles/audio/control.c
+++ b/profiles/audio/control.c
@@ -68,14 +68,14 @@ struct control {
 	unsigned int avctp_id;
 };
 
-void control_target_connected(struct control *control, int err)
+void control_target_connected(struct control *control)
 {
-	btd_service_connecting_complete(control->target, err);
+	btd_service_connecting_complete(control->target, 0);
 }
 
-void control_remote_connected(struct control *control, int err)
+void control_remote_connected(struct control *control)
 {
-	btd_service_connecting_complete(control->remote, err);
+	btd_service_connecting_complete(control->remote, 0);
 }
 
 void control_disconnected(struct control *control)
diff --git a/profiles/audio/control.h b/profiles/audio/control.h
index a3e44a3..c0c5ac3 100644
--- a/profiles/audio/control.h
+++ b/profiles/audio/control.h
@@ -35,6 +35,6 @@ gboolean control_is_active(struct audio_device *dev);
 int control_connect(struct audio_device *dev);
 int control_disconnect(struct audio_device *dev);
 
-void control_target_connected(struct control *control, int err);
-void control_remote_connected(struct control *control, int err);
+void control_target_connected(struct control *control);
+void control_remote_connected(struct control *control);
 void control_disconnected(struct control *control);
-- 
1.8.1.4

[PATCH BlueZ v0 3/4] avrcp: Fix service connections not reported to core

[Mikel Astiz]

From: Mikel Astiz <mikel.astiz@bmw-carit.de>

Similarly to disconnections, control.c cannot rely on the role
information used in avrcp.c in order to report the core about service
state changes. Therefore, consider both roles as connected once the
AVRCP session has been created if the core had previouls requested the
connection of the other role.
---
 profiles/audio/control.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/profiles/audio/control.c b/profiles/audio/control.c
index f17e271..d2cc163 100644
--- a/profiles/audio/control.c
+++ b/profiles/audio/control.c
@@ -71,11 +71,19 @@ struct control {
 void control_target_connected(struct control *control)
 {
 	btd_service_connecting_complete(control->target, 0);
+
+	if (control->remote != NULL && btd_service_get_state(control->remote) ==
+						BTD_SERVICE_STATE_CONNECTING)
+		btd_service_connecting_complete(control->remote, 0);
 }
 
 void control_remote_connected(struct control *control)
 {
 	btd_service_connecting_complete(control->remote, 0);
+
+	if (control->target != NULL && btd_service_get_state(control->target) ==
+						BTD_SERVICE_STATE_CONNECTING)
+		btd_service_connecting_complete(control->target, 0);
 }
 
 void control_disconnected(struct control *control)
-- 
1.8.1.4

[PATCH BlueZ v0 4/4] avrcp: Don't require active sink in role heuristic

[Mikel Astiz]

From: Mikel Astiz <mikel.astiz@bmw-carit.de>

When guessing the AVRCP role of the session being created, do not rely
on the state of the sink. This is actually a common scenario when the
connection of the sink failed.
---
 profiles/audio/avrcp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
index e9d352c..677bf88 100644
--- a/profiles/audio/avrcp.c
+++ b/profiles/audio/avrcp.c
@@ -2862,7 +2862,7 @@ static struct avrcp *session_create(struct avrcp_server *server,
 		session->target = TRUE;
 	else if (dev->source && !dev->sink)
 		session->target = FALSE;
-	else if (dev->sink && sink_is_active(dev))
+	else if (dev->sink && !dev->source)
 		session->target = TRUE;
 	else
 		session->target = FALSE;
-- 
1.8.1.4

Re: [PATCH BlueZ v0 0/4] AVRCP connection-tracking issues

[Luiz Augusto von Dentz]

Hi Mikel,

On Thu, May 23, 2013 at 2:28 AM, Mikel Astiz <mikel.astiz.oss@gmail.com> wrote:
> From: Mikel Astiz <mikel.astiz@bmw-carit.de>
>
> This patchset addresses the issues reported by Alex Deymo in the thread "audio: Connect doesn't return when audio device is off". Extracted from his message:
>
> "There are two ways to hit this problem:
> * One is to attempt a connection when the device is off,
> * the other one is to attempt a connection from the host right after
> you short press the button with the bluetooth logo on the speakers.
> This button normally reconnects the speakers to the host, but if you
> attempt a connection while the device is also doing that, you end up
> in the same situation."
>
> I have been able to reproduce the first issue, which should be fixed with patch 1/4. The second issue is addressed in patch 3/4 but I couldn't actually test it.
>
> Patch 4/4 tries to improve the AVRCP role heuristic, which could alone fix Alex's issues, but I think the core cannot rely on this heuristic nevertheless.
>
> Mikel Astiz (4):
>   avrcp: Fix missing reply to profile connect
>   control: Remove unused parameter
>   avrcp: Fix service connections not reported to core
>   avrcp: Don't require active sink in role heuristic
>
>  profiles/audio/avrcp.c   | 17 ++++-------------
>  profiles/audio/control.c | 37 +++++++++++++++++++++++++++----------
>  profiles/audio/control.h |  7 +++----
>  3 files changed, 34 insertions(+), 27 deletions(-)
>
> --
> 1.8.1.4

By looking at your patch 2/4 it seems we are not able to really tell
if a connection attempt has failed anymore, so I think there is
probably something wrong. The host down error should probably stop
continuing connection whenever it fails the first time, the issue with
the connection clash is probably different though and perhaps we
should go ahead with the heuristic fix and see if that fixes all the
problems.

@Alex: Can you test the last patch from Mikel for the second issue
with the remote device connecting to us while we are connecting to it?
The host down I think Johan has been working on that and we should
have a patch soon.


--
Luiz Augusto von Dentz

Re: [PATCH BlueZ v0 0/4] AVRCP connection-tracking issues

[Luiz Augusto von Dentz]

Hi Mikel,

On Thu, May 23, 2013 at 9:21 AM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> Hi Mikel,
>
> On Thu, May 23, 2013 at 2:28 AM, Mikel Astiz <mikel.astiz.oss@gmail.com> wrote:
>> From: Mikel Astiz <mikel.astiz@bmw-carit.de>
>>
>> This patchset addresses the issues reported by Alex Deymo in the thread "audio: Connect doesn't return when audio device is off". Extracted from his message:
>>
>> "There are two ways to hit this problem:
>> * One is to attempt a connection when the device is off,
>> * the other one is to attempt a connection from the host right after
>> you short press the button with the bluetooth logo on the speakers.
>> This button normally reconnects the speakers to the host, but if you
>> attempt a connection while the device is also doing that, you end up
>> in the same situation."
>>
>> I have been able to reproduce the first issue, which should be fixed with patch 1/4. The second issue is addressed in patch 3/4 but I couldn't actually test it.
>>
>> Patch 4/4 tries to improve the AVRCP role heuristic, which could alone fix Alex's issues, but I think the core cannot rely on this heuristic nevertheless.
>>
>> Mikel Astiz (4):
>>   avrcp: Fix missing reply to profile connect
>>   control: Remove unused parameter
>>   avrcp: Fix service connections not reported to core
>>   avrcp: Don't require active sink in role heuristic
>>
>>  profiles/audio/avrcp.c   | 17 ++++-------------
>>  profiles/audio/control.c | 37 +++++++++++++++++++++++++++----------
>>  profiles/audio/control.h |  7 +++----
>>  3 files changed, 34 insertions(+), 27 deletions(-)
>>
>> --
>> 1.8.1.4
>
> By looking at your patch 2/4 it seems we are not able to really tell
> if a connection attempt has failed anymore, so I think there is
> probably something wrong. The host down error should probably stop
> continuing connection whenever it fails the first time, the issue with
> the connection clash is probably different though and perhaps we
> should go ahead with the heuristic fix and see if that fixes all the
> problems.
>
> @Alex: Can you test the last patch from Mikel for the second issue
> with the remote device connecting to us while we are connecting to it?
> The host down I think Johan has been working on that and we should
> have a patch soon.

Actually let me take it back, the heuristic fix actually doesn't do
anything since we already have the same check four line above this
should never happen. A potential fix is to remove auto_connect from
avrcp_target_profile so if sink fails to connect it won't connect
automatically, anyway when the sink connects device.c will make sure
to connect avrcp as well.

--
Luiz Augusto von Dentz

Re: [PATCH BlueZ v0 0/4] AVRCP connection-tracking issues

[Alex Deymo]

Hi all,

On Thu, May 23, 2013 at 9:45 AM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> On Thu, May 23, 2013 at 9:21 AM, Luiz Augusto von Dentz
> > @Alex: Can you test the last patch from Mikel for the second issue
> > with the remote device connecting to us while we are connecting to it?
> > The host down I think Johan has been working on that and we should
> > have a patch soon.
>
> Actually let me take it back, the heuristic fix actually doesn't do
> anything since we already have the same check four line above this
> should never happen. A potential fix is to remove auto_connect from
> avrcp_target_profile so if sink fails to connect it won't connect
> automatically, anyway when the sink connects device.c will make sure
> to connect avrcp as well.

I tried the patchset with the latest master and it fixes the first
scenario on my dev machine (attempt a connect when device is off).
For the second issue (attempt a connection while the device is also
attempting it), there is some timing involved and I couldn't run into
the same problematic state on my dev machine with this patchset (which
doesn't mean that I'm confirming this fixes the second issue). So, to
be more confident about the second scenario I also tried the same
patchset in a more controlled environment (a chromebook laptop), but
on top of bluez-5.5 instead of master (is almost the same). The repro
case is as follows:
* Our audio server is playing music all the time, when the bluetooth
profile is connected it switches the music from the laptop speakers to
the bluetooth speakers. When the profile is disconnected, the music is
switched back to the laptop speakers.
* The Bose SoundLink pairing memory is cleared before the test (hold
the bluetooth logo button 10s on the device until you hear a tone).
This makes the bluetooth logo button connect back to the only known
host in the memory (it may attempt to connect to previously paired
hosts if not).
Steps:
1. Put the device in discovery mode (should be anyway after the memory erase).
2. on bluetoothctl: power, agent on, scan on, wait for the [NEW]
signal, scan off, pair BD_ADDR, trust BD_ADDR, connect BD_ADDR
3. Verify music plays out from the speakers.
- At this point pressing once the bluetooth logo will disconnect the
device. pressing it again will connect it again.
- Also, running "disconnect BD_ADDR" will disconnect it, and running
"connect BD_ADDR" will connect it back. Great. It works.
4. Disconnect the device with "disconnect BD_ADDR".
5. Wait a few seconds until the device is disconnected.
6. press once the bluetooth logo button on the device.
7. Wait 0.5 sec.
8. "connect BD_ADDR" on bluetoothctl (you may want to type the command
before step 6 and just hit enter here).

Logs as follows:
without the patch --> http://ix.io/5LE
snippet:
bluetoothd[16286]: profiles/audio/manager.c:avrcp_target_connect()
path /org/bluez/hci0/dev_00_0C_8A_XX_XX_XX
bluetoothd[16286]: profiles/audio/avctp.c:avctp_set_state() AVCTP Connecting
bluetoothd[16286]: profiles/audio/sink.c:sink_set_state() State
changed /org/bluez/hci0/dev_00_0C_8A_XX_XX_XX: SINK_STATE_CONNECTING
-> SINK_STATE_DISCONNECTED
bluetoothd[16286]: profiles/audio/avrcp.c:session_tg_destroy() 0x779a0e60
bluetoothd[16286]: profiles/audio/avctp.c:avctp_set_state() AVCTP Disconnected
bluetoothd[16286]: profiles/audio/avdtp.c:avdtp_free() 0x779a1e18

with the patch --> http://ix.io/5LF
snippet:
bluetoothd[17037]: profiles/audio/manager.c:avrcp_target_connect()
path /org/bluez/hci0/dev_00_0C_8A_XX_XX_XX
bluetoothd[17037]: profiles/audio/avctp.c:avctp_set_state() AVCTP Connecting
bluetoothd[17037]: profiles/audio/sink.c:sink_set_state() State
changed /org/bluez/hci0/dev_00_0C_8A_XX_XX_XX: SINK_STATE_CONNECTING
-> SINK_STATE_DISCONNECTED
bluetoothd[17037]: profiles/audio/avrcp.c:session_tg_destroy() 0x7843b580
bluetoothd[17037]: src/service.c:change_state() 0x784647d8: device
00:0C:8A:XX:XX:XX profile audio-avrcp-target state changed: connecting
-> disconnected (-5)
bluetoothd[17037]: src/device.c:device_profile_connected()
audio-avrcp-target Input/output error (5)
bluetoothd[17037]: src/device.c:device_profile_connected() returning
response to :1.75
bluetoothd[17037]: profiles/audio/avctp.c:avctp_set_state() AVCTP Disconnected
bluetoothd[17037]: profiles/audio/avdtp.c:avdtp_free() 0x7845c790

So, from my side, it looks good. Let me know if you need more debug.
Do you think it's possible to port this fix to bluez-5.4? I see you
are using btd_service on one of those patches.
Thanks all,
Alex.

Re: [PATCH BlueZ v0 0/4] AVRCP connection-tracking issues

[Luiz Augusto von Dentz]

Hi Alex,

On Thu, May 23, 2013 at 12:45 PM, Alex Deymo <deymo@chromium.org> wrote:
> Hi all,
>
> On Thu, May 23, 2013 at 9:45 AM, Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
>> On Thu, May 23, 2013 at 9:21 AM, Luiz Augusto von Dentz
>> > @Alex: Can you test the last patch from Mikel for the second issue
>> > with the remote device connecting to us while we are connecting to it?
>> > The host down I think Johan has been working on that and we should
>> > have a patch soon.
>>
>> Actually let me take it back, the heuristic fix actually doesn't do
>> anything since we already have the same check four line above this
>> should never happen. A potential fix is to remove auto_connect from
>> avrcp_target_profile so if sink fails to connect it won't connect
>> automatically, anyway when the sink connects device.c will make sure
>> to connect avrcp as well.
>
> I tried the patchset with the latest master and it fixes the first
> scenario on my dev machine (attempt a connect when device is off).
> For the second issue (attempt a connection while the device is also
> attempting it), there is some timing involved and I couldn't run into
> the same problematic state on my dev machine with this patchset (which
> doesn't mean that I'm confirming this fixes the second issue). So, to
> be more confident about the second scenario I also tried the same
> patchset in a more controlled environment (a chromebook laptop), but
> on top of bluez-5.5 instead of master (is almost the same). The repro
> case is as follows:
> * Our audio server is playing music all the time, when the bluetooth
> profile is connected it switches the music from the laptop speakers to
> the bluetooth speakers. When the profile is disconnected, the music is
> switched back to the laptop speakers.
> * The Bose SoundLink pairing memory is cleared before the test (hold
> the bluetooth logo button 10s on the device until you hear a tone).
> This makes the bluetooth logo button connect back to the only known
> host in the memory (it may attempt to connect to previously paired
> hosts if not).
> Steps:
> 1. Put the device in discovery mode (should be anyway after the memory erase).
> 2. on bluetoothctl: power, agent on, scan on, wait for the [NEW]
> signal, scan off, pair BD_ADDR, trust BD_ADDR, connect BD_ADDR
> 3. Verify music plays out from the speakers.
> - At this point pressing once the bluetooth logo will disconnect the
> device. pressing it again will connect it again.
> - Also, running "disconnect BD_ADDR" will disconnect it, and running
> "connect BD_ADDR" will connect it back. Great. It works.
> 4. Disconnect the device with "disconnect BD_ADDR".
> 5. Wait a few seconds until the device is disconnected.
> 6. press once the bluetooth logo button on the device.
> 7. Wait 0.5 sec.
> 8. "connect BD_ADDR" on bluetoothctl (you may want to type the command
> before step 6 and just hit enter here).
>
> Logs as follows:
> without the patch --> http://ix.io/5LE
> snippet:
> bluetoothd[16286]: profiles/audio/manager.c:avrcp_target_connect()
> path /org/bluez/hci0/dev_00_0C_8A_XX_XX_XX
> bluetoothd[16286]: profiles/audio/avctp.c:avctp_set_state() AVCTP Connecting
> bluetoothd[16286]: profiles/audio/sink.c:sink_set_state() State
> changed /org/bluez/hci0/dev_00_0C_8A_XX_XX_XX: SINK_STATE_CONNECTING
> -> SINK_STATE_DISCONNECTED
> bluetoothd[16286]: profiles/audio/avrcp.c:session_tg_destroy() 0x779a0e60
> bluetoothd[16286]: profiles/audio/avctp.c:avctp_set_state() AVCTP Disconnected
> bluetoothd[16286]: profiles/audio/avdtp.c:avdtp_free() 0x779a1e18
>
> with the patch --> http://ix.io/5LF
> snippet:
> bluetoothd[17037]: profiles/audio/manager.c:avrcp_target_connect()
> path /org/bluez/hci0/dev_00_0C_8A_XX_XX_XX
> bluetoothd[17037]: profiles/audio/avctp.c:avctp_set_state() AVCTP Connecting
> bluetoothd[17037]: profiles/audio/sink.c:sink_set_state() State
> changed /org/bluez/hci0/dev_00_0C_8A_XX_XX_XX: SINK_STATE_CONNECTING
> -> SINK_STATE_DISCONNECTED
> bluetoothd[17037]: profiles/audio/avrcp.c:session_tg_destroy() 0x7843b580
> bluetoothd[17037]: src/service.c:change_state() 0x784647d8: device
> 00:0C:8A:XX:XX:XX profile audio-avrcp-target state changed: connecting
> -> disconnected (-5)
> bluetoothd[17037]: src/device.c:device_profile_connected()
> audio-avrcp-target Input/output error (5)
> bluetoothd[17037]: src/device.c:device_profile_connected() returning
> response to :1.75
> bluetoothd[17037]: profiles/audio/avctp.c:avctp_set_state() AVCTP Disconnected
> bluetoothd[17037]: profiles/audio/avdtp.c:avdtp_free() 0x7845c790
>
> So, from my side, it looks good. Let me know if you need more debug.
> Do you think it's possible to port this fix to bluez-5.4? I see you
> are using btd_service on one of those patches.

So with what patch-set have you tested, mine that Ive send a few hours
ago or Mikel's?


--
Luiz Augusto von Dentz

Re: [PATCH BlueZ v0 0/4] AVRCP connection-tracking issues

[Alex Deymo]

On Thu, May 23, 2013 at 1:27 PM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Alex,
> > So, from my side, it looks good. Let me know if you need more debug.
> > Do you think it's possible to port this fix to bluez-5.4? I see you
> > are using btd_service on one of those patches.
>
> So with what patch-set have you tested, mine that Ive send a few hours
> ago or Mikel's?

I tried Mikel's patchset. I can try your patchset now if you want.
Alex.

Re: [PATCH BlueZ v0 0/4] AVRCP connection-tracking issues

[Luiz Augusto von Dentz]

Hi Alex,

On Thu, May 23, 2013 at 2:05 PM, Alex Deymo <deymo@chromium.org> wrote:
> On Thu, May 23, 2013 at 1:27 PM, Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
>>
>> Hi Alex,
>> > So, from my side, it looks good. Let me know if you need more debug.
>> > Do you think it's possible to port this fix to bluez-5.4? I see you
>> > are using btd_service on one of those patches.
>>
>> So with what patch-set have you tested, mine that Ive send a few hours
>> ago or Mikel's?
>
> I tried Mikel's patchset. I can try your patchset now if you want.

Please do it, Ive done some testing with the devices I have with me
(Sony MW600 and a couple phones for the CT role) and it seems to fix
the problem. It is not that Mikel's patches wouldn't work, they
actually do fix it too but it fix the effect not the cause that is
connecting the wrong profile in first place.


--
Luiz Augusto von Dentz

Re: [PATCH BlueZ v0 0/4] AVRCP connection-tracking issues

[Alex Deymo]

Hi Luiz,

On Thu, May 23, 2013 at 2:12 PM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>>> So with what patch-set have you tested, mine that Ive send a few hours
>>> ago or Mikel's?
>>
>> I tried Mikel's patchset. I can try your patchset now if you want.
>
> Please do it, Ive done some testing with the devices I have with me
> (Sony MW600 and a couple phones for the CT role) and it seems to fix
> the problem. It is not that Mikel's patches wouldn't work, they
> actually do fix it too but it fix the effect not the cause that is
> connecting the wrong profile in first place.

Ok, I tried your patch in the same environment as before and the logs
are here ( http://ix.io/5LR ).
That one also works fine:
* the device is idle and would accept a connection from the host.
* pressing the bluetooth logo button makes it leave that mode and
start a connection to the host... but it takes some time to do that
(about 2sec).
* a "connect" command in the bluetoothctl will fail and (now) return an error.
* the "ongoing" connection from the device succeeds after that and
everything works :)

Thank you!
Alex.

Re: [PATCH BlueZ v0 0/4] AVRCP connection-tracking issues

[Mikel Astiz]

Hi Luis,

On Thu, May 23, 2013 at 6:45 PM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> Hi Mikel,
>
> On Thu, May 23, 2013 at 9:21 AM, Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
>> Hi Mikel,
>>
>> On Thu, May 23, 2013 at 2:28 AM, Mikel Astiz <mikel.astiz.oss@gmail.com> wrote:
>>> From: Mikel Astiz <mikel.astiz@bmw-carit.de>
>>>
>>> This patchset addresses the issues reported by Alex Deymo in the thread "audio: Connect doesn't return when audio device is off". Extracted from his message:
>>>
>>> "There are two ways to hit this problem:
>>> * One is to attempt a connection when the device is off,
>>> * the other one is to attempt a connection from the host right after
>>> you short press the button with the bluetooth logo on the speakers.
>>> This button normally reconnects the speakers to the host, but if you
>>> attempt a connection while the device is also doing that, you end up
>>> in the same situation."
>>>
>>> I have been able to reproduce the first issue, which should be fixed with patch 1/4. The second issue is addressed in patch 3/4 but I couldn't actually test it.
>>>
>>> Patch 4/4 tries to improve the AVRCP role heuristic, which could alone fix Alex's issues, but I think the core cannot rely on this heuristic nevertheless.
>>>
>>> Mikel Astiz (4):
>>>   avrcp: Fix missing reply to profile connect
>>>   control: Remove unused parameter
>>>   avrcp: Fix service connections not reported to core
>>>   avrcp: Don't require active sink in role heuristic
>>>
>>>  profiles/audio/avrcp.c   | 17 ++++-------------
>>>  profiles/audio/control.c | 37 +++++++++++++++++++++++++++----------
>>>  profiles/audio/control.h |  7 +++----
>>>  3 files changed, 34 insertions(+), 27 deletions(-)
>>>
>>> --
>>> 1.8.1.4
>>
>> By looking at your patch 2/4 it seems we are not able to really tell
>> if a connection attempt has failed anymore, so I think there is
>> probably something wrong. The host down error should probably stop
>> continuing connection whenever it fails the first time, the issue with
>> the connection clash is probably different though and perhaps we
>> should go ahead with the heuristic fix and see if that fixes all the
>> problems.
>>
>> @Alex: Can you test the last patch from Mikel for the second issue
>> with the remote device connecting to us while we are connecting to it?
>> The host down I think Johan has been working on that and we should
>> have a patch soon.
>
> Actually let me take it back, the heuristic fix actually doesn't do
> anything since we already have the same check four line above this
> should never happen. A potential fix is to remove auto_connect from
> avrcp_target_profile so if sink fails to connect it won't connect
> automatically, anyway when the sink connects device.c will make sure
> to connect avrcp as well.
>
> --
> Luiz Augusto von Dentz

You're right, the last patch doesn't help at all. What about the first
three? Even if the originally reported issue seems to be fixed now, I
think the change still makes sense.

The issue might now be hidden because Device.Connect() doesn't connect
AVRCP, but I believe you could still hit the issue with
Device.ConnectProfile() assuming that the role heuristic makes a wrong
guess.

Cheers,
Mikel

Re: [PATCH BlueZ v0 0/4] AVRCP connection-tracking issues

[Luiz Augusto von Dentz]

Hi Mikel,

On Fri, May 24, 2013 at 12:05 AM, Mikel Astiz <mikel.astiz.oss@gmail.com> wrote:
> Hi Luis,
>
> On Thu, May 23, 2013 at 6:45 PM, Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
>> Hi Mikel,
>>
>> On Thu, May 23, 2013 at 9:21 AM, Luiz Augusto von Dentz
>> <luiz.dentz@gmail.com> wrote:
>>> Hi Mikel,
>>>
>>> On Thu, May 23, 2013 at 2:28 AM, Mikel Astiz <mikel.astiz.oss@gmail.com> wrote:
>>>> From: Mikel Astiz <mikel.astiz@bmw-carit.de>
>>>>
>>>> This patchset addresses the issues reported by Alex Deymo in the thread "audio: Connect doesn't return when audio device is off". Extracted from his message:
>>>>
>>>> "There are two ways to hit this problem:
>>>> * One is to attempt a connection when the device is off,
>>>> * the other one is to attempt a connection from the host right after
>>>> you short press the button with the bluetooth logo on the speakers.
>>>> This button normally reconnects the speakers to the host, but if you
>>>> attempt a connection while the device is also doing that, you end up
>>>> in the same situation."
>>>>
>>>> I have been able to reproduce the first issue, which should be fixed with patch 1/4. The second issue is addressed in patch 3/4 but I couldn't actually test it.
>>>>
>>>> Patch 4/4 tries to improve the AVRCP role heuristic, which could alone fix Alex's issues, but I think the core cannot rely on this heuristic nevertheless.
>>>>
>>>> Mikel Astiz (4):
>>>>   avrcp: Fix missing reply to profile connect
>>>>   control: Remove unused parameter
>>>>   avrcp: Fix service connections not reported to core
>>>>   avrcp: Don't require active sink in role heuristic
>>>>
>>>>  profiles/audio/avrcp.c   | 17 ++++-------------
>>>>  profiles/audio/control.c | 37 +++++++++++++++++++++++++++----------
>>>>  profiles/audio/control.h |  7 +++----
>>>>  3 files changed, 34 insertions(+), 27 deletions(-)
>>>>
>>>> --
>>>> 1.8.1.4
>>>
>>> By looking at your patch 2/4 it seems we are not able to really tell
>>> if a connection attempt has failed anymore, so I think there is
>>> probably something wrong. The host down error should probably stop
>>> continuing connection whenever it fails the first time, the issue with
>>> the connection clash is probably different though and perhaps we
>>> should go ahead with the heuristic fix and see if that fixes all the
>>> problems.
>>>
>>> @Alex: Can you test the last patch from Mikel for the second issue
>>> with the remote device connecting to us while we are connecting to it?
>>> The host down I think Johan has been working on that and we should
>>> have a patch soon.
>>
>> Actually let me take it back, the heuristic fix actually doesn't do
>> anything since we already have the same check four line above this
>> should never happen. A potential fix is to remove auto_connect from
>> avrcp_target_profile so if sink fails to connect it won't connect
>> automatically, anyway when the sink connects device.c will make sure
>> to connect avrcp as well.
>>
>> --
>> Luiz Augusto von Dentz
>
> You're right, the last patch doesn't help at all. What about the first
> three? Even if the originally reported issue seems to be fixed now, I
> think the change still makes sense.
>
> The issue might now be hidden because Device.Connect() doesn't connect
> AVRCP, but I believe you could still hit the issue with
> Device.ConnectProfile() assuming that the role heuristic makes a wrong
> guess.

In that case I would like to remove the heuristic for outgoing
connections using the service when it attempt to connect, so I would
like to wait you to complete the service code so I can remove a lot of
code in the audio including the audio_dev and perhaps deprecate
MediaControl interface in favor of Service, this would leave the
heuristic only for incoming connections where the heuristic is
necessary to figure out which role the remote device is expecting.

--
Luiz Augusto von Dentz

Re: [PATCH BlueZ v0 0/4] AVRCP connection-tracking issues

[Mikel Astiz]

Hi Luiz,

On Sat, May 25, 2013 at 2:56 AM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> Hi Mikel,
>
> On Fri, May 24, 2013 at 12:05 AM, Mikel Astiz <mikel.astiz.oss@gmail.com> wrote:
>> Hi Luis,
>>
>> On Thu, May 23, 2013 at 6:45 PM, Luiz Augusto von Dentz
>> <luiz.dentz@gmail.com> wrote:
>>> Hi Mikel,
>>>
>>> On Thu, May 23, 2013 at 9:21 AM, Luiz Augusto von Dentz
>>> <luiz.dentz@gmail.com> wrote:
>>>> Hi Mikel,
>>>>
>>>> On Thu, May 23, 2013 at 2:28 AM, Mikel Astiz <mikel.astiz.oss@gmail.com> wrote:
>>>>> From: Mikel Astiz <mikel.astiz@bmw-carit.de>
>>>>>
>>>>> This patchset addresses the issues reported by Alex Deymo in the thread "audio: Connect doesn't return when audio device is off". Extracted from his message:
>>>>>
>>>>> "There are two ways to hit this problem:
>>>>> * One is to attempt a connection when the device is off,
>>>>> * the other one is to attempt a connection from the host right after
>>>>> you short press the button with the bluetooth logo on the speakers.
>>>>> This button normally reconnects the speakers to the host, but if you
>>>>> attempt a connection while the device is also doing that, you end up
>>>>> in the same situation."
>>>>>
>>>>> I have been able to reproduce the first issue, which should be fixed with patch 1/4. The second issue is addressed in patch 3/4 but I couldn't actually test it.
>>>>>
>>>>> Patch 4/4 tries to improve the AVRCP role heuristic, which could alone fix Alex's issues, but I think the core cannot rely on this heuristic nevertheless.
>>>>>
>>>>> Mikel Astiz (4):
>>>>>   avrcp: Fix missing reply to profile connect
>>>>>   control: Remove unused parameter
>>>>>   avrcp: Fix service connections not reported to core
>>>>>   avrcp: Don't require active sink in role heuristic
>>>>>
>>>>>  profiles/audio/avrcp.c   | 17 ++++-------------
>>>>>  profiles/audio/control.c | 37 +++++++++++++++++++++++++++----------
>>>>>  profiles/audio/control.h |  7 +++----
>>>>>  3 files changed, 34 insertions(+), 27 deletions(-)
>>>>>
>>>>> --
>>>>> 1.8.1.4
>>>>
>>>> By looking at your patch 2/4 it seems we are not able to really tell
>>>> if a connection attempt has failed anymore, so I think there is
>>>> probably something wrong. The host down error should probably stop
>>>> continuing connection whenever it fails the first time, the issue with
>>>> the connection clash is probably different though and perhaps we
>>>> should go ahead with the heuristic fix and see if that fixes all the
>>>> problems.
>>>>
>>>> @Alex: Can you test the last patch from Mikel for the second issue
>>>> with the remote device connecting to us while we are connecting to it?
>>>> The host down I think Johan has been working on that and we should
>>>> have a patch soon.
>>>
>>> Actually let me take it back, the heuristic fix actually doesn't do
>>> anything since we already have the same check four line above this
>>> should never happen. A potential fix is to remove auto_connect from
>>> avrcp_target_profile so if sink fails to connect it won't connect
>>> automatically, anyway when the sink connects device.c will make sure
>>> to connect avrcp as well.
>>>
>>> --
>>> Luiz Augusto von Dentz
>>
>> You're right, the last patch doesn't help at all. What about the first
>> three? Even if the originally reported issue seems to be fixed now, I
>> think the change still makes sense.
>>
>> The issue might now be hidden because Device.Connect() doesn't connect
>> AVRCP, but I believe you could still hit the issue with
>> Device.ConnectProfile() assuming that the role heuristic makes a wrong
>> guess.
>
> In that case I would like to remove the heuristic for outgoing
> connections using the service when it attempt to connect, so I would
> like to wait you to complete the service code so I can remove a lot of
> code in the audio including the audio_dev and perhaps deprecate
> MediaControl interface in favor of Service, this would leave the
> heuristic only for incoming connections where the heuristic is
> necessary to figure out which role the remote device is expecting.

I consider the core service infrastructure finished, with the
exception of the D-Bus part which I already submitted as RFC. There
might still be issues to fix (as recently reported by Vinicius and
Christian) but this is more about profile-specific adoption of
btd_service.

Regarding the audio part, there's definitely room for simplification
by exploiting new features of the core. Some ideas I have in mind are:

1. Register btd_profile (and therefore btd_service) instances for
UUIDs like AVDTP and AVCTP. This would presumably simplify the
connection logic a lot, but it might also require some additional core
support in order to handle disconnections properly (i.e. disconnect
AVDTP when all users have finished, in this case with a delay).

2. Exploit btd_service's userdata to avoid maintaining internal lists
and associations. This should be fairly easy and specially interesting
after (1), making for example avdtp_get_internal() trivial.

3. Extend btd_service_state_t with BTD_SERVICE_STATE_ACTIVE (i.e.
"Service is using a considerable bandwidth"), which would map to
PLAYING for many audio profiles. I don't have a strong opinion on this
one since the concept might not be generic enough to be adopted in the
core. For example, it might make sense for networking profiles (i.e.
an active connection exists) but on the contrary the HID profile would
make little use of this state.

4. Assuming (1) and (3) are acceptable, remove all internal
audio-specific states and their callbacks (e.g. source_state_t,
sink_state_t, avdtp_session_state_t...) and replace them with the core
btd_service states.

5. Extend the core with something like btd_server, which is analogous
to btd_service but representing local profile implementations. This is
an orthogonal discussion but I believe the audio profiles could
benefit a lot from this infrastructure as well.

Any thoughts?

IMO the key design decision here is (1), since it would simplify the
code significantly at the expense of requiring a considerable
refactoring.

Cheers,
Mikel

Re: [PATCH BlueZ v0 0/4] AVRCP connection-tracking issues

[Luiz Augusto von Dentz]

Hi Mikel,

On Sun, May 26, 2013 at 2:30 AM, Mikel Astiz <mikel.astiz.oss@gmail.com> wrote:
> Hi Luiz,
>
> On Sat, May 25, 2013 at 2:56 AM, Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
>> Hi Mikel,
>>
>> On Fri, May 24, 2013 at 12:05 AM, Mikel Astiz <mikel.astiz.oss@gmail.com> wrote:
>>> Hi Luis,
>>>
>>> On Thu, May 23, 2013 at 6:45 PM, Luiz Augusto von Dentz
>>> <luiz.dentz@gmail.com> wrote:
>>>> Hi Mikel,
>>>>
>>>> On Thu, May 23, 2013 at 9:21 AM, Luiz Augusto von Dentz
>>>> <luiz.dentz@gmail.com> wrote:
>>>>> Hi Mikel,
>>>>>
>>>>> On Thu, May 23, 2013 at 2:28 AM, Mikel Astiz <mikel.astiz.oss@gmail.com> wrote:
>>>>>> From: Mikel Astiz <mikel.astiz@bmw-carit.de>
>>>>>>
>>>>>> This patchset addresses the issues reported by Alex Deymo in the thread "audio: Connect doesn't return when audio device is off". Extracted from his message:
>>>>>>
>>>>>> "There are two ways to hit this problem:
>>>>>> * One is to attempt a connection when the device is off,
>>>>>> * the other one is to attempt a connection from the host right after
>>>>>> you short press the button with the bluetooth logo on the speakers.
>>>>>> This button normally reconnects the speakers to the host, but if you
>>>>>> attempt a connection while the device is also doing that, you end up
>>>>>> in the same situation."
>>>>>>
>>>>>> I have been able to reproduce the first issue, which should be fixed with patch 1/4. The second issue is addressed in patch 3/4 but I couldn't actually test it.
>>>>>>
>>>>>> Patch 4/4 tries to improve the AVRCP role heuristic, which could alone fix Alex's issues, but I think the core cannot rely on this heuristic nevertheless.
>>>>>>
>>>>>> Mikel Astiz (4):
>>>>>>   avrcp: Fix missing reply to profile connect
>>>>>>   control: Remove unused parameter
>>>>>>   avrcp: Fix service connections not reported to core
>>>>>>   avrcp: Don't require active sink in role heuristic
>>>>>>
>>>>>>  profiles/audio/avrcp.c   | 17 ++++-------------
>>>>>>  profiles/audio/control.c | 37 +++++++++++++++++++++++++++----------
>>>>>>  profiles/audio/control.h |  7 +++----
>>>>>>  3 files changed, 34 insertions(+), 27 deletions(-)
>>>>>>
>>>>>> --
>>>>>> 1.8.1.4
>>>>>
>>>>> By looking at your patch 2/4 it seems we are not able to really tell
>>>>> if a connection attempt has failed anymore, so I think there is
>>>>> probably something wrong. The host down error should probably stop
>>>>> continuing connection whenever it fails the first time, the issue with
>>>>> the connection clash is probably different though and perhaps we
>>>>> should go ahead with the heuristic fix and see if that fixes all the
>>>>> problems.
>>>>>
>>>>> @Alex: Can you test the last patch from Mikel for the second issue
>>>>> with the remote device connecting to us while we are connecting to it?
>>>>> The host down I think Johan has been working on that and we should
>>>>> have a patch soon.
>>>>
>>>> Actually let me take it back, the heuristic fix actually doesn't do
>>>> anything since we already have the same check four line above this
>>>> should never happen. A potential fix is to remove auto_connect from
>>>> avrcp_target_profile so if sink fails to connect it won't connect
>>>> automatically, anyway when the sink connects device.c will make sure
>>>> to connect avrcp as well.
>>>>
>>>> --
>>>> Luiz Augusto von Dentz
>>>
>>> You're right, the last patch doesn't help at all. What about the first
>>> three? Even if the originally reported issue seems to be fixed now, I
>>> think the change still makes sense.
>>>
>>> The issue might now be hidden because Device.Connect() doesn't connect
>>> AVRCP, but I believe you could still hit the issue with
>>> Device.ConnectProfile() assuming that the role heuristic makes a wrong
>>> guess.
>>
>> In that case I would like to remove the heuristic for outgoing
>> connections using the service when it attempt to connect, so I would
>> like to wait you to complete the service code so I can remove a lot of
>> code in the audio including the audio_dev and perhaps deprecate
>> MediaControl interface in favor of Service, this would leave the
>> heuristic only for incoming connections where the heuristic is
>> necessary to figure out which role the remote device is expecting.
>
> I consider the core service infrastructure finished, with the
> exception of the D-Bus part which I already submitted as RFC. There
> might still be issues to fix (as recently reported by Vinicius and
> Christian) but this is more about profile-specific adoption of
> btd_service.
>
> Regarding the audio part, there's definitely room for simplification
> by exploiting new features of the core. Some ideas I have in mind are:
>
> 1. Register btd_profile (and therefore btd_service) instances for
> UUIDs like AVDTP and AVCTP. This would presumably simplify the
> connection logic a lot, but it might also require some additional core
> support in order to handle disconnections properly (i.e. disconnect
> AVDTP when all users have finished, in this case with a delay).

It could be a good idea, but Im not sure it would work because we are
only using the service classes, but perhaps internally we can add
those UUIDs but they should probably not be exposed to the
applications.

> 2. Exploit btd_service's userdata to avoid maintaining internal lists
> and associations. This should be fairly easy and specially interesting
> after (1), making for example avdtp_get_internal() trivial.

This should be fine provided with can do (1).

> 3. Extend btd_service_state_t with BTD_SERVICE_STATE_ACTIVE (i.e.
> "Service is using a considerable bandwidth"), which would map to
> PLAYING for many audio profiles. I don't have a strong opinion on this
> one since the concept might not be generic enough to be adopted in the
> core. For example, it might make sense for networking profiles (i.e.
> an active connection exists) but on the contrary the HID profile would
> make little use of this state.

I would't have this extra state in the service, it is too profile
specific, what we really want is that the core takes care of
connection logic but the underlining signalling/data stream is profile
specif so it is better to leave the plugins/external profiles to
handle.

> 4. Assuming (1) and (3) are acceptable, remove all internal
> audio-specific states and their callbacks (e.g. source_state_t,
> sink_state_t, avdtp_session_state_t...) and replace them with the core
> btd_service states.

Sink and Source state will probably remain due to A2DP design of
Suspend/Start, but the other can probably be removed in favor of
service states.

> 5. Extend the core with something like btd_server, which is analogous
> to btd_service but representing local profile implementations. This is
> an orthogonal discussion but I believe the audio profiles could
> benefit a lot from this infrastructure as well.

Well ultimately I would like to have the core doing the connection
management for plugins as it does for external profiles, perhaps this
is your idea with btd_server as well, but for that to happen we would
need a bit more logic for handling records that have several PSMs to
be listen or those that can receive multiple connections such as A2DP.


--
Luiz Augusto von Dentz

Re: [PATCH BlueZ v0 0/4] AVRCP connection-tracking issues

[Mikel Astiz]

Hi Luiz,

On Mon, May 27, 2013 at 3:07 PM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> Hi Mikel,
>
> On Sun, May 26, 2013 at 2:30 AM, Mikel Astiz <mikel.astiz.oss@gmail.com> wrote:
>> Hi Luiz,
>>
>> On Sat, May 25, 2013 at 2:56 AM, Luiz Augusto von Dentz
>> <luiz.dentz@gmail.com> wrote:
>>> Hi Mikel,
>>>
>>> On Fri, May 24, 2013 at 12:05 AM, Mikel Astiz <mikel.astiz.oss@gmail.com> wrote:
>>>> Hi Luis,
>>>>
>>>> On Thu, May 23, 2013 at 6:45 PM, Luiz Augusto von Dentz
>>>> <luiz.dentz@gmail.com> wrote:
>>>>> Hi Mikel,
>>>>>
>>>>> On Thu, May 23, 2013 at 9:21 AM, Luiz Augusto von Dentz
>>>>> <luiz.dentz@gmail.com> wrote:
>>>>>> Hi Mikel,
>>>>>>
>>>>>> On Thu, May 23, 2013 at 2:28 AM, Mikel Astiz <mikel.astiz.oss@gmail.com> wrote:
>>>>>>> From: Mikel Astiz <mikel.astiz@bmw-carit.de>
>>>>>>>
>>>>>>> This patchset addresses the issues reported by Alex Deymo in the thread "audio: Connect doesn't return when audio device is off". Extracted from his message:
>>>>>>>
>>>>>>> "There are two ways to hit this problem:
>>>>>>> * One is to attempt a connection when the device is off,
>>>>>>> * the other one is to attempt a connection from the host right after
>>>>>>> you short press the button with the bluetooth logo on the speakers.
>>>>>>> This button normally reconnects the speakers to the host, but if you
>>>>>>> attempt a connection while the device is also doing that, you end up
>>>>>>> in the same situation."
>>>>>>>
>>>>>>> I have been able to reproduce the first issue, which should be fixed with patch 1/4. The second issue is addressed in patch 3/4 but I couldn't actually test it.
>>>>>>>
>>>>>>> Patch 4/4 tries to improve the AVRCP role heuristic, which could alone fix Alex's issues, but I think the core cannot rely on this heuristic nevertheless.
>>>>>>>
>>>>>>> Mikel Astiz (4):
>>>>>>>   avrcp: Fix missing reply to profile connect
>>>>>>>   control: Remove unused parameter
>>>>>>>   avrcp: Fix service connections not reported to core
>>>>>>>   avrcp: Don't require active sink in role heuristic
>>>>>>>
>>>>>>>  profiles/audio/avrcp.c   | 17 ++++-------------
>>>>>>>  profiles/audio/control.c | 37 +++++++++++++++++++++++++++----------
>>>>>>>  profiles/audio/control.h |  7 +++----
>>>>>>>  3 files changed, 34 insertions(+), 27 deletions(-)
>>>>>>>
>>>>>>> --
>>>>>>> 1.8.1.4
>>>>>>
>>>>>> By looking at your patch 2/4 it seems we are not able to really tell
>>>>>> if a connection attempt has failed anymore, so I think there is
>>>>>> probably something wrong. The host down error should probably stop
>>>>>> continuing connection whenever it fails the first time, the issue with
>>>>>> the connection clash is probably different though and perhaps we
>>>>>> should go ahead with the heuristic fix and see if that fixes all the
>>>>>> problems.
>>>>>>
>>>>>> @Alex: Can you test the last patch from Mikel for the second issue
>>>>>> with the remote device connecting to us while we are connecting to it?
>>>>>> The host down I think Johan has been working on that and we should
>>>>>> have a patch soon.
>>>>>
>>>>> Actually let me take it back, the heuristic fix actually doesn't do
>>>>> anything since we already have the same check four line above this
>>>>> should never happen. A potential fix is to remove auto_connect from
>>>>> avrcp_target_profile so if sink fails to connect it won't connect
>>>>> automatically, anyway when the sink connects device.c will make sure
>>>>> to connect avrcp as well.
>>>>>
>>>>> --
>>>>> Luiz Augusto von Dentz
>>>>
>>>> You're right, the last patch doesn't help at all. What about the first
>>>> three? Even if the originally reported issue seems to be fixed now, I
>>>> think the change still makes sense.
>>>>
>>>> The issue might now be hidden because Device.Connect() doesn't connect
>>>> AVRCP, but I believe you could still hit the issue with
>>>> Device.ConnectProfile() assuming that the role heuristic makes a wrong
>>>> guess.
>>>
>>> In that case I would like to remove the heuristic for outgoing
>>> connections using the service when it attempt to connect, so I would
>>> like to wait you to complete the service code so I can remove a lot of
>>> code in the audio including the audio_dev and perhaps deprecate
>>> MediaControl interface in favor of Service, this would leave the
>>> heuristic only for incoming connections where the heuristic is
>>> necessary to figure out which role the remote device is expecting.
>>
>> I consider the core service infrastructure finished, with the
>> exception of the D-Bus part which I already submitted as RFC. There
>> might still be issues to fix (as recently reported by Vinicius and
>> Christian) but this is more about profile-specific adoption of
>> btd_service.
>>
>> Regarding the audio part, there's definitely room for simplification
>> by exploiting new features of the core. Some ideas I have in mind are:
>>
>> 1. Register btd_profile (and therefore btd_service) instances for
>> UUIDs like AVDTP and AVCTP. This would presumably simplify the
>> connection logic a lot, but it might also require some additional core
>> support in order to handle disconnections properly (i.e. disconnect
>> AVDTP when all users have finished, in this case with a delay).
>
> It could be a good idea, but Im not sure it would work because we are
> only using the service classes, but perhaps internally we can add
> those UUIDs but they should probably not be exposed to the
> applications.

I briefly discussed this with Johan and he is apparently not against
this idea either. I'll send some proposal after we first clarify the
adoption of something like btd_server, discussed below in the message.

As you mention, there needs to be some filtering to avoid exposing too
much external information. The first idea that comes to mind is to
extend btd_profile with a member that specifies it's type, but I have
to think this through.

>
>> 2. Exploit btd_service's userdata to avoid maintaining internal lists
>> and associations. This should be fairly easy and specially interesting
>> after (1), making for example avdtp_get_internal() trivial.
>
> This should be fine provided with can do (1).
>
>> 3. Extend btd_service_state_t with BTD_SERVICE_STATE_ACTIVE (i.e.
>> "Service is using a considerable bandwidth"), which would map to
>> PLAYING for many audio profiles. I don't have a strong opinion on this
>> one since the concept might not be generic enough to be adopted in the
>> core. For example, it might make sense for networking profiles (i.e.
>> an active connection exists) but on the contrary the HID profile would
>> make little use of this state.
>
> I would't have this extra state in the service, it is too profile
> specific, what we really want is that the core takes care of
> connection logic but the underlining signalling/data stream is profile
> specif so it is better to leave the plugins/external profiles to
> handle.

OK, let's proceed this way.

>
>> 4. Assuming (1) and (3) are acceptable, remove all internal
>> audio-specific states and their callbacks (e.g. source_state_t,
>> sink_state_t, avdtp_session_state_t...) and replace them with the core
>> btd_service states.
>
> Sink and Source state will probably remain due to A2DP design of
> Suspend/Start, but the other can probably be removed in favor of
> service states.

OK.

>
>> 5. Extend the core with something like btd_server, which is analogous
>> to btd_service but representing local profile implementations. This is
>> an orthogonal discussion but I believe the audio profiles could
>> benefit a lot from this infrastructure as well.
>
> Well ultimately I would like to have the core doing the connection
> management for plugins as it does for external profiles, perhaps this
> is your idea with btd_server as well, but for that to happen we would
> need a bit more logic for handling records that have several PSMs to
> be listen or those that can receive multiple connections such as A2DP.

I think the idea is basically the same, and as you suggest we would
require the support of multiple PSMs. I'll try to elaborate this idea
and see if I can submit a RFC as a base for further discussions.

Cheers,
Mikel