diff mbox series

[BlueZ,3/3] vcp: allow volume control when acting as a server

Message ID 20250110145019.2380299-4-michal.dzik@streamunlimited.com (mailing list archive)
State New
Headers show
Series connect VCP profile to MediaTransport volume | expand

Checks

Context Check Description
tedd_an/pre-ci_am success Success

Commit Message

Michal Dzik Jan. 10, 2025, 2:50 p.m. UTC 
Local VCP chatacteristics are now connected to transport properties.
---
 profiles/audio/vcp.c |   7 +--
 src/shared/vcp.c     | 109 ++++++++++++++++++++++++++++++++-----------
 2 files changed, 86 insertions(+), 30 deletions(-)

Comments

Luiz Augusto von Dentz Jan. 10, 2025, 3:34 p.m. UTC | #1 
Hi Michal,

On Fri, Jan 10, 2025 at 9:54 AM Michal Dzik
<michal.dzik@streamunlimited.com> wrote:
>
> Local VCP chatacteristics are now connected to transport properties.
> ---
>  profiles/audio/vcp.c |   7 +--
>  src/shared/vcp.c     | 109 ++++++++++++++++++++++++++++++++-----------
>  2 files changed, 86 insertions(+), 30 deletions(-)
>
> diff --git a/profiles/audio/vcp.c b/profiles/audio/vcp.c
> index 0203673bf..07b227107 100644
> --- a/profiles/audio/vcp.c
> +++ b/profiles/audio/vcp.c
> @@ -186,7 +186,7 @@ bool bt_audio_vcp_set_volume(struct btd_device *device, int8_t volume)
>         return FALSE;
>  }
>
> -static void vcp_detached(struct bt_vcp *vcp, void *user_data)
> +static void vcp_remote_client_detached(struct bt_vcp *vcp, void *user_data)
>  {
>         struct vcp_data *data;
>
> @@ -201,7 +201,7 @@ static void vcp_detached(struct bt_vcp *vcp, void *user_data)
>         vcp_data_remove(data);
>  }
>
> -static void vcp_attached(struct bt_vcp *vcp, void *user_data)
> +static void vcp_remote_client_attached(struct bt_vcp *vcp, void *user_data)
>  {
>         struct vcp_data *data;
>         struct bt_att *att;
> @@ -352,7 +352,8 @@ static int vcp_init(void)
>         if (err)
>                 return err;
>
> -       vcp_id = bt_vcp_register(vcp_attached, vcp_detached, NULL);
> +       vcp_id = bt_vcp_register(vcp_remote_client_attached,
> +                                           vcp_remote_client_detached, NULL);
>
>         return 0;
>  }
> diff --git a/src/shared/vcp.c b/src/shared/vcp.c

Don't mix the changes of shared and plugins, they should be changed
independently since they may affect different parts e.g. shared can be
used for unit testing, tools, etc.

> index c92eb00d4..45d0f4485 100644
> --- a/src/shared/vcp.c
> +++ b/src/shared/vcp.c
> @@ -379,7 +379,15 @@ static struct bt_aics *vcp_get_aics(struct bt_vcp *vcp)
>         return vcp->rdb->aics;
>  }
>
> -static void vcp_detached(void *data, void *user_data)
> +static void vcp_remote_client_attached(void *data, void *user_data)
> +{
> +       struct bt_vcp_cb *cb = data;
> +       struct bt_vcp *vcp = user_data;
> +
> +       cb->attached(vcp, cb->user_data);
> +}
> +
> +static void vcp_remote_client_detached(void *data, void *user_data)
>  {
>         struct bt_vcp_cb *cb = data;
>         struct bt_vcp *vcp = user_data;
> @@ -389,7 +397,6 @@ static void vcp_detached(void *data, void *user_data)
>
>  void bt_vcp_detach(struct bt_vcp *vcp)
>  {
> -       struct bt_vcs *vcs;
>         if (!queue_remove(sessions, vcp))
>                 return;
>
> @@ -399,7 +406,7 @@ void bt_vcp_detach(struct bt_vcp *vcp)
>         }
>
>         if (vcp->rdb) {
> -               vcs = vcp_get_vcs(vcp);
> +               struct bt_vcs *vcs = vcp_get_vcs(vcp);
>                 vcs->service = NULL;
>                 vcs->vs = NULL;
>                 vcs->vs_ccc = NULL;
> @@ -506,6 +513,7 @@ static void vcp_disconnected(int err, void *user_data)
>         DBG(vcp, "vcp %p disconnected err %d", vcp, err);
>
>         bt_vcp_detach(vcp);
> +       queue_foreach(vcp_cbs, vcp_remote_client_detached, vcp);
>  }
>
>  static struct bt_vcp *vcp_get_session(struct bt_att *att, struct gatt_db *db)
> @@ -524,6 +532,8 @@ static struct bt_vcp *vcp_get_session(struct bt_att *att, struct gatt_db *db)
>         vcp = bt_vcp_new(db, NULL);
>         vcp->att = att;
>
> +       queue_foreach(vcp_cbs, vcp_remote_client_attached, vcp);
> +
>         bt_att_register_disconnect(att, vcp_disconnected, vcp, NULL);
>
>         if (!sessions)
> @@ -566,6 +576,10 @@ static uint8_t vcs_rel_vol_down(struct bt_vcs *vcs, struct bt_vcp *vcp,
>
>         vstate->vol_set = MAX((vstate->vol_set - VCP_STEP_SIZE), 0);
>         vstate->counter = -~vstate->counter; /*Increment Change Counter*/
> +       vcp->volume = vstate->vol_set;
> +
> +       if (vcp->volume_changed)
> +               vcp->volume_changed(vcp, vcp->volume / 2);

What is this division by 2 for? If this is by the spec then Id suggest
adding a define, otherwise Id just store the actual value.

>
>         gatt_db_attribute_notify(vdb->vcs->vs, (void *)vstate,
>                                  sizeof(struct vol_state),
> @@ -605,6 +619,10 @@ static uint8_t vcs_rel_vol_up(struct bt_vcs *vcs, struct bt_vcp *vcp,
>
>         vstate->vol_set = MIN((vstate->vol_set + VCP_STEP_SIZE), 255);
>         vstate->counter = -~vstate->counter; /*Increment Change Counter*/
> +       vcp->volume = vstate->vol_set;
> +
> +       if (vcp->volume_changed)
> +               vcp->volume_changed(vcp, vcp->volume / 2);
>
>         gatt_db_attribute_notify(vdb->vcs->vs, (void *)vstate,
>                                  sizeof(struct vol_state),
> @@ -645,6 +663,10 @@ static uint8_t vcs_unmute_rel_vol_down(struct bt_vcs *vcs, struct bt_vcp *vcp,
>         vstate->mute = 0x00;
>         vstate->vol_set = MAX((vstate->vol_set - VCP_STEP_SIZE), 0);
>         vstate->counter = -~vstate->counter; /*Increment Change Counter*/
> +       vcp->volume = vstate->vol_set;
> +
> +       if (vcp->volume_changed)
> +               vcp->volume_changed(vcp, vcp->volume / 2);
>
>         gatt_db_attribute_notify(vdb->vcs->vs, (void *)vstate,
>                                  sizeof(struct vol_state),
> @@ -685,6 +707,10 @@ static uint8_t vcs_unmute_rel_vol_up(struct bt_vcs *vcs, struct bt_vcp *vcp,
>         vstate->mute = 0x00;
>         vstate->vol_set = MIN((vstate->vol_set + VCP_STEP_SIZE), 255);
>         vstate->counter = -~vstate->counter; /*Increment Change Counter*/
> +       vcp->volume = vstate->vol_set;
> +
> +       if (vcp->volume_changed)
> +               vcp->volume_changed(vcp, vcp->volume / 2);
>
>         gatt_db_attribute_notify(vdb->vcs->vs, (void *)vstate,
>                                  sizeof(struct vol_state),
> @@ -724,6 +750,10 @@ static uint8_t vcs_set_absolute_vol(struct bt_vcs *vcs, struct bt_vcp *vcp,
>
>         vstate->vol_set = req->vol_set;
>         vstate->counter = -~vstate->counter; /*Increment Change Counter*/
> +       vcp->volume = vstate->vol_set;
> +
> +       if (vcp->volume_changed)
> +               vcp->volume_changed(vcp, vcp->volume / 2);
>
>         gatt_db_attribute_notify(vdb->vcs->vs, (void *)vstate,
>                                  sizeof(struct vol_state),
> @@ -2023,34 +2053,59 @@ bool bt_vcp_set_volume(struct bt_vcp *vcp, int8_t volume)
>  {
>         struct bt_vcs_client_ab_vol req;
>         uint16_t value_handle;
> -       struct bt_vcs *vcs = vcp_get_vcs(vcp);
>
> -       if (!vcs) {
> -               DBG(vcp, "error: vcs not available");
> -               return false;
> -       }
> +       vcp->volume = volume * 2;

Ditto, very odd to have to multiply by 2 and then divide when sending,
and I'm afraid we may have overflow problems as well.

>
> -       if (!vcs->vol_cp) {
> -               DBG(vcp, "error: vol_cp characteristics not available");
> -               return false;
> -       }
> +       if (vcp->rdb) {
> +               /* local gatt client */

Note that the fact that there is an rdb does _not_ guarantee we are
acting as a client, since the remote may support both roles e.g. bluez
vs bluez, instead you should really be checking if vcp->client has
been set.

> +               struct bt_vcs *vcs = vcp_get_vcs(vcp);
>
> -       if (!gatt_db_attribute_get_char_data(vcs->vol_cp, NULL, &value_handle,
> -                                                       NULL, NULL, NULL)) {
> -               DBG(vcp, "error: vol_cp characteristics not available");
> -               return false;
> -       }
> +               if (!vcs) {
> +                       DBG(vcp, "error: vcs not available");
> +                       return false;
> +               }
>
> -       vcp->volume = volume * 2;
> -       req.op = BT_VCS_SET_ABSOLUTE_VOL;
> -       req.vol_set = vcp->volume;
> -       req.change_counter = vcp->volume_counter;
> -
> -       if (!bt_gatt_client_write_value(vcp->client, value_handle, (void *) &req,
> -               sizeof(struct bt_vcs_client_ab_vol), vcp_volume_cp_sent, vcp,
> -                                                                       NULL)) {
> -               DBG(vcp, "error writing volume");
> -               return false;
> +               if (!vcs->vol_cp) {
> +                       DBG(vcp, "error: vol_cp characteristics not available");
> +                       return false;
> +               }
> +
> +               if (!gatt_db_attribute_get_char_data(vcs->vol_cp, NULL,
> +                                       &value_handle, NULL, NULL, NULL)) {
> +                       DBG(vcp, "error: vol_cp characteristics not available");
> +                       return false;
> +               }
> +
> +               req.op = BT_VCS_SET_ABSOLUTE_VOL;
> +               req.vol_set = vcp->volume;
> +               req.change_counter = vcp->volume_counter;
> +
> +               if (!bt_gatt_client_write_value(vcp->client, value_handle,
> +                       (void *) &req, sizeof(struct bt_vcs_client_ab_vol),
> +                                       vcp_volume_cp_sent, vcp, NULL)) {
> +                       DBG(vcp, "error writing volume");
> +                       return false;
> +               }
> +       } else {
> +               // local gatt server
> +               struct bt_vcp_db *vdb = vcp_get_vdb(vcp);
> +               struct vol_state *vstate;
> +
> +               if (!vdb) {
> +                       DBG(vcp, "error: VDB not available");
> +                       return false;
> +               }
> +
> +               vstate = vdb_get_vstate(vdb);
> +               if (!vstate) {
> +                       DBG(vcp, "error: VSTATE not available");
> +                       return false;
> +               }
> +
> +               vstate->vol_set = vcp->volume;
> +               vstate->counter = -~vstate->counter; /*Increment Change Counter*/
> +               gatt_db_attribute_notify(vdb->vcs->vs, (void *) vstate,
> +                               sizeof(struct vol_state), bt_vcp_get_att(vcp));

I'd recommend splitting these into their own functions to make it more
readable and you can name it according to the role as well.

>         }
>         return true;
>  }
> --
> 2.34.1
>
>
diff mbox series

Patch

diff --git a/profiles/audio/vcp.c b/profiles/audio/vcp.c
index 0203673bf..07b227107 100644
--- a/profiles/audio/vcp.c
+++ b/profiles/audio/vcp.c
@@ -186,7 +186,7 @@  bool bt_audio_vcp_set_volume(struct btd_device *device, int8_t volume)
 	return FALSE;
 }
 
-static void vcp_detached(struct bt_vcp *vcp, void *user_data)
+static void vcp_remote_client_detached(struct bt_vcp *vcp, void *user_data)
 {
 	struct vcp_data *data;
 
@@ -201,7 +201,7 @@  static void vcp_detached(struct bt_vcp *vcp, void *user_data)
 	vcp_data_remove(data);
 }
 
-static void vcp_attached(struct bt_vcp *vcp, void *user_data)
+static void vcp_remote_client_attached(struct bt_vcp *vcp, void *user_data)
 {
 	struct vcp_data *data;
 	struct bt_att *att;
@@ -352,7 +352,8 @@  static int vcp_init(void)
 	if (err)
 		return err;
 
-	vcp_id = bt_vcp_register(vcp_attached, vcp_detached, NULL);
+	vcp_id = bt_vcp_register(vcp_remote_client_attached,
+					    vcp_remote_client_detached, NULL);
 
 	return 0;
 }
diff --git a/src/shared/vcp.c b/src/shared/vcp.c
index c92eb00d4..45d0f4485 100644
--- a/src/shared/vcp.c
+++ b/src/shared/vcp.c
@@ -379,7 +379,15 @@  static struct bt_aics *vcp_get_aics(struct bt_vcp *vcp)
 	return vcp->rdb->aics;
 }
 
-static void vcp_detached(void *data, void *user_data)
+static void vcp_remote_client_attached(void *data, void *user_data)
+{
+	struct bt_vcp_cb *cb = data;
+	struct bt_vcp *vcp = user_data;
+
+	cb->attached(vcp, cb->user_data);
+}
+
+static void vcp_remote_client_detached(void *data, void *user_data)
 {
 	struct bt_vcp_cb *cb = data;
 	struct bt_vcp *vcp = user_data;
@@ -389,7 +397,6 @@  static void vcp_detached(void *data, void *user_data)
 
 void bt_vcp_detach(struct bt_vcp *vcp)
 {
-	struct bt_vcs *vcs;
 	if (!queue_remove(sessions, vcp))
 		return;
 
@@ -399,7 +406,7 @@  void bt_vcp_detach(struct bt_vcp *vcp)
 	}
 
 	if (vcp->rdb) {
-		vcs = vcp_get_vcs(vcp);
+		struct bt_vcs *vcs = vcp_get_vcs(vcp);
 		vcs->service = NULL;
 		vcs->vs = NULL;
 		vcs->vs_ccc = NULL;
@@ -506,6 +513,7 @@  static void vcp_disconnected(int err, void *user_data)
 	DBG(vcp, "vcp %p disconnected err %d", vcp, err);
 
 	bt_vcp_detach(vcp);
+	queue_foreach(vcp_cbs, vcp_remote_client_detached, vcp);
 }
 
 static struct bt_vcp *vcp_get_session(struct bt_att *att, struct gatt_db *db)
@@ -524,6 +532,8 @@  static struct bt_vcp *vcp_get_session(struct bt_att *att, struct gatt_db *db)
 	vcp = bt_vcp_new(db, NULL);
 	vcp->att = att;
 
+	queue_foreach(vcp_cbs, vcp_remote_client_attached, vcp);
+
 	bt_att_register_disconnect(att, vcp_disconnected, vcp, NULL);
 
 	if (!sessions)
@@ -566,6 +576,10 @@  static uint8_t vcs_rel_vol_down(struct bt_vcs *vcs, struct bt_vcp *vcp,
 
 	vstate->vol_set = MAX((vstate->vol_set - VCP_STEP_SIZE), 0);
 	vstate->counter = -~vstate->counter; /*Increment Change Counter*/
+	vcp->volume = vstate->vol_set;
+
+	if (vcp->volume_changed)
+		vcp->volume_changed(vcp, vcp->volume / 2);
 
 	gatt_db_attribute_notify(vdb->vcs->vs, (void *)vstate,
 				 sizeof(struct vol_state),
@@ -605,6 +619,10 @@  static uint8_t vcs_rel_vol_up(struct bt_vcs *vcs, struct bt_vcp *vcp,
 
 	vstate->vol_set = MIN((vstate->vol_set + VCP_STEP_SIZE), 255);
 	vstate->counter = -~vstate->counter; /*Increment Change Counter*/
+	vcp->volume = vstate->vol_set;
+
+	if (vcp->volume_changed)
+		vcp->volume_changed(vcp, vcp->volume / 2);
 
 	gatt_db_attribute_notify(vdb->vcs->vs, (void *)vstate,
 				 sizeof(struct vol_state),
@@ -645,6 +663,10 @@  static uint8_t vcs_unmute_rel_vol_down(struct bt_vcs *vcs, struct bt_vcp *vcp,
 	vstate->mute = 0x00;
 	vstate->vol_set = MAX((vstate->vol_set - VCP_STEP_SIZE), 0);
 	vstate->counter = -~vstate->counter; /*Increment Change Counter*/
+	vcp->volume = vstate->vol_set;
+
+	if (vcp->volume_changed)
+		vcp->volume_changed(vcp, vcp->volume / 2);
 
 	gatt_db_attribute_notify(vdb->vcs->vs, (void *)vstate,
 				 sizeof(struct vol_state),
@@ -685,6 +707,10 @@  static uint8_t vcs_unmute_rel_vol_up(struct bt_vcs *vcs, struct bt_vcp *vcp,
 	vstate->mute = 0x00;
 	vstate->vol_set = MIN((vstate->vol_set + VCP_STEP_SIZE), 255);
 	vstate->counter = -~vstate->counter; /*Increment Change Counter*/
+	vcp->volume = vstate->vol_set;
+
+	if (vcp->volume_changed)
+		vcp->volume_changed(vcp, vcp->volume / 2);
 
 	gatt_db_attribute_notify(vdb->vcs->vs, (void *)vstate,
 				 sizeof(struct vol_state),
@@ -724,6 +750,10 @@  static uint8_t vcs_set_absolute_vol(struct bt_vcs *vcs, struct bt_vcp *vcp,
 
 	vstate->vol_set = req->vol_set;
 	vstate->counter = -~vstate->counter; /*Increment Change Counter*/
+	vcp->volume = vstate->vol_set;
+
+	if (vcp->volume_changed)
+		vcp->volume_changed(vcp, vcp->volume / 2);
 
 	gatt_db_attribute_notify(vdb->vcs->vs, (void *)vstate,
 				 sizeof(struct vol_state),
@@ -2023,34 +2053,59 @@  bool bt_vcp_set_volume(struct bt_vcp *vcp, int8_t volume)
 {
 	struct bt_vcs_client_ab_vol req;
 	uint16_t value_handle;
-	struct bt_vcs *vcs = vcp_get_vcs(vcp);
 
-	if (!vcs) {
-		DBG(vcp, "error: vcs not available");
-		return false;
-	}
+	vcp->volume = volume * 2;
 
-	if (!vcs->vol_cp) {
-		DBG(vcp, "error: vol_cp characteristics not available");
-		return false;
-	}
+	if (vcp->rdb) {
+		/* local gatt client */
+		struct bt_vcs *vcs = vcp_get_vcs(vcp);
 
-	if (!gatt_db_attribute_get_char_data(vcs->vol_cp, NULL, &value_handle,
-							NULL, NULL, NULL)) {
-		DBG(vcp, "error: vol_cp characteristics not available");
-		return false;
-	}
+		if (!vcs) {
+			DBG(vcp, "error: vcs not available");
+			return false;
+		}
 
-	vcp->volume = volume * 2;
-	req.op = BT_VCS_SET_ABSOLUTE_VOL;
-	req.vol_set = vcp->volume;
-	req.change_counter = vcp->volume_counter;
-
-	if (!bt_gatt_client_write_value(vcp->client, value_handle, (void *) &req,
-		sizeof(struct bt_vcs_client_ab_vol), vcp_volume_cp_sent, vcp,
-									NULL)) {
-		DBG(vcp, "error writing volume");
-		return false;
+		if (!vcs->vol_cp) {
+			DBG(vcp, "error: vol_cp characteristics not available");
+			return false;
+		}
+
+		if (!gatt_db_attribute_get_char_data(vcs->vol_cp, NULL,
+					&value_handle, NULL, NULL, NULL)) {
+			DBG(vcp, "error: vol_cp characteristics not available");
+			return false;
+		}
+
+		req.op = BT_VCS_SET_ABSOLUTE_VOL;
+		req.vol_set = vcp->volume;
+		req.change_counter = vcp->volume_counter;
+
+		if (!bt_gatt_client_write_value(vcp->client, value_handle,
+			(void *) &req, sizeof(struct bt_vcs_client_ab_vol),
+					vcp_volume_cp_sent, vcp, NULL)) {
+			DBG(vcp, "error writing volume");
+			return false;
+		}
+	} else {
+		// local gatt server
+		struct bt_vcp_db *vdb = vcp_get_vdb(vcp);
+		struct vol_state *vstate;
+
+		if (!vdb) {
+			DBG(vcp, "error: VDB not available");
+			return false;
+		}
+
+		vstate = vdb_get_vstate(vdb);
+		if (!vstate) {
+			DBG(vcp, "error: VSTATE not available");
+			return false;
+		}
+
+		vstate->vol_set = vcp->volume;
+		vstate->counter = -~vstate->counter; /*Increment Change Counter*/
+		gatt_db_attribute_notify(vdb->vcs->vs, (void *) vstate,
+				sizeof(struct vol_state), bt_vcp_get_att(vcp));
 	}
 	return true;
 }