From: Luiz Augusto von Dentz <[email protected]>
If SEP is in configured state that means OPEN is about to happen so just
mark start flag and send START once OPEN completes.
---
profiles/audio/a2dp.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 54 insertions(+)
diff --git a/profiles/audio/a2dp.c b/profiles/audio/a2dp.c
index f1646ee..d9680a7 100644
--- a/profiles/audio/a2dp.c
+++ b/profiles/audio/a2dp.c
@@ -336,6 +336,20 @@ static struct a2dp_setup *find_setup_by_dev(struct audio_device *dev)
return NULL;
}
+static struct a2dp_setup *find_setup_by_stream(struct avdtp_stream *stream)
+{
+ GSList *l;
+
+ for (l = setups; l != NULL; l = l->next) {
+ struct a2dp_setup *setup = l->data;
+
+ if (setup->stream == stream)
+ return setup;
+ }
+
+ return NULL;
+}
+
static void stream_state_changed(struct avdtp_stream *stream,
avdtp_state_t old_state,
avdtp_state_t new_state,
@@ -344,6 +358,29 @@ static void stream_state_changed(struct avdtp_stream *stream,
{
struct a2dp_sep *sep = user_data;
+ if (new_state == AVDTP_STATE_OPEN) {
+ struct a2dp_setup *setup;
+ int err;
+
+ setup = find_setup_by_stream(stream);
+ if (!setup || !setup->start)
+ return;
+
+ setup->start = FALSE;
+
+ err = avdtp_start(setup->session, stream);
+ if (err < 0 && err != -EINPROGRESS) {
+ error("avdtp_start: %s (%d)", strerror(-err), -err);
+ finalize_setup_errno(setup, err, finalize_resume,
+ NULL);
+ return;
+ }
+
+ sep->starting = TRUE;
+
+ return;
+ }
+
if (new_state != AVDTP_STATE_IDLE)
return;
@@ -661,6 +698,12 @@ static gboolean open_ind(struct avdtp *session, struct avdtp_local_sep *sep,
finalize_config(setup);
+ if (!setup->start || !err)
+ return TRUE;
+
+ setup->start = FALSE;
+ finalize_resume(setup);
+
return TRUE;
}
@@ -689,6 +732,14 @@ static void open_cfm(struct avdtp *session, struct avdtp_local_sep *sep,
}
finalize_config(setup);
+
+ if (!setup->start || !err)
+ return;
+
+ setup->start = FALSE;
+ finalize_resume(setup);
+
+ return;
}
static gboolean suspend_timeout(struct a2dp_sep *sep)
@@ -1718,6 +1769,9 @@ unsigned int a2dp_resume(struct avdtp *session, struct a2dp_sep *sep,
case AVDTP_STATE_IDLE:
goto failed;
break;
+ case AVDTP_STATE_CONFIGURED:
+ setup->start = TRUE;
+ break;
case AVDTP_STATE_OPEN:
if (avdtp_start(session, sep->stream) < 0) {
error("avdtp_start failed");
--
1.8.1.2
Hi Mikel,
On Wed, Feb 20, 2013 at 3:37 PM, Mikel Astiz <[email protected]> wrote:
> Hi Luiz,
>
> On Tue, Feb 19, 2013 at 4:30 PM, Luiz Augusto von Dentz
> <[email protected]> wrote:
>> Hi Mikel,
>>
>> On Tue, Feb 19, 2013 at 5:06 PM, Mikel Astiz <[email protected]> wrote:
>>> Hi Luiz,
>>>
>>> On Tue, Feb 19, 2013 at 3:33 PM, Luiz Augusto von Dentz
>>> <[email protected]> wrote:
>>>> From: Luiz Augusto von Dentz <[email protected]>
>>>>
>>>> If SEP is in configured state that means OPEN is about to happen so just
>>>> mark start flag and send START once OPEN completes.
>>>> ---
>>>> profiles/audio/a2dp.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>>> 1 file changed, 54 insertions(+)
>>>>
>>>> diff --git a/profiles/audio/a2dp.c b/profiles/audio/a2dp.c
>>>> index f1646ee..d9680a7 100644
>>>> --- a/profiles/audio/a2dp.c
>>>> +++ b/profiles/audio/a2dp.c
>>>> @@ -336,6 +336,20 @@ static struct a2dp_setup *find_setup_by_dev(struct audio_device *dev)
>>>> return NULL;
>>>> }
>>>>
>>>> +static struct a2dp_setup *find_setup_by_stream(struct avdtp_stream *stream)
>>>> +{
>>>> + GSList *l;
>>>> +
>>>> + for (l = setups; l != NULL; l = l->next) {
>>>> + struct a2dp_setup *setup = l->data;
>>>> +
>>>> + if (setup->stream == stream)
>>>> + return setup;
>>>> + }
>>>> +
>>>> + return NULL;
>>>> +}
>>>> +
>>>> static void stream_state_changed(struct avdtp_stream *stream,
>>>> avdtp_state_t old_state,
>>>> avdtp_state_t new_state,
>>>> @@ -344,6 +358,29 @@ static void stream_state_changed(struct avdtp_stream *stream,
>>>> {
>>>> struct a2dp_sep *sep = user_data;
>>>>
>>>> + if (new_state == AVDTP_STATE_OPEN) {
>>>> + struct a2dp_setup *setup;
>>>> + int err;
>>>> +
>>>> + setup = find_setup_by_stream(stream);
>>>> + if (!setup || !setup->start)
>>>> + return;
>>>> +
>>>> + setup->start = FALSE;
>>>> +
>>>> + err = avdtp_start(setup->session, stream);
>>>> + if (err < 0 && err != -EINPROGRESS) {
>>>> + error("avdtp_start: %s (%d)", strerror(-err), -err);
>>>> + finalize_setup_errno(setup, err, finalize_resume,
>>>> + NULL);
>>>> + return;
>>>> + }
>>>> +
>>>> + sep->starting = TRUE;
>>>> +
>>>> + return;
>>>> + }
>>>> +
>>>
>>> Shouldn't you handle the error case here? Any other transition from
>>> AVDTP_STATE_CONFIGURED should probably be considered a failure.
>>
>> The failure case is where OPEN fails, which is handled by open_ind and
>> open_cfm, or if stream connection timeout which will generate an ABORT
>> that takes care of terminating the setup.
>>
>>>> if (new_state != AVDTP_STATE_IDLE)
>>>> return;
>>>>
>>>> @@ -661,6 +698,12 @@ static gboolean open_ind(struct avdtp *session, struct avdtp_local_sep *sep,
>>>>
>>>> finalize_config(setup);
>>>>
>>>> + if (!setup->start || !err)
>>>> + return TRUE;
>>>> +
>>>> + setup->start = FALSE;
>>>> + finalize_resume(setup);
>>>> +
>>>> return TRUE;
>>>> }
>>>>
>>>> @@ -689,6 +732,14 @@ static void open_cfm(struct avdtp *session, struct avdtp_local_sep *sep,
>>>> }
>>>>
>>>> finalize_config(setup);
>>>> +
>>>> + if (!setup->start || !err)
>>>> + return;
>>>> +
>>>> + setup->start = FALSE;
>>>> + finalize_resume(setup);
>>>> +
>>>> + return;
>>>> }
>>>>
>>>> static gboolean suspend_timeout(struct a2dp_sep *sep)
>>>> @@ -1718,6 +1769,9 @@ unsigned int a2dp_resume(struct avdtp *session, struct a2dp_sep *sep,
>>>> case AVDTP_STATE_IDLE:
>>>> goto failed;
>>>> break;
>>>> + case AVDTP_STATE_CONFIGURED:
>>>> + setup->start = TRUE;
>>>> + break;
>>>> case AVDTP_STATE_OPEN:
>>>> if (avdtp_start(session, sep->stream) < 0) {
>>>> error("avdtp_start failed");
>>>> --
>>>
>>> This alternative has the obvious advantage of not introducing an
>>> additional state to the transport and simpler APIs are generally good.
>>>
>>> However, I see little benefit in the client's side and the
>>> implementation gets a bit tricky in the server's side (BlueZ and
>>> oFono).
>>>
>>> I'd personally prefer to go simple and make this state explicit,
>>> instead of trying to internally defer the operation.
>>
>> I don't see this as being so complicated, in fact it is probably less
>> code than having to deal with another state because nothing has to be
>> changed in the clients, start flag is actually already there in case
>> the client resume while we are waiting for suspend to complete and for
>> HFP there is probably no mapping to configuring state since the SCO
>> configuration is managed by the kernel.
>
> Ack from my side.
Applied
--
Luiz Augusto von Dentz
Hi Luiz,
On Tue, Feb 19, 2013 at 4:30 PM, Luiz Augusto von Dentz
<[email protected]> wrote:
> Hi Mikel,
>
> On Tue, Feb 19, 2013 at 5:06 PM, Mikel Astiz <[email protected]> wrote:
>> Hi Luiz,
>>
>> On Tue, Feb 19, 2013 at 3:33 PM, Luiz Augusto von Dentz
>> <[email protected]> wrote:
>>> From: Luiz Augusto von Dentz <[email protected]>
>>>
>>> If SEP is in configured state that means OPEN is about to happen so just
>>> mark start flag and send START once OPEN completes.
>>> ---
>>> profiles/audio/a2dp.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>> 1 file changed, 54 insertions(+)
>>>
>>> diff --git a/profiles/audio/a2dp.c b/profiles/audio/a2dp.c
>>> index f1646ee..d9680a7 100644
>>> --- a/profiles/audio/a2dp.c
>>> +++ b/profiles/audio/a2dp.c
>>> @@ -336,6 +336,20 @@ static struct a2dp_setup *find_setup_by_dev(struct audio_device *dev)
>>> return NULL;
>>> }
>>>
>>> +static struct a2dp_setup *find_setup_by_stream(struct avdtp_stream *stream)
>>> +{
>>> + GSList *l;
>>> +
>>> + for (l = setups; l != NULL; l = l->next) {
>>> + struct a2dp_setup *setup = l->data;
>>> +
>>> + if (setup->stream == stream)
>>> + return setup;
>>> + }
>>> +
>>> + return NULL;
>>> +}
>>> +
>>> static void stream_state_changed(struct avdtp_stream *stream,
>>> avdtp_state_t old_state,
>>> avdtp_state_t new_state,
>>> @@ -344,6 +358,29 @@ static void stream_state_changed(struct avdtp_stream *stream,
>>> {
>>> struct a2dp_sep *sep = user_data;
>>>
>>> + if (new_state == AVDTP_STATE_OPEN) {
>>> + struct a2dp_setup *setup;
>>> + int err;
>>> +
>>> + setup = find_setup_by_stream(stream);
>>> + if (!setup || !setup->start)
>>> + return;
>>> +
>>> + setup->start = FALSE;
>>> +
>>> + err = avdtp_start(setup->session, stream);
>>> + if (err < 0 && err != -EINPROGRESS) {
>>> + error("avdtp_start: %s (%d)", strerror(-err), -err);
>>> + finalize_setup_errno(setup, err, finalize_resume,
>>> + NULL);
>>> + return;
>>> + }
>>> +
>>> + sep->starting = TRUE;
>>> +
>>> + return;
>>> + }
>>> +
>>
>> Shouldn't you handle the error case here? Any other transition from
>> AVDTP_STATE_CONFIGURED should probably be considered a failure.
>
> The failure case is where OPEN fails, which is handled by open_ind and
> open_cfm, or if stream connection timeout which will generate an ABORT
> that takes care of terminating the setup.
>
>>> if (new_state != AVDTP_STATE_IDLE)
>>> return;
>>>
>>> @@ -661,6 +698,12 @@ static gboolean open_ind(struct avdtp *session, struct avdtp_local_sep *sep,
>>>
>>> finalize_config(setup);
>>>
>>> + if (!setup->start || !err)
>>> + return TRUE;
>>> +
>>> + setup->start = FALSE;
>>> + finalize_resume(setup);
>>> +
>>> return TRUE;
>>> }
>>>
>>> @@ -689,6 +732,14 @@ static void open_cfm(struct avdtp *session, struct avdtp_local_sep *sep,
>>> }
>>>
>>> finalize_config(setup);
>>> +
>>> + if (!setup->start || !err)
>>> + return;
>>> +
>>> + setup->start = FALSE;
>>> + finalize_resume(setup);
>>> +
>>> + return;
>>> }
>>>
>>> static gboolean suspend_timeout(struct a2dp_sep *sep)
>>> @@ -1718,6 +1769,9 @@ unsigned int a2dp_resume(struct avdtp *session, struct a2dp_sep *sep,
>>> case AVDTP_STATE_IDLE:
>>> goto failed;
>>> break;
>>> + case AVDTP_STATE_CONFIGURED:
>>> + setup->start = TRUE;
>>> + break;
>>> case AVDTP_STATE_OPEN:
>>> if (avdtp_start(session, sep->stream) < 0) {
>>> error("avdtp_start failed");
>>> --
>>
>> This alternative has the obvious advantage of not introducing an
>> additional state to the transport and simpler APIs are generally good.
>>
>> However, I see little benefit in the client's side and the
>> implementation gets a bit tricky in the server's side (BlueZ and
>> oFono).
>>
>> I'd personally prefer to go simple and make this state explicit,
>> instead of trying to internally defer the operation.
>
> I don't see this as being so complicated, in fact it is probably less
> code than having to deal with another state because nothing has to be
> changed in the clients, start flag is actually already there in case
> the client resume while we are waiting for suspend to complete and for
> HFP there is probably no mapping to configuring state since the SCO
> configuration is managed by the kernel.
Ack from my side.
Cheers,
Mikel
Hi Mikel,
On Tue, Feb 19, 2013 at 5:06 PM, Mikel Astiz <[email protected]> wrote:
> Hi Luiz,
>
> On Tue, Feb 19, 2013 at 3:33 PM, Luiz Augusto von Dentz
> <[email protected]> wrote:
>> From: Luiz Augusto von Dentz <[email protected]>
>>
>> If SEP is in configured state that means OPEN is about to happen so just
>> mark start flag and send START once OPEN completes.
>> ---
>> profiles/audio/a2dp.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 54 insertions(+)
>>
>> diff --git a/profiles/audio/a2dp.c b/profiles/audio/a2dp.c
>> index f1646ee..d9680a7 100644
>> --- a/profiles/audio/a2dp.c
>> +++ b/profiles/audio/a2dp.c
>> @@ -336,6 +336,20 @@ static struct a2dp_setup *find_setup_by_dev(struct audio_device *dev)
>> return NULL;
>> }
>>
>> +static struct a2dp_setup *find_setup_by_stream(struct avdtp_stream *stream)
>> +{
>> + GSList *l;
>> +
>> + for (l = setups; l != NULL; l = l->next) {
>> + struct a2dp_setup *setup = l->data;
>> +
>> + if (setup->stream == stream)
>> + return setup;
>> + }
>> +
>> + return NULL;
>> +}
>> +
>> static void stream_state_changed(struct avdtp_stream *stream,
>> avdtp_state_t old_state,
>> avdtp_state_t new_state,
>> @@ -344,6 +358,29 @@ static void stream_state_changed(struct avdtp_stream *stream,
>> {
>> struct a2dp_sep *sep = user_data;
>>
>> + if (new_state == AVDTP_STATE_OPEN) {
>> + struct a2dp_setup *setup;
>> + int err;
>> +
>> + setup = find_setup_by_stream(stream);
>> + if (!setup || !setup->start)
>> + return;
>> +
>> + setup->start = FALSE;
>> +
>> + err = avdtp_start(setup->session, stream);
>> + if (err < 0 && err != -EINPROGRESS) {
>> + error("avdtp_start: %s (%d)", strerror(-err), -err);
>> + finalize_setup_errno(setup, err, finalize_resume,
>> + NULL);
>> + return;
>> + }
>> +
>> + sep->starting = TRUE;
>> +
>> + return;
>> + }
>> +
>
> Shouldn't you handle the error case here? Any other transition from
> AVDTP_STATE_CONFIGURED should probably be considered a failure.
The failure case is where OPEN fails, which is handled by open_ind and
open_cfm, or if stream connection timeout which will generate an ABORT
that takes care of terminating the setup.
>> if (new_state != AVDTP_STATE_IDLE)
>> return;
>>
>> @@ -661,6 +698,12 @@ static gboolean open_ind(struct avdtp *session, struct avdtp_local_sep *sep,
>>
>> finalize_config(setup);
>>
>> + if (!setup->start || !err)
>> + return TRUE;
>> +
>> + setup->start = FALSE;
>> + finalize_resume(setup);
>> +
>> return TRUE;
>> }
>>
>> @@ -689,6 +732,14 @@ static void open_cfm(struct avdtp *session, struct avdtp_local_sep *sep,
>> }
>>
>> finalize_config(setup);
>> +
>> + if (!setup->start || !err)
>> + return;
>> +
>> + setup->start = FALSE;
>> + finalize_resume(setup);
>> +
>> + return;
>> }
>>
>> static gboolean suspend_timeout(struct a2dp_sep *sep)
>> @@ -1718,6 +1769,9 @@ unsigned int a2dp_resume(struct avdtp *session, struct a2dp_sep *sep,
>> case AVDTP_STATE_IDLE:
>> goto failed;
>> break;
>> + case AVDTP_STATE_CONFIGURED:
>> + setup->start = TRUE;
>> + break;
>> case AVDTP_STATE_OPEN:
>> if (avdtp_start(session, sep->stream) < 0) {
>> error("avdtp_start failed");
>> --
>
> This alternative has the obvious advantage of not introducing an
> additional state to the transport and simpler APIs are generally good.
>
> However, I see little benefit in the client's side and the
> implementation gets a bit tricky in the server's side (BlueZ and
> oFono).
>
> I'd personally prefer to go simple and make this state explicit,
> instead of trying to internally defer the operation.
I don't see this as being so complicated, in fact it is probably less
code than having to deal with another state because nothing has to be
changed in the clients, start flag is actually already there in case
the client resume while we are waiting for suspend to complete and for
HFP there is probably no mapping to configuring state since the SCO
configuration is managed by the kernel.
--
Luiz Augusto von Dentz
Hi Luiz,
On Tue, Feb 19, 2013 at 3:33 PM, Luiz Augusto von Dentz
<[email protected]> wrote:
> From: Luiz Augusto von Dentz <[email protected]>
>
> If SEP is in configured state that means OPEN is about to happen so just
> mark start flag and send START once OPEN completes.
> ---
> profiles/audio/a2dp.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 54 insertions(+)
>
> diff --git a/profiles/audio/a2dp.c b/profiles/audio/a2dp.c
> index f1646ee..d9680a7 100644
> --- a/profiles/audio/a2dp.c
> +++ b/profiles/audio/a2dp.c
> @@ -336,6 +336,20 @@ static struct a2dp_setup *find_setup_by_dev(struct audio_device *dev)
> return NULL;
> }
>
> +static struct a2dp_setup *find_setup_by_stream(struct avdtp_stream *stream)
> +{
> + GSList *l;
> +
> + for (l = setups; l != NULL; l = l->next) {
> + struct a2dp_setup *setup = l->data;
> +
> + if (setup->stream == stream)
> + return setup;
> + }
> +
> + return NULL;
> +}
> +
> static void stream_state_changed(struct avdtp_stream *stream,
> avdtp_state_t old_state,
> avdtp_state_t new_state,
> @@ -344,6 +358,29 @@ static void stream_state_changed(struct avdtp_stream *stream,
> {
> struct a2dp_sep *sep = user_data;
>
> + if (new_state == AVDTP_STATE_OPEN) {
> + struct a2dp_setup *setup;
> + int err;
> +
> + setup = find_setup_by_stream(stream);
> + if (!setup || !setup->start)
> + return;
> +
> + setup->start = FALSE;
> +
> + err = avdtp_start(setup->session, stream);
> + if (err < 0 && err != -EINPROGRESS) {
> + error("avdtp_start: %s (%d)", strerror(-err), -err);
> + finalize_setup_errno(setup, err, finalize_resume,
> + NULL);
> + return;
> + }
> +
> + sep->starting = TRUE;
> +
> + return;
> + }
> +
Shouldn't you handle the error case here? Any other transition from
AVDTP_STATE_CONFIGURED should probably be considered a failure.
> if (new_state != AVDTP_STATE_IDLE)
> return;
>
> @@ -661,6 +698,12 @@ static gboolean open_ind(struct avdtp *session, struct avdtp_local_sep *sep,
>
> finalize_config(setup);
>
> + if (!setup->start || !err)
> + return TRUE;
> +
> + setup->start = FALSE;
> + finalize_resume(setup);
> +
> return TRUE;
> }
>
> @@ -689,6 +732,14 @@ static void open_cfm(struct avdtp *session, struct avdtp_local_sep *sep,
> }
>
> finalize_config(setup);
> +
> + if (!setup->start || !err)
> + return;
> +
> + setup->start = FALSE;
> + finalize_resume(setup);
> +
> + return;
> }
>
> static gboolean suspend_timeout(struct a2dp_sep *sep)
> @@ -1718,6 +1769,9 @@ unsigned int a2dp_resume(struct avdtp *session, struct a2dp_sep *sep,
> case AVDTP_STATE_IDLE:
> goto failed;
> break;
> + case AVDTP_STATE_CONFIGURED:
> + setup->start = TRUE;
> + break;
> case AVDTP_STATE_OPEN:
> if (avdtp_start(session, sep->stream) < 0) {
> error("avdtp_start failed");
> --
This alternative has the obvious advantage of not introducing an
additional state to the transport and simpler APIs are generally good.
However, I see little benefit in the client's side and the
implementation gets a bit tricky in the server's side (BlueZ and
oFono).
I'd personally prefer to go simple and make this state explicit,
instead of trying to internally defer the operation.
Cheers,
Mikel
From: Luiz Augusto von Dentz <[email protected]>
---
profiles/audio/avdtp.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/profiles/audio/avdtp.c b/profiles/audio/avdtp.c
index 403a22b..0205644 100644
--- a/profiles/audio/avdtp.c
+++ b/profiles/audio/avdtp.c
@@ -1706,7 +1706,7 @@ static gboolean avdtp_open_cmd(struct avdtp *session, uint8_t transaction,
stream = sep->stream;
if (sep->ind && sep->ind->open) {
- if (!sep->ind->open(session, sep, stream, &err,
+ if (!sep->ind->open(session, sep, stream, NULL,
sep->user_data))
goto failed;
}
--
1.8.1.2