| Message ID | 06f0a1ed2fc2726b8dd46eef113e07ad742048c1.1737921917.git.pav@iki.fi (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [BlueZ,v2] shared/vcp: have only one volume change in flight at a time | 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 | success | CheckSparse PASS |
| 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=928426 ---Test result--- Test Summary: CheckPatch PENDING 0.18 seconds GitLint PENDING 0.17 seconds BuildEll PASS 20.45 seconds BluezMake PASS 1644.03 seconds MakeCheck PASS 13.58 seconds MakeDistcheck PASS 159.53 seconds CheckValgrind PASS 215.13 seconds CheckSmatch PASS 271.33 seconds bluezmakeextell PASS 98.65 seconds IncrementalBuild PENDING 0.26 seconds ScanBuild PASS 871.08 seconds Details ############################## Test: CheckPatch - PENDING Desc: Run checkpatch.pl script Output: ############################## Test: GitLint - PENDING Desc: Run gitlint Output: ############################## Test: IncrementalBuild - PENDING Desc: Incremental build with the patches in the series Output: --- Regards, Linux Bluetooth
Hi Pauli, On Sun, Jan 26, 2025 at 3:09 PM Pauli Virtanen <pav@iki.fi> wrote: > > If bt_vcp_set_volume() is called again before the previous operation has > completed, the requests get the same change counter, and all except the > first one fail. > > Fix by sending another volume set request only after the previous has > either failed or generated a volume notification. > > Send only volume requests that change the value to a different one than > last notification we have seen: in this case the request either fails, > or succeeds and generates a new notification. In all these cases, we > exit the wait state correctly. > --- > > Notes: > v2: reset pending_ops when attaching, needs to be cleared on reconnect > > src/shared/vcp.c | 39 +++++++++++++++++++++++++++++++++++++-- > 1 file changed, 37 insertions(+), 2 deletions(-) > > diff --git a/src/shared/vcp.c b/src/shared/vcp.c > index 6b0f2f9db..2b5d186b8 100644 > --- a/src/shared/vcp.c > +++ b/src/shared/vcp.c > @@ -203,6 +203,9 @@ struct bt_vcp { > uint8_t volume; > uint8_t volume_counter; > > + uint8_t pending_volume; > + unsigned int pending_ops; > + > void *debug_data; > void *user_data; > }; > @@ -2003,6 +2006,19 @@ done: > return vcp; > } > > +static void vcp_set_pending_volume(struct bt_vcp *vcp) > +{ > + /* Send pending request if any */ > + if (vcp->pending_ops <= 1) { Not following you here, why are you skipping peding_ops == 1? Or 1 means the current one? Perhaps a comment would help here. > + vcp->pending_ops = 0; > + return; > + } > + vcp->pending_ops = 0; > + > + DBG(vcp, "set pending volume 0x%x", vcp->pending_volume); > + bt_vcp_set_volume(vcp, vcp->pending_volume); > +} > + > static void vcp_vstate_notify(struct bt_vcp *vcp, uint16_t value_handle, > const uint8_t *value, uint16_t length, > void *user_data) > @@ -2020,6 +2036,8 @@ static void vcp_vstate_notify(struct bt_vcp *vcp, uint16_t value_handle, > > if (vcp->volume_changed) > vcp->volume_changed(vcp, vcp->volume); > + > + vcp_set_pending_volume(vcp); > } > > static void vcp_volume_cp_sent(bool success, uint8_t err, void *user_data) > @@ -2031,6 +2049,8 @@ static void vcp_volume_cp_sent(bool success, uint8_t err, void *user_data) > DBG(vcp, "setting volume failed: invalid counter"); > else > DBG(vcp, "setting volume failed: error 0x%x", err); > + > + vcp_set_pending_volume(vcp); > } > } > > @@ -2061,9 +2081,20 @@ static bool vcp_set_volume_client(struct bt_vcp *vcp, uint8_t volume) > return false; > } > > - vcp->volume = volume; > + vcp->pending_volume = volume; Interesting, so the design is that we update the pending_volume so only the last value is sent, I guess that should work but I probably leave a comment that is the intended behavior since otherwise one could assume that all intermediate volume changes would be sent (which is probably a waste o time when setting an absolute rather than relative volume). > + if (vcp->pending_ops) { > + /* Wait for current operation to complete */ > + vcp->pending_ops++; > + return true; This seems to assume we always will receive a notification that completes the volume change? Id consider putting a timeout associated with the request in case the server doesn't generate a notification for some reason then we should consider proceeding to the next pending_volume or fail and abort setting it. > + } else if (vcp->volume == vcp->pending_volume) { > + /* Do not set to current value, as that doesn't generate > + * a notification > + */ > + return true; > + } > + > req.op = BT_VCS_SET_ABSOLUTE_VOL; > - req.vol_set = vcp->volume; > + req.vol_set = vcp->pending_volume; > req.change_counter = vcp->volume_counter; > > if (!bt_gatt_client_write_value(vcp->client, value_handle, (void *)&req, > @@ -2072,6 +2103,8 @@ static bool vcp_set_volume_client(struct bt_vcp *vcp, uint8_t volume) > DBG(vcp, "error writing volume"); > return false; > } > + > + vcp->pending_ops++; > return true; > } > > @@ -2896,6 +2929,8 @@ bool bt_vcp_attach(struct bt_vcp *vcp, struct bt_gatt_client *client) > if (!vcp->client) > return false; > > + vcp->pending_ops = 0; > + > bt_uuid16_create(&uuid, VCS_UUID); > gatt_db_foreach_service(vcp->rdb->db, &uuid, foreach_vcs_service, vcp); > > -- > 2.48.1 > >
diff --git a/src/shared/vcp.c b/src/shared/vcp.c index 6b0f2f9db..2b5d186b8 100644 --- a/src/shared/vcp.c +++ b/src/shared/vcp.c @@ -203,6 +203,9 @@ struct bt_vcp { uint8_t volume; uint8_t volume_counter; + uint8_t pending_volume; + unsigned int pending_ops; + void *debug_data; void *user_data; }; @@ -2003,6 +2006,19 @@ done: return vcp; } +static void vcp_set_pending_volume(struct bt_vcp *vcp) +{ + /* Send pending request if any */ + if (vcp->pending_ops <= 1) { + vcp->pending_ops = 0; + return; + } + vcp->pending_ops = 0; + + DBG(vcp, "set pending volume 0x%x", vcp->pending_volume); + bt_vcp_set_volume(vcp, vcp->pending_volume); +} + static void vcp_vstate_notify(struct bt_vcp *vcp, uint16_t value_handle, const uint8_t *value, uint16_t length, void *user_data) @@ -2020,6 +2036,8 @@ static void vcp_vstate_notify(struct bt_vcp *vcp, uint16_t value_handle, if (vcp->volume_changed) vcp->volume_changed(vcp, vcp->volume); + + vcp_set_pending_volume(vcp); } static void vcp_volume_cp_sent(bool success, uint8_t err, void *user_data) @@ -2031,6 +2049,8 @@ static void vcp_volume_cp_sent(bool success, uint8_t err, void *user_data) DBG(vcp, "setting volume failed: invalid counter"); else DBG(vcp, "setting volume failed: error 0x%x", err); + + vcp_set_pending_volume(vcp); } } @@ -2061,9 +2081,20 @@ static bool vcp_set_volume_client(struct bt_vcp *vcp, uint8_t volume) return false; } - vcp->volume = volume; + vcp->pending_volume = volume; + if (vcp->pending_ops) { + /* Wait for current operation to complete */ + vcp->pending_ops++; + return true; + } else if (vcp->volume == vcp->pending_volume) { + /* Do not set to current value, as that doesn't generate + * a notification + */ + return true; + } + req.op = BT_VCS_SET_ABSOLUTE_VOL; - req.vol_set = vcp->volume; + req.vol_set = vcp->pending_volume; req.change_counter = vcp->volume_counter; if (!bt_gatt_client_write_value(vcp->client, value_handle, (void *)&req, @@ -2072,6 +2103,8 @@ static bool vcp_set_volume_client(struct bt_vcp *vcp, uint8_t volume) DBG(vcp, "error writing volume"); return false; } + + vcp->pending_ops++; return true; } @@ -2896,6 +2929,8 @@ bool bt_vcp_attach(struct bt_vcp *vcp, struct bt_gatt_client *client) if (!vcp->client) return false; + vcp->pending_ops = 0; + bt_uuid16_create(&uuid, VCS_UUID); gatt_db_foreach_service(vcp->rdb->db, &uuid, foreach_vcs_service, vcp);
If bt_vcp_set_volume() is called again before the previous operation has completed, the requests get the same change counter, and all except the first one fail. Fix by sending another volume set request only after the previous has either failed or generated a volume notification. Send only volume requests that change the value to a different one than last notification we have seen: in this case the request either fails, or succeeds and generates a new notification. In all these cases, we exit the wait state correctly. --- Notes: v2: reset pending_ops when attaching, needs to be cleared on reconnect src/shared/vcp.c | 39 +++++++++++++++++++++++++++++++++++++-- 1 file changed, 37 insertions(+), 2 deletions(-)