diff mbox series

[BlueZ,bluez] bap: Add idle notification for ASE State

Message ID 20250407-bap_aes_state-v1-1-dfc090c49cea@amlogic.com (mailing list archive)
State New
Headers show
Series [BlueZ,bluez] bap: Add idle notification for ASE State | expand

Checks

Context Check Description
tedd_an/pre-ci_am success Success
tedd_an/BuildEll success Build ELL PASS
tedd_an/BluezMake success Bluez Make PASS
tedd_an/MakeCheck success Bluez Make Check PASS
tedd_an/MakeDistcheck success Make Distcheck PASS
tedd_an/CheckValgrind success Check Valgrind PASS
tedd_an/CheckSmatch warning CheckSparse WARNING src/shared/bap.c:314:25: warning: array of flexible structuressrc/shared/bap.c: note: in included file:./src/shared/ascs.h:88:25: warning: array of flexible structuressrc/shared/bap.c:314:25: warning: array of flexible structuressrc/shared/bap.c: note: in included file:./src/shared/ascs.h:88:25: warning: array of flexible structuressrc/shared/bap.c:314:25: warning: array of flexible structuressrc/shared/bap.c: note: in included file:./src/shared/ascs.h:88:25: warning: array of flexible structures
tedd_an/bluezmakeextell success Make External ELL PASS
tedd_an/ScanBuild success Scan Build PASS

Commit Message

Yang Li April 7, 2025, 10:34 a.m. UTC 
From: Yang Li <yang.li@amlogic.com>

When the ASE state changes from releasing(6) -> idle(0),
the idle state needs to be notified to the Client.

---
Signed-off-by: Yang Li <yang.li@amlogic.com>
---
 src/shared/bap.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)


---
base-commit: 0efa20cbf3fb5693c7c2f14ba8cf67053ca029e5
change-id: 20250407-bap_aes_state-9306798ff95a

Best regards,

Comments

bluez.test.bot@gmail.com April 7, 2025, 11:39 a.m. UTC | #1 
This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=950501

---Test result---

Test Summary:
CheckPatch                    PENDING   0.32 seconds
GitLint                       PENDING   0.32 seconds
BuildEll                      PASS      20.39 seconds
BluezMake                     PASS      1606.58 seconds
MakeCheck                     PASS      20.68 seconds
MakeDistcheck                 PASS      157.73 seconds
CheckValgrind                 PASS      213.33 seconds
CheckSmatch                   WARNING   283.70 seconds
bluezmakeextell               PASS      97.99 seconds
IncrementalBuild              PENDING   0.34 seconds
ScanBuild                     PASS      870.13 seconds

Details
##############################
Test: CheckPatch - PENDING
Desc: Run checkpatch.pl script
Output:

##############################
Test: GitLint - PENDING
Desc: Run gitlint
Output:

##############################
Test: CheckSmatch - WARNING
Desc: Run smatch tool with source
Output:
src/shared/bap.c:314:25: warning: array of flexible structuressrc/shared/bap.c: note: in included file:./src/shared/ascs.h:88:25: warning: array of flexible structuressrc/shared/bap.c:314:25: warning: array of flexible structuressrc/shared/bap.c: note: in included file:./src/shared/ascs.h:88:25: warning: array of flexible structuressrc/shared/bap.c:314:25: warning: array of flexible structuressrc/shared/bap.c: note: in included file:./src/shared/ascs.h:88:25: warning: array of flexible structures
##############################
Test: IncrementalBuild - PENDING
Desc: Incremental build with the patches in the series
Output:



---
Regards,
Linux Bluetooth
Luiz Augusto von Dentz April 7, 2025, 1:08 p.m. UTC | #2 
Hi Yang,

