| Message ID | 20250106-upstream-v1-1-a16879b78ffd@amlogic.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [BlueZ,bluez] bap: fixed issue of muting music silent after pause and resume. | expand |
| 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:296: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:296: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:296: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 |
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=922416 ---Test result--- Test Summary: CheckPatch PENDING 0.20 seconds GitLint PENDING 0.20 seconds BuildEll PASS 20.89 seconds BluezMake PASS 1558.72 seconds MakeCheck PASS 13.50 seconds MakeDistcheck PASS 159.43 seconds CheckValgrind PASS 216.34 seconds CheckSmatch WARNING 273.05 seconds bluezmakeextell PASS 101.18 seconds IncrementalBuild PENDING 0.22 seconds ScanBuild PASS 865.03 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:296: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:296: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:296: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
Hi, ma, 2025-01-06 kello 10:50 +0800, Yang Li via B4 Relay kirjoitti: > From: Yang Li <yang.li@amlogic.com> > > CIS sink need caching the Codec Configured when releasing by Pixel, > state machine is releasing -> Codec. If streamming -> idle, CIS sink > was silent after resume music. AFAIK, ASCS v1.0.1 §5.9 does not require that codec configuration be cached. > > Signed-off-by: Yang Li <yang.li@amlogic.com> > --- > src/shared/bap.c | 43 +++++++++++++++++++++++++++++++------------ > 1 file changed, 31 insertions(+), 12 deletions(-) > > diff --git a/src/shared/bap.c b/src/shared/bap.c > index 167501282..a7f5dec92 100644 > --- a/src/shared/bap.c > +++ b/src/shared/bap.c > @@ -1063,6 +1063,28 @@ static void stream_notify_metadata(struct bt_bap_stream *stream) > free(status); > } > > +static void stream_notify_release(struct bt_bap_stream *stream) > +{ > + struct bt_bap_endpoint *ep = stream->ep; > + struct bt_ascs_ase_status *status; > + size_t len; > + > + DBG(stream->bap, "stream %p", stream); > + > + len = sizeof(*status); > + status = malloc(len); > + > + memset(status, 0, len); > + status->id = ep->id; > + ep->state = BT_BAP_STREAM_STATE_RELEASING; > + status->state = ep->state; > + > + gatt_db_attribute_notify(ep->attr, (void *) status, len, > + bt_bap_get_att(stream->bap)); > + > + free(status); No need for malloc above. > +} > + > static struct bt_bap *bt_bap_ref_safe(struct bt_bap *bap) > { > if (!bap || !bap->ref_count || !queue_find(sessions, NULL, bap)) > @@ -1634,7 +1656,7 @@ static bool stream_notify_state(void *data) > struct bt_bap_stream *stream = data; > struct bt_bap_endpoint *ep = stream->ep; > > - DBG(stream->bap, "stream %p", stream); > + DBG(stream->bap, "stream %p status %d", stream, ep->state); > > if (stream->state_id) { > timeout_remove(stream->state_id); > @@ -1655,6 +1677,9 @@ static bool stream_notify_state(void *data) > case BT_ASCS_ASE_STATE_DISABLING: > stream_notify_metadata(stream); > break; > + case BT_ASCS_ASE_STATE_RELEASING: > + stream_notify_release(stream); > + break; > } > > return false; > @@ -1936,9 +1961,7 @@ static uint8_t stream_disable(struct bt_bap_stream *stream, struct iovec *rsp) > /* Sink can autonomously transit to QOS while source needs to go to > * Disabling until BT_ASCS_STOP is received. > */ > - if (stream->ep->dir == BT_BAP_SINK) > - stream_set_state(stream, BT_BAP_STREAM_STATE_QOS); > - else > + if (stream->ep->dir == BT_BAP_SOURCE) > stream_set_state(stream, BT_BAP_STREAM_STATE_DISABLING); This looks like it makes ASCS Disable for BAP_SINK to only send the accept response, but not make any state transition? But ASCS v1.0.1 §5.5 seems to require it: -------------------- If the server accepts the requested Disable operation parameter values for a Sink ASE, or if the server autonomously initiates the Disable operation for a Sink ASE, the server shall: • Transition the ASE to the QoS Configured state and write a value of 0x02 (QoS Configured) to the ASE_State field. • Write the previously accepted Config QoS operation parameter values to the matching Additional_ASE_Parameters fields defined in Table 4.4. -------------------- > > return 0; > @@ -2068,17 +2091,11 @@ static unsigned int bap_ucast_metadata(struct bt_bap_stream *stream, > > static uint8_t stream_release(struct bt_bap_stream *stream, struct iovec *rsp) > { > - struct bt_bap_pac *pac; > - > DBG(stream->bap, "stream %p", stream); > > ascs_ase_rsp_success(rsp, stream->ep->id); > > - pac = stream->lpac; > - if (pac->ops && pac->ops->clear) > - pac->ops->clear(stream, pac->user_data); > - > - stream_set_state(stream, BT_BAP_STREAM_STATE_IDLE); > + stream_set_state(stream, BT_BAP_STREAM_STATE_RELEASING); > > return 0; > } > @@ -6172,8 +6189,10 @@ static bool stream_io_disconnected(struct io *io, void *user_data) > > DBG(stream->bap, "stream %p io disconnected", stream); > > - bt_bap_stream_set_io(stream, -1); > + if (stream->ep->state == BT_ASCS_ASE_STATE_RELEASING) > + stream_set_state(stream, BT_BAP_STREAM_STATE_CONFIG); > > + bt_bap_stream_set_io(stream, -1); > return false; > } > > > --- > base-commit: dfb1ffdc95a00bc06d81a75c11ab5ad2e24d37bf > change-id: 20250106-upstream-1ec9ae96cda4 > > Best regards,
Hi Yang, On Sun, Jan 5, 2025 at 9:50 PM Yang Li via B4 Relay <devnull+yang.li.amlogic.com@kernel.org> wrote: > > From: Yang Li <yang.li@amlogic.com> > > CIS sink need caching the Codec Configured when releasing by Pixel, > state machine is releasing -> Codec. If streamming -> idle, CIS sink > was silent after resume music. You need to work on the commit message, perhaps quote the spec if there is a description of the state transition. > Signed-off-by: Yang Li <yang.li@amlogic.com> > --- > src/shared/bap.c | 43 +++++++++++++++++++++++++++++++------------ > 1 file changed, 31 insertions(+), 12 deletions(-) > > diff --git a/src/shared/bap.c b/src/shared/bap.c > index 167501282..a7f5dec92 100644 > --- a/src/shared/bap.c > +++ b/src/shared/bap.c > @@ -1063,6 +1063,28 @@ static void stream_notify_metadata(struct bt_bap_stream *stream) > free(status); > } > > +static void stream_notify_release(struct bt_bap_stream *stream) > +{ > + struct bt_bap_endpoint *ep = stream->ep; > + struct bt_ascs_ase_status *status; > + size_t len; > + > + DBG(stream->bap, "stream %p", stream); > + > + len = sizeof(*status); > + status = malloc(len); Just use a stack variable instead of using malloc. > + > + memset(status, 0, len); > + status->id = ep->id; > + ep->state = BT_BAP_STREAM_STATE_RELEASING; > + status->state = ep->state; > + > + gatt_db_attribute_notify(ep->attr, (void *) status, len, > + bt_bap_get_att(stream->bap)); > + > + free(status); > +} > + > static struct bt_bap *bt_bap_ref_safe(struct bt_bap *bap) > { > if (!bap || !bap->ref_count || !queue_find(sessions, NULL, bap)) > @@ -1634,7 +1656,7 @@ static bool stream_notify_state(void *data) > struct bt_bap_stream *stream = data; > struct bt_bap_endpoint *ep = stream->ep; > > - DBG(stream->bap, "stream %p", stream); > + DBG(stream->bap, "stream %p status %d", stream, ep->state); > > if (stream->state_id) { > timeout_remove(stream->state_id); > @@ -1655,6 +1677,9 @@ static bool stream_notify_state(void *data) > case BT_ASCS_ASE_STATE_DISABLING: > stream_notify_metadata(stream); > break; > + case BT_ASCS_ASE_STATE_RELEASING: > + stream_notify_release(stream); Ok, I see where this is going, but the spec doesn't actually mandate to send releasing or caching the codec configuration: https://www.bluetooth.com/wp-content/uploads/Files/Specification/HTML/23166-ASCS-html5/out/en/index-en.html#UUID-c37600a3-4541-1926-2f13-eb29057e41d5_N1661459513418 Perhaps you are saying that we need to send Releasing at least? There is also the thing that I don't understand is why would someone send release command and get rid of QoS/CIG while keeping the Codec Configuration? > + break; > } > > return false; > @@ -1936,9 +1961,7 @@ static uint8_t stream_disable(struct bt_bap_stream *stream, struct iovec *rsp) > /* Sink can autonomously transit to QOS while source needs to go to > * Disabling until BT_ASCS_STOP is received. > */ > - if (stream->ep->dir == BT_BAP_SINK) > - stream_set_state(stream, BT_BAP_STREAM_STATE_QOS); > - else > + if (stream->ep->dir == BT_BAP_SOURCE) Don't think this is correct, why are you taking away the setting to QoS like it is documented? > stream_set_state(stream, BT_BAP_STREAM_STATE_DISABLING); > > return 0; > @@ -2068,17 +2091,11 @@ static unsigned int bap_ucast_metadata(struct bt_bap_stream *stream, > > static uint8_t stream_release(struct bt_bap_stream *stream, struct iovec *rsp) > { > - struct bt_bap_pac *pac; > - > DBG(stream->bap, "stream %p", stream); > > ascs_ase_rsp_success(rsp, stream->ep->id); > > - pac = stream->lpac; > - if (pac->ops && pac->ops->clear) > - pac->ops->clear(stream, pac->user_data); Hmm, I think we do depend on clear to be called to tell the upper stack the transport is being released, or you did test this with the likes of pipewire and found that is not really required? > - stream_set_state(stream, BT_BAP_STREAM_STATE_IDLE); > + stream_set_state(stream, BT_BAP_STREAM_STATE_RELEASING); > > return 0; > } > @@ -6172,8 +6189,10 @@ static bool stream_io_disconnected(struct io *io, void *user_data) > > DBG(stream->bap, "stream %p io disconnected", stream); > > - bt_bap_stream_set_io(stream, -1); > + if (stream->ep->state == BT_ASCS_ASE_STATE_RELEASING) > + stream_set_state(stream, BT_BAP_STREAM_STATE_CONFIG); > > + bt_bap_stream_set_io(stream, -1); > return false; > } > > > --- > base-commit: dfb1ffdc95a00bc06d81a75c11ab5ad2e24d37bf > change-id: 20250106-upstream-1ec9ae96cda4 > > Best regards, > -- > Yang Li <yang.li@amlogic.com> > > >
Hi Luiz & Pauli, ASCS v1.0.1 §5.9 introduces two operation processes for the server to handle the released state: ---------------- If the server wants to cache a codec configuration: - Transition the ASE to the Codec Configured state and write a value of 0x01 (Codec Configured) to the ASE_State field - Write the configured values or the server’s preferred values for the Codec_ID, Codec_Specific_Configuration_Length, and Codec_Specific_Configuration parameters to the matching Additional_ASE_Parameters fields defined in Table 4.3 <https://www.bluetooth.com/wp-content/uploads/Files/Specification/HTML/23166-ASCS-html5/out/en/index-en.html#UUID-8e961f8e-98b7-23b8-53d7-59cc88762fc0_N1661460117035>. - Write the server’s preferred values for the remaining Additional_ASE_Parameters fields defined in Table 4.3 <https://www.bluetooth.com/wp-content/uploads/Files/Specification/HTML/23166-ASCS-html5/out/en/index-en.html#UUID-8e961f8e-98b7-23b8-53d7-59cc88762fc0_N1661460117035>. If the server does not want to cache a codec configuration: - Transition the ASE to the Idle state and write a value of 0x00 (Idle) to the ASE_State field. - Delete any Additional_ASE_Parameters fields present. ---------------- https://www.bluetooth.com/wp-content/uploads/Files/Specification/HTML/23166-ASCS-html5/out/en/index-en.html#UUID-9d5221b2-eadd-1bde-09d4-5b3bb9e6d7b8 Currently BlueZ only uses non-cached operation, It seems that the Android platform is not compatible with the non-cached Codec Configuration scenario, I raised an issue when testing playback and pause using a Pixel phone. https://github.com/bluez/bluez/issues/1053 So I tried modifying the code to use a cached Codec Configuration (referencing the process for Pixel + Redmi Buds 5Pro headphones). I believe there are two issues that need to be confirmed: - Does BlueZ need to support both operations and how to decide which one to use? (cached or non-cached); - Whether the Android platform will be compatible with non-cached in the future. > [ EXTERNAL EMAIL ] > > Hi Yang, > > On Sun, Jan 5, 2025 at 9:50 PM Yang Li via B4 Relay > <devnull+yang.li.amlogic.com@kernel.org> wrote: >> From: Yang Li <yang.li@amlogic.com> >> >> CIS sink need caching the Codec Configured when releasing by Pixel, >> state machine is releasing -> Codec. If streamming -> idle, CIS sink >> was silent after resume music. > You need to work on the commit message, perhaps quote the spec if > there is a description of the state transition. Well, I got it. > >> Signed-off-by: Yang Li <yang.li@amlogic.com> >> --- >> src/shared/bap.c | 43 +++++++++++++++++++++++++++++++------------ >> 1 file changed, 31 insertions(+), 12 deletions(-) >> >> diff --git a/src/shared/bap.c b/src/shared/bap.c >> index 167501282..a7f5dec92 100644 >> --- a/src/shared/bap.c >> +++ b/src/shared/bap.c >> @@ -1063,6 +1063,28 @@ static void stream_notify_metadata(struct bt_bap_stream *stream) >> free(status); >> } >> >> +static void stream_notify_release(struct bt_bap_stream *stream) >> +{ >> + struct bt_bap_endpoint *ep = stream->ep; >> + struct bt_ascs_ase_status *status; >> + size_t len; >> + >> + DBG(stream->bap, "stream %p", stream); >> + >> + len = sizeof(*status); >> + status = malloc(len); > Just use a stack variable instead of using malloc. I will do it. I just referred to other function flows, like stream_notify_metadata() > >> + >> + memset(status, 0, len); >> + status->id = ep->id; >> + ep->state = BT_BAP_STREAM_STATE_RELEASING; >> + status->state = ep->state; >> + >> + gatt_db_attribute_notify(ep->attr, (void *) status, len, >> + bt_bap_get_att(stream->bap)); >> + >> + free(status); >> +} >> + >> static struct bt_bap *bt_bap_ref_safe(struct bt_bap *bap) >> { >> if (!bap || !bap->ref_count || !queue_find(sessions, NULL, bap)) >> @@ -1634,7 +1656,7 @@ static bool stream_notify_state(void *data) >> struct bt_bap_stream *stream = data; >> struct bt_bap_endpoint *ep = stream->ep; >> >> - DBG(stream->bap, "stream %p", stream); >> + DBG(stream->bap, "stream %p status %d", stream, ep->state); >> >> if (stream->state_id) { >> timeout_remove(stream->state_id); >> @@ -1655,6 +1677,9 @@ static bool stream_notify_state(void *data) >> case BT_ASCS_ASE_STATE_DISABLING: >> stream_notify_metadata(stream); >> break; >> + case BT_ASCS_ASE_STATE_RELEASING: >> + stream_notify_release(stream); > Ok, I see where this is going, but the spec doesn't actually mandate > to send releasing or caching the codec configuration: > > https://www.bluetooth.com/wp-content/uploads/Files/Specification/HTML/23166-ASCS-html5/out/en/index-en.html#UUID-c37600a3-4541-1926-2f13-eb29057e41d5_N1661459513418 > > Perhaps you are saying that we need to send Releasing at least? There > is also the thing that I don't understand is why would someone send > release command and get rid of QoS/CIG while keeping the Codec > Configuration? I think the client requires the server to notify the current state as released. > >> + break; >> } >> >> return false; >> @@ -1936,9 +1961,7 @@ static uint8_t stream_disable(struct bt_bap_stream *stream, struct iovec *rsp) >> /* Sink can autonomously transit to QOS while source needs to go to >> * Disabling until BT_ASCS_STOP is received. >> */ >> - if (stream->ep->dir == BT_BAP_SINK) >> - stream_set_state(stream, BT_BAP_STREAM_STATE_QOS); >> - else >> + if (stream->ep->dir == BT_BAP_SOURCE) > Don't think this is correct, why are you taking away the setting to > QoS like it is documented? This is also what I am confused about. Why do we need to set state=QoS in stream_disable()? From the ASE state machine, the ASE state should stay in Codec Configured after Released, and switch to QoS state when Client configures QoS. https://www.bluetooth.com/wp-content/uploads/Files/Specification/HTML/23166-ASCS-html5/out/en/index-en.html#UUID-363eeef6-abc5-6f54-cfa6-09fe4451fd15 >> stream_set_state(stream, BT_BAP_STREAM_STATE_DISABLING); >> >> return 0; >> @@ -2068,17 +2091,11 @@ static unsigned int bap_ucast_metadata(struct bt_bap_stream *stream, >> >> static uint8_t stream_release(struct bt_bap_stream *stream, struct iovec *rsp) >> { >> - struct bt_bap_pac *pac; >> - >> DBG(stream->bap, "stream %p", stream); >> >> ascs_ase_rsp_success(rsp, stream->ep->id); >> >> - pac = stream->lpac; >> - if (pac->ops && pac->ops->clear) >> - pac->ops->clear(stream, pac->user_data); > Hmm, I think we do depend on clear to be called to tell the upper > stack the transport is being released, or you did test this with the > likes of pipewire and found that is not really required? If clear is executed, CIS will be disconnected automatically. I compared it with the pixel+Buds5 headphones, and it was the mobile phone that disconnected the CIS, so in the cached Codec Configuration operation process, there is no need to disconnect the CIS. > >> - stream_set_state(stream, BT_BAP_STREAM_STATE_IDLE); >> + stream_set_state(stream, BT_BAP_STREAM_STATE_RELEASING); >> >> return 0; >> } >> @@ -6172,8 +6189,10 @@ static bool stream_io_disconnected(struct io *io, void *user_data) >> >> DBG(stream->bap, "stream %p io disconnected", stream); >> >> - bt_bap_stream_set_io(stream, -1); >> + if (stream->ep->state == BT_ASCS_ASE_STATE_RELEASING) >> + stream_set_state(stream, BT_BAP_STREAM_STATE_CONFIG); >> >> + bt_bap_stream_set_io(stream, -1); >> return false; >> } >> >> >> --- >> base-commit: dfb1ffdc95a00bc06d81a75c11ab5ad2e24d37bf >> change-id: 20250106-upstream-1ec9ae96cda4 >> >> Best regards, >> -- >> Yang Li <yang.li@amlogic.com> >> >> >> > > -- > Luiz Augusto von Dentz
Hi Yang, On Tue, Jan 7, 2025 at 8:50 PM Yang Li <Yang.Li@amlogic.com> wrote: > > Hi Luiz & Pauli, > > ASCS v1.0.1 §5.9 introduces two operation processes for the server to > handle the released state: > ---------------- > If the server wants to cache a codec configuration: > - Transition the ASE to the Codec Configured state and write a value of > 0x01 (Codec Configured) to the ASE_State field > - Write the configured values or the server’s preferred values for the > Codec_ID, Codec_Specific_Configuration_Length, and > Codec_Specific_Configuration parameters to the matching > Additional_ASE_Parameters fields defined in Table 4.3 > <https://www.bluetooth.com/wp-content/uploads/Files/Specification/HTML/23166-ASCS-html5/out/en/index-en.html#UUID-8e961f8e-98b7-23b8-53d7-59cc88762fc0_N1661460117035>. > - Write the server’s preferred values for the remaining > Additional_ASE_Parameters fields defined in Table 4.3 > <https://www.bluetooth.com/wp-content/uploads/Files/Specification/HTML/23166-ASCS-html5/out/en/index-en.html#UUID-8e961f8e-98b7-23b8-53d7-59cc88762fc0_N1661460117035>. > > If the server does not want to cache a codec configuration: > - Transition the ASE to the Idle state and write a value of 0x00 (Idle) > to the ASE_State field. > - Delete any Additional_ASE_Parameters fields present. > ---------------- > https://www.bluetooth.com/wp-content/uploads/Files/Specification/HTML/23166-ASCS-html5/out/en/index-en.html#UUID-9d5221b2-eadd-1bde-09d4-5b3bb9e6d7b8 > Currently BlueZ only uses non-cached operation, It seems that the > Android platform is not compatible with the non-cached Codec > Configuration scenario, > I raised an issue when testing playback and pause using a Pixel phone. > https://github.com/bluez/bluez/issues/1053 > So I tried modifying the code to use a cached Codec Configuration > (referencing the process for Pixel + Redmi Buds 5Pro headphones). > I believe there are two issues that need to be confirmed: > - Does BlueZ need to support both operations and how to decide which one > to use? (cached or non-cached); > - Whether the Android platform will be compatible with non-cached in the > future. > > > [ EXTERNAL EMAIL ] > > > > Hi Yang, > > > > On Sun, Jan 5, 2025 at 9:50 PM Yang Li via B4 Relay > > <devnull+yang.li.amlogic.com@kernel.org> wrote: > >> From: Yang Li <yang.li@amlogic.com> > >> > >> CIS sink need caching the Codec Configured when releasing by Pixel, > >> state machine is releasing -> Codec. If streamming -> idle, CIS sink > >> was silent after resume music. > > You need to work on the commit message, perhaps quote the spec if > > there is a description of the state transition. > Well, I got it. > > > >> Signed-off-by: Yang Li <yang.li@amlogic.com> > >> --- > >> src/shared/bap.c | 43 +++++++++++++++++++++++++++++++------------ > >> 1 file changed, 31 insertions(+), 12 deletions(-) > >> > >> diff --git a/src/shared/bap.c b/src/shared/bap.c > >> index 167501282..a7f5dec92 100644 > >> --- a/src/shared/bap.c > >> +++ b/src/shared/bap.c > >> @@ -1063,6 +1063,28 @@ static void stream_notify_metadata(struct bt_bap_stream *stream) > >> free(status); > >> } > >> > >> +static void stream_notify_release(struct bt_bap_stream *stream) > >> +{ > >> + struct bt_bap_endpoint *ep = stream->ep; > >> + struct bt_ascs_ase_status *status; > >> + size_t len; > >> + > >> + DBG(stream->bap, "stream %p", stream); > >> + > >> + len = sizeof(*status); > >> + status = malloc(len); > > Just use a stack variable instead of using malloc. > > I will do it. > > I just referred to other function flows, like stream_notify_metadata() > > > > >> + > >> + memset(status, 0, len); > >> + status->id = ep->id; > >> + ep->state = BT_BAP_STREAM_STATE_RELEASING; > >> + status->state = ep->state; > >> + > >> + gatt_db_attribute_notify(ep->attr, (void *) status, len, > >> + bt_bap_get_att(stream->bap)); > >> + > >> + free(status); > >> +} > >> + > >> static struct bt_bap *bt_bap_ref_safe(struct bt_bap *bap) > >> { > >> if (!bap || !bap->ref_count || !queue_find(sessions, NULL, bap)) > >> @@ -1634,7 +1656,7 @@ static bool stream_notify_state(void *data) > >> struct bt_bap_stream *stream = data; > >> struct bt_bap_endpoint *ep = stream->ep; > >> > >> - DBG(stream->bap, "stream %p", stream); > >> + DBG(stream->bap, "stream %p status %d", stream, ep->state); > >> > >> if (stream->state_id) { > >> timeout_remove(stream->state_id); > >> @@ -1655,6 +1677,9 @@ static bool stream_notify_state(void *data) > >> case BT_ASCS_ASE_STATE_DISABLING: > >> stream_notify_metadata(stream); > >> break; > >> + case BT_ASCS_ASE_STATE_RELEASING: > >> + stream_notify_release(stream); > > Ok, I see where this is going, but the spec doesn't actually mandate > > to send releasing or caching the codec configuration: > > > > https://www.bluetooth.com/wp-content/uploads/Files/Specification/HTML/23166-ASCS-html5/out/en/index-en.html#UUID-c37600a3-4541-1926-2f13-eb29057e41d5_N1661459513418 > > > > Perhaps you are saying that we need to send Releasing at least? There > > is also the thing that I don't understand is why would someone send > > release command and get rid of QoS/CIG while keeping the Codec > > Configuration? > I think the client requires the server to notify the current state as > released. There is no released state, only idle, that said the spec clearly state that we need to transition to Releasing (0x06) before moving to Idle (0x00), which is something we are not currently doing, anyway we need to properly document this and perhaps we need to add a timer in case the remote doesn't disconnect the ISO link we should probably cleanup ourselves other we would be stuck in releasing state forever. > > > >> + break; > >> } > >> > >> return false; > >> @@ -1936,9 +1961,7 @@ static uint8_t stream_disable(struct bt_bap_stream *stream, struct iovec *rsp) > >> /* Sink can autonomously transit to QOS while source needs to go to > >> * Disabling until BT_ASCS_STOP is received. > >> */ > >> - if (stream->ep->dir == BT_BAP_SINK) > >> - stream_set_state(stream, BT_BAP_STREAM_STATE_QOS); > >> - else > >> + if (stream->ep->dir == BT_BAP_SOURCE) > > Don't think this is correct, why are you taking away the setting to > > QoS like it is documented? > > This is also what I am confused about. Why do we need to set state=QoS > in stream_disable()? Disable is different from the Release command, it is used to 'pause' the stream but keep the QoS settings, but the server still needs to transition back to QoS, and no I'm not guessing here the state machine does really require these state transitions. > From the ASE state machine, the ASE state should stay in Codec > Configured after Released, and switch to QoS state when Client > configures QoS. > > https://www.bluetooth.com/wp-content/uploads/Files/Specification/HTML/23166-ASCS-html5/out/en/index-en.html#UUID-363eeef6-abc5-6f54-cfa6-09fe4451fd15 Sounds like you are confusing Disable with Release again here, over each line connecting the states there is the operations, transition from Releasing to Codec Configuration is clearly _not_ mandatory, in fact I think for embedded devices they most like want to save memory when a stream is released, so Id only really recommend caching the Codec Configuration in case the connection is lost or something like that, not when the remote peer intentionally release. > > >> stream_set_state(stream, BT_BAP_STREAM_STATE_DISABLING); > >> > >> return 0; > >> @@ -2068,17 +2091,11 @@ static unsigned int bap_ucast_metadata(struct bt_bap_stream *stream, > >> > >> static uint8_t stream_release(struct bt_bap_stream *stream, struct iovec *rsp) > >> { > >> - struct bt_bap_pac *pac; > >> - > >> DBG(stream->bap, "stream %p", stream); > >> > >> ascs_ase_rsp_success(rsp, stream->ep->id); > >> > >> - pac = stream->lpac; > >> - if (pac->ops && pac->ops->clear) > >> - pac->ops->clear(stream, pac->user_data); > > Hmm, I think we do depend on clear to be called to tell the upper > > stack the transport is being released, or you did test this with the > > likes of pipewire and found that is not really required? > If clear is executed, CIS will be disconnected automatically. I compared > it with the pixel+Buds5 headphones, and it was the mobile phone that > disconnected the CIS, so in the cached Codec Configuration operation > process, there is no need to disconnect the CIS. Fair enough, that said I don't think this behavior is invalid either, and like I pointed above we should probably trigger a timer if the remote doesn't cleanup the CIS on releasing. > > > >> - stream_set_state(stream, BT_BAP_STREAM_STATE_IDLE); > >> + stream_set_state(stream, BT_BAP_STREAM_STATE_RELEASING); > >> > >> return 0; > >> } > >> @@ -6172,8 +6189,10 @@ static bool stream_io_disconnected(struct io *io, void *user_data) > >> > >> DBG(stream->bap, "stream %p io disconnected", stream); > >> > >> - bt_bap_stream_set_io(stream, -1); > >> + if (stream->ep->state == BT_ASCS_ASE_STATE_RELEASING) > >> + stream_set_state(stream, BT_BAP_STREAM_STATE_CONFIG); > >> > >> + bt_bap_stream_set_io(stream, -1); > >> return false; > >> } > >> > >> > >> --- > >> base-commit: dfb1ffdc95a00bc06d81a75c11ab5ad2e24d37bf > >> change-id: 20250106-upstream-1ec9ae96cda4 > >> > >> Best regards, > >> -- > >> Yang Li <yang.li@amlogic.com> > >> > >> > >> > > > > -- > > Luiz Augusto von Dentz > >
Hi Luiz， > [ EXTERNAL EMAIL ] > > Hi Yang, > > On Tue, Jan 7, 2025 at 8:50 PM Yang Li <Yang.Li@amlogic.com> wrote: >> Hi Luiz & Pauli, >> >> ASCS v1.0.1 §5.9 introduces two operation processes for the server to >> handle the released state: >> ---------------- >> If the server wants to cache a codec configuration: >> - Transition the ASE to the Codec Configured state and write a value of >> 0x01 (Codec Configured) to the ASE_State field >> - Write the configured values or the server’s preferred values for the >> Codec_ID, Codec_Specific_Configuration_Length, and >> Codec_Specific_Configuration parameters to the matching >> Additional_ASE_Parameters fields defined in Table 4.3 >> <https://www.bluetooth.com/wp-content/uploads/Files/Specification/HTML/23166-ASCS-html5/out/en/index-en.html#UUID-8e961f8e-98b7-23b8-53d7-59cc88762fc0_N1661460117035>. >> - Write the server’s preferred values for the remaining >> Additional_ASE_Parameters fields defined in Table 4.3 >> <https://www.bluetooth.com/wp-content/uploads/Files/Specification/HTML/23166-ASCS-html5/out/en/index-en.html#UUID-8e961f8e-98b7-23b8-53d7-59cc88762fc0_N1661460117035>. >> >> If the server does not want to cache a codec configuration: >> - Transition the ASE to the Idle state and write a value of 0x00 (Idle) >> to the ASE_State field. >> - Delete any Additional_ASE_Parameters fields present. >> ---------------- >> https://www.bluetooth.com/wp-content/uploads/Files/Specification/HTML/23166-ASCS-html5/out/en/index-en.html#UUID-9d5221b2-eadd-1bde-09d4-5b3bb9e6d7b8 >> Currently BlueZ only uses non-cached operation, It seems that the >> Android platform is not compatible with the non-cached Codec >> Configuration scenario, >> I raised an issue when testing playback and pause using a Pixel phone. >> https://github.com/bluez/bluez/issues/1053 >> So I tried modifying the code to use a cached Codec Configuration >> (referencing the process for Pixel + Redmi Buds 5Pro headphones). >> I believe there are two issues that need to be confirmed: >> - Does BlueZ need to support both operations and how to decide which one >> to use? (cached or non-cached); >> - Whether the Android platform will be compatible with non-cached in the >> future. >> >>> [ EXTERNAL EMAIL ] >>> >>> Hi Yang, >>> >>> On Sun, Jan 5, 2025 at 9:50 PM Yang Li via B4 Relay >>> <devnull+yang.li.amlogic.com@kernel.org> wrote: >>>> From: Yang Li <yang.li@amlogic.com> >>>> >>>> CIS sink need caching the Codec Configured when releasing by Pixel, >>>> state machine is releasing -> Codec. If streamming -> idle, CIS sink >>>> was silent after resume music. >>> You need to work on the commit message, perhaps quote the spec if >>> there is a description of the state transition. >> Well, I got it. >>>> Signed-off-by: Yang Li <yang.li@amlogic.com> >>>> --- >>>> src/shared/bap.c | 43 +++++++++++++++++++++++++++++++------------ >>>> 1 file changed, 31 insertions(+), 12 deletions(-) >>>> >>>> diff --git a/src/shared/bap.c b/src/shared/bap.c >>>> index 167501282..a7f5dec92 100644 >>>> --- a/src/shared/bap.c >>>> +++ b/src/shared/bap.c >>>> @@ -1063,6 +1063,28 @@ static void stream_notify_metadata(struct bt_bap_stream *stream) >>>> free(status); >>>> } >>>> >>>> +static void stream_notify_release(struct bt_bap_stream *stream) >>>> +{ >>>> + struct bt_bap_endpoint *ep = stream->ep; >>>> + struct bt_ascs_ase_status *status; >>>> + size_t len; >>>> + >>>> + DBG(stream->bap, "stream %p", stream); >>>> + >>>> + len = sizeof(*status); >>>> + status = malloc(len); >>> Just use a stack variable instead of using malloc. >> I will do it. >> >> I just referred to other function flows, like stream_notify_metadata() >> >>>> + >>>> + memset(status, 0, len); >>>> + status->id = ep->id; >>>> + ep->state = BT_BAP_STREAM_STATE_RELEASING; >>>> + status->state = ep->state; >>>> + >>>> + gatt_db_attribute_notify(ep->attr, (void *) status, len, >>>> + bt_bap_get_att(stream->bap)); >>>> + >>>> + free(status); >>>> +} >>>> + >>>> static struct bt_bap *bt_bap_ref_safe(struct bt_bap *bap) >>>> { >>>> if (!bap || !bap->ref_count || !queue_find(sessions, NULL, bap)) >>>> @@ -1634,7 +1656,7 @@ static bool stream_notify_state(void *data) >>>> struct bt_bap_stream *stream = data; >>>> struct bt_bap_endpoint *ep = stream->ep; >>>> >>>> - DBG(stream->bap, "stream %p", stream); >>>> + DBG(stream->bap, "stream %p status %d", stream, ep->state); >>>> >>>> if (stream->state_id) { >>>> timeout_remove(stream->state_id); >>>> @@ -1655,6 +1677,9 @@ static bool stream_notify_state(void *data) >>>> case BT_ASCS_ASE_STATE_DISABLING: >>>> stream_notify_metadata(stream); >>>> break; >>>> + case BT_ASCS_ASE_STATE_RELEASING: >>>> + stream_notify_release(stream); >>> Ok, I see where this is going, but the spec doesn't actually mandate >>> to send releasing or caching the codec configuration: >>> >>> https://www.bluetooth.com/wp-content/uploads/Files/Specification/HTML/23166-ASCS-html5/out/en/index-en.html#UUID-c37600a3-4541-1926-2f13-eb29057e41d5_N1661459513418 >>> >>> Perhaps you are saying that we need to send Releasing at least? There >>> is also the thing that I don't understand is why would someone send >>> release command and get rid of QoS/CIG while keeping the Codec >>> Configuration? >> I think the client requires the server to notify the current state as >> released. > There is no released state, only idle, that said the spec clearly > state that we need to transition to Releasing (0x06) before moving to > Idle (0x00), which is something we are not currently doing, anyway we > need to properly document this and perhaps we need to add a timer in > case the remote doesn't disconnect the ISO link we should probably > cleanup ourselves other we would be stuck in releasing state forever. Well，I understand. >>>> + break; >>>> } >>>> >>>> return false; >>>> @@ -1936,9 +1961,7 @@ static uint8_t stream_disable(struct bt_bap_stream *stream, struct iovec *rsp) >>>> /* Sink can autonomously transit to QOS while source needs to go to >>>> * Disabling until BT_ASCS_STOP is received. >>>> */ >>>> - if (stream->ep->dir == BT_BAP_SINK) >>>> - stream_set_state(stream, BT_BAP_STREAM_STATE_QOS); >>>> - else >>>> + if (stream->ep->dir == BT_BAP_SOURCE) >>> Don't think this is correct, why are you taking away the setting to >>> QoS like it is documented? >> This is also what I am confused about. Why do we need to set state=QoS >> in stream_disable()? > Disable is different from the Release command, it is used to 'pause' > the stream but keep the QoS settings, but the server still needs to > transition back to QoS, and no I'm not guessing here the state machine > does really require these state transitions. I got it. > >> From the ASE state machine, the ASE state should stay in Codec >> Configured after Released, and switch to QoS state when Client >> configures QoS. >> >> https://www.bluetooth.com/wp-content/uploads/Files/Specification/HTML/23166-ASCS-html5/out/en/index-en.html#UUID-363eeef6-abc5-6f54-cfa6-09fe4451fd15 > Sounds like you are confusing Disable with Release again here, over > each line connecting the states there is the operations, transition > from Releasing to Codec Configuration is clearly _not_ mandatory, in > fact I think for embedded devices they most like want to save memory > when a stream is released, so Id only really recommend caching the > Codec Configuration in case the connection is lost or something like > that, not when the remote peer intentionally release. Yes, I agree. I retested the original code and analyzed logcat, and found that its MediaController had an exception. I have submitted the issue on the issuetracker. It seems that Android needs to solve the idle problem head-on. https://issuetracker.google.com/issues/388956587 >>>> stream_set_state(stream, BT_BAP_STREAM_STATE_DISABLING); >>>> >>>> return 0; >>>> @@ -2068,17 +2091,11 @@ static unsigned int bap_ucast_metadata(struct bt_bap_stream *stream, >>>> >>>> static uint8_t stream_release(struct bt_bap_stream *stream, struct iovec *rsp) >>>> { >>>> - struct bt_bap_pac *pac; >>>> - >>>> DBG(stream->bap, "stream %p", stream); >>>> >>>> ascs_ase_rsp_success(rsp, stream->ep->id); >>>> >>>> - pac = stream->lpac; >>>> - if (pac->ops && pac->ops->clear) >>>> - pac->ops->clear(stream, pac->user_data); >>> Hmm, I think we do depend on clear to be called to tell the upper >>> stack the transport is being released, or you did test this with the >>> likes of pipewire and found that is not really required? >> If clear is executed, CIS will be disconnected automatically. I compared >> it with the pixel+Buds5 headphones, and it was the mobile phone that >> disconnected the CIS, so in the cached Codec Configuration operation >> process, there is no need to disconnect the CIS. > Fair enough, that said I don't think this behavior is invalid either, > and like I pointed above we should probably trigger a timer if the > remote doesn't cleanup the CIS on releasing. > >>>> - stream_set_state(stream, BT_BAP_STREAM_STATE_IDLE); >>>> + stream_set_state(stream, BT_BAP_STREAM_STATE_RELEASING); >>>> >>>> return 0; >>>> } >>>> @@ -6172,8 +6189,10 @@ static bool stream_io_disconnected(struct io *io, void *user_data) >>>> >>>> DBG(stream->bap, "stream %p io disconnected", stream); >>>> >>>> - bt_bap_stream_set_io(stream, -1); >>>> + if (stream->ep->state == BT_ASCS_ASE_STATE_RELEASING) >>>> + stream_set_state(stream, BT_BAP_STREAM_STATE_CONFIG); >>>> >>>> + bt_bap_stream_set_io(stream, -1); >>>> return false; >>>> } >>>> >>>> >>>> --- >>>> base-commit: dfb1ffdc95a00bc06d81a75c11ab5ad2e24d37bf >>>> change-id: 20250106-upstream-1ec9ae96cda4 >>>> >>>> Best regards, >>>> -- >>>> Yang Li <yang.li@amlogic.com> >>>> >>>> >>>> >>> -- >>> Luiz Augusto von Dentz >> > > -- > Luiz Augusto von Dentz
diff --git a/src/shared/bap.c b/src/shared/bap.c index 167501282..a7f5dec92 100644 --- a/src/shared/bap.c +++ b/src/shared/bap.c @@ -1063,6 +1063,28 @@ static void stream_notify_metadata(struct bt_bap_stream *stream) free(status); } +static void stream_notify_release(struct bt_bap_stream *stream) +{ + struct bt_bap_endpoint *ep = stream->ep; + struct bt_ascs_ase_status *status; + size_t len; + + DBG(stream->bap, "stream %p", stream); + + len = sizeof(*status); + status = malloc(len); + + memset(status, 0, len); + status->id = ep->id; + ep->state = BT_BAP_STREAM_STATE_RELEASING; + status->state = ep->state; + + gatt_db_attribute_notify(ep->attr, (void *) status, len, + bt_bap_get_att(stream->bap)); + + free(status); +} + static struct bt_bap *bt_bap_ref_safe(struct bt_bap *bap) { if (!bap || !bap->ref_count || !queue_find(sessions, NULL, bap)) @@ -1634,7 +1656,7 @@ static bool stream_notify_state(void *data) struct bt_bap_stream *stream = data; struct bt_bap_endpoint *ep = stream->ep; - DBG(stream->bap, "stream %p", stream); + DBG(stream->bap, "stream %p status %d", stream, ep->state); if (stream->state_id) { timeout_remove(stream->state_id); @@ -1655,6 +1677,9 @@ static bool stream_notify_state(void *data) case BT_ASCS_ASE_STATE_DISABLING: stream_notify_metadata(stream); break; + case BT_ASCS_ASE_STATE_RELEASING: + stream_notify_release(stream); + break; } return false; @@ -1936,9 +1961,7 @@ static uint8_t stream_disable(struct bt_bap_stream *stream, struct iovec *rsp) /* Sink can autonomously transit to QOS while source needs to go to * Disabling until BT_ASCS_STOP is received. */ - if (stream->ep->dir == BT_BAP_SINK) - stream_set_state(stream, BT_BAP_STREAM_STATE_QOS); - else + if (stream->ep->dir == BT_BAP_SOURCE) stream_set_state(stream, BT_BAP_STREAM_STATE_DISABLING); return 0; @@ -2068,17 +2091,11 @@ static unsigned int bap_ucast_metadata(struct bt_bap_stream *stream, static uint8_t stream_release(struct bt_bap_stream *stream, struct iovec *rsp) { - struct bt_bap_pac *pac; - DBG(stream->bap, "stream %p", stream); ascs_ase_rsp_success(rsp, stream->ep->id); - pac = stream->lpac; - if (pac->ops && pac->ops->clear) - pac->ops->clear(stream, pac->user_data); - - stream_set_state(stream, BT_BAP_STREAM_STATE_IDLE); + stream_set_state(stream, BT_BAP_STREAM_STATE_RELEASING); return 0; } @@ -6172,8 +6189,10 @@ static bool stream_io_disconnected(struct io *io, void *user_data) DBG(stream->bap, "stream %p io disconnected", stream); - bt_bap_stream_set_io(stream, -1); + if (stream->ep->state == BT_ASCS_ASE_STATE_RELEASING) + stream_set_state(stream, BT_BAP_STREAM_STATE_CONFIG); + bt_bap_stream_set_io(stream, -1); return false; }