A2DP reconfigure with BMW Carkit (supporting multi streams) timeouts.

A2DP reconfigure with BMW Carkit (supporting multi streams) timeouts.

[roystonr]

Hi All,

During the interop tests with BMW Carkit using DUT (Device under Test)
running Bluez stack, we observe that A2DP reconfiguration timeouts.

Scenario:
1)	Connect A2DP from DUT
2)	Start streaming
3)	Power down the BMW carkit
4)	Power on the BMW carkit
5)	A2DP streaming not resuming.

>From the sniffer logs, firstly we observe that the BMW carkit supports
multiple stream endpoints whereas the DUT supports only one. We observe
that after powering on, the BMW carkit(CK) was initiating an inbound A2DP
connection there by configuring the stream (INT and ACP SEID were 1 for
set configuration). Later on we observe that the DUT tries to reconfigure
the stream by sending a AVDTP_CLOSE as the stream had capabilities
differing from the remote device (CK in this case). The BMW CK rightly
acknowledges the AVDTP_CLOSE (here ACP SEID = 1). Following this the DUT
sends a stream configuration with INT SEID as 1 and ACP SEID as 2. To this
the remote CK doesn't respond.

We suspect this behaviour because the set configuration lastly send from
the DUT after an AVDTP_CLOSE should have been with ACP SEID as 1 and not
2. As per our understanding of the code, 2 is used as the ACP SEID because
avdtp_get_seps returns the 1st not in use SEP from the list that was
contructed based on the device capabalities retreived by the DUT from the
remote CK. It is observed that the DISCOVER response from the remote CK
provided SEID 2 info before 1.

Change expected:
avdtp_get_seps should be able to provide the SEP of the one used in the
AVDTP_CLOSE previously. But there weren't any previously closed streams
then provide the SEP which is not in use (as done currently).

Kindly let us know whether our understanding is right and can this be a
suspected cause of the issue seen. Correct if mistaken.

Regards,
Royston Rodrigues

Re: A2DP reconfigure with BMW Carkit (supporting multi streams) timeouts.

[Johan Hedberg]

[-- Attachment #1: Type: text/plain, Size: 1208 bytes --]

Hi Royston,

On Thu, Dec 09, 2010, roystonr@codeaurora.org wrote:
> Change expected:
> avdtp_get_seps should be able to provide the SEP of the one used in the
> AVDTP_CLOSE previously. But there weren't any previously closed streams
> then provide the SEP which is not in use (as done currently).
> 
> Kindly let us know whether our understanding is right and can this be a
> suspected cause of the issue seen. Correct if mistaken.

Sounds like that might be the cause. Have you experimented with patching
the bluez code to reuse the same SEP as was used for the Close? If not
I'd advice you to do that so we can be sure that a change to the ACP SEP
selection logic makes sense.

Right now the reconfiguration happens in audio/a2dp.c where there's a
setup->reconfigure flag to track if a new stream should be configured
after receiving close_ind. The a2dp_reconfigure function in a2dp.c is
responsible for selecting the remote SEP. Could you try the attached
(completely untested) patch to see if it helps? It stores the old remote
SEP in the setup structure when starting the Close procedure in which
case a2dp_reconfigure() shouldn't try to reselect a new remote SEP but
use the stored one instead.

Johan

[-- Attachment #2: acp_sep_selection.patch --]
[-- Type: text/x-diff, Size: 1602 bytes --]

diff --git a/audio/a2dp.c b/audio/a2dp.c
index b1e94d9..7ed84e9 100644
--- a/audio/a2dp.c
+++ b/audio/a2dp.c
@@ -2032,6 +2032,7 @@ unsigned int a2dp_config(struct avdtp *session, struct a2dp_sep *sep,
 				error("avdtp_close failed");
 				goto failed;
 			}
+			setup->rsep = avdtp_stream_get_remote_sep(tmp->stream);
 			break;
 		}
 
@@ -2061,6 +2062,7 @@ unsigned int a2dp_config(struct avdtp *session, struct a2dp_sep *sep,
 				error("avdtp_close failed");
 				goto failed;
 			}
+			setup->rsep = avdtp_stream_get_remote_sep(sep->stream);
 		}
 		break;
 	default:
