Hi Pauli,
On Sun, Jan 26, 2025 at 3:09 PM Pauli Virtanen <pav@xxxxxx> 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
>
>
--
Luiz Augusto von Dentz