On Mon, Apr 7, 2025 at 6:34 AM Yang Li via B4 Relay
<devnull+yang.li.amlogic.com@kernel.org> wrote:
>
> From: Yang Li <yang.li@amlogic.com>
>
> When the ASE state changes from releasing(6) -> idle(0),
> the idle state needs to be notified to the Client.
>
> ---
> Signed-off-by: Yang Li <yang.li@amlogic.com>
> ---
>  src/shared/bap.c | 13 ++++---------
>  1 file changed, 4 insertions(+), 9 deletions(-)
>
> diff --git a/src/shared/bap.c b/src/shared/bap.c
> index 650bea2f4..c40d6e051 100644
> --- a/src/shared/bap.c
> +++ b/src/shared/bap.c
> @@ -1123,17 +1123,12 @@ static void stream_notify_metadata(struct bt_bap_stream *stream)
>         free(status);
>  }
>
> -static void stream_notify_release(struct bt_bap_stream *stream)
> +static void stream_notify_ase_state(struct bt_bap_stream *stream)
>  {
>         struct bt_bap_endpoint *ep = stream->ep;
>         struct bt_ascs_ase_status status;
>
> -       DBG(stream->bap, "stream %p", stream);
> -
> -
> -       memset(&status, 0, sizeof(status));
>         status.id = ep->id;
> -       ep->state = BT_BAP_STREAM_STATE_RELEASING;

Not really sure why you are taking away releasing state, we actually
depend on it for the new tests:

https://patchwork.kernel.org/project/bluetooth/list/?series=950067

>         status.state = ep->state;
>
>         gatt_db_attribute_notify(ep->attr, (void *)&status, sizeof(status),
> @@ -1713,6 +1708,7 @@ static void stream_notify(struct bt_bap_stream *stream, uint8_t state)
>
>         switch (state) {
>         case BT_ASCS_ASE_STATE_IDLE:
> +               stream_notify_ase_state(stream);

We need something like stream_notify_idle.

>                 break;
>         case BT_ASCS_ASE_STATE_CONFIG:
>                 stream_notify_config(stream);
> @@ -1726,7 +1722,7 @@ static void stream_notify(struct bt_bap_stream *stream, uint8_t state)
>                 stream_notify_metadata(stream);
>                 break;
>         case BT_ASCS_ASE_STATE_RELEASING:
> -               stream_notify_release(stream);
> +               stream_notify_ase_state(stream);

This is actually wrong, we need to notify the releasing state.

>                 break;
>         }
>  }
> @@ -6397,9 +6393,8 @@ static bool stream_io_disconnected(struct io *io, void *user_data)
>         DBG(stream->bap, "stream %p io disconnected", stream);
>
>         if (stream->ep->state == BT_ASCS_ASE_STATE_RELEASING)
> -               stream_set_state(stream, BT_BAP_STREAM_STATE_CONFIG);
> +               stream_set_state(stream, BT_BAP_STREAM_STATE_IDLE);

Ok, so this is sort of reverting from  bap: Fix not generating
releasing state? I was wondering how much testing you guys did to
confirm this is the right behavior, I don't think it is and Im trying
to figure out if there are any tests for the testing spec that do
properly verify this behavior.

> -       bt_bap_stream_set_io(stream, -1);
>         return false;
>  }
>
>
> ---
> base-commit: 0efa20cbf3fb5693c7c2f14ba8cf67053ca029e5
> change-id: 20250407-bap_aes_state-9306798ff95a
>
> Best regards,
> --
> Yang Li <yang.li@amlogic.com>
>
>
>
Yang Li April 10, 2025, 2:50 a.m. UTC | #3 
Hi Luiz，
> [ EXTERNAL EMAIL ]
>
> Hi Yang,
>
> On Mon, Apr 7, 2025 at 6:34 AM Yang Li via B4 Relay
> <devnull+yang.li.amlogic.com@kernel.org> wrote:
>> From: Yang Li <yang.li@amlogic.com>
>>
>> When the ASE state changes from releasing(6) -> idle(0),
>> the idle state needs to be notified to the Client.
>>
>> ---
>> Signed-off-by: Yang Li <yang.li@amlogic.com>
>> ---
>>   src/shared/bap.c | 13 ++++---------
>>   1 file changed, 4 insertions(+), 9 deletions(-)
>>
>> diff --git a/src/shared/bap.c b/src/shared/bap.c
>> index 650bea2f4..c40d6e051 100644
>> --- a/src/shared/bap.c
>> +++ b/src/shared/bap.c
>> @@ -1123,17 +1123,12 @@ static void stream_notify_metadata(struct bt_bap_stream *stream)
>>          free(status);
>>   }
>>
>> -static void stream_notify_release(struct bt_bap_stream *stream)
>> +static void stream_notify_ase_state(struct bt_bap_stream *stream)
>>   {
>>          struct bt_bap_endpoint *ep = stream->ep;
>>          struct bt_ascs_ase_status status;
>>
>> -       DBG(stream->bap, "stream %p", stream);
>> -
>> -
>> -       memset(&status, 0, sizeof(status));
>>          status.id = ep->id;
>> -       ep->state = BT_BAP_STREAM_STATE_RELEASING;
> Not really sure why you are taking away releasing state, we actually
> depend on it for the new tests:
>
> https://patchwork.kernel.org/project/bluetooth/list/?series=950067
Well, I got it.
>>          status.state = ep->state;
>>
>>          gatt_db_attribute_notify(ep->attr, (void *)&status, sizeof(status),
>> @@ -1713,6 +1708,7 @@ static void stream_notify(struct bt_bap_stream *stream, uint8_t state)
>>
>>          switch (state) {
>>          case BT_ASCS_ASE_STATE_IDLE:
>> +               stream_notify_ase_state(stream);
> We need something like stream_notify_idle.
Well, I got it.
>>                  break;
>>          case BT_ASCS_ASE_STATE_CONFIG:
>>                  stream_notify_config(stream);
>> @@ -1726,7 +1722,7 @@ static void stream_notify(struct bt_bap_stream *stream, uint8_t state)
>>                  stream_notify_metadata(stream);
>>                  break;
>>          case BT_ASCS_ASE_STATE_RELEASING:
>> -               stream_notify_release(stream);
>> +               stream_notify_ase_state(stream);
> This is actually wrong, we need to notify the releasing state.
>
>>                  break;
>>          }
>>   }
>> @@ -6397,9 +6393,8 @@ static bool stream_io_disconnected(struct io *io, void *user_data)
>>          DBG(stream->bap, "stream %p io disconnected", stream);
>>
>>          if (stream->ep->state == BT_ASCS_ASE_STATE_RELEASING)
>> -               stream_set_state(stream, BT_BAP_STREAM_STATE_CONFIG);
>> +               stream_set_state(stream, BT_BAP_STREAM_STATE_IDLE);
> Ok, so this is sort of reverting from  bap: Fix not generating
> releasing state? I was wondering how much testing you guys did to
> confirm this is the right behavior, I don't think it is and Im trying
> to figure out if there are any tests for the testing spec that do
> properly verify this behavior.

There are some misunderstandings that need to be clarified. Initially, 
patchset V1 
https://lore.kernel.org/all/20250106-upstream-v1-1-a16879b78ffd@amlogic.com/ 
was proposed to solve the issue 
https://github.com/bluez/bluez/issues/1053, but after discussion, I felt 
that the process of Streaming->Releasing->Idle was OK, so V1 was 
abandoned. Then I submitted patchset V2/V3 
https://lore.kernel.org/all/20250213-ascs_bug-v3-1-d5594f0f20c6@amlogic.com/. 
I tested K70 and Pixel phones on V3, and both can solve the above 
issues. So my original intention was to merge V3, but after the release 
of version 5.82, I checked the code and found that V1 was merged. So I 
submitted the modification again on the latest version.

I sorted out the process of ASE state switching when the media of 
different mobile phones is paused:

1. Redmi K70+DUT: Pause operation, ASE state process is 
Streaming--->Disabling->QoS
2. Pixel+DUT: Pause operation, DUT does not cache Codec, ASE state 
process is Streaming->Releasing->Idl
3. Pixel+RedmiBud5 earbuds: Pause operation, earbuds cache Codec, ASE 
state process is Streaming->Releasing->Codec
If the DUT also caches Codec, the latest version of the process is 
Streaming->Releasing->Codec->QoS, but the QoS status will be abnormal on 
Pixel phones, causing LE disconnection, so the process without caching 
Codec is still adopted.
>
>> -       bt_bap_stream_set_io(stream, -1);
>>          return false;
>>   }
>>
>>
>> ---
>> base-commit: 0efa20cbf3fb5693c7c2f14ba8cf67053ca029e5
>> change-id: 20250407-bap_aes_state-9306798ff95a
>>
>> Best regards,
>> --
>> Yang Li <yang.li@amlogic.com>
>>
>>
>>
>
> --
> Luiz Augusto von Dentz
diff mbox series