diff --git a/audio/avdtp.c b/audio/avdtp.c
index 1683e7c..33178c3 100644
--- a/audio/avdtp.c
+++ b/audio/avdtp.c
@@ -3133,6 +3133,12 @@ gboolean avdtp_stream_has_capabilities(struct avdtp_stream *stream,
 	return TRUE;
 }
 
+struct avdtp_remote_sep *avdtp_stream_get_remote_sep(
+						struct avdtp_stream *stream)
+{
+	return avdtp_get_remote_sep(stream->session, stream->rseid);
+}
+
 gboolean avdtp_stream_get_transport(struct avdtp_stream *stream, int *sock,
 					uint16_t *imtu, uint16_t *omtu,
 					GSList **caps)
diff --git a/audio/avdtp.h b/audio/avdtp.h
index 9406fa7..5f37dc3 100644
--- a/audio/avdtp.h
+++ b/audio/avdtp.h
@@ -257,6 +257,8 @@ gboolean avdtp_stream_has_capability(struct avdtp_stream *stream,
 				struct avdtp_service_capability *cap);
 gboolean avdtp_stream_has_capabilities(struct avdtp_stream *stream,
 					GSList *caps);
+struct avdtp_remote_sep *avdtp_stream_get_remote_sep(
+						struct avdtp_stream *stream);
 
 unsigned int avdtp_add_state_cb(avdtp_session_state_cb cb, void *user_data);
 

Re: A2DP reconfigure with BMW Carkit (supporting multi streams) timeouts.

[Luiz Augusto von Dentz]

Hi,

On Thu, Dec 9, 2010 at 4:41 PM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> Hi Royston,
>
> On Thu, Dec 09, 2010, roystonr@codeaurora.org wrote:
>> Change expected:
>> avdtp_get_seps should be able to provide the SEP of the one used in the
>> AVDTP_CLOSE previously. But there weren't any previously closed streams
>> then provide the SEP which is not in use (as done currently).
>>
>> Kindly let us know whether our understanding is right and can this be a
>> suspected cause of the issue seen. Correct if mistaken.
>
> Sounds like that might be the cause. Have you experimented with patching
> the bluez code to reuse the same SEP as was used for the Close? If not
> I'd advice you to do that so we can be sure that a change to the ACP SEP
> selection logic makes sense.
>
> Right now the reconfiguration happens in audio/a2dp.c where there's a
> setup->reconfigure flag to track if a new stream should be configured
> after receiving close_ind. The a2dp_reconfigure function in a2dp.c is
> responsible for selecting the remote SEP. Could you try the attached
> (completely untested) patch to see if it helps? It stores the old remote
> SEP in the setup structure when starting the Close procedure in which
> case a2dp_reconfigure() shouldn't try to reselect a new remote SEP but
> use the stored one instead.

I guess it would be better to have the call to
avdtp_stream_get_remote_sep on close_cfm, it is probably safer to do
there and less code too, but the fix is probably right.

-- 
Luiz Augusto von Dentz
Computer Engineer

Re: A2DP reconfigure with BMW Carkit (supporting multi streams) timeouts.

[Johan Hedberg]

[-- Attachment #1: Type: text/plain, Size: 376 bytes --]

Hi Luiz,

On Fri, Dec 10, 2010, Luiz Augusto von Dentz wrote:
> I guess it would be better to have the call to
> avdtp_stream_get_remote_sep on close_cfm, it is probably safer to do
> there and less code too, but the fix is probably right.

Good point. Attach is a patch which does this. Could we now please get
some feedback from the reporter if this really helps? :)

Johan

[-- Attachment #2: acp_sep_selection.patch --]
[-- Type: text/x-diff, Size: 1420 bytes --]

diff --git a/audio/a2dp.c b/audio/a2dp.c
index b1e94d9..b46cef1 100644
--- a/audio/a2dp.c
+++ b/audio/a2dp.c
@@ -1097,6 +1097,9 @@ static void close_cfm(struct avdtp *session, struct avdtp_local_sep *sep,
 		return;
 	}
 
+	if (!setup->rsep)
+		setup->rsep = avdtp_stream_get_remote_sep(stream);
+
 	if (setup->reconfigure)
 		g_timeout_add(RECONFIGURE_TIMEOUT, a2dp_reconfigure, setup);
 }
