* [bluez PATCH v2] avrcp: include all player settings in notif event
@ 2020-07-08 4:19 Howard Chung
2020-08-04 8:26 ` Archie Pusaka
0 siblings, 1 reply; 7+ messages in thread
From: Howard Chung @ 2020-07-08 4:19 UTC (permalink / raw)
To: linux-bluetooth; +Cc: luiz.dentz, Howard Chung, Archie Pusaka
According to AVRCP 1.6.2 spec section 6.7.2 table 6.39, all player
application settings should be returned to the CT and let CT to
determine which settings have changed. Currently bluez only returns
the changed attribute instead. This patch also addresses a potential
issue on which the number of application settings mismatches with
the actual number returned.
Reviewed-by: Archie Pusaka <apusaka@chromium.org>
---
Changes in v2:
- Fixed unused variables
profiles/audio/avrcp.c | 71 +++++++++++++++++++-----------------------
1 file changed, 32 insertions(+), 39 deletions(-)
diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
index e2428250e..a4de7530e 100644
--- a/profiles/audio/avrcp.c
+++ b/profiles/audio/avrcp.c
@@ -369,6 +369,7 @@ static uint32_t company_ids[] = {
};
static void avrcp_register_notification(struct avrcp *session, uint8_t event);
+static GList *player_list_settings(struct avrcp_player *player);
static sdp_record_t *avrcp_ct_record(void)
{
@@ -743,6 +744,35 @@ static int play_status_to_val(const char *status)
return -EINVAL;
}
+static uint16_t player_settings_changed(struct avrcp_player *player,
+ struct avrcp_header *pdu)
+{
+ GList *settings = player_list_settings(player);
+ int size = 2;
+
+ for (; settings; settings = settings->next) {
+ const char *key = settings->data;
+ int attr;
+ int val;
+
+ attr = attr_to_val(key);
+ if (attr < 0)
+ continue;
+
+ val = player_get_setting(player, attr);
+ if (val < 0)
+ continue;
+
+ pdu->params[size++] = attr;
+ pdu->params[size++] = val;
+ }
+
+ g_list_free(settings);
+
+ pdu->params[1] = (size - 2) >> 1;
+ return size;
+}
+
void avrcp_player_event(struct avrcp_player *player, uint8_t id,
const void *data)
{
@@ -751,8 +781,6 @@ void avrcp_player_event(struct avrcp_player *player, uint8_t id,
uint8_t code;
uint16_t size;
GSList *l;
- int attr;
- int val;
if (player->sessions == NULL)
return;
@@ -791,19 +819,7 @@ void avrcp_player_event(struct avrcp_player *player, uint8_t id,
size = 1;
break;
case AVRCP_EVENT_SETTINGS_CHANGED:
- size = 2;
- pdu->params[1] = 1;
-
- attr = attr_to_val(data);
- if (attr < 0)
- return;
-
- val = player_get_setting(player, attr);
- if (val < 0)
- return;
-
- pdu->params[size++] = attr;
- pdu->params[size++] = val;
+ size = player_settings_changed(player, pdu);
break;
case AVRCP_EVENT_ADDRESSED_PLAYER_CHANGED:
size = 5;
@@ -1595,7 +1611,6 @@ static uint8_t avrcp_handle_register_notification(struct avrcp *session,
struct btd_device *dev = session->dev;
uint16_t len = ntohs(pdu->params_len);
uint64_t uid;
- GList *settings;
/*
* 1 byte for EventID, 4 bytes for Playback interval but the latest
@@ -1626,29 +1641,7 @@ static uint8_t avrcp_handle_register_notification(struct avrcp *session,
len = 1;
break;
case AVRCP_EVENT_SETTINGS_CHANGED:
- len = 1;
- settings = player_list_settings(player);
-
- pdu->params[len++] = g_list_length(settings);
- for (; settings; settings = settings->next) {
- const char *key = settings->data;
- int attr;
- int val;
-
- attr = attr_to_val(key);
- if (attr < 0)
- continue;
-
- val = player_get_setting(player, attr);
- if (val < 0)
- continue;
-
- pdu->params[len++] = attr;
- pdu->params[len++] = val;
- }
-
- g_list_free(settings);
-
+ len = player_settings_changed(player, pdu);
break;
case AVRCP_EVENT_ADDRESSED_PLAYER_CHANGED:
len = 5;
--
2.27.0.383.g050319c2ae-goog
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [bluez PATCH v2] avrcp: include all player settings in notif event
2020-07-08 4:19 [bluez PATCH v2] avrcp: include all player settings in notif event Howard Chung
@ 2020-08-04 8:26 ` Archie Pusaka
2020-08-04 17:07 ` Luiz Augusto von Dentz
0 siblings, 1 reply; 7+ messages in thread
From: Archie Pusaka @ 2020-08-04 8:26 UTC (permalink / raw)
To: Howard Chung; +Cc: BlueZ, Luiz Augusto von Dentz, Archie Pusaka
Hi Bluez maintainers,
Could you take a look at this fix?
Thank you!
Regards,
Archie
On Wed, 8 Jul 2020 at 12:19, Howard Chung <howardchung@google.com> wrote:
>
> According to AVRCP 1.6.2 spec section 6.7.2 table 6.39, all player
> application settings should be returned to the CT and let CT to
> determine which settings have changed. Currently bluez only returns
> the changed attribute instead. This patch also addresses a potential
> issue on which the number of application settings mismatches with
> the actual number returned.
>
> Reviewed-by: Archie Pusaka <apusaka@chromium.org>
> ---
>
> Changes in v2:
> - Fixed unused variables
>
> profiles/audio/avrcp.c | 71 +++++++++++++++++++-----------------------
> 1 file changed, 32 insertions(+), 39 deletions(-)
>
> diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
> index e2428250e..a4de7530e 100644
> --- a/profiles/audio/avrcp.c
> +++ b/profiles/audio/avrcp.c
> @@ -369,6 +369,7 @@ static uint32_t company_ids[] = {
> };
>
> static void avrcp_register_notification(struct avrcp *session, uint8_t event);
> +static GList *player_list_settings(struct avrcp_player *player);
>
> static sdp_record_t *avrcp_ct_record(void)
> {
> @@ -743,6 +744,35 @@ static int play_status_to_val(const char *status)
> return -EINVAL;
> }
>
> +static uint16_t player_settings_changed(struct avrcp_player *player,
> + struct avrcp_header *pdu)
> +{
> + GList *settings = player_list_settings(player);
> + int size = 2;
> +
> + for (; settings; settings = settings->next) {
> + const char *key = settings->data;
> + int attr;
> + int val;
> +
> + attr = attr_to_val(key);
> + if (attr < 0)
> + continue;
> +
> + val = player_get_setting(player, attr);
> + if (val < 0)
> + continue;
> +
> + pdu->params[size++] = attr;
> + pdu->params[size++] = val;
> + }
> +
> + g_list_free(settings);
> +
> + pdu->params[1] = (size - 2) >> 1;
> + return size;
> +}
> +
> void avrcp_player_event(struct avrcp_player *player, uint8_t id,
> const void *data)
> {
> @@ -751,8 +781,6 @@ void avrcp_player_event(struct avrcp_player *player, uint8_t id,
> uint8_t code;
> uint16_t size;
> GSList *l;
> - int attr;
> - int val;
>
> if (player->sessions == NULL)
> return;
> @@ -791,19 +819,7 @@ void avrcp_player_event(struct avrcp_player *player, uint8_t id,
> size = 1;
> break;
> case AVRCP_EVENT_SETTINGS_CHANGED:
> - size = 2;
> - pdu->params[1] = 1;
> -
> - attr = attr_to_val(data);
> - if (attr < 0)
> - return;
> -
> - val = player_get_setting(player, attr);
> - if (val < 0)
> - return;
> -
> - pdu->params[size++] = attr;
> - pdu->params[size++] = val;
> + size = player_settings_changed(player, pdu);
> break;
> case AVRCP_EVENT_ADDRESSED_PLAYER_CHANGED:
> size = 5;
> @@ -1595,7 +1611,6 @@ static uint8_t avrcp_handle_register_notification(struct avrcp *session,
> struct btd_device *dev = session->dev;
> uint16_t len = ntohs(pdu->params_len);
> uint64_t uid;
> - GList *settings;
>
> /*
> * 1 byte for EventID, 4 bytes for Playback interval but the latest
> @@ -1626,29 +1641,7 @@ static uint8_t avrcp_handle_register_notification(struct avrcp *session,
> len = 1;
> break;
> case AVRCP_EVENT_SETTINGS_CHANGED:
> - len = 1;
> - settings = player_list_settings(player);
> -
> - pdu->params[len++] = g_list_length(settings);
> - for (; settings; settings = settings->next) {
> - const char *key = settings->data;
> - int attr;
> - int val;
> -
> - attr = attr_to_val(key);
> - if (attr < 0)
> - continue;
> -
> - val = player_get_setting(player, attr);
> - if (val < 0)
> - continue;
> -
> - pdu->params[len++] = attr;
> - pdu->params[len++] = val;
> - }
> -
> - g_list_free(settings);
> -
> + len = player_settings_changed(player, pdu);
> break;
> case AVRCP_EVENT_ADDRESSED_PLAYER_CHANGED:
> len = 5;
> --
> 2.27.0.383.g050319c2ae-goog
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [bluez PATCH v2] avrcp: include all player settings in notif event
2020-08-04 8:26 ` Archie Pusaka
@ 2020-08-04 17:07 ` Luiz Augusto von Dentz
2020-08-04 17:30 ` Archie Pusaka
0 siblings, 1 reply; 7+ messages in thread
From: Luiz Augusto von Dentz @ 2020-08-04 17:07 UTC (permalink / raw)
To: Archie Pusaka; +Cc: Howard Chung, BlueZ, Archie Pusaka
Hi Archie,
On Tue, Aug 4, 2020 at 1:26 AM Archie Pusaka <apusaka@google.com> wrote:
>
> Hi Bluez maintainers,
>
> Could you take a look at this fix?
> Thank you!
Has there been any new version? It looks like CI has caught some
problems with it or that has been resolved by V2?
> Regards,
> Archie
>
>
> On Wed, 8 Jul 2020 at 12:19, Howard Chung <howardchung@google.com> wrote:
> >
> > According to AVRCP 1.6.2 spec section 6.7.2 table 6.39, all player
> > application settings should be returned to the CT and let CT to
> > determine which settings have changed. Currently bluez only returns
> > the changed attribute instead. This patch also addresses a potential
> > issue on which the number of application settings mismatches with
> > the actual number returned.
> >
> > Reviewed-by: Archie Pusaka <apusaka@chromium.org>
> > ---
> >
> > Changes in v2:
> > - Fixed unused variables
> >
> > profiles/audio/avrcp.c | 71 +++++++++++++++++++-----------------------
> > 1 file changed, 32 insertions(+), 39 deletions(-)
> >
> > diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
> > index e2428250e..a4de7530e 100644
> > --- a/profiles/audio/avrcp.c
> > +++ b/profiles/audio/avrcp.c
> > @@ -369,6 +369,7 @@ static uint32_t company_ids[] = {
> > };
> >
> > static void avrcp_register_notification(struct avrcp *session, uint8_t event);
> > +static GList *player_list_settings(struct avrcp_player *player);
> >
> > static sdp_record_t *avrcp_ct_record(void)
> > {
> > @@ -743,6 +744,35 @@ static int play_status_to_val(const char *status)
> > return -EINVAL;
> > }
> >
> > +static uint16_t player_settings_changed(struct avrcp_player *player,
> > + struct avrcp_header *pdu)
> > +{
> > + GList *settings = player_list_settings(player);
> > + int size = 2;
> > +
> > + for (; settings; settings = settings->next) {
> > + const char *key = settings->data;
> > + int attr;
> > + int val;
> > +
> > + attr = attr_to_val(key);
> > + if (attr < 0)
> > + continue;
> > +
> > + val = player_get_setting(player, attr);
> > + if (val < 0)
> > + continue;
> > +
> > + pdu->params[size++] = attr;
> > + pdu->params[size++] = val;
> > + }
> > +
> > + g_list_free(settings);
> > +
> > + pdu->params[1] = (size - 2) >> 1;
> > + return size;
> > +}
> > +
> > void avrcp_player_event(struct avrcp_player *player, uint8_t id,
> > const void *data)
> > {
> > @@ -751,8 +781,6 @@ void avrcp_player_event(struct avrcp_player *player, uint8_t id,
> > uint8_t code;
> > uint16_t size;
> > GSList *l;
> > - int attr;
> > - int val;
> >
> > if (player->sessions == NULL)
> > return;
> > @@ -791,19 +819,7 @@ void avrcp_player_event(struct avrcp_player *player, uint8_t id,
> > size = 1;
> > break;
> > case AVRCP_EVENT_SETTINGS_CHANGED:
> > - size = 2;
> > - pdu->params[1] = 1;
> > -
> > - attr = attr_to_val(data);
> > - if (attr < 0)
> > - return;
> > -
> > - val = player_get_setting(player, attr);
> > - if (val < 0)
> > - return;
> > -
> > - pdu->params[size++] = attr;
> > - pdu->params[size++] = val;
> > + size = player_settings_changed(player, pdu);
> > break;
> > case AVRCP_EVENT_ADDRESSED_PLAYER_CHANGED:
> > size = 5;
> > @@ -1595,7 +1611,6 @@ static uint8_t avrcp_handle_register_notification(struct avrcp *session,
> > struct btd_device *dev = session->dev;
> > uint16_t len = ntohs(pdu->params_len);
> > uint64_t uid;
> > - GList *settings;
> >
> > /*
> > * 1 byte for EventID, 4 bytes for Playback interval but the latest
> > @@ -1626,29 +1641,7 @@ static uint8_t avrcp_handle_register_notification(struct avrcp *session,
> > len = 1;
> > break;
> > case AVRCP_EVENT_SETTINGS_CHANGED:
> > - len = 1;
> > - settings = player_list_settings(player);
> > -
> > - pdu->params[len++] = g_list_length(settings);
> > - for (; settings; settings = settings->next) {
> > - const char *key = settings->data;
> > - int attr;
> > - int val;
> > -
> > - attr = attr_to_val(key);
> > - if (attr < 0)
> > - continue;
> > -
> > - val = player_get_setting(player, attr);
> > - if (val < 0)
> > - continue;
> > -
> > - pdu->params[len++] = attr;
> > - pdu->params[len++] = val;
> > - }
> > -
> > - g_list_free(settings);
> > -
> > + len = player_settings_changed(player, pdu);
> > break;
> > case AVRCP_EVENT_ADDRESSED_PLAYER_CHANGED:
> > len = 5;
> > --
> > 2.27.0.383.g050319c2ae-goog
> >
--
Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [bluez PATCH v2] avrcp: include all player settings in notif event
2020-08-04 17:07 ` Luiz Augusto von Dentz
@ 2020-08-04 17:30 ` Archie Pusaka
2020-08-05 0:27 ` Luiz Augusto von Dentz
0 siblings, 1 reply; 7+ messages in thread
From: Archie Pusaka @ 2020-08-04 17:30 UTC (permalink / raw)
To: Luiz Augusto von Dentz; +Cc: Howard Chung, BlueZ, Archie Pusaka
Hi Luiz,
Is this the problem?
https://lore.kernel.org/linux-bluetooth/5f05427c.1c69fb81.f61e.0992@mx.google.com/
If so, that has been resolved in v2.
Regards,
Archie
On Wed, 5 Aug 2020 at 01:07, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Archie,
>
> On Tue, Aug 4, 2020 at 1:26 AM Archie Pusaka <apusaka@google.com> wrote:
> >
> > Hi Bluez maintainers,
> >
> > Could you take a look at this fix?
> > Thank you!
>
> Has there been any new version? It looks like CI has caught some
> problems with it or that has been resolved by V2?
>
> > Regards,
> > Archie
> >
> >
> > On Wed, 8 Jul 2020 at 12:19, Howard Chung <howardchung@google.com> wrote:
> > >
> > > According to AVRCP 1.6.2 spec section 6.7.2 table 6.39, all player
> > > application settings should be returned to the CT and let CT to
> > > determine which settings have changed. Currently bluez only returns
> > > the changed attribute instead. This patch also addresses a potential
> > > issue on which the number of application settings mismatches with
> > > the actual number returned.
> > >
> > > Reviewed-by: Archie Pusaka <apusaka@chromium.org>
> > > ---
> > >
> > > Changes in v2:
> > > - Fixed unused variables
> > >
> > > profiles/audio/avrcp.c | 71 +++++++++++++++++++-----------------------
> > > 1 file changed, 32 insertions(+), 39 deletions(-)
> > >
> > > diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
> > > index e2428250e..a4de7530e 100644
> > > --- a/profiles/audio/avrcp.c
> > > +++ b/profiles/audio/avrcp.c
> > > @@ -369,6 +369,7 @@ static uint32_t company_ids[] = {
> > > };
> > >
> > > static void avrcp_register_notification(struct avrcp *session, uint8_t event);
> > > +static GList *player_list_settings(struct avrcp_player *player);
> > >
> > > static sdp_record_t *avrcp_ct_record(void)
> > > {
> > > @@ -743,6 +744,35 @@ static int play_status_to_val(const char *status)
> > > return -EINVAL;
> > > }
> > >
> > > +static uint16_t player_settings_changed(struct avrcp_player *player,
> > > + struct avrcp_header *pdu)
> > > +{
> > > + GList *settings = player_list_settings(player);
> > > + int size = 2;
> > > +
> > > + for (; settings; settings = settings->next) {
> > > + const char *key = settings->data;
> > > + int attr;
> > > + int val;
> > > +
> > > + attr = attr_to_val(key);
> > > + if (attr < 0)
> > > + continue;
> > > +
> > > + val = player_get_setting(player, attr);
> > > + if (val < 0)
> > > + continue;
> > > +
> > > + pdu->params[size++] = attr;
> > > + pdu->params[size++] = val;
> > > + }
> > > +
> > > + g_list_free(settings);
> > > +
> > > + pdu->params[1] = (size - 2) >> 1;
> > > + return size;
> > > +}
> > > +
> > > void avrcp_player_event(struct avrcp_player *player, uint8_t id,
> > > const void *data)
> > > {
> > > @@ -751,8 +781,6 @@ void avrcp_player_event(struct avrcp_player *player, uint8_t id,
> > > uint8_t code;
> > > uint16_t size;
> > > GSList *l;
> > > - int attr;
> > > - int val;
> > >
> > > if (player->sessions == NULL)
> > > return;
> > > @@ -791,19 +819,7 @@ void avrcp_player_event(struct avrcp_player *player, uint8_t id,
> > > size = 1;
> > > break;
> > > case AVRCP_EVENT_SETTINGS_CHANGED:
> > > - size = 2;
> > > - pdu->params[1] = 1;
> > > -
> > > - attr = attr_to_val(data);
> > > - if (attr < 0)
> > > - return;
> > > -
> > > - val = player_get_setting(player, attr);
> > > - if (val < 0)
> > > - return;
> > > -
> > > - pdu->params[size++] = attr;
> > > - pdu->params[size++] = val;
> > > + size = player_settings_changed(player, pdu);
> > > break;
> > > case AVRCP_EVENT_ADDRESSED_PLAYER_CHANGED:
> > > size = 5;
> > > @@ -1595,7 +1611,6 @@ static uint8_t avrcp_handle_register_notification(struct avrcp *session,
> > > struct btd_device *dev = session->dev;
> > > uint16_t len = ntohs(pdu->params_len);
> > > uint64_t uid;
> > > - GList *settings;
> > >
> > > /*
> > > * 1 byte for EventID, 4 bytes for Playback interval but the latest
> > > @@ -1626,29 +1641,7 @@ static uint8_t avrcp_handle_register_notification(struct avrcp *session,
> > > len = 1;
> > > break;
> > > case AVRCP_EVENT_SETTINGS_CHANGED:
> > > - len = 1;
> > > - settings = player_list_settings(player);
> > > -
> > > - pdu->params[len++] = g_list_length(settings);
> > > - for (; settings; settings = settings->next) {
> > > - const char *key = settings->data;
> > > - int attr;
> > > - int val;
> > > -
> > > - attr = attr_to_val(key);
> > > - if (attr < 0)
> > > - continue;
> > > -
> > > - val = player_get_setting(player, attr);
> > > - if (val < 0)
> > > - continue;
> > > -
> > > - pdu->params[len++] = attr;
> > > - pdu->params[len++] = val;
> > > - }
> > > -
> > > - g_list_free(settings);
> > > -
> > > + len = player_settings_changed(player, pdu);
> > > break;
> > > case AVRCP_EVENT_ADDRESSED_PLAYER_CHANGED:
> > > len = 5;
> > > --
> > > 2.27.0.383.g050319c2ae-goog
> > >
>
>
>
> --
> Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [bluez PATCH v2] avrcp: include all player settings in notif event
2020-08-04 17:30 ` Archie Pusaka
@ 2020-08-05 0:27 ` Luiz Augusto von Dentz
2020-08-05 0:30 ` Archie Pusaka
0 siblings, 1 reply; 7+ messages in thread
From: Luiz Augusto von Dentz @ 2020-08-05 0:27 UTC (permalink / raw)
To: Archie Pusaka; +Cc: Howard Chung, BlueZ, Archie Pusaka
Hi Archie,
On Tue, Aug 4, 2020 at 10:30 AM Archie Pusaka <apusaka@google.com> wrote:
>
> Hi Luiz,
>
> Is this the problem?
> https://lore.kernel.org/linux-bluetooth/5f05427c.1c69fb81.f61e.0992@mx.google.com/
>
> If so, that has been resolved in v2.
Found it, but it doesn't seem to apply on top of master:
Applying: avrcp: include all player settings in notif event
error: patch failed: profiles/audio/avrcp.c:1595
error: profiles/audio/avrcp.c: patch does not apply
Patch failed at 0001 avrcp: include all player settings in notif event
> Regards,
> Archie
>
> On Wed, 5 Aug 2020 at 01:07, Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
> >
> > Hi Archie,
> >
> > On Tue, Aug 4, 2020 at 1:26 AM Archie Pusaka <apusaka@google.com> wrote:
> > >
> > > Hi Bluez maintainers,
> > >
> > > Could you take a look at this fix?
> > > Thank you!
> >
> > Has there been any new version? It looks like CI has caught some
> > problems with it or that has been resolved by V2?
> >
> > > Regards,
> > > Archie
> > >
> > >
> > > On Wed, 8 Jul 2020 at 12:19, Howard Chung <howardchung@google.com> wrote:
> > > >
> > > > According to AVRCP 1.6.2 spec section 6.7.2 table 6.39, all player
> > > > application settings should be returned to the CT and let CT to
> > > > determine which settings have changed. Currently bluez only returns
> > > > the changed attribute instead. This patch also addresses a potential
> > > > issue on which the number of application settings mismatches with
> > > > the actual number returned.
> > > >
> > > > Reviewed-by: Archie Pusaka <apusaka@chromium.org>
> > > > ---
> > > >
> > > > Changes in v2:
> > > > - Fixed unused variables
> > > >
> > > > profiles/audio/avrcp.c | 71 +++++++++++++++++++-----------------------
> > > > 1 file changed, 32 insertions(+), 39 deletions(-)
> > > >
> > > > diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
> > > > index e2428250e..a4de7530e 100644
> > > > --- a/profiles/audio/avrcp.c
> > > > +++ b/profiles/audio/avrcp.c
> > > > @@ -369,6 +369,7 @@ static uint32_t company_ids[] = {
> > > > };
> > > >
> > > > static void avrcp_register_notification(struct avrcp *session, uint8_t event);
> > > > +static GList *player_list_settings(struct avrcp_player *player);
> > > >
> > > > static sdp_record_t *avrcp_ct_record(void)
> > > > {
> > > > @@ -743,6 +744,35 @@ static int play_status_to_val(const char *status)
> > > > return -EINVAL;
> > > > }
> > > >
> > > > +static uint16_t player_settings_changed(struct avrcp_player *player,
> > > > + struct avrcp_header *pdu)
> > > > +{
> > > > + GList *settings = player_list_settings(player);
> > > > + int size = 2;
> > > > +
> > > > + for (; settings; settings = settings->next) {
> > > > + const char *key = settings->data;
> > > > + int attr;
> > > > + int val;
> > > > +
> > > > + attr = attr_to_val(key);
> > > > + if (attr < 0)
> > > > + continue;
> > > > +
> > > > + val = player_get_setting(player, attr);
> > > > + if (val < 0)
> > > > + continue;
> > > > +
> > > > + pdu->params[size++] = attr;
> > > > + pdu->params[size++] = val;
> > > > + }
> > > > +
> > > > + g_list_free(settings);
> > > > +
> > > > + pdu->params[1] = (size - 2) >> 1;
> > > > + return size;
> > > > +}
> > > > +
> > > > void avrcp_player_event(struct avrcp_player *player, uint8_t id,
> > > > const void *data)
> > > > {
> > > > @@ -751,8 +781,6 @@ void avrcp_player_event(struct avrcp_player *player, uint8_t id,
> > > > uint8_t code;
> > > > uint16_t size;
> > > > GSList *l;
> > > > - int attr;
> > > > - int val;
> > > >
> > > > if (player->sessions == NULL)
> > > > return;
> > > > @@ -791,19 +819,7 @@ void avrcp_player_event(struct avrcp_player *player, uint8_t id,
> > > > size = 1;
> > > > break;
> > > > case AVRCP_EVENT_SETTINGS_CHANGED:
> > > > - size = 2;
> > > > - pdu->params[1] = 1;
> > > > -
> > > > - attr = attr_to_val(data);
> > > > - if (attr < 0)
> > > > - return;
> > > > -
> > > > - val = player_get_setting(player, attr);
> > > > - if (val < 0)
> > > > - return;
> > > > -
> > > > - pdu->params[size++] = attr;
> > > > - pdu->params[size++] = val;
> > > > + size = player_settings_changed(player, pdu);
> > > > break;
> > > > case AVRCP_EVENT_ADDRESSED_PLAYER_CHANGED:
> > > > size = 5;
> > > > @@ -1595,7 +1611,6 @@ static uint8_t avrcp_handle_register_notification(struct avrcp *session,
> > > > struct btd_device *dev = session->dev;
> > > > uint16_t len = ntohs(pdu->params_len);
> > > > uint64_t uid;
> > > > - GList *settings;
> > > >
> > > > /*
> > > > * 1 byte for EventID, 4 bytes for Playback interval but the latest
> > > > @@ -1626,29 +1641,7 @@ static uint8_t avrcp_handle_register_notification(struct avrcp *session,
> > > > len = 1;
> > > > break;
> > > > case AVRCP_EVENT_SETTINGS_CHANGED:
> > > > - len = 1;
> > > > - settings = player_list_settings(player);
> > > > -
> > > > - pdu->params[len++] = g_list_length(settings);
> > > > - for (; settings; settings = settings->next) {
> > > > - const char *key = settings->data;
> > > > - int attr;
> > > > - int val;
> > > > -
> > > > - attr = attr_to_val(key);
> > > > - if (attr < 0)
> > > > - continue;
> > > > -
> > > > - val = player_get_setting(player, attr);
> > > > - if (val < 0)
> > > > - continue;
> > > > -
> > > > - pdu->params[len++] = attr;
> > > > - pdu->params[len++] = val;
> > > > - }
> > > > -
> > > > - g_list_free(settings);
> > > > -
> > > > + len = player_settings_changed(player, pdu);
> > > > break;
> > > > case AVRCP_EVENT_ADDRESSED_PLAYER_CHANGED:
> > > > len = 5;
> > > > --
> > > > 2.27.0.383.g050319c2ae-goog
> > > >
> >
> >
> >
> > --
> > Luiz Augusto von Dentz
--
Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [bluez PATCH v2] avrcp: include all player settings in notif event
2020-08-05 0:27 ` Luiz Augusto von Dentz
@ 2020-08-05 0:30 ` Archie Pusaka
2020-08-05 2:09 ` Yun-hao Chung
0 siblings, 1 reply; 7+ messages in thread
From: Archie Pusaka @ 2020-08-05 0:30 UTC (permalink / raw)
To: Luiz Augusto von Dentz; +Cc: Howard Chung, BlueZ, Archie Pusaka
Got it, we will rebase to master and resubmit.
Regards,
Archie
On Wed, 5 Aug 2020 at 08:28, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Archie,
>
> On Tue, Aug 4, 2020 at 10:30 AM Archie Pusaka <apusaka@google.com> wrote:
> >
> > Hi Luiz,
> >
> > Is this the problem?
> > https://lore.kernel.org/linux-bluetooth/5f05427c.1c69fb81.f61e.0992@mx.google.com/
> >
> > If so, that has been resolved in v2.
>
> Found it, but it doesn't seem to apply on top of master:
>
> Applying: avrcp: include all player settings in notif event
> error: patch failed: profiles/audio/avrcp.c:1595
> error: profiles/audio/avrcp.c: patch does not apply
> Patch failed at 0001 avrcp: include all player settings in notif event
>
> > Regards,
> > Archie
> >
> > On Wed, 5 Aug 2020 at 01:07, Luiz Augusto von Dentz
> > <luiz.dentz@gmail.com> wrote:
> > >
> > > Hi Archie,
> > >
> > > On Tue, Aug 4, 2020 at 1:26 AM Archie Pusaka <apusaka@google.com> wrote:
> > > >
> > > > Hi Bluez maintainers,
> > > >
> > > > Could you take a look at this fix?
> > > > Thank you!
> > >
> > > Has there been any new version? It looks like CI has caught some
> > > problems with it or that has been resolved by V2?
> > >
> > > > Regards,
> > > > Archie
> > > >
> > > >
> > > > On Wed, 8 Jul 2020 at 12:19, Howard Chung <howardchung@google.com> wrote:
> > > > >
> > > > > According to AVRCP 1.6.2 spec section 6.7.2 table 6.39, all player
> > > > > application settings should be returned to the CT and let CT to
> > > > > determine which settings have changed. Currently bluez only returns
> > > > > the changed attribute instead. This patch also addresses a potential
> > > > > issue on which the number of application settings mismatches with
> > > > > the actual number returned.
> > > > >
> > > > > Reviewed-by: Archie Pusaka <apusaka@chromium.org>
> > > > > ---
> > > > >
> > > > > Changes in v2:
> > > > > - Fixed unused variables
> > > > >
> > > > > profiles/audio/avrcp.c | 71 +++++++++++++++++++-----------------------
> > > > > 1 file changed, 32 insertions(+), 39 deletions(-)
> > > > >
> > > > > diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
> > > > > index e2428250e..a4de7530e 100644
> > > > > --- a/profiles/audio/avrcp.c
> > > > > +++ b/profiles/audio/avrcp.c
> > > > > @@ -369,6 +369,7 @@ static uint32_t company_ids[] = {
> > > > > };
> > > > >
> > > > > static void avrcp_register_notification(struct avrcp *session, uint8_t event);
> > > > > +static GList *player_list_settings(struct avrcp_player *player);
> > > > >
> > > > > static sdp_record_t *avrcp_ct_record(void)
> > > > > {
> > > > > @@ -743,6 +744,35 @@ static int play_status_to_val(const char *status)
> > > > > return -EINVAL;
> > > > > }
> > > > >
> > > > > +static uint16_t player_settings_changed(struct avrcp_player *player,
> > > > > + struct avrcp_header *pdu)
> > > > > +{
> > > > > + GList *settings = player_list_settings(player);
> > > > > + int size = 2;
> > > > > +
> > > > > + for (; settings; settings = settings->next) {
> > > > > + const char *key = settings->data;
> > > > > + int attr;
> > > > > + int val;
> > > > > +
> > > > > + attr = attr_to_val(key);
> > > > > + if (attr < 0)
> > > > > + continue;
> > > > > +
> > > > > + val = player_get_setting(player, attr);
> > > > > + if (val < 0)
> > > > > + continue;
> > > > > +
> > > > > + pdu->params[size++] = attr;
> > > > > + pdu->params[size++] = val;
> > > > > + }
> > > > > +
> > > > > + g_list_free(settings);
> > > > > +
> > > > > + pdu->params[1] = (size - 2) >> 1;
> > > > > + return size;
> > > > > +}
> > > > > +
> > > > > void avrcp_player_event(struct avrcp_player *player, uint8_t id,
> > > > > const void *data)
> > > > > {
> > > > > @@ -751,8 +781,6 @@ void avrcp_player_event(struct avrcp_player *player, uint8_t id,
> > > > > uint8_t code;
> > > > > uint16_t size;
> > > > > GSList *l;
> > > > > - int attr;
> > > > > - int val;
> > > > >
> > > > > if (player->sessions == NULL)
> > > > > return;
> > > > > @@ -791,19 +819,7 @@ void avrcp_player_event(struct avrcp_player *player, uint8_t id,
> > > > > size = 1;
> > > > > break;
> > > > > case AVRCP_EVENT_SETTINGS_CHANGED:
> > > > > - size = 2;
> > > > > - pdu->params[1] = 1;
> > > > > -
> > > > > - attr = attr_to_val(data);
> > > > > - if (attr < 0)
> > > > > - return;
> > > > > -
> > > > > - val = player_get_setting(player, attr);
> > > > > - if (val < 0)
> > > > > - return;
> > > > > -
> > > > > - pdu->params[size++] = attr;
> > > > > - pdu->params[size++] = val;
> > > > > + size = player_settings_changed(player, pdu);
> > > > > break;
> > > > > case AVRCP_EVENT_ADDRESSED_PLAYER_CHANGED:
> > > > > size = 5;
> > > > > @@ -1595,7 +1611,6 @@ static uint8_t avrcp_handle_register_notification(struct avrcp *session,
> > > > > struct btd_device *dev = session->dev;
> > > > > uint16_t len = ntohs(pdu->params_len);
> > > > > uint64_t uid;
> > > > > - GList *settings;
> > > > >
> > > > > /*
> > > > > * 1 byte for EventID, 4 bytes for Playback interval but the latest
> > > > > @@ -1626,29 +1641,7 @@ static uint8_t avrcp_handle_register_notification(struct avrcp *session,
> > > > > len = 1;
> > > > > break;
> > > > > case AVRCP_EVENT_SETTINGS_CHANGED:
> > > > > - len = 1;
> > > > > - settings = player_list_settings(player);
> > > > > -
> > > > > - pdu->params[len++] = g_list_length(settings);
> > > > > - for (; settings; settings = settings->next) {
> > > > > - const char *key = settings->data;
> > > > > - int attr;
> > > > > - int val;
> > > > > -
> > > > > - attr = attr_to_val(key);
> > > > > - if (attr < 0)
> > > > > - continue;
> > > > > -
> > > > > - val = player_get_setting(player, attr);
> > > > > - if (val < 0)
> > > > > - continue;
> > > > > -
> > > > > - pdu->params[len++] = attr;
> > > > > - pdu->params[len++] = val;
> > > > > - }
> > > > > -
> > > > > - g_list_free(settings);
> > > > > -
> > > > > + len = player_settings_changed(player, pdu);
> > > > > break;
> > > > > case AVRCP_EVENT_ADDRESSED_PLAYER_CHANGED:
> > > > > len = 5;
> > > > > --
> > > > > 2.27.0.383.g050319c2ae-goog
> > > > >
> > >
> > >
> > >
> > > --
> > > Luiz Augusto von Dentz
>
>
>
> --
> Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [bluez PATCH v2] avrcp: include all player settings in notif event
2020-08-05 0:30 ` Archie Pusaka
@ 2020-08-05 2:09 ` Yun-hao Chung
0 siblings, 0 replies; 7+ messages in thread
From: Yun-hao Chung @ 2020-08-05 2:09 UTC (permalink / raw)
To: Archie Pusaka; +Cc: Luiz Augusto von Dentz, BlueZ, Archie Pusaka
I've rebased and submitted as patch v3, please check.
Thanks,
Howard
On Wed, Aug 5, 2020 at 8:31 AM Archie Pusaka <apusaka@google.com> wrote:
>
> Got it, we will rebase to master and resubmit.
>
> Regards,
> Archie
>
> On Wed, 5 Aug 2020 at 08:28, Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
> >
> > Hi Archie,
> >
> > On Tue, Aug 4, 2020 at 10:30 AM Archie Pusaka <apusaka@google.com> wrote:
> > >
> > > Hi Luiz,
> > >
> > > Is this the problem?
> > > https://lore.kernel.org/linux-bluetooth/5f05427c.1c69fb81.f61e.0992@mx.google.com/
> > >
> > > If so, that has been resolved in v2.
> >
> > Found it, but it doesn't seem to apply on top of master:
> >
> > Applying: avrcp: include all player settings in notif event
> > error: patch failed: profiles/audio/avrcp.c:1595
> > error: profiles/audio/avrcp.c: patch does not apply
> > Patch failed at 0001 avrcp: include all player settings in notif event
> >
> > > Regards,
> > > Archie
> > >
> > > On Wed, 5 Aug 2020 at 01:07, Luiz Augusto von Dentz
> > > <luiz.dentz@gmail.com> wrote:
> > > >
> > > > Hi Archie,
> > > >
> > > > On Tue, Aug 4, 2020 at 1:26 AM Archie Pusaka <apusaka@google.com> wrote:
> > > > >
> > > > > Hi Bluez maintainers,
> > > > >
> > > > > Could you take a look at this fix?
> > > > > Thank you!
> > > >
> > > > Has there been any new version? It looks like CI has caught some
> > > > problems with it or that has been resolved by V2?
> > > >
> > > > > Regards,
> > > > > Archie
> > > > >
> > > > >
> > > > > On Wed, 8 Jul 2020 at 12:19, Howard Chung <howardchung@google.com> wrote:
> > > > > >
> > > > > > According to AVRCP 1.6.2 spec section 6.7.2 table 6.39, all player
> > > > > > application settings should be returned to the CT and let CT to
> > > > > > determine which settings have changed. Currently bluez only returns
> > > > > > the changed attribute instead. This patch also addresses a potential
> > > > > > issue on which the number of application settings mismatches with
> > > > > > the actual number returned.
> > > > > >
> > > > > > Reviewed-by: Archie Pusaka <apusaka@chromium.org>
> > > > > > ---
> > > > > >
> > > > > > Changes in v2:
> > > > > > - Fixed unused variables
> > > > > >
> > > > > > profiles/audio/avrcp.c | 71 +++++++++++++++++++-----------------------
> > > > > > 1 file changed, 32 insertions(+), 39 deletions(-)
> > > > > >
> > > > > > diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
> > > > > > index e2428250e..a4de7530e 100644
> > > > > > --- a/profiles/audio/avrcp.c
> > > > > > +++ b/profiles/audio/avrcp.c
> > > > > > @@ -369,6 +369,7 @@ static uint32_t company_ids[] = {
> > > > > > };
> > > > > >
> > > > > > static void avrcp_register_notification(struct avrcp *session, uint8_t event);
> > > > > > +static GList *player_list_settings(struct avrcp_player *player);
> > > > > >
> > > > > > static sdp_record_t *avrcp_ct_record(void)
> > > > > > {
> > > > > > @@ -743,6 +744,35 @@ static int play_status_to_val(const char *status)
> > > > > > return -EINVAL;
> > > > > > }
> > > > > >
> > > > > > +static uint16_t player_settings_changed(struct avrcp_player *player,
> > > > > > + struct avrcp_header *pdu)
> > > > > > +{
> > > > > > + GList *settings = player_list_settings(player);
> > > > > > + int size = 2;
> > > > > > +
> > > > > > + for (; settings; settings = settings->next) {
> > > > > > + const char *key = settings->data;
> > > > > > + int attr;
> > > > > > + int val;
> > > > > > +
> > > > > > + attr = attr_to_val(key);
> > > > > > + if (attr < 0)
> > > > > > + continue;
> > > > > > +
> > > > > > + val = player_get_setting(player, attr);
> > > > > > + if (val < 0)
> > > > > > + continue;
> > > > > > +
> > > > > > + pdu->params[size++] = attr;
> > > > > > + pdu->params[size++] = val;
> > > > > > + }
> > > > > > +
> > > > > > + g_list_free(settings);
> > > > > > +
> > > > > > + pdu->params[1] = (size - 2) >> 1;
> > > > > > + return size;
> > > > > > +}
> > > > > > +
> > > > > > void avrcp_player_event(struct avrcp_player *player, uint8_t id,
> > > > > > const void *data)
> > > > > > {
> > > > > > @@ -751,8 +781,6 @@ void avrcp_player_event(struct avrcp_player *player, uint8_t id,
> > > > > > uint8_t code;
> > > > > > uint16_t size;
> > > > > > GSList *l;
> > > > > > - int attr;
> > > > > > - int val;
> > > > > >
> > > > > > if (player->sessions == NULL)
> > > > > > return;
> > > > > > @@ -791,19 +819,7 @@ void avrcp_player_event(struct avrcp_player *player, uint8_t id,
> > > > > > size = 1;
> > > > > > break;
> > > > > > case AVRCP_EVENT_SETTINGS_CHANGED:
> > > > > > - size = 2;
> > > > > > - pdu->params[1] = 1;
> > > > > > -
> > > > > > - attr = attr_to_val(data);
> > > > > > - if (attr < 0)
> > > > > > - return;
> > > > > > -
> > > > > > - val = player_get_setting(player, attr);
> > > > > > - if (val < 0)
> > > > > > - return;
> > > > > > -
> > > > > > - pdu->params[size++] = attr;
> > > > > > - pdu->params[size++] = val;
> > > > > > + size = player_settings_changed(player, pdu);
> > > > > > break;
> > > > > > case AVRCP_EVENT_ADDRESSED_PLAYER_CHANGED:
> > > > > > size = 5;
> > > > > > @@ -1595,7 +1611,6 @@ static uint8_t avrcp_handle_register_notification(struct avrcp *session,
> > > > > > struct btd_device *dev = session->dev;
> > > > > > uint16_t len = ntohs(pdu->params_len);
> > > > > > uint64_t uid;
> > > > > > - GList *settings;
> > > > > >
> > > > > > /*
> > > > > > * 1 byte for EventID, 4 bytes for Playback interval but the latest
> > > > > > @@ -1626,29 +1641,7 @@ static uint8_t avrcp_handle_register_notification(struct avrcp *session,
> > > > > > len = 1;
> > > > > > break;
> > > > > > case AVRCP_EVENT_SETTINGS_CHANGED:
> > > > > > - len = 1;
> > > > > > - settings = player_list_settings(player);
> > > > > > -
> > > > > > - pdu->params[len++] = g_list_length(settings);
> > > > > > - for (; settings; settings = settings->next) {
> > > > > > - const char *key = settings->data;
> > > > > > - int attr;
> > > > > > - int val;
> > > > > > -
> > > > > > - attr = attr_to_val(key);
> > > > > > - if (attr < 0)
> > > > > > - continue;
> > > > > > -
> > > > > > - val = player_get_setting(player, attr);
> > > > > > - if (val < 0)
> > > > > > - continue;
> > > > > > -
> > > > > > - pdu->params[len++] = attr;
> > > > > > - pdu->params[len++] = val;
> > > > > > - }
> > > > > > -
> > > > > > - g_list_free(settings);
> > > > > > -
> > > > > > + len = player_settings_changed(player, pdu);
> > > > > > break;
> > > > > > case AVRCP_EVENT_ADDRESSED_PLAYER_CHANGED:
> > > > > > len = 5;
> > > > > > --
> > > > > > 2.27.0.383.g050319c2ae-goog
> > > > > >
> > > >
> > > >
> > > >
> > > > --
> > > > Luiz Augusto von Dentz
> >
> >
> >
> > --
> > Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-08-05 2:09 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-08 4:19 [bluez PATCH v2] avrcp: include all player settings in notif event Howard Chung
2020-08-04 8:26 ` Archie Pusaka
2020-08-04 17:07 ` Luiz Augusto von Dentz
2020-08-04 17:30 ` Archie Pusaka
2020-08-05 0:27 ` Luiz Augusto von Dentz
2020-08-05 0:30 ` Archie Pusaka
2020-08-05 2:09 ` Yun-hao Chung
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).