| Message ID | 20210520041142.332534-1-eh5@sokka.cn (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [BlueZ,v2] avrcp: Fix unregister AVRCP player | expand |
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=485461 ---Test result--- Test Summary: CheckPatch PASS 0.33 seconds GitLint PASS 0.12 seconds Prep - Setup ELL PASS 46.41 seconds Build - Prep PASS 0.11 seconds Build - Configure PASS 8.40 seconds Build - Make PASS 206.91 seconds Make Check PASS 9.50 seconds Make Distcheck PASS 220.80 seconds Build w/ext ELL - Configure PASS 7.70 seconds Build w/ext ELL - Make PASS 172.24 seconds Details ############################## Test: CheckPatch - PASS Desc: Run checkpatch.pl script with rule in .checkpatch.conf ############################## Test: GitLint - PASS Desc: Run gitlint with rule in .gitlint ############################## Test: Prep - Setup ELL - PASS Desc: Clone, build, and install ELL ############################## Test: Build - Prep - PASS Desc: Prepare environment for build ############################## Test: Build - Configure - PASS Desc: Configure the BlueZ source tree ############################## Test: Build - Make - PASS Desc: Build the BlueZ source tree ############################## Test: Make Check - PASS Desc: Run 'make check' ############################## Test: Make Distcheck - PASS Desc: Run distcheck to check the distribution ############################## Test: Build w/ext ELL - Configure - PASS Desc: Configure BlueZ source with '--enable-external-ell' configuration ############################## Test: Build w/ext ELL - Make - PASS Desc: Build BlueZ source with '--enable-external-ell' configuration --- Regards, Linux Bluetooth
Hi Huang-Huang, On Wed, May 19, 2021 at 9:14 PM Huang-Huang Bao <eh5@sokka.cn> wrote: > > v2: fix commit message & code styles > > 'notify_addressed_player_changed()' expected to be called with > 'player->changed_id' set to what 'g_idle_add()' returns. > > player->changed_id = g_idle_add(notify_addressed_player_changed, > player); > > And 'avrcp_player_event()' relies on 'player->changed_id' to perform > Addressed Player Changed notification. However, > 'avrcp_unregister_player()' calls 'notify_addressed_player_changed()' > without adding it to the main loop and set 'player->changed_id'. To > make 'notify_addressed_player_changed()' can be called without set > 'player->changed_id' flag. We add antoher flag > 'player->addressed_changing' to indicate addressed player changing. > > Fixes https://github.com/bluez/bluez/issues/142 > --- > profiles/audio/avrcp.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c > index 58d30b24d..5058a6848 100644 > --- a/profiles/audio/avrcp.c > +++ b/profiles/audio/avrcp.c > @@ -239,6 +239,7 @@ struct avrcp_player { > uint8_t *features; > char *path; > guint changed_id; > + bool addressed_changing; > > struct pending_list_items *p; > char *change_path; > @@ -792,7 +793,8 @@ void avrcp_player_event(struct avrcp_player *player, uint8_t id, > > DBG("id=%u", id); > > - if (id != AVRCP_EVENT_ADDRESSED_PLAYER_CHANGED && player->changed_id) { > + if (id != AVRCP_EVENT_ADDRESSED_PLAYER_CHANGED && > + player->addressed_changing) { > code = AVC_CTYPE_REJECTED; > size = 1; > pdu->params[0] = AVRCP_STATUS_ADDRESSED_PLAYER_CHANGED; > @@ -1794,6 +1796,8 @@ static gboolean notify_addressed_player_changed(gpointer user_data) > }; > uint8_t i; > > + player->addressed_changing = true; Well we could just set the change_id manually instead since it is anyway set to 0 in the code below which is equivalent to what you are doing here. > + > avrcp_player_event(player, AVRCP_EVENT_ADDRESSED_PLAYER_CHANGED, NULL); > > /* > @@ -1804,6 +1808,7 @@ static gboolean notify_addressed_player_changed(gpointer user_data) > for (i = 0; i < sizeof(events); i++) > avrcp_player_event(player, events[i], NULL); > > + player->addressed_changing = false; > player->changed_id = 0; > > return FALSE; > -- > 2.31.1
On 5/20/21 1:44 PM, Luiz Augusto von Dentz wrote: > Hi Huang-Huang, > > On Wed, May 19, 2021 at 9:14 PM Huang-Huang Bao <eh5@sokka.cn> wrote: >> >> v2: fix commit message & code styles >> >> 'notify_addressed_player_changed()' expected to be called with >> 'player->changed_id' set to what 'g_idle_add()' returns. >> >> player->changed_id = g_idle_add(notify_addressed_player_changed, >> player); >> >> And 'avrcp_player_event()' relies on 'player->changed_id' to perform >> Addressed Player Changed notification. However, >> 'avrcp_unregister_player()' calls 'notify_addressed_player_changed()' >> without adding it to the main loop and set 'player->changed_id'. To >> make 'notify_addressed_player_changed()' can be called without set >> 'player->changed_id' flag. We add antoher flag >> 'player->addressed_changing' to indicate addressed player changing. >> >> Fixes https://github.com/bluez/bluez/issues/142 >> --- >> profiles/audio/avrcp.c | 7 ++++++- >> 1 file changed, 6 insertions(+), 1 deletion(-) >> >> diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c >> index 58d30b24d..5058a6848 100644 >> --- a/profiles/audio/avrcp.c >> +++ b/profiles/audio/avrcp.c >> @@ -239,6 +239,7 @@ struct avrcp_player { >> uint8_t *features; >> char *path; >> guint changed_id; >> + bool addressed_changing; >> >> struct pending_list_items *p; >> char *change_path; >> @@ -792,7 +793,8 @@ void avrcp_player_event(struct avrcp_player *player, uint8_t id, >> >> DBG("id=%u", id); >> >> - if (id != AVRCP_EVENT_ADDRESSED_PLAYER_CHANGED && player->changed_id) { >> + if (id != AVRCP_EVENT_ADDRESSED_PLAYER_CHANGED && >> + player->addressed_changing) { >> code = AVC_CTYPE_REJECTED; >> size = 1; >> pdu->params[0] = AVRCP_STATUS_ADDRESSED_PLAYER_CHANGED; >> @@ -1794,6 +1796,8 @@ static gboolean notify_addressed_player_changed(gpointer user_data) >> }; >> uint8_t i; >> >> + player->addressed_changing = true; > > Well we could just set the change_id manually instead since it is > anyway set to 0 in the code below which is equivalent to what you are > doing here. Yeah, make since. I will submit a v3 soon. Regards, Bao > >> + >> avrcp_player_event(player, AVRCP_EVENT_ADDRESSED_PLAYER_CHANGED, NULL); >> >> /* >> @@ -1804,6 +1808,7 @@ static gboolean notify_addressed_player_changed(gpointer user_data) >> for (i = 0; i < sizeof(events); i++) >> avrcp_player_event(player, events[i], NULL); >> >> + player->addressed_changing = false; >> player->changed_id = 0; >> >> return FALSE; >> -- >> 2.31.1 > > >
diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c index 58d30b24d..5058a6848 100644 --- a/profiles/audio/avrcp.c +++ b/profiles/audio/avrcp.c @@ -239,6 +239,7 @@ struct avrcp_player { uint8_t *features; char *path; guint changed_id; + bool addressed_changing; struct pending_list_items *p; char *change_path; @@ -792,7 +793,8 @@ void avrcp_player_event(struct avrcp_player *player, uint8_t id, DBG("id=%u", id); - if (id != AVRCP_EVENT_ADDRESSED_PLAYER_CHANGED && player->changed_id) { + if (id != AVRCP_EVENT_ADDRESSED_PLAYER_CHANGED && + player->addressed_changing) { code = AVC_CTYPE_REJECTED; size = 1; pdu->params[0] = AVRCP_STATUS_ADDRESSED_PLAYER_CHANGED; @@ -1794,6 +1796,8 @@ static gboolean notify_addressed_player_changed(gpointer user_data) }; uint8_t i; + player->addressed_changing = true; + avrcp_player_event(player, AVRCP_EVENT_ADDRESSED_PLAYER_CHANGED, NULL); /* @@ -1804,6 +1808,7 @@ static gboolean notify_addressed_player_changed(gpointer user_data) for (i = 0; i < sizeof(events); i++) avrcp_player_event(player, events[i], NULL); + player->addressed_changing = false; player->changed_id = 0; return FALSE;
v2: fix commit message & code styles 'notify_addressed_player_changed()' expected to be called with 'player->changed_id' set to what 'g_idle_add()' returns. player->changed_id = g_idle_add(notify_addressed_player_changed, player); And 'avrcp_player_event()' relies on 'player->changed_id' to perform Addressed Player Changed notification. However, 'avrcp_unregister_player()' calls 'notify_addressed_player_changed()' without adding it to the main loop and set 'player->changed_id'. To make 'notify_addressed_player_changed()' can be called without set 'player->changed_id' flag. We add antoher flag 'player->addressed_changing' to indicate addressed player changing. Fixes https://github.com/bluez/bluez/issues/142 --- profiles/audio/avrcp.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) -- 2.31.1