diff --git a/audio/avdtp.c b/audio/avdtp.c
index 1683e7c..33178c3 100644
--- a/audio/avdtp.c
+++ b/audio/avdtp.c
@@ -3133,6 +3133,12 @@ gboolean avdtp_stream_has_capabilities(struct avdtp_stream *stream,
 	return TRUE;
 }
 
+struct avdtp_remote_sep *avdtp_stream_get_remote_sep(
+						struct avdtp_stream *stream)
+{
+	return avdtp_get_remote_sep(stream->session, stream->rseid);
+}
+
 gboolean avdtp_stream_get_transport(struct avdtp_stream *stream, int *sock,
 					uint16_t *imtu, uint16_t *omtu,
 					GSList **caps)
diff --git a/audio/avdtp.h b/audio/avdtp.h
index 9406fa7..5f37dc3 100644
--- a/audio/avdtp.h
+++ b/audio/avdtp.h
@@ -257,6 +257,8 @@ gboolean avdtp_stream_has_capability(struct avdtp_stream *stream,
 				struct avdtp_service_capability *cap);
 gboolean avdtp_stream_has_capabilities(struct avdtp_stream *stream,
 					GSList *caps);
+struct avdtp_remote_sep *avdtp_stream_get_remote_sep(
+						struct avdtp_stream *stream);
 
 unsigned int avdtp_add_state_cb(avdtp_session_state_cb cb, void *user_data);
 

Re: A2DP reconfigure with BMW Carkit (supporting multi streams) timeouts.

[roystonr]

Hi Johan/Luiz,

> On Fri, Dec 10, 2010, Luiz Augusto von Dentz wrote:
> I guess it would be better to have the call to
> avdtp_stream_get_remote_sep on close_cfm, it is probably safer to do
> there and less code too, but the fix is probably right.

Along with this as per my understanding, struct avdtp_remote_sep *rsep has
to be added as member to struct a2dp_setup and rsep should be overridden
as follows in a2dp_reconfigure after call to avdtp_get_seps which returns
the sep corresponding to the stream not in use. If the remote device
supports multi-stream then sep returned would be based on the order of
get_capabilities response. Hence the following override is needed to
ensure that closed stream is reconfigured. Correct me if mistaken.

if (setup->rsep) {
  rsep = setup->rsep;
}

Have provided the patch to the tester to verify as we don't have the BMW
CK available locally. Would update the feedback received.

Regards,
Royston Rodrigues

Re: A2DP reconfigure with BMW Carkit (supporting multi streams) timeouts.

[Lukasz Rymanowski]

Hi Roystonr,

> Have provided the patch to the tester to verify as we don't have the BMW
> CK available locally. Would update the feedback received.

I'm just wondering, Is it working for you ?

>
> Regards,
> Royston Rodrigues
>

Regards
\Lukasz

RE: A2DP reconfigure with BMW Carkit (supporting multi streams) timeouts.

[Höglind, Henrik]

Hi Lukasz, Roystonr, Johan,

>> Have provided the patch to the tester to verify as we don't have the BMW
>> CK available locally. Would update the feedback received.
>
>I'm just wondering, Is it working for you ?

We have been testing this patch and it fixes the problem.

Best regards,
Henrik

Re: A2DP reconfigure with BMW Carkit (supporting multi streams) timeouts.

[Johan Hedberg]

Hi Henrik,

On Tue, Feb 08, 2011, Höglind, Henrik wrote:
> Hi Lukasz, Roystonr, Johan,
> 
> >> Have provided the patch to the tester to verify as we don't have the BMW
> >> CK available locally. Would update the feedback received.
> >
> >I'm just wondering, Is it working for you ?
> 
> We have been testing this patch and it fixes the problem.

Thanks for the confirmation. I've now pushed a patch for this upstream.

Johan