Patch

diff --git a/src/shared/bap.c b/src/shared/bap.c
index 650bea2f4..c40d6e051 100644
--- a/src/shared/bap.c
+++ b/src/shared/bap.c
@@ -1123,17 +1123,12 @@  static void stream_notify_metadata(struct bt_bap_stream *stream)
 	free(status);
 }
 
-static void stream_notify_release(struct bt_bap_stream *stream)
+static void stream_notify_ase_state(struct bt_bap_stream *stream)
 {
 	struct bt_bap_endpoint *ep = stream->ep;
 	struct bt_ascs_ase_status status;
 
-	DBG(stream->bap, "stream %p", stream);
-
-
-	memset(&status, 0, sizeof(status));
 	status.id = ep->id;
-	ep->state = BT_BAP_STREAM_STATE_RELEASING;
 	status.state = ep->state;
 
 	gatt_db_attribute_notify(ep->attr, (void *)&status, sizeof(status),
@@ -1713,6 +1708,7 @@  static void stream_notify(struct bt_bap_stream *stream, uint8_t state)
 
 	switch (state) {
 	case BT_ASCS_ASE_STATE_IDLE:
+		stream_notify_ase_state(stream);
 		break;
 	case BT_ASCS_ASE_STATE_CONFIG:
 		stream_notify_config(stream);
@@ -1726,7 +1722,7 @@  static void stream_notify(struct bt_bap_stream *stream, uint8_t state)
 		stream_notify_metadata(stream);
 		break;
 	case BT_ASCS_ASE_STATE_RELEASING:
-		stream_notify_release(stream);
+		stream_notify_ase_state(stream);
 		break;
 	}
 }
@@ -6397,9 +6393,8 @@  static bool stream_io_disconnected(struct io *io, void *user_data)
 	DBG(stream->bap, "stream %p io disconnected", stream);
 
 	if (stream->ep->state == BT_ASCS_ASE_STATE_RELEASING)
-		stream_set_state(stream, BT_BAP_STREAM_STATE_CONFIG);
+		stream_set_state(stream, BT_BAP_STREAM_STATE_IDLE);
 
-	bt_bap_stream_set_io(stream, -1);
 	return false;
 }