[PATCH BlueZ v2] shared/vcp: have only one volume change in flight at a time

[PATCH BlueZ v2] shared/vcp: have only one volume change in flight at a time

[Pauli Virtanen]

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) {
+		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);
 
-- 
2.48.1

RE: [BlueZ,v2] shared/vcp: have only one volume change in flight at a time

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 1260 bytes --]

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

Re: [PATCH BlueZ v2] shared/vcp: have only one volume change in flight at a time

[Luiz Augusto von Dentz]

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
>
>


-- 
Luiz Augusto von Dentz