bluez: Export SDP "Remote audio volume control" item for HSP profile

bluez: Export SDP "Remote audio volume control" item for HSP profile

[Pali Rohár]

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

Hello!

According to HSP 1.2 specification, section 4.7 Remote Audio Volume
Control, Support for remote audio volume control is optional, so an
implementation may support none, either, or both of the controls for
microphone volume and speaker volume.

According to HSP 1.2 specification, section 5.3 SDP Interoperability
Requirements, bluetooth device with HSP profile announce via SDP "Remote
audio volume control" field information if device itself supports volume
control.

But currently I did not found any way how to access "Remote audio volume
control" SDP field in (pulseaudio) application as bluez does not export
it.

Can you please export this field? E.g. for HFP profile all optional
features from SDP are passed to NewConnection() DBus method via
fd_properties dictionary under Features key. Could you export that
"Remote audio volume control" bit for HSP profile in Features key?

And in same way, this needs to be handled also in RegisterProfile() DBus
method.

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: bluez: Export SDP "Remote audio volume control" item for HSP profile

[Luiz Augusto von Dentz]

Hi Pali,

On Thu, Dec 12, 2019 at 7:31 PM Pali Rohár <pali.rohar@gmail.com> wrote:
>
> Hello!
>
> According to HSP 1.2 specification, section 4.7 Remote Audio Volume
> Control, Support for remote audio volume control is optional, so an
> implementation may support none, either, or both of the controls for
> microphone volume and speaker volume.
>
> According to HSP 1.2 specification, section 5.3 SDP Interoperability
> Requirements, bluetooth device with HSP profile announce via SDP "Remote
> audio volume control" field information if device itself supports volume
> control.
>
> But currently I did not found any way how to access "Remote audio volume
> control" SDP field in (pulseaudio) application as bluez does not export
> it.
>
> Can you please export this field? E.g. for HFP profile all optional
> features from SDP are passed to NewConnection() DBus method via
> fd_properties dictionary under Features key. Could you export that
> "Remote audio volume control" bit for HSP profile in Features key?
>
> And in same way, this needs to be handled also in RegisterProfile() DBus
> method.

Do you have a use case in mind?

-- 
Luiz Augusto von Dentz

Re: bluez: Export SDP "Remote audio volume control" item for HSP profile

[Pali Rohár]

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

On Thursday 12 December 2019 21:42:27 Luiz Augusto von Dentz wrote:
> Hi Pali,
> 
> On Thu, Dec 12, 2019 at 7:31 PM Pali Rohár <pali.rohar@gmail.com> wrote:
> >
> > Hello!
> >
> > According to HSP 1.2 specification, section 4.7 Remote Audio Volume
> > Control, Support for remote audio volume control is optional, so an
> > implementation may support none, either, or both of the controls for
> > microphone volume and speaker volume.
> >
> > According to HSP 1.2 specification, section 5.3 SDP Interoperability
> > Requirements, bluetooth device with HSP profile announce via SDP "Remote
> > audio volume control" field information if device itself supports volume
> > control.
> >
> > But currently I did not found any way how to access "Remote audio volume
> > control" SDP field in (pulseaudio) application as bluez does not export
> > it.
> >
> > Can you please export this field? E.g. for HFP profile all optional
> > features from SDP are passed to NewConnection() DBus method via
> > fd_properties dictionary under Features key. Could you export that
> > "Remote audio volume control" bit for HSP profile in Features key?
> >
> > And in same way, this needs to be handled also in RegisterProfile() DBus
> > method.
> 
> Do you have a use case in mind?

Yes, checking in pulseaudio if remote side announce that supports remote
volume control or not. And based on this switch to software volume
level.

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: bluez: Export SDP "Remote audio volume control" item for HSP profile

[Pali Rohár]

On Friday 13 December 2019 00:03:14 Pali Rohár wrote:
> On Thursday 12 December 2019 21:42:27 Luiz Augusto von Dentz wrote:
> > Hi Pali,
> > 
> > On Thu, Dec 12, 2019 at 7:31 PM Pali Rohár <pali.rohar@gmail.com> wrote:
> > >
> > > Hello!
> > >
> > > According to HSP 1.2 specification, section 4.7 Remote Audio Volume
> > > Control, Support for remote audio volume control is optional, so an
> > > implementation may support none, either, or both of the controls for
> > > microphone volume and speaker volume.
> > >
> > > According to HSP 1.2 specification, section 5.3 SDP Interoperability
> > > Requirements, bluetooth device with HSP profile announce via SDP "Remote
> > > audio volume control" field information if device itself supports volume
> > > control.
> > >
> > > But currently I did not found any way how to access "Remote audio volume
> > > control" SDP field in (pulseaudio) application as bluez does not export
> > > it.
> > >
> > > Can you please export this field? E.g. for HFP profile all optional
> > > features from SDP are passed to NewConnection() DBus method via
> > > fd_properties dictionary under Features key. Could you export that
> > > "Remote audio volume control" bit for HSP profile in Features key?
> > >
> > > And in same way, this needs to be handled also in RegisterProfile() DBus
> > > method.
> > 
> > Do you have a use case in mind?
> 
> Yes, checking in pulseaudio if remote side announce that supports remote
> volume control or not. And based on this switch to software volume
> level.

Hello Luiz! Is it is possible to export this field?

-- 
Pali Rohár
pali.rohar@gmail.com

Re: bluez: Export SDP "Remote audio volume control" item for HSP profile

[Pali Rohár]

On Sunday 09 February 2020 14:02:43 Pali Rohár wrote:
> On Friday 13 December 2019 00:03:14 Pali Rohár wrote:
> > On Thursday 12 December 2019 21:42:27 Luiz Augusto von Dentz wrote:
> > > Hi Pali,
> > > 
> > > On Thu, Dec 12, 2019 at 7:31 PM Pali Rohár <pali.rohar@gmail.com> wrote:
> > > >
> > > > Hello!
> > > >
> > > > According to HSP 1.2 specification, section 4.7 Remote Audio Volume
> > > > Control, Support for remote audio volume control is optional, so an
> > > > implementation may support none, either, or both of the controls for
> > > > microphone volume and speaker volume.
> > > >
> > > > According to HSP 1.2 specification, section 5.3 SDP Interoperability
> > > > Requirements, bluetooth device with HSP profile announce via SDP "Remote
> > > > audio volume control" field information if device itself supports volume
> > > > control.
> > > >
> > > > But currently I did not found any way how to access "Remote audio volume
> > > > control" SDP field in (pulseaudio) application as bluez does not export
> > > > it.
> > > >
> > > > Can you please export this field? E.g. for HFP profile all optional
> > > > features from SDP are passed to NewConnection() DBus method via
> > > > fd_properties dictionary under Features key. Could you export that
> > > > "Remote audio volume control" bit for HSP profile in Features key?
> > > >
> > > > And in same way, this needs to be handled also in RegisterProfile() DBus
> > > > method.
> > > 
> > > Do you have a use case in mind?
> > 
> > Yes, checking in pulseaudio if remote side announce that supports remote
> > volume control or not. And based on this switch to software volume
> > level.
> 
> Hello Luiz! Is it is possible to export this field?

Hello, I have not got any answer to my question.

So may I ask again how to retrieve SDP attribute 0x0302 "Remote audio
volume control" for a remote bluetooth headset with HSP profile?

It is really important as this attribute says if remote bluetooth
headset supports volume control or not. In case it does not support, we
need to switch to software volume control on host side.

There is open pulseaudio bug that on some headsets it is not possible to
control volume level and therefore pulseaudio needs to switch to
software volume control.

But without checking this SDP attribute 0x0302 this is not possible.

-- 
Pali Rohár
pali.rohar@gmail.com

Re: bluez: Export SDP "Remote audio volume control" item for HSP profile

[Luiz Augusto von Dentz]

Hi Pali,

On Fri, Mar 20, 2020 at 6:02 AM Pali Rohár <pali.rohar@gmail.com> wrote:
>
> On Sunday 09 February 2020 14:02:43 Pali Rohár wrote:
> > On Friday 13 December 2019 00:03:14 Pali Rohár wrote:
> > > On Thursday 12 December 2019 21:42:27 Luiz Augusto von Dentz wrote:
> > > > Hi Pali,
> > > >
> > > > On Thu, Dec 12, 2019 at 7:31 PM Pali Rohár <pali.rohar@gmail.com> wrote:
> > > > >
> > > > > Hello!
> > > > >
> > > > > According to HSP 1.2 specification, section 4.7 Remote Audio Volume
> > > > > Control, Support for remote audio volume control is optional, so an
> > > > > implementation may support none, either, or both of the controls for
> > > > > microphone volume and speaker volume.
> > > > >
> > > > > According to HSP 1.2 specification, section 5.3 SDP Interoperability
> > > > > Requirements, bluetooth device with HSP profile announce via SDP "Remote
> > > > > audio volume control" field information if device itself supports volume
> > > > > control.
> > > > >
> > > > > But currently I did not found any way how to access "Remote audio volume
> > > > > control" SDP field in (pulseaudio) application as bluez does not export
> > > > > it.
> > > > >
> > > > > Can you please export this field? E.g. for HFP profile all optional
> > > > > features from SDP are passed to NewConnection() DBus method via
> > > > > fd_properties dictionary under Features key. Could you export that
> > > > > "Remote audio volume control" bit for HSP profile in Features key?
> > > > >
> > > > > And in same way, this needs to be handled also in RegisterProfile() DBus
> > > > > method.
> > > >
> > > > Do you have a use case in mind?
> > >
> > > Yes, checking in pulseaudio if remote side announce that supports remote
> > > volume control or not. And based on this switch to software volume
> > > level.
> >
> > Hello Luiz! Is it is possible to export this field?
>
> Hello, I have not got any answer to my question.
>
> So may I ask again how to retrieve SDP attribute 0x0302 "Remote audio
> volume control" for a remote bluetooth headset with HSP profile?
>
> It is really important as this attribute says if remote bluetooth
> headset supports volume control or not. In case it does not support, we
> need to switch to software volume control on host side.
>
> There is open pulseaudio bug that on some headsets it is not possible to
> control volume level and therefore pulseaudio needs to switch to
> software volume control.
>
> But without checking this SDP attribute 0x0302 this is not possible.

Not sure when I will have time to work on this one, btw can't you
actually detect this via some AT command, or the headset simple don't
respond to the AT command for volume control?

-- 
Luiz Augusto von Dentz

Re: bluez: Export SDP "Remote audio volume control" item for HSP profile

[Pali Rohár]

On Friday 20 March 2020 10:48:02 Luiz Augusto von Dentz wrote:
> Hi Pali,
> 
> On Fri, Mar 20, 2020 at 6:02 AM Pali Rohár <pali.rohar@gmail.com> wrote:
> >
> > On Sunday 09 February 2020 14:02:43 Pali Rohár wrote:
> > > On Friday 13 December 2019 00:03:14 Pali Rohár wrote:
> > > > On Thursday 12 December 2019 21:42:27 Luiz Augusto von Dentz wrote:
> > > > > Hi Pali,
> > > > >
> > > > > On Thu, Dec 12, 2019 at 7:31 PM Pali Rohár <pali.rohar@gmail.com> wrote:
> > > > > >
> > > > > > Hello!
> > > > > >
> > > > > > According to HSP 1.2 specification, section 4.7 Remote Audio Volume
> > > > > > Control, Support for remote audio volume control is optional, so an
> > > > > > implementation may support none, either, or both of the controls for
> > > > > > microphone volume and speaker volume.
> > > > > >
> > > > > > According to HSP 1.2 specification, section 5.3 SDP Interoperability
> > > > > > Requirements, bluetooth device with HSP profile announce via SDP "Remote
> > > > > > audio volume control" field information if device itself supports volume
> > > > > > control.
> > > > > >
> > > > > > But currently I did not found any way how to access "Remote audio volume
> > > > > > control" SDP field in (pulseaudio) application as bluez does not export
> > > > > > it.
> > > > > >
> > > > > > Can you please export this field? E.g. for HFP profile all optional
> > > > > > features from SDP are passed to NewConnection() DBus method via
> > > > > > fd_properties dictionary under Features key. Could you export that
> > > > > > "Remote audio volume control" bit for HSP profile in Features key?
> > > > > >
> > > > > > And in same way, this needs to be handled also in RegisterProfile() DBus
> > > > > > method.
> > > > >
> > > > > Do you have a use case in mind?
> > > >
> > > > Yes, checking in pulseaudio if remote side announce that supports remote
> > > > volume control or not. And based on this switch to software volume
> > > > level.
> > >
> > > Hello Luiz! Is it is possible to export this field?
> >
> > Hello, I have not got any answer to my question.
> >
> > So may I ask again how to retrieve SDP attribute 0x0302 "Remote audio
> > volume control" for a remote bluetooth headset with HSP profile?
> >
> > It is really important as this attribute says if remote bluetooth
> > headset supports volume control or not. In case it does not support, we
> > need to switch to software volume control on host side.
> >
> > There is open pulseaudio bug that on some headsets it is not possible to
> > control volume level and therefore pulseaudio needs to switch to
> > software volume control.
> >
> > But without checking this SDP attribute 0x0302 this is not possible.
> 
> Not sure when I will have time to work on this one, btw can't you
> actually detect this via some AT command, or the headset simple don't
> respond to the AT command for volume control?

Hello Luiz! I'm planing to add some "workaround" by detecting what
happen when VGS or VGM commands are sent. But this is just a workaround
and I would like to know how would proper solution would like.

Does it mean that currently it is really not possible to retrieve SDP
attributes of remote device? And we need to wait until bluez daemon
exports it via DBus API (fd_properties dictionary under Features key)?

-- 
Pali Rohár
pali.rohar@gmail.com

Re: bluez: Export SDP "Remote audio volume control" item for HSP profile

[Pali Rohár]

On Saturday 21 March 2020 19:40:15 Pali Rohár wrote:
> On Friday 20 March 2020 10:48:02 Luiz Augusto von Dentz wrote:
> > Hi Pali,
> > 
> > On Fri, Mar 20, 2020 at 6:02 AM Pali Rohár <pali.rohar@gmail.com> wrote:
> > >
> > > On Sunday 09 February 2020 14:02:43 Pali Rohár wrote:
> > > > On Friday 13 December 2019 00:03:14 Pali Rohár wrote:
> > > > > On Thursday 12 December 2019 21:42:27 Luiz Augusto von Dentz wrote:
> > > > > > Hi Pali,
> > > > > >
> > > > > > On Thu, Dec 12, 2019 at 7:31 PM Pali Rohár <pali.rohar@gmail.com> wrote:
> > > > > > >
> > > > > > > Hello!
> > > > > > >
> > > > > > > According to HSP 1.2 specification, section 4.7 Remote Audio Volume
> > > > > > > Control, Support for remote audio volume control is optional, so an
> > > > > > > implementation may support none, either, or both of the controls for
> > > > > > > microphone volume and speaker volume.
> > > > > > >
> > > > > > > According to HSP 1.2 specification, section 5.3 SDP Interoperability
> > > > > > > Requirements, bluetooth device with HSP profile announce via SDP "Remote
> > > > > > > audio volume control" field information if device itself supports volume
> > > > > > > control.
> > > > > > >
> > > > > > > But currently I did not found any way how to access "Remote audio volume
> > > > > > > control" SDP field in (pulseaudio) application as bluez does not export
> > > > > > > it.
> > > > > > >
> > > > > > > Can you please export this field? E.g. for HFP profile all optional
> > > > > > > features from SDP are passed to NewConnection() DBus method via
> > > > > > > fd_properties dictionary under Features key. Could you export that
> > > > > > > "Remote audio volume control" bit for HSP profile in Features key?
> > > > > > >
> > > > > > > And in same way, this needs to be handled also in RegisterProfile() DBus
> > > > > > > method.
> > > > > >
> > > > > > Do you have a use case in mind?
> > > > >
> > > > > Yes, checking in pulseaudio if remote side announce that supports remote
> > > > > volume control or not. And based on this switch to software volume
> > > > > level.
> > > >
> > > > Hello Luiz! Is it is possible to export this field?
> > >
> > > Hello, I have not got any answer to my question.
> > >
> > > So may I ask again how to retrieve SDP attribute 0x0302 "Remote audio
> > > volume control" for a remote bluetooth headset with HSP profile?
> > >
> > > It is really important as this attribute says if remote bluetooth
> > > headset supports volume control or not. In case it does not support, we
> > > need to switch to software volume control on host side.
> > >
> > > There is open pulseaudio bug that on some headsets it is not possible to
> > > control volume level and therefore pulseaudio needs to switch to
> > > software volume control.
> > >
> > > But without checking this SDP attribute 0x0302 this is not possible.
> > 
> > Not sure when I will have time to work on this one, btw can't you
> > actually detect this via some AT command, or the headset simple don't
> > respond to the AT command for volume control?

Hello Luiz! Headsets really do not respond to VGS and VGM command. Well
in direction from computer to headset it is not an AT command but
Unsolicited result code for which there is no reply.

So it is not possible to detect it as there is no AT command for it.

AT commands are sent in opposite direction, from headset to computer.
But in this case we want to change volume from computer to headset and
for it there is only "Unsolicited result code" which is without
response.

> Hello Luiz! I'm planing to add some "workaround" by detecting what
> happen when VGS or VGM commands are sent. But this is just a workaround
> and I would like to know how would proper solution would like.
> 
> Does it mean that currently it is really not possible to retrieve SDP
> attributes of remote device? And we need to wait until bluez daemon
> exports it via DBus API (fd_properties dictionary under Features key)?
> 

-- 
Pali Rohár
pali.rohar@gmail.com

[PATCH 0/3] bluez: Export SDP "Remote audio volume control" item for HSP profile

[Pali Rohár]

This patch series fixes handling of zero value in feature list and
provides Remote audio volume control support for HSP profile in both HS
and AG roles.

Luiz, you wrote that you do not have time to work on this, so I
implemented it myself in this patch series. Could you please find time
at least for reviewing and merging these patches? Thanks.

Pali Rohár (3):
  src/profile: Distinguish between zero-set HFP AG features and unset
    HFP AG features
  src/profile: Export Remote Audio Volume Control SDP value for HSP HS
    role via first bit in features value
  src/profile: Add default SDP record for Headset role of HSP 1.2
    profile with Erratum 3507

 src/profile.c | 112 ++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 104 insertions(+), 8 deletions(-)

-- 
2.20.1

[PATCH 1/3] src/profile: Distinguish between zero-set HFP AG features and unset HFP AG features

[Pali Rohár]

When HFP AG features are not set then according to HFP 1.7.2 specification
it has value 0b001001. But when HFP AG features were explicitly set to
zero, bluez did not propagated this value via NewConnection() DBus call, so
callee used default value 0b001001 (according to HFP 1.7.2 specification)
as bluez did not provided explicit zero value.

To fix this problem add a new boolean variable have_features which
indicates if SDP features were provided (with possible zero value) or not
(when default value needs to be used). Code for generating SDP XML records
were also updated to handle this fact.

Signed-off-by: Pali Rohár <pali@kernel.org>
---
 src/profile.c | 32 ++++++++++++++++++++++++--------
 1 file changed, 24 insertions(+), 8 deletions(-)

diff --git a/src/profile.c b/src/profile.c
index 192fd0245..884440408 100644
--- a/src/profile.c
+++ b/src/profile.c
@@ -647,6 +647,7 @@ struct ext_profile {
 
 	uint16_t version;
 	uint16_t features;
+	bool have_features;
 
 	GSList *records;
 	GSList *servers;
@@ -669,6 +670,7 @@ struct ext_io {
 
 	uint16_t version;
 	uint16_t features;
+	bool have_features;
 
 	uint16_t psm;
 	uint8_t chan;
@@ -920,14 +922,18 @@ static void append_prop(gpointer a, gpointer b)
 	dbus_message_iter_close_container(dict, &entry);
 }
 
-static uint16_t get_supported_features(const sdp_record_t *rec)
+static uint16_t get_supported_features(const sdp_record_t *rec,
+					bool *have_features)
 {
 	sdp_data_t *data;
 
 	data = sdp_data_get(rec, SDP_ATTR_SUPPORTED_FEATURES);
-	if (!data || data->dtd != SDP_UINT16)
+	if (!data || data->dtd != SDP_UINT16) {
+		*have_features = false;
 		return 0;
+	}
 
+	*have_features = true;
 	return data->val.uint16;
 }
 
@@ -972,7 +978,8 @@ static bool send_new_connection(struct ext_profile *ext, struct ext_io *conn)
 	if (remote_uuid) {
 		rec = btd_device_get_record(conn->device, remote_uuid);
 		if (rec) {
-			conn->features = get_supported_features(rec);
+			conn->features = get_supported_features(rec,
+							&conn->have_features);
 			conn->version = get_profile_version(rec);
 		}
 	}
@@ -991,7 +998,7 @@ static bool send_new_connection(struct ext_profile *ext, struct ext_io *conn)
 		dict_append_entry(&dict, "Version", DBUS_TYPE_UINT16,
 							&conn->version);
 
-	if (conn->features)
+	if (conn->have_features)
 		dict_append_entry(&dict, "Features", DBUS_TYPE_UINT16,
 							&conn->features);
 
@@ -1589,7 +1596,8 @@ static void record_cb(sdp_list_t *recs, int err, gpointer user_data)
 		if (conn->psm == 0 && sdp_get_proto_desc(protos, OBEX_UUID))
 			conn->psm = get_goep_l2cap_psm(rec);
 
-		conn->features = get_supported_features(rec);
+		conn->features = get_supported_features(rec,
+							&conn->have_features);
 		conn->version = get_profile_version(rec);
 
 		sdp_list_foreach(protos, (sdp_list_func_t) sdp_list_free,
@@ -1750,15 +1758,19 @@ static int ext_disconnect_dev(struct btd_service *service)
 static char *get_hfp_hf_record(struct ext_profile *ext, struct ext_io *l2cap,
 							struct ext_io *rfcomm)
 {
+	/* HFP 1.7.2: By default features bitfield is 0b000000 */
 	return g_strdup_printf(HFP_HF_RECORD, rfcomm->chan, ext->version,
-						ext->name, ext->features);
+				ext->name,
+				ext->have_features ? ext->features : 0x0);
 }
 
 static char *get_hfp_ag_record(struct ext_profile *ext, struct ext_io *l2cap,
 							struct ext_io *rfcomm)
 {
+	/* HFP 1.7.2: By default features bitfield is 0b001001 */
 	return g_strdup_printf(HFP_AG_RECORD, rfcomm->chan, ext->version,
-						ext->name, ext->features);
+				ext->name,
+				ext->have_features ? ext->features : 0x9);
 }
 
 static char *get_hsp_ag_record(struct ext_profile *ext, struct ext_io *l2cap,
@@ -1948,6 +1960,7 @@ static struct default_settings {
 					struct ext_io *rfcomm);
 	uint16_t	version;
 	uint16_t	features;
+	bool		have_features;
 } defaults[] = {
 	{
 		.uuid		= SPP_UUID,
@@ -2107,8 +2120,10 @@ static void ext_set_defaults(struct ext_profile *ext)
 		if (settings->version)
 			ext->version = settings->version;
 
-		if (settings->features)
+		if (settings->have_features) {
 			ext->features = settings->features;
+			ext->have_features = true;
+		}
 
 		if (settings->name)
 			ext->name = g_strdup(settings->name);
@@ -2198,6 +2213,7 @@ static int parse_ext_opt(struct ext_profile *ext, const char *key,
 
 		dbus_message_iter_get_basic(value, &feat);
 		ext->features = feat;
+		ext->have_features = true;
 	} else if (strcasecmp(key, "Service") == 0) {
 		if (type != DBUS_TYPE_STRING)
 			return -EINVAL;
-- 
2.20.1

Re: [PATCH 1/3] src/profile: Distinguish between zero-set HFP AG features and unset HFP AG features

[Luiz Augusto von Dentz]

Hi Pali,

On Mon, Apr 13, 2020 at 9:25 AM Pali Rohár <pali@kernel.org> wrote:
>
> When HFP AG features are not set then according to HFP 1.7.2 specification
> it has value 0b001001. But when HFP AG features were explicitly set to
> zero, bluez did not propagated this value via NewConnection() DBus call, so
> callee used default value 0b001001 (according to HFP 1.7.2 specification)
> as bluez did not provided explicit zero value.
>
> To fix this problem add a new boolean variable have_features which
> indicates if SDP features were provided (with possible zero value) or not
> (when default value needs to be used). Code for generating SDP XML records
> were also updated to handle this fact.
>
> Signed-off-by: Pali Rohár <pali@kernel.org>
> ---
>  src/profile.c | 32 ++++++++++++++++++++++++--------
>  1 file changed, 24 insertions(+), 8 deletions(-)
>
> diff --git a/src/profile.c b/src/profile.c
> index 192fd0245..884440408 100644
> --- a/src/profile.c
> +++ b/src/profile.c
> @@ -647,6 +647,7 @@ struct ext_profile {
>
>         uint16_t version;
>         uint16_t features;
> +       bool have_features;
>
>         GSList *records;
>         GSList *servers;
> @@ -669,6 +670,7 @@ struct ext_io {
>
>         uint16_t version;
>         uint16_t features;
> +       bool have_features;
>
>         uint16_t psm;
>         uint8_t chan;
> @@ -920,14 +922,18 @@ static void append_prop(gpointer a, gpointer b)
>         dbus_message_iter_close_container(dict, &entry);
>  }
>
> -static uint16_t get_supported_features(const sdp_record_t *rec)
> +static uint16_t get_supported_features(const sdp_record_t *rec,
> +                                       bool *have_features)
>  {
>         sdp_data_t *data;
>
>         data = sdp_data_get(rec, SDP_ATTR_SUPPORTED_FEATURES);
> -       if (!data || data->dtd != SDP_UINT16)
> +       if (!data || data->dtd != SDP_UINT16) {
> +               *have_features = false;
>                 return 0;
> +       }
>
> +       *have_features = true;
>         return data->val.uint16;
>  }
>
> @@ -972,7 +978,8 @@ static bool send_new_connection(struct ext_profile *ext, struct ext_io *conn)
>         if (remote_uuid) {
>                 rec = btd_device_get_record(conn->device, remote_uuid);
>                 if (rec) {
> -                       conn->features = get_supported_features(rec);
> +                       conn->features = get_supported_features(rec,
> +                                                       &conn->have_features);
>                         conn->version = get_profile_version(rec);
>                 }
>         }
> @@ -991,7 +998,7 @@ static bool send_new_connection(struct ext_profile *ext, struct ext_io *conn)
>                 dict_append_entry(&dict, "Version", DBUS_TYPE_UINT16,
>                                                         &conn->version);
>
> -       if (conn->features)
> +       if (conn->have_features)
>                 dict_append_entry(&dict, "Features", DBUS_TYPE_UINT16,
>                                                         &conn->features);
>
> @@ -1589,7 +1596,8 @@ static void record_cb(sdp_list_t *recs, int err, gpointer user_data)
>                 if (conn->psm == 0 && sdp_get_proto_desc(protos, OBEX_UUID))
>                         conn->psm = get_goep_l2cap_psm(rec);
>
> -               conn->features = get_supported_features(rec);
> +               conn->features = get_supported_features(rec,
> +                                                       &conn->have_features);
>                 conn->version = get_profile_version(rec);
>
>                 sdp_list_foreach(protos, (sdp_list_func_t) sdp_list_free,
> @@ -1750,15 +1758,19 @@ static int ext_disconnect_dev(struct btd_service *service)
>  static char *get_hfp_hf_record(struct ext_profile *ext, struct ext_io *l2cap,
>                                                         struct ext_io *rfcomm)
>  {
> +       /* HFP 1.7.2: By default features bitfield is 0b000000 */
>         return g_strdup_printf(HFP_HF_RECORD, rfcomm->chan, ext->version,
> -                                               ext->name, ext->features);
> +                               ext->name,
> +                               ext->have_features ? ext->features : 0x0);
>  }
>
>  static char *get_hfp_ag_record(struct ext_profile *ext, struct ext_io *l2cap,
>                                                         struct ext_io *rfcomm)
>  {
> +       /* HFP 1.7.2: By default features bitfield is 0b001001 */
>         return g_strdup_printf(HFP_AG_RECORD, rfcomm->chan, ext->version,
> -                                               ext->name, ext->features);
> +                               ext->name,
> +                               ext->have_features ? ext->features : 0x9);

I wonder why you didn't just initialize the features wiht 0x9 instead
of adding a flag to track it, btw add a define with value 0x09 like
HFP_DEFAULT_FEATURES that way it is clearer why we are doing this.

>  }
>
>  static char *get_hsp_ag_record(struct ext_profile *ext, struct ext_io *l2cap,
> @@ -1948,6 +1960,7 @@ static struct default_settings {
>                                         struct ext_io *rfcomm);
>         uint16_t        version;
>         uint16_t        features;
> +       bool            have_features;
>  } defaults[] = {
>         {
>                 .uuid           = SPP_UUID,
> @@ -2107,8 +2120,10 @@ static void ext_set_defaults(struct ext_profile *ext)
>                 if (settings->version)
>                         ext->version = settings->version;
>
> -               if (settings->features)
> +               if (settings->have_features) {
>                         ext->features = settings->features;
> +                       ext->have_features = true;
> +               }
>
>                 if (settings->name)
>                         ext->name = g_strdup(settings->name);
> @@ -2198,6 +2213,7 @@ static int parse_ext_opt(struct ext_profile *ext, const char *key,
>
>                 dbus_message_iter_get_basic(value, &feat);
>                 ext->features = feat;
> +               ext->have_features = true;
>         } else if (strcasecmp(key, "Service") == 0) {
>                 if (type != DBUS_TYPE_STRING)
>                         return -EINVAL;
> --
> 2.20.1
>


-- 
Luiz Augusto von Dentz

Re: [PATCH 1/3] src/profile: Distinguish between zero-set HFP AG features and unset HFP AG features

[Pali Rohár]

On Monday 13 April 2020 09:44:14 Luiz Augusto von Dentz wrote:
> On Mon, Apr 13, 2020 at 9:25 AM Pali Rohár <pali@kernel.org> wrote:
> > @@ -1750,15 +1758,19 @@ static int ext_disconnect_dev(struct btd_service *service)
> >  static char *get_hfp_hf_record(struct ext_profile *ext, struct ext_io *l2cap,
> >                                                         struct ext_io *rfcomm)
> >  {
> > +       /* HFP 1.7.2: By default features bitfield is 0b000000 */
> >         return g_strdup_printf(HFP_HF_RECORD, rfcomm->chan, ext->version,
> > -                                               ext->name, ext->features);
> > +                               ext->name,
> > +                               ext->have_features ? ext->features : 0x0);
> >  }
> >
> >  static char *get_hfp_ag_record(struct ext_profile *ext, struct ext_io *l2cap,
> >                                                         struct ext_io *rfcomm)
> >  {
> > +       /* HFP 1.7.2: By default features bitfield is 0b001001 */
> >         return g_strdup_printf(HFP_AG_RECORD, rfcomm->chan, ext->version,
> > -                                               ext->name, ext->features);
> > +                               ext->name,
> > +                               ext->have_features ? ext->features : 0x9);
> 
> I wonder why you didn't just initialize the features wiht 0x9 instead
> of adding a flag to track it, btw add a define with value 0x09 like
> HFP_DEFAULT_FEATURES that way it is clearer why we are doing this.

This function get_hfp_ag_record() is for parsing local features. You are
right that for local features we do not need a flag to track it.

But flag for tracking is needed for parsing remote features. And to have
unified code for storing local and remote features it is easier to have
always a flag for checking if features were provided or not.

I can put these default values in profile-role specific macros e.g.:

#define HFP_AG_DEFAULT_FEATURES 0x09
#define HFP_HF_DEFAULT_FEATURES 0x00
#define HSP_HS_DEFAULT_VOLCNTRL "false"

Or do you prefer different names?

Re: [PATCH 1/3] src/profile: Distinguish between zero-set HFP AG features and unset HFP AG features

[Luiz Augusto von Dentz]

Hi Pali,

On Mon, Apr 13, 2020 at 9:52 AM Pali Rohár <pali@kernel.org> wrote:
>
> On Monday 13 April 2020 09:44:14 Luiz Augusto von Dentz wrote:
> > On Mon, Apr 13, 2020 at 9:25 AM Pali Rohár <pali@kernel.org> wrote:
> > > @@ -1750,15 +1758,19 @@ static int ext_disconnect_dev(struct btd_service *service)
> > >  static char *get_hfp_hf_record(struct ext_profile *ext, struct ext_io *l2cap,
> > >                                                         struct ext_io *rfcomm)
> > >  {
> > > +       /* HFP 1.7.2: By default features bitfield is 0b000000 */
> > >         return g_strdup_printf(HFP_HF_RECORD, rfcomm->chan, ext->version,
> > > -                                               ext->name, ext->features);
> > > +                               ext->name,
> > > +                               ext->have_features ? ext->features : 0x0);
> > >  }
> > >
> > >  static char *get_hfp_ag_record(struct ext_profile *ext, struct ext_io *l2cap,
> > >                                                         struct ext_io *rfcomm)
> > >  {
> > > +       /* HFP 1.7.2: By default features bitfield is 0b001001 */
> > >         return g_strdup_printf(HFP_AG_RECORD, rfcomm->chan, ext->version,
> > > -                                               ext->name, ext->features);
> > > +                               ext->name,
> > > +                               ext->have_features ? ext->features : 0x9);
> >
> > I wonder why you didn't just initialize the features wiht 0x9 instead
> > of adding a flag to track it, btw add a define with value 0x09 like
> > HFP_DEFAULT_FEATURES that way it is clearer why we are doing this.
>
> This function get_hfp_ag_record() is for parsing local features. You are
> right that for local features we do not need a flag to track it.
>
> But flag for tracking is needed for parsing remote features. And to have
> unified code for storing local and remote features it is easier to have
> always a flag for checking if features were provided or not.

Im not following you about the remote features beinf different, I
though both would be could be initialized in the same way and then if
we read a different value from the SDP record we just overwrite it.

> I can put these default values in profile-role specific macros e.g.:
>
> #define HFP_AG_DEFAULT_FEATURES 0x09
> #define HFP_HF_DEFAULT_FEATURES 0x00
> #define HSP_HS_DEFAULT_VOLCNTRL "false"

Don't bother with default that are 0x00/false, we can assume this is
implicit when allocating the memory everything is set to 0 so these
defines wouldn't be needed in the first place.

> Or do you prefer different names?



-- 
Luiz Augusto von Dentz

Re: [PATCH 1/3] src/profile: Distinguish between zero-set HFP AG features and unset HFP AG features

[Pali Rohár]

On Monday 13 April 2020 09:58:34 Luiz Augusto von Dentz wrote:
> Hi Pali,
> 
> On Mon, Apr 13, 2020 at 9:52 AM Pali Rohár <pali@kernel.org> wrote:
> >
> > On Monday 13 April 2020 09:44:14 Luiz Augusto von Dentz wrote:
> > > On Mon, Apr 13, 2020 at 9:25 AM Pali Rohár <pali@kernel.org> wrote:
> > > > @@ -1750,15 +1758,19 @@ static int ext_disconnect_dev(struct btd_service *service)
> > > >  static char *get_hfp_hf_record(struct ext_profile *ext, struct ext_io *l2cap,
> > > >                                                         struct ext_io *rfcomm)
> > > >  {
> > > > +       /* HFP 1.7.2: By default features bitfield is 0b000000 */
> > > >         return g_strdup_printf(HFP_HF_RECORD, rfcomm->chan, ext->version,
> > > > -                                               ext->name, ext->features);
> > > > +                               ext->name,
> > > > +                               ext->have_features ? ext->features : 0x0);
> > > >  }
> > > >
> > > >  static char *get_hfp_ag_record(struct ext_profile *ext, struct ext_io *l2cap,
> > > >                                                         struct ext_io *rfcomm)
> > > >  {
> > > > +       /* HFP 1.7.2: By default features bitfield is 0b001001 */
> > > >         return g_strdup_printf(HFP_AG_RECORD, rfcomm->chan, ext->version,
> > > > -                                               ext->name, ext->features);
> > > > +                               ext->name,
> > > > +                               ext->have_features ? ext->features : 0x9);
> > >
> > > I wonder why you didn't just initialize the features wiht 0x9 instead
> > > of adding a flag to track it, btw add a define with value 0x09 like
> > > HFP_DEFAULT_FEATURES that way it is clearer why we are doing this.
> >
> > This function get_hfp_ag_record() is for parsing local features. You are
> > right that for local features we do not need a flag to track it.
> >
> > But flag for tracking is needed for parsing remote features. And to have
> > unified code for storing local and remote features it is easier to have
> > always a flag for checking if features were provided or not.
> 
> Im not following you about the remote features beinf different, I
> though both would be could be initialized in the same way and then if
> we read a different value from the SDP record we just overwrite it.

But in this case we put these default remote features to HFP DBus agent
like if they were specified in SDP. Default value is specific for
profile version. And if e.g. new HFP version defines a new bit in
features with its own default value then HFP DBus agent would not be
able to distinguish between state when remote device did not specified
any value (as bluez will put there some defaults) and when remote device
specified same features as bluez has defined in its current default
value.

Before and also after this change remote features are not send to HFP
DBus agent when they were not specified by remote device.

After your suggestion HFP DBus agent would not be able to check if
remote device provided features or not. And this is I think a
regression.

> > I can put these default values in profile-role specific macros e.g.:
> >
> > #define HFP_AG_DEFAULT_FEATURES 0x09
> > #define HFP_HF_DEFAULT_FEATURES 0x00
> > #define HSP_HS_DEFAULT_VOLCNTRL "false"
> 
> Don't bother with default that are 0x00/false, we can assume this is
> implicit when allocating the memory everything is set to 0 so these
> defines wouldn't be needed in the first place.
> 
> > Or do you prefer different names?
> 
> 
> 
> -- 
> Luiz Augusto von Dentz

Re: [PATCH 1/3] src/profile: Distinguish between zero-set HFP AG features and unset HFP AG features

[Luiz Augusto von Dentz]

Hi Pali,

On Mon, Apr 13, 2020 at 10:17 AM Pali Rohár <pali@kernel.org> wrote:
>
> On Monday 13 April 2020 09:58:34 Luiz Augusto von Dentz wrote:
> > Hi Pali,
> >
> > On Mon, Apr 13, 2020 at 9:52 AM Pali Rohár <pali@kernel.org> wrote:
> > >
> > > On Monday 13 April 2020 09:44:14 Luiz Augusto von Dentz wrote:
> > > > On Mon, Apr 13, 2020 at 9:25 AM Pali Rohár <pali@kernel.org> wrote:
> > > > > @@ -1750,15 +1758,19 @@ static int ext_disconnect_dev(struct btd_service *service)
> > > > >  static char *get_hfp_hf_record(struct ext_profile *ext, struct ext_io *l2cap,
> > > > >                                                         struct ext_io *rfcomm)
> > > > >  {
> > > > > +       /* HFP 1.7.2: By default features bitfield is 0b000000 */
> > > > >         return g_strdup_printf(HFP_HF_RECORD, rfcomm->chan, ext->version,
> > > > > -                                               ext->name, ext->features);
> > > > > +                               ext->name,
> > > > > +                               ext->have_features ? ext->features : 0x0);
> > > > >  }
> > > > >
> > > > >  static char *get_hfp_ag_record(struct ext_profile *ext, struct ext_io *l2cap,
> > > > >                                                         struct ext_io *rfcomm)
> > > > >  {
> > > > > +       /* HFP 1.7.2: By default features bitfield is 0b001001 */
> > > > >         return g_strdup_printf(HFP_AG_RECORD, rfcomm->chan, ext->version,
> > > > > -                                               ext->name, ext->features);
> > > > > +                               ext->name,
> > > > > +                               ext->have_features ? ext->features : 0x9);
> > > >
> > > > I wonder why you didn't just initialize the features wiht 0x9 instead
> > > > of adding a flag to track it, btw add a define with value 0x09 like
> > > > HFP_DEFAULT_FEATURES that way it is clearer why we are doing this.
> > >
> > > This function get_hfp_ag_record() is for parsing local features. You are
> > > right that for local features we do not need a flag to track it.
> > >
> > > But flag for tracking is needed for parsing remote features. And to have
> > > unified code for storing local and remote features it is easier to have
> > > always a flag for checking if features were provided or not.
> >
> > Im not following you about the remote features beinf different, I
> > though both would be could be initialized in the same way and then if
> > we read a different value from the SDP record we just overwrite it.
>
> But in this case we put these default remote features to HFP DBus agent
> like if they were specified in SDP. Default value is specific for
> profile version. And if e.g. new HFP version defines a new bit in
> features with its own default value then HFP DBus agent would not be
> able to distinguish between state when remote device did not specified
> any value (as bluez will put there some defaults) and when remote device
> specified same features as bluez has defined in its current default
> value.

Different version may have different defaults but we can initialize
them correctly.

> Before and also after this change remote features are not send to HFP
> DBus agent when they were not specified by remote device.
>
> After your suggestion HFP DBus agent would not be able to check if
> remote device provided features or not. And this is I think a
> regression.

I don't think we can consider a regression to always provide features,
besides what would be the reason for the agent to know if the features
were from the SDP record or they are auto initiliazed? I that was
really necessary Id just dump the whole record as a ServiceRecord
option and leave it up to the client to parse it.

> > > I can put these default values in profile-role specific macros e.g.:
> > >
> > > #define HFP_AG_DEFAULT_FEATURES 0x09
> > > #define HFP_HF_DEFAULT_FEATURES 0x00
> > > #define HSP_HS_DEFAULT_VOLCNTRL "false"
> >
> > Don't bother with default that are 0x00/false, we can assume this is
> > implicit when allocating the memory everything is set to 0 so these
> > defines wouldn't be needed in the first place.
> >
> > > Or do you prefer different names?
> >
> >
> >
> > --
> > Luiz Augusto von Dentz



-- 
Luiz Augusto von Dentz

Re: [PATCH 1/3] src/profile: Distinguish between zero-set HFP AG features and unset HFP AG features

[Pali Rohár]

On Monday 13 April 2020 10:47:04 Luiz Augusto von Dentz wrote:
> Hi Pali,
> 
> On Mon, Apr 13, 2020 at 10:17 AM Pali Rohár <pali@kernel.org> wrote:
> >
> > On Monday 13 April 2020 09:58:34 Luiz Augusto von Dentz wrote:
> > > Hi Pali,
> > >
> > > On Mon, Apr 13, 2020 at 9:52 AM Pali Rohár <pali@kernel.org> wrote:
> > > >
> > > > On Monday 13 April 2020 09:44:14 Luiz Augusto von Dentz wrote:
> > > > > On Mon, Apr 13, 2020 at 9:25 AM Pali Rohár <pali@kernel.org> wrote:
> > > > > > @@ -1750,15 +1758,19 @@ static int ext_disconnect_dev(struct btd_service *service)
> > > > > >  static char *get_hfp_hf_record(struct ext_profile *ext, struct ext_io *l2cap,
> > > > > >                                                         struct ext_io *rfcomm)
> > > > > >  {
> > > > > > +       /* HFP 1.7.2: By default features bitfield is 0b000000 */
> > > > > >         return g_strdup_printf(HFP_HF_RECORD, rfcomm->chan, ext->version,
> > > > > > -                                               ext->name, ext->features);
> > > > > > +                               ext->name,
> > > > > > +                               ext->have_features ? ext->features : 0x0);
> > > > > >  }
> > > > > >
> > > > > >  static char *get_hfp_ag_record(struct ext_profile *ext, struct ext_io *l2cap,
> > > > > >                                                         struct ext_io *rfcomm)
> > > > > >  {
> > > > > > +       /* HFP 1.7.2: By default features bitfield is 0b001001 */
> > > > > >         return g_strdup_printf(HFP_AG_RECORD, rfcomm->chan, ext->version,
> > > > > > -                                               ext->name, ext->features);
> > > > > > +                               ext->name,
> > > > > > +                               ext->have_features ? ext->features : 0x9);
> > > > >
> > > > > I wonder why you didn't just initialize the features wiht 0x9 instead
> > > > > of adding a flag to track it, btw add a define with value 0x09 like
> > > > > HFP_DEFAULT_FEATURES that way it is clearer why we are doing this.
> > > >
> > > > This function get_hfp_ag_record() is for parsing local features. You are
> > > > right that for local features we do not need a flag to track it.
> > > >
> > > > But flag for tracking is needed for parsing remote features. And to have
> > > > unified code for storing local and remote features it is easier to have
> > > > always a flag for checking if features were provided or not.
> > >
> > > Im not following you about the remote features beinf different, I
> > > though both would be could be initialized in the same way and then if
> > > we read a different value from the SDP record we just overwrite it.
> >
> > But in this case we put these default remote features to HFP DBus agent
> > like if they were specified in SDP. Default value is specific for
> > profile version. And if e.g. new HFP version defines a new bit in
> > features with its own default value then HFP DBus agent would not be
> > able to distinguish between state when remote device did not specified
> > any value (as bluez will put there some defaults) and when remote device
> > specified same features as bluez has defined in its current default
> > value.
> 
> Different version may have different defaults but we can initialize
> them correctly.

But we do not know default values for future versions which have not
been released yet. There would be a problem if user run current
bluez daemon e.g. in 5 years when new version of HFP profile would be
released and remote device would use it.

> > Before and also after this change remote features are not send to HFP
> > DBus agent when they were not specified by remote device.
> >
> > After your suggestion HFP DBus agent would not be able to check if
> > remote device provided features or not. And this is I think a
> > regression.
> 
> I don't think we can consider a regression to always provide features,
> besides what would be the reason for the agent to know if the features
> were from the SDP record or they are auto initiliazed?

Autoinitialized for which version? Bluez has currently defined default
values for HFP 1.7. But DBus agent can register HFP profile also for
version 1.5 or also 1.8.

To which value should it be autoinitialized when remote device announce
that is in version 1.8 but does not specify features (= default for
version 1.8 should be used)? The only option which make sense is to not
autoinitiaze this value as version 1.8 was not released yet and now we
do not know what would be the default value.

> I that was
> really necessary Id just dump the whole record as a ServiceRecord
> option and leave it up to the client to parse it.
> 
> > > > I can put these default values in profile-role specific macros e.g.:
> > > >
> > > > #define HFP_AG_DEFAULT_FEATURES 0x09
> > > > #define HFP_HF_DEFAULT_FEATURES 0x00
> > > > #define HSP_HS_DEFAULT_VOLCNTRL "false"
> > >
> > > Don't bother with default that are 0x00/false, we can assume this is
> > > implicit when allocating the memory everything is set to 0 so these
> > > defines wouldn't be needed in the first place.
> > >
> > > > Or do you prefer different names?
> > >
> > >
> > >
> > > --
> > > Luiz Augusto von Dentz
> 
> 
> 
> -- 
> Luiz Augusto von Dentz

Re: [PATCH 1/3] src/profile: Distinguish between zero-set HFP AG features and unset HFP AG features

[Luiz Augusto von Dentz]

Hi Pali,

On Mon, Apr 13, 2020 at 10:58 AM Pali Rohár <pali@kernel.org> wrote:
>
> On Monday 13 April 2020 10:47:04 Luiz Augusto von Dentz wrote:
> > Hi Pali,
> >
> > On Mon, Apr 13, 2020 at 10:17 AM Pali Rohár <pali@kernel.org> wrote:
> > >
> > > On Monday 13 April 2020 09:58:34 Luiz Augusto von Dentz wrote:
> > > > Hi Pali,
> > > >
> > > > On Mon, Apr 13, 2020 at 9:52 AM Pali Rohár <pali@kernel.org> wrote:
> > > > >
> > > > > On Monday 13 April 2020 09:44:14 Luiz Augusto von Dentz wrote:
> > > > > > On Mon, Apr 13, 2020 at 9:25 AM Pali Rohár <pali@kernel.org> wrote:
> > > > > > > @@ -1750,15 +1758,19 @@ static int ext_disconnect_dev(struct btd_service *service)
> > > > > > >  static char *get_hfp_hf_record(struct ext_profile *ext, struct ext_io *l2cap,
> > > > > > >                                                         struct ext_io *rfcomm)
> > > > > > >  {
> > > > > > > +       /* HFP 1.7.2: By default features bitfield is 0b000000 */
> > > > > > >         return g_strdup_printf(HFP_HF_RECORD, rfcomm->chan, ext->version,
> > > > > > > -                                               ext->name, ext->features);
> > > > > > > +                               ext->name,
> > > > > > > +                               ext->have_features ? ext->features : 0x0);
> > > > > > >  }
> > > > > > >
> > > > > > >  static char *get_hfp_ag_record(struct ext_profile *ext, struct ext_io *l2cap,
> > > > > > >                                                         struct ext_io *rfcomm)
> > > > > > >  {
> > > > > > > +       /* HFP 1.7.2: By default features bitfield is 0b001001 */
> > > > > > >         return g_strdup_printf(HFP_AG_RECORD, rfcomm->chan, ext->version,
> > > > > > > -                                               ext->name, ext->features);
> > > > > > > +                               ext->name,
> > > > > > > +                               ext->have_features ? ext->features : 0x9);
> > > > > >
> > > > > > I wonder why you didn't just initialize the features wiht 0x9 instead
> > > > > > of adding a flag to track it, btw add a define with value 0x09 like
> > > > > > HFP_DEFAULT_FEATURES that way it is clearer why we are doing this.
> > > > >
> > > > > This function get_hfp_ag_record() is for parsing local features. You are
> > > > > right that for local features we do not need a flag to track it.
> > > > >
> > > > > But flag for tracking is needed for parsing remote features. And to have
> > > > > unified code for storing local and remote features it is easier to have
> > > > > always a flag for checking if features were provided or not.
> > > >
> > > > Im not following you about the remote features beinf different, I
> > > > though both would be could be initialized in the same way and then if
> > > > we read a different value from the SDP record we just overwrite it.
> > >
> > > But in this case we put these default remote features to HFP DBus agent
> > > like if they were specified in SDP. Default value is specific for
> > > profile version. And if e.g. new HFP version defines a new bit in
> > > features with its own default value then HFP DBus agent would not be
> > > able to distinguish between state when remote device did not specified
> > > any value (as bluez will put there some defaults) and when remote device
> > > specified same features as bluez has defined in its current default
> > > value.
> >
> > Different version may have different defaults but we can initialize
> > them correctly.
>
> But we do not know default values for future versions which have not
> been released yet. There would be a problem if user run current
> bluez daemon e.g. in 5 years when new version of HFP profile would be
> released and remote device would use it.

We would have to match what is the features that the agent can handle.

> > > Before and also after this change remote features are not send to HFP
> > > DBus agent when they were not specified by remote device.
> > >
> > > After your suggestion HFP DBus agent would not be able to check if
> > > remote device provided features or not. And this is I think a
> > > regression.
> >
> > I don't think we can consider a regression to always provide features,
> > besides what would be the reason for the agent to know if the features
> > were from the SDP record or they are auto initiliazed?
>
> Autoinitialized for which version? Bluez has currently defined default
> values for HFP 1.7. But DBus agent can register HFP profile also for
> version 1.5 or also 1.8.
>
> To which value should it be autoinitialized when remote device announce
> that is in version 1.8 but does not specify features (= default for
> version 1.8 should be used)? The only option which make sense is to not
> autoinitiaze this value as version 1.8 was not released yet and now we
> do not know what would be the default value.

We should probably use the mininum version of the client and server,
so if the device supports 1.8 but our agent is 1.5 it should default
to 1.5, this actually would have to be done anyway because othewise we
would make agents duplicating the logic of handling the features
properly so we might as well try to consolidate this on the daemon and
only expose the features that can actually be used in the session,
that means the autoinitilize logic the to take into account both the
device and agent version.

> > I that was
> > really necessary Id just dump the whole record as a ServiceRecord
> > option and leave it up to the client to parse it.
> >
> > > > > I can put these default values in profile-role specific macros e.g.:
> > > > >
> > > > > #define HFP_AG_DEFAULT_FEATURES 0x09
> > > > > #define HFP_HF_DEFAULT_FEATURES 0x00
> > > > > #define HSP_HS_DEFAULT_VOLCNTRL "false"
> > > >
> > > > Don't bother with default that are 0x00/false, we can assume this is
> > > > implicit when allocating the memory everything is set to 0 so these
> > > > defines wouldn't be needed in the first place.
> > > >
> > > > > Or do you prefer different names?
> > > >
> > > >
> > > >
> > > > --
> > > > Luiz Augusto von Dentz
> >
> >
> >
> > --
> > Luiz Augusto von Dentz



-- 
Luiz Augusto von Dentz

Re: [PATCH 1/3] src/profile: Distinguish between zero-set HFP AG features and unset HFP AG features

[Pali Rohár]

On Monday 13 April 2020 12:41:44 Luiz Augusto von Dentz wrote:
> Hi Pali,
> 
> On Mon, Apr 13, 2020 at 10:58 AM Pali Rohár <pali@kernel.org> wrote:
> >
> > On Monday 13 April 2020 10:47:04 Luiz Augusto von Dentz wrote:
> > > Hi Pali,
> > >
> > > On Mon, Apr 13, 2020 at 10:17 AM Pali Rohár <pali@kernel.org> wrote:
> > > >
> > > > On Monday 13 April 2020 09:58:34 Luiz Augusto von Dentz wrote:
> > > > > Hi Pali,
> > > > >
> > > > > On Mon, Apr 13, 2020 at 9:52 AM Pali Rohár <pali@kernel.org> wrote:
> > > > > >
> > > > > > On Monday 13 April 2020 09:44:14 Luiz Augusto von Dentz wrote:
> > > > > > > On Mon, Apr 13, 2020 at 9:25 AM Pali Rohár <pali@kernel.org> wrote:
> > > > > > > > @@ -1750,15 +1758,19 @@ static int ext_disconnect_dev(struct btd_service *service)
> > > > > > > >  static char *get_hfp_hf_record(struct ext_profile *ext, struct ext_io *l2cap,
> > > > > > > >                                                         struct ext_io *rfcomm)
> > > > > > > >  {
> > > > > > > > +       /* HFP 1.7.2: By default features bitfield is 0b000000 */
> > > > > > > >         return g_strdup_printf(HFP_HF_RECORD, rfcomm->chan, ext->version,
> > > > > > > > -                                               ext->name, ext->features);
> > > > > > > > +                               ext->name,
> > > > > > > > +                               ext->have_features ? ext->features : 0x0);
> > > > > > > >  }
> > > > > > > >
> > > > > > > >  static char *get_hfp_ag_record(struct ext_profile *ext, struct ext_io *l2cap,
> > > > > > > >                                                         struct ext_io *rfcomm)
> > > > > > > >  {
> > > > > > > > +       /* HFP 1.7.2: By default features bitfield is 0b001001 */
> > > > > > > >         return g_strdup_printf(HFP_AG_RECORD, rfcomm->chan, ext->version,
> > > > > > > > -                                               ext->name, ext->features);
> > > > > > > > +                               ext->name,
> > > > > > > > +                               ext->have_features ? ext->features : 0x9);
> > > > > > >
> > > > > > > I wonder why you didn't just initialize the features wiht 0x9 instead
> > > > > > > of adding a flag to track it, btw add a define with value 0x09 like
> > > > > > > HFP_DEFAULT_FEATURES that way it is clearer why we are doing this.
> > > > > >
> > > > > > This function get_hfp_ag_record() is for parsing local features. You are
> > > > > > right that for local features we do not need a flag to track it.
> > > > > >
> > > > > > But flag for tracking is needed for parsing remote features. And to have
> > > > > > unified code for storing local and remote features it is easier to have
> > > > > > always a flag for checking if features were provided or not.
> > > > >
> > > > > Im not following you about the remote features beinf different, I
> > > > > though both would be could be initialized in the same way and then if
> > > > > we read a different value from the SDP record we just overwrite it.
> > > >
> > > > But in this case we put these default remote features to HFP DBus agent
> > > > like if they were specified in SDP. Default value is specific for
> > > > profile version. And if e.g. new HFP version defines a new bit in
> > > > features with its own default value then HFP DBus agent would not be
> > > > able to distinguish between state when remote device did not specified
> > > > any value (as bluez will put there some defaults) and when remote device
> > > > specified same features as bluez has defined in its current default
> > > > value.
> > >
> > > Different version may have different defaults but we can initialize
> > > them correctly.
> >
> > But we do not know default values for future versions which have not
> > been released yet. There would be a problem if user run current
> > bluez daemon e.g. in 5 years when new version of HFP profile would be
> > released and remote device would use it.
> 
> We would have to match what is the features that the agent can handle.

But we do not know what features can agent handle. There is no API for
it.

Agent which works in AG role accept connections from remote HF role.
Agent register to bluez with AG role features. HF and AG features are
different and bluez has absolutely no idea what features can agent
accept from remote device.

> > > > Before and also after this change remote features are not send to HFP
> > > > DBus agent when they were not specified by remote device.
> > > >
> > > > After your suggestion HFP DBus agent would not be able to check if
> > > > remote device provided features or not. And this is I think a
> > > > regression.
> > >
> > > I don't think we can consider a regression to always provide features,
> > > besides what would be the reason for the agent to know if the features
> > > were from the SDP record or they are auto initiliazed?
> >
> > Autoinitialized for which version? Bluez has currently defined default
> > values for HFP 1.7. But DBus agent can register HFP profile also for
> > version 1.5 or also 1.8.
> >
> > To which value should it be autoinitialized when remote device announce
> > that is in version 1.8 but does not specify features (= default for
> > version 1.8 should be used)? The only option which make sense is to not
> > autoinitiaze this value as version 1.8 was not released yet and now we
> > do not know what would be the default value.
> 
> We should probably use the mininum version of the client and server,
> so if the device supports 1.8 but our agent is 1.5 it should default
> to 1.5, this actually would have to be done anyway because othewise we
> would make agents duplicating the logic of handling the features
> properly so we might as well try to consolidate this on the daemon and
> only expose the features that can actually be used in the session,
> that means the autoinitilize logic the to take into account both the
> device and agent version.

And what would happen if both agent and remote device supports 1.8?

When remote device do specified features, default one should be used.
So this cannot be solved with some default values in bluez.

> > > I that was
> > > really necessary Id just dump the whole record as a ServiceRecord
> > > option and leave it up to the client to parse it.
> > >
> > > > > > I can put these default values in profile-role specific macros e.g.:
> > > > > >
> > > > > > #define HFP_AG_DEFAULT_FEATURES 0x09
> > > > > > #define HFP_HF_DEFAULT_FEATURES 0x00
> > > > > > #define HSP_HS_DEFAULT_VOLCNTRL "false"
> > > > >
> > > > > Don't bother with default that are 0x00/false, we can assume this is
> > > > > implicit when allocating the memory everything is set to 0 so these
> > > > > defines wouldn't be needed in the first place.
> > > > >
> > > > > > Or do you prefer different names?
> > > > >
> > > > >
> > > > >
> > > > > --
> > > > > Luiz Augusto von Dentz
> > >
> > >
> > >
> > > --
> > > Luiz Augusto von Dentz
> 
> 
> 
> -- 
> Luiz Augusto von Dentz

Re: [PATCH 1/3] src/profile: Distinguish between zero-set HFP AG features and unset HFP AG features

[Luiz Augusto von Dentz]

Hi Pali,

On Mon, Apr 13, 2020 at 12:51 PM Pali Rohár <pali@kernel.org> wrote:
>
> On Monday 13 April 2020 12:41:44 Luiz Augusto von Dentz wrote:
> > Hi Pali,
> >
> > On Mon, Apr 13, 2020 at 10:58 AM Pali Rohár <pali@kernel.org> wrote:
> > >
> > > On Monday 13 April 2020 10:47:04 Luiz Augusto von Dentz wrote:
> > > > Hi Pali,
> > > >
> > > > On Mon, Apr 13, 2020 at 10:17 AM Pali Rohár <pali@kernel.org> wrote:
> > > > >
> > > > > On Monday 13 April 2020 09:58:34 Luiz Augusto von Dentz wrote:
> > > > > > Hi Pali,
> > > > > >
> > > > > > On Mon, Apr 13, 2020 at 9:52 AM Pali Rohár <pali@kernel.org> wrote:
> > > > > > >
> > > > > > > On Monday 13 April 2020 09:44:14 Luiz Augusto von Dentz wrote:
> > > > > > > > On Mon, Apr 13, 2020 at 9:25 AM Pali Rohár <pali@kernel.org> wrote:
> > > > > > > > > @@ -1750,15 +1758,19 @@ static int ext_disconnect_dev(struct btd_service *service)
> > > > > > > > >  static char *get_hfp_hf_record(struct ext_profile *ext, struct ext_io *l2cap,
> > > > > > > > >                                                         struct ext_io *rfcomm)
> > > > > > > > >  {
> > > > > > > > > +       /* HFP 1.7.2: By default features bitfield is 0b000000 */
> > > > > > > > >         return g_strdup_printf(HFP_HF_RECORD, rfcomm->chan, ext->version,
> > > > > > > > > -                                               ext->name, ext->features);
> > > > > > > > > +                               ext->name,
> > > > > > > > > +                               ext->have_features ? ext->features : 0x0);
> > > > > > > > >  }
> > > > > > > > >
> > > > > > > > >  static char *get_hfp_ag_record(struct ext_profile *ext, struct ext_io *l2cap,
> > > > > > > > >                                                         struct ext_io *rfcomm)
> > > > > > > > >  {
> > > > > > > > > +       /* HFP 1.7.2: By default features bitfield is 0b001001 */
> > > > > > > > >         return g_strdup_printf(HFP_AG_RECORD, rfcomm->chan, ext->version,
> > > > > > > > > -                                               ext->name, ext->features);
> > > > > > > > > +                               ext->name,
> > > > > > > > > +                               ext->have_features ? ext->features : 0x9);
> > > > > > > >
> > > > > > > > I wonder why you didn't just initialize the features wiht 0x9 instead
> > > > > > > > of adding a flag to track it, btw add a define with value 0x09 like
> > > > > > > > HFP_DEFAULT_FEATURES that way it is clearer why we are doing this.
> > > > > > >
> > > > > > > This function get_hfp_ag_record() is for parsing local features. You are
> > > > > > > right that for local features we do not need a flag to track it.
> > > > > > >
> > > > > > > But flag for tracking is needed for parsing remote features. And to have
> > > > > > > unified code for storing local and remote features it is easier to have
> > > > > > > always a flag for checking if features were provided or not.
> > > > > >
> > > > > > Im not following you about the remote features beinf different, I
> > > > > > though both would be could be initialized in the same way and then if
> > > > > > we read a different value from the SDP record we just overwrite it.
> > > > >
> > > > > But in this case we put these default remote features to HFP DBus agent
> > > > > like if they were specified in SDP. Default value is specific for
> > > > > profile version. And if e.g. new HFP version defines a new bit in
> > > > > features with its own default value then HFP DBus agent would not be
> > > > > able to distinguish between state when remote device did not specified
> > > > > any value (as bluez will put there some defaults) and when remote device
> > > > > specified same features as bluez has defined in its current default
> > > > > value.
> > > >
> > > > Different version may have different defaults but we can initialize
> > > > them correctly.
> > >
> > > But we do not know default values for future versions which have not
> > > been released yet. There would be a problem if user run current
> > > bluez daemon e.g. in 5 years when new version of HFP profile would be
> > > released and remote device would use it.
> >
> > We would have to match what is the features that the agent can handle.
>
> But we do not know what features can agent handle. There is no API for
> it.

Yes we do, the version would tell exactly what features the agent can handle.

> Agent which works in AG role accept connections from remote HF role.
> Agent register to bluez with AG role features. HF and AG features are
> different and bluez has absolutely no idea what features can agent
> accept from remote device.

The version needs to be compatible no matter if the features are
different for AG/HF.

> > > > > Before and also after this change remote features are not send to HFP
> > > > > DBus agent when they were not specified by remote device.
> > > > >
> > > > > After your suggestion HFP DBus agent would not be able to check if
> > > > > remote device provided features or not. And this is I think a
> > > > > regression.
> > > >
> > > > I don't think we can consider a regression to always provide features,
> > > > besides what would be the reason for the agent to know if the features
> > > > were from the SDP record or they are auto initiliazed?
> > >
> > > Autoinitialized for which version? Bluez has currently defined default
> > > values for HFP 1.7. But DBus agent can register HFP profile also for
> > > version 1.5 or also 1.8.
> > >
> > > To which value should it be autoinitialized when remote device announce
> > > that is in version 1.8 but does not specify features (= default for
> > > version 1.8 should be used)? The only option which make sense is to not
> > > autoinitiaze this value as version 1.8 was not released yet and now we
> > > do not know what would be the default value.
> >
> > We should probably use the mininum version of the client and server,
> > so if the device supports 1.8 but our agent is 1.5 it should default
> > to 1.5, this actually would have to be done anyway because othewise we
> > would make agents duplicating the logic of handling the features
> > properly so we might as well try to consolidate this on the daemon and
> > only expose the features that can actually be used in the session,
> > that means the autoinitilize logic the to take into account both the
> > device and agent version.
>
> And what would happen if both agent and remote device supports 1.8?

Then 1.8 defaults are used, if the daemon don't support 1.8 then the
agent cannot register for 1.8 to begin with.

> When remote device do specified features, default one should be used.
> So this cannot be solved with some default values in bluez.

Like I said before, bluez controls the SDP record so it will need to
understand what features are supported based on version, this is
exactly why we want the daemon to control the sdp record so we don't
have the agents dealing version compatibility, etc.

> > > > I that was
> > > > really necessary Id just dump the whole record as a ServiceRecord
> > > > option and leave it up to the client to parse it.
> > > >
> > > > > > > I can put these default values in profile-role specific macros e.g.:
> > > > > > >
> > > > > > > #define HFP_AG_DEFAULT_FEATURES 0x09
> > > > > > > #define HFP_HF_DEFAULT_FEATURES 0x00
> > > > > > > #define HSP_HS_DEFAULT_VOLCNTRL "false"
> > > > > >
> > > > > > Don't bother with default that are 0x00/false, we can assume this is
> > > > > > implicit when allocating the memory everything is set to 0 so these
> > > > > > defines wouldn't be needed in the first place.
> > > > > >
> > > > > > > Or do you prefer different names?
> > > > > >
> > > > > >
> > > > > >
> > > > > > --
> > > > > > Luiz Augusto von Dentz
> > > >
> > > >
> > > >
> > > > --
> > > > Luiz Augusto von Dentz
> >
> >
> >
> > --
> > Luiz Augusto von Dentz



-- 
Luiz Augusto von Dentz

Re: [PATCH 1/3] src/profile: Distinguish between zero-set HFP AG features and unset HFP AG features

[Pali Rohár]

On Monday 13 April 2020 13:24:56 Luiz Augusto von Dentz wrote:
> Hi Pali,
> 
> On Mon, Apr 13, 2020 at 12:51 PM Pali Rohár <pali@kernel.org> wrote:
> >
> > On Monday 13 April 2020 12:41:44 Luiz Augusto von Dentz wrote:
> > > Hi Pali,
> > >
> > > On Mon, Apr 13, 2020 at 10:58 AM Pali Rohár <pali@kernel.org> wrote:
> > > >
> > > > On Monday 13 April 2020 10:47:04 Luiz Augusto von Dentz wrote:
> > > > > Hi Pali,
> > > > >
> > > > > On Mon, Apr 13, 2020 at 10:17 AM Pali Rohár <pali@kernel.org> wrote:
> > > > > >
> > > > > > On Monday 13 April 2020 09:58:34 Luiz Augusto von Dentz wrote:
> > > > > > > Hi Pali,
> > > > > > >
> > > > > > > On Mon, Apr 13, 2020 at 9:52 AM Pali Rohár <pali@kernel.org> wrote:
> > > > > > > >
> > > > > > > > On Monday 13 April 2020 09:44:14 Luiz Augusto von Dentz wrote:
> > > > > > > > > On Mon, Apr 13, 2020 at 9:25 AM Pali Rohár <pali@kernel.org> wrote:
> > > > > > > > > > @@ -1750,15 +1758,19 @@ static int ext_disconnect_dev(struct btd_service *service)
> > > > > > > > > >  static char *get_hfp_hf_record(struct ext_profile *ext, struct ext_io *l2cap,
> > > > > > > > > >                                                         struct ext_io *rfcomm)
> > > > > > > > > >  {
> > > > > > > > > > +       /* HFP 1.7.2: By default features bitfield is 0b000000 */
> > > > > > > > > >         return g_strdup_printf(HFP_HF_RECORD, rfcomm->chan, ext->version,
> > > > > > > > > > -                                               ext->name, ext->features);
> > > > > > > > > > +                               ext->name,
> > > > > > > > > > +                               ext->have_features ? ext->features : 0x0);
> > > > > > > > > >  }
> > > > > > > > > >
> > > > > > > > > >  static char *get_hfp_ag_record(struct ext_profile *ext, struct ext_io *l2cap,
> > > > > > > > > >                                                         struct ext_io *rfcomm)
> > > > > > > > > >  {
> > > > > > > > > > +       /* HFP 1.7.2: By default features bitfield is 0b001001 */
> > > > > > > > > >         return g_strdup_printf(HFP_AG_RECORD, rfcomm->chan, ext->version,
> > > > > > > > > > -                                               ext->name, ext->features);
> > > > > > > > > > +                               ext->name,
> > > > > > > > > > +                               ext->have_features ? ext->features : 0x9);
> > > > > > > > >
> > > > > > > > > I wonder why you didn't just initialize the features wiht 0x9 instead
> > > > > > > > > of adding a flag to track it, btw add a define with value 0x09 like
> > > > > > > > > HFP_DEFAULT_FEATURES that way it is clearer why we are doing this.
> > > > > > > >
> > > > > > > > This function get_hfp_ag_record() is for parsing local features. You are
> > > > > > > > right that for local features we do not need a flag to track it.
> > > > > > > >
> > > > > > > > But flag for tracking is needed for parsing remote features. And to have
> > > > > > > > unified code for storing local and remote features it is easier to have
> > > > > > > > always a flag for checking if features were provided or not.
> > > > > > >
> > > > > > > Im not following you about the remote features beinf different, I
> > > > > > > though both would be could be initialized in the same way and then if
> > > > > > > we read a different value from the SDP record we just overwrite it.
> > > > > >
> > > > > > But in this case we put these default remote features to HFP DBus agent
> > > > > > like if they were specified in SDP. Default value is specific for
> > > > > > profile version. And if e.g. new HFP version defines a new bit in
> > > > > > features with its own default value then HFP DBus agent would not be
> > > > > > able to distinguish between state when remote device did not specified
> > > > > > any value (as bluez will put there some defaults) and when remote device
> > > > > > specified same features as bluez has defined in its current default
> > > > > > value.
> > > > >
> > > > > Different version may have different defaults but we can initialize
> > > > > them correctly.
> > > >
> > > > But we do not know default values for future versions which have not
> > > > been released yet. There would be a problem if user run current
> > > > bluez daemon e.g. in 5 years when new version of HFP profile would be
> > > > released and remote device would use it.
> > >
> > > We would have to match what is the features that the agent can handle.
> >
> > But we do not know what features can agent handle. There is no API for
> > it.
> 
> Yes we do, the version would tell exactly what features the agent can handle.

No. Endpoints (local agent or remote device) does not have to support
all features provided by version. This is why feature list exist.

> > Agent which works in AG role accept connections from remote HF role.
> > Agent register to bluez with AG role features. HF and AG features are
> > different and bluez has absolutely no idea what features can agent
> > accept from remote device.
> 
> The version needs to be compatible no matter if the features are
> different for AG/HF.

This is not truth. Version X is compatible with all versions less then
X. So if remote device support version 1.5, then it can connect to our
local dbus agent which is running at version 1.7. And version 1.7 is
registered to bluez. In HFP spec is written how implementation should
deal with this backward compatibility. It is at protocol level, so it
cannot be done in bluez, only in application which implements HFP
protocol, therefore external DBus HFP agent.

> > > > > > Before and also after this change remote features are not send to HFP
> > > > > > DBus agent when they were not specified by remote device.
> > > > > >
> > > > > > After your suggestion HFP DBus agent would not be able to check if
> > > > > > remote device provided features or not. And this is I think a
> > > > > > regression.
> > > > >
> > > > > I don't think we can consider a regression to always provide features,
> > > > > besides what would be the reason for the agent to know if the features
> > > > > were from the SDP record or they are auto initiliazed?
> > > >
> > > > Autoinitialized for which version? Bluez has currently defined default
> > > > values for HFP 1.7. But DBus agent can register HFP profile also for
> > > > version 1.5 or also 1.8.
> > > >
> > > > To which value should it be autoinitialized when remote device announce
> > > > that is in version 1.8 but does not specify features (= default for
> > > > version 1.8 should be used)? The only option which make sense is to not
> > > > autoinitiaze this value as version 1.8 was not released yet and now we
> > > > do not know what would be the default value.
> > >
> > > We should probably use the mininum version of the client and server,
> > > so if the device supports 1.8 but our agent is 1.5 it should default
> > > to 1.5, this actually would have to be done anyway because othewise we
> > > would make agents duplicating the logic of handling the features
> > > properly so we might as well try to consolidate this on the daemon and
> > > only expose the features that can actually be used in the session,
> > > that means the autoinitilize logic the to take into account both the
> > > device and agent version.
> >
> > And what would happen if both agent and remote device supports 1.8?
> 
> Then 1.8 defaults are used, if the daemon don't support 1.8 then the
> agent cannot register for 1.8 to begin with.

bluez daemon does not implement HFP profile. It is external DBus agent
which implements it. Bluez provides just some default values for SDP
records, but external DBus agent can provide own SDP values including
version number and feature list.

So default version numberin bluez is meaningless for HFP implementation
in external DBus agent.

> > When remote device do specified features, default one should be used.
> > So this cannot be solved with some default values in bluez.
> 
> Like I said before, bluez controls the SDP record so it will need to
> understand what features are supported based on version, this is

Why it needs to know it? It just export SDP features which agent can
provides on its own. Bluez is not doing more then providing SDP records.

> exactly why we want the daemon to control the sdp record so we don't
> have the agents dealing version compatibility, etc.
> 
> > > > > I that was
> > > > > really necessary Id just dump the whole record as a ServiceRecord
> > > > > option and leave it up to the client to parse it.
> > > > >
> > > > > > > > I can put these default values in profile-role specific macros e.g.:
> > > > > > > >
> > > > > > > > #define HFP_AG_DEFAULT_FEATURES 0x09
> > > > > > > > #define HFP_HF_DEFAULT_FEATURES 0x00
> > > > > > > > #define HSP_HS_DEFAULT_VOLCNTRL "false"
> > > > > > >
> > > > > > > Don't bother with default that are 0x00/false, we can assume this is
> > > > > > > implicit when allocating the memory everything is set to 0 so these
> > > > > > > defines wouldn't be needed in the first place.
> > > > > > >
> > > > > > > > Or do you prefer different names?
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > --
> > > > > > > Luiz Augusto von Dentz
> > > > >
> > > > >
> > > > >
> > > > > --
> > > > > Luiz Augusto von Dentz
> > >
> > >
> > >
> > > --
> > > Luiz Augusto von Dentz
> 
> 
> 
> -- 
> Luiz Augusto von Dentz

Re: [PATCH 1/3] src/profile: Distinguish between zero-set HFP AG features and unset HFP AG features

[Luiz Augusto von Dentz]

Hi Pali,

On Mon, Apr 13, 2020 at 1:42 PM Pali Rohár <pali@kernel.org> wrote:
>
> On Monday 13 April 2020 13:24:56 Luiz Augusto von Dentz wrote:
> > Hi Pali,
> >
> > On Mon, Apr 13, 2020 at 12:51 PM Pali Rohár <pali@kernel.org> wrote:
> > >
> > > On Monday 13 April 2020 12:41:44 Luiz Augusto von Dentz wrote:
> > > > Hi Pali,
> > > >
> > > > On Mon, Apr 13, 2020 at 10:58 AM Pali Rohár <pali@kernel.org> wrote:
> > > > >
> > > > > On Monday 13 April 2020 10:47:04 Luiz Augusto von Dentz wrote:
> > > > > > Hi Pali,
> > > > > >
> > > > > > On Mon, Apr 13, 2020 at 10:17 AM Pali Rohár <pali@kernel.org> wrote:
> > > > > > >
> > > > > > > On Monday 13 April 2020 09:58:34 Luiz Augusto von Dentz wrote:
> > > > > > > > Hi Pali,
> > > > > > > >
> > > > > > > > On Mon, Apr 13, 2020 at 9:52 AM Pali Rohár <pali@kernel.org> wrote:
> > > > > > > > >
> > > > > > > > > On Monday 13 April 2020 09:44:14 Luiz Augusto von Dentz wrote:
> > > > > > > > > > On Mon, Apr 13, 2020 at 9:25 AM Pali Rohár <pali@kernel.org> wrote:
> > > > > > > > > > > @@ -1750,15 +1758,19 @@ static int ext_disconnect_dev(struct btd_service *service)
> > > > > > > > > > >  static char *get_hfp_hf_record(struct ext_profile *ext, struct ext_io *l2cap,
> > > > > > > > > > >                                                         struct ext_io *rfcomm)
> > > > > > > > > > >  {
> > > > > > > > > > > +       /* HFP 1.7.2: By default features bitfield is 0b000000 */
> > > > > > > > > > >         return g_strdup_printf(HFP_HF_RECORD, rfcomm->chan, ext->version,
> > > > > > > > > > > -                                               ext->name, ext->features);
> > > > > > > > > > > +                               ext->name,
> > > > > > > > > > > +                               ext->have_features ? ext->features : 0x0);
> > > > > > > > > > >  }
> > > > > > > > > > >
> > > > > > > > > > >  static char *get_hfp_ag_record(struct ext_profile *ext, struct ext_io *l2cap,
> > > > > > > > > > >                                                         struct ext_io *rfcomm)
> > > > > > > > > > >  {
> > > > > > > > > > > +       /* HFP 1.7.2: By default features bitfield is 0b001001 */
> > > > > > > > > > >         return g_strdup_printf(HFP_AG_RECORD, rfcomm->chan, ext->version,
> > > > > > > > > > > -                                               ext->name, ext->features);
> > > > > > > > > > > +                               ext->name,
> > > > > > > > > > > +                               ext->have_features ? ext->features : 0x9);
> > > > > > > > > >
> > > > > > > > > > I wonder why you didn't just initialize the features wiht 0x9 instead
> > > > > > > > > > of adding a flag to track it, btw add a define with value 0x09 like
> > > > > > > > > > HFP_DEFAULT_FEATURES that way it is clearer why we are doing this.
> > > > > > > > >
> > > > > > > > > This function get_hfp_ag_record() is for parsing local features. You are
> > > > > > > > > right that for local features we do not need a flag to track it.
> > > > > > > > >
> > > > > > > > > But flag for tracking is needed for parsing remote features. And to have
> > > > > > > > > unified code for storing local and remote features it is easier to have
> > > > > > > > > always a flag for checking if features were provided or not.
> > > > > > > >
> > > > > > > > Im not following you about the remote features beinf different, I
> > > > > > > > though both would be could be initialized in the same way and then if
> > > > > > > > we read a different value from the SDP record we just overwrite it.
> > > > > > >
> > > > > > > But in this case we put these default remote features to HFP DBus agent
> > > > > > > like if they were specified in SDP. Default value is specific for
> > > > > > > profile version. And if e.g. new HFP version defines a new bit in
> > > > > > > features with its own default value then HFP DBus agent would not be
> > > > > > > able to distinguish between state when remote device did not specified
> > > > > > > any value (as bluez will put there some defaults) and when remote device
> > > > > > > specified same features as bluez has defined in its current default
> > > > > > > value.
> > > > > >
> > > > > > Different version may have different defaults but we can initialize
> > > > > > them correctly.
> > > > >
> > > > > But we do not know default values for future versions which have not
> > > > > been released yet. There would be a problem if user run current
> > > > > bluez daemon e.g. in 5 years when new version of HFP profile would be
> > > > > released and remote device would use it.
> > > >
> > > > We would have to match what is the features that the agent can handle.
> > >
> > > But we do not know what features can agent handle. There is no API for
> > > it.
> >
> > Yes we do, the version would tell exactly what features the agent can handle.
>
> No. Endpoints (local agent or remote device) does not have to support
> all features provided by version. This is why feature list exist.

Support no, understand yes, it must understand all the values exposed
by the remote features if it support the version, and here Im
suggesting just the default values so it surprises me the sort
assertive tone.

> > > Agent which works in AG role accept connections from remote HF role.
> > > Agent register to bluez with AG role features. HF and AG features are
> > > different and bluez has absolutely no idea what features can agent
> > > accept from remote device.
> >
> > The version needs to be compatible no matter if the features are
> > different for AG/HF.
>
> This is not truth. Version X is compatible with all versions less then
> X. So if remote device support version 1.5, then it can connect to our
> local dbus agent which is running at version 1.7. And version 1.7 is
> registered to bluez. In HFP spec is written how implementation should
> deal with this backward compatibility. It is at protocol level, so it
> cannot be done in bluez, only in application which implements HFP
> protocol, therefore external DBus HFP agent.

Yes it needs to be backward compatible, but what Im saying is that the
agent need to be able to understand the features bits, how else would
then it goes and enable events, etc, via AT commands, and actually
although your protocol can handle 1.7 the remote side may only be
handling 1.5 features then essentially all features are limited to
1.5. Im not sure why we always come to this bits and pieces in our
discussions, it is as if I was not involved with any of this in the
past, I know pretty well how HFP operates, in fact most profiles work
in the same way regarding the version, the record only indicates what
is maximum supported version and all version previous to that shall
work so at protocol level you just downgrade the version according to
what both sides can supports, what we cannot do is to upgrade the
version so features from e.g. 1.8 shall never creep into the features
bits since they wouldn't be compatible.

> > > > > > > Before and also after this change remote features are not send to HFP
> > > > > > > DBus agent when they were not specified by remote device.
> > > > > > >
> > > > > > > After your suggestion HFP DBus agent would not be able to check if
> > > > > > > remote device provided features or not. And this is I think a
> > > > > > > regression.
> > > > > >
> > > > > > I don't think we can consider a regression to always provide features,
> > > > > > besides what would be the reason for the agent to know if the features
> > > > > > were from the SDP record or they are auto initiliazed?
> > > > >
> > > > > Autoinitialized for which version? Bluez has currently defined default
> > > > > values for HFP 1.7. But DBus agent can register HFP profile also for
> > > > > version 1.5 or also 1.8.
> > > > >
> > > > > To which value should it be autoinitialized when remote device announce
> > > > > that is in version 1.8 but does not specify features (= default for
> > > > > version 1.8 should be used)? The only option which make sense is to not
> > > > > autoinitiaze this value as version 1.8 was not released yet and now we
> > > > > do not know what would be the default value.
> > > >
> > > > We should probably use the mininum version of the client and server,
> > > > so if the device supports 1.8 but our agent is 1.5 it should default
> > > > to 1.5, this actually would have to be done anyway because othewise we
> > > > would make agents duplicating the logic of handling the features
> > > > properly so we might as well try to consolidate this on the daemon and
> > > > only expose the features that can actually be used in the session,
> > > > that means the autoinitilize logic the to take into account both the
> > > > device and agent version.
> > >
> > > And what would happen if both agent and remote device supports 1.8?
> >
> > Then 1.8 defaults are used, if the daemon don't support 1.8 then the
> > agent cannot register for 1.8 to begin with.
>
> bluez daemon does not implement HFP profile. It is external DBus agent
> which implements it. Bluez provides just some default values for SDP
> records, but external DBus agent can provide own SDP values including
> version number and feature list.

Yes and in order to construct a valid SDP record it needs to
understand the version and features.

> So default version numberin bluez is meaningless for HFP implementation
> in external DBus agent.
>
> > > When remote device do specified features, default one should be used.
> > > So this cannot be solved with some default values in bluez.
> >
> > Like I said before, bluez controls the SDP record so it will need to
> > understand what features are supported based on version, this is
>
> Why it needs to know it? It just export SDP features which agent can
> provides on its own. Bluez is not doing more then providing SDP records.

Read above, we still need to construct a valid SDP record, this
shouldn't be a big change compared to what you already have all we
would be doing is:

1. version = MIN(remote_version, local_version)
2. features =  default_features(version)
3. features = get_features()
4. NewConnectiion(version, features)

Then all the agents out there would don't need to keep doing on their own.

> > exactly why we want the daemon to control the sdp record so we don't
> > have the agents dealing version compatibility, etc.
> >
> > > > > > I that was
> > > > > > really necessary Id just dump the whole record as a ServiceRecord
> > > > > > option and leave it up to the client to parse it.
> > > > > >
> > > > > > > > > I can put these default values in profile-role specific macros e.g.:
> > > > > > > > >
> > > > > > > > > #define HFP_AG_DEFAULT_FEATURES 0x09
> > > > > > > > > #define HFP_HF_DEFAULT_FEATURES 0x00
> > > > > > > > > #define HSP_HS_DEFAULT_VOLCNTRL "false"
> > > > > > > >
> > > > > > > > Don't bother with default that are 0x00/false, we can assume this is
> > > > > > > > implicit when allocating the memory everything is set to 0 so these
> > > > > > > > defines wouldn't be needed in the first place.
> > > > > > > >
> > > > > > > > > Or do you prefer different names?
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > > --
> > > > > > > > Luiz Augusto von Dentz
> > > > > >
> > > > > >
> > > > > >
> > > > > > --
> > > > > > Luiz Augusto von Dentz
> > > >
> > > >
> > > >
> > > > --
> > > > Luiz Augusto von Dentz
> >
> >
> >
> > --
> > Luiz Augusto von Dentz



-- 
Luiz Augusto von Dentz

Re: [PATCH 1/3] src/profile: Distinguish between zero-set HFP AG features and unset HFP AG features

[Pali Rohár]

On Monday 13 April 2020 14:07:15 Luiz Augusto von Dentz wrote:
> Hi Pali,
> 
> On Mon, Apr 13, 2020 at 1:42 PM Pali Rohár <pali@kernel.org> wrote:
> >
> > On Monday 13 April 2020 13:24:56 Luiz Augusto von Dentz wrote:
> > > Hi Pali,
> > >
> > > On Mon, Apr 13, 2020 at 12:51 PM Pali Rohár <pali@kernel.org> wrote:
> > > >
> > > > On Monday 13 April 2020 12:41:44 Luiz Augusto von Dentz wrote:
> > > > > Hi Pali,
> > > > >
> > > > > On Mon, Apr 13, 2020 at 10:58 AM Pali Rohár <pali@kernel.org> wrote:
> > > > > >
> > > > > > On Monday 13 April 2020 10:47:04 Luiz Augusto von Dentz wrote:
> > > > > > > Hi Pali,
> > > > > > >
> > > > > > > On Mon, Apr 13, 2020 at 10:17 AM Pali Rohár <pali@kernel.org> wrote:
> > > > > > > >
> > > > > > > > On Monday 13 April 2020 09:58:34 Luiz Augusto von Dentz wrote:
> > > > > > > > > Hi Pali,
> > > > > > > > >
> > > > > > > > > On Mon, Apr 13, 2020 at 9:52 AM Pali Rohár <pali@kernel.org> wrote:
> > > > > > > > > >
> > > > > > > > > > On Monday 13 April 2020 09:44:14 Luiz Augusto von Dentz wrote:
> > > > > > > > > > > On Mon, Apr 13, 2020 at 9:25 AM Pali Rohár <pali@kernel.org> wrote:
> > > > > > > > > > > > @@ -1750,15 +1758,19 @@ static int ext_disconnect_dev(struct btd_service *service)
> > > > > > > > > > > >  static char *get_hfp_hf_record(struct ext_profile *ext, struct ext_io *l2cap,
> > > > > > > > > > > >                                                         struct ext_io *rfcomm)
> > > > > > > > > > > >  {
> > > > > > > > > > > > +       /* HFP 1.7.2: By default features bitfield is 0b000000 */
> > > > > > > > > > > >         return g_strdup_printf(HFP_HF_RECORD, rfcomm->chan, ext->version,
> > > > > > > > > > > > -                                               ext->name, ext->features);
> > > > > > > > > > > > +                               ext->name,
> > > > > > > > > > > > +                               ext->have_features ? ext->features : 0x0);
> > > > > > > > > > > >  }
> > > > > > > > > > > >
> > > > > > > > > > > >  static char *get_hfp_ag_record(struct ext_profile *ext, struct ext_io *l2cap,
> > > > > > > > > > > >                                                         struct ext_io *rfcomm)
> > > > > > > > > > > >  {
> > > > > > > > > > > > +       /* HFP 1.7.2: By default features bitfield is 0b001001 */
> > > > > > > > > > > >         return g_strdup_printf(HFP_AG_RECORD, rfcomm->chan, ext->version,
> > > > > > > > > > > > -                                               ext->name, ext->features);
> > > > > > > > > > > > +                               ext->name,
> > > > > > > > > > > > +                               ext->have_features ? ext->features : 0x9);
> > > > > > > > > > >
> > > > > > > > > > > I wonder why you didn't just initialize the features wiht 0x9 instead
> > > > > > > > > > > of adding a flag to track it, btw add a define with value 0x09 like
> > > > > > > > > > > HFP_DEFAULT_FEATURES that way it is clearer why we are doing this.
> > > > > > > > > >
> > > > > > > > > > This function get_hfp_ag_record() is for parsing local features. You are
> > > > > > > > > > right that for local features we do not need a flag to track it.
> > > > > > > > > >
> > > > > > > > > > But flag for tracking is needed for parsing remote features. And to have
> > > > > > > > > > unified code for storing local and remote features it is easier to have
> > > > > > > > > > always a flag for checking if features were provided or not.
> > > > > > > > >
> > > > > > > > > Im not following you about the remote features beinf different, I
> > > > > > > > > though both would be could be initialized in the same way and then if
> > > > > > > > > we read a different value from the SDP record we just overwrite it.
> > > > > > > >
> > > > > > > > But in this case we put these default remote features to HFP DBus agent
> > > > > > > > like if they were specified in SDP. Default value is specific for
> > > > > > > > profile version. And if e.g. new HFP version defines a new bit in
> > > > > > > > features with its own default value then HFP DBus agent would not be
> > > > > > > > able to distinguish between state when remote device did not specified
> > > > > > > > any value (as bluez will put there some defaults) and when remote device
> > > > > > > > specified same features as bluez has defined in its current default
> > > > > > > > value.
> > > > > > >
> > > > > > > Different version may have different defaults but we can initialize
> > > > > > > them correctly.
> > > > > >
> > > > > > But we do not know default values for future versions which have not
> > > > > > been released yet. There would be a problem if user run current
> > > > > > bluez daemon e.g. in 5 years when new version of HFP profile would be
> > > > > > released and remote device would use it.
> > > > >
> > > > > We would have to match what is the features that the agent can handle.
> > > >
> > > > But we do not know what features can agent handle. There is no API for
> > > > it.
> > >
> > > Yes we do, the version would tell exactly what features the agent can handle.
> >
> > No. Endpoints (local agent or remote device) does not have to support
> > all features provided by version. This is why feature list exist.
> 
> Support no, understand yes, it must understand all the values exposed
> by the remote features if it support the version, and here Im
> suggesting just the default values so it surprises me the sort
> assertive tone.

It really does not have to even understand those bits. Remote device may
support 1.7, local device only 1.5. When doing establishment of SLC
connection both sides exchange via AT commands what they support and
remote device from that can know that local device is not able to
support some feature bits (because e.g. they were added in new version
1.7 which is not supported by local device 1.5).

> > > > Agent which works in AG role accept connections from remote HF role.
> > > > Agent register to bluez with AG role features. HF and AG features are
> > > > different and bluez has absolutely no idea what features can agent
> > > > accept from remote device.
> > >
> > > The version needs to be compatible no matter if the features are
> > > different for AG/HF.
> >
> > This is not truth. Version X is compatible with all versions less then
> > X. So if remote device support version 1.5, then it can connect to our
> > local dbus agent which is running at version 1.7. And version 1.7 is
> > registered to bluez. In HFP spec is written how implementation should
> > deal with this backward compatibility. It is at protocol level, so it
> > cannot be done in bluez, only in application which implements HFP
> > protocol, therefore external DBus HFP agent.
> 
> Yes it needs to be backward compatible, but what Im saying is that the
> agent need to be able to understand the features bits, how else would
> then it goes and enable events, etc, via AT commands, and actually
> although your protocol can handle 1.7 the remote side may only be
> handling 1.5 features then essentially all features are limited to
> 1.5. Im not sure why we always come to this bits and pieces in our
> discussions, it is as if I was not involved with any of this in the
> past, I know pretty well how HFP operates, in fact most profiles work
> in the same way regarding the version, the record only indicates what
> is maximum supported version and all version previous to that shall
> work so at protocol level you just downgrade the version according to
> what both sides can supports, what we cannot do is to upgrade the
> version so features from e.g. 1.8 shall never creep into the features
> bits since they wouldn't be compatible.

I'm not saying that agent does not need to understand those feature
bits. Agent of course needs to understand meaning for version which it
implements.

I'm saying that bluez does not have to know meaning of these bits.

> > > > > > > > Before and also after this change remote features are not send to HFP
> > > > > > > > DBus agent when they were not specified by remote device.
> > > > > > > >
> > > > > > > > After your suggestion HFP DBus agent would not be able to check if
> > > > > > > > remote device provided features or not. And this is I think a
> > > > > > > > regression.
> > > > > > >
> > > > > > > I don't think we can consider a regression to always provide features,
> > > > > > > besides what would be the reason for the agent to know if the features
> > > > > > > were from the SDP record or they are auto initiliazed?
> > > > > >
> > > > > > Autoinitialized for which version? Bluez has currently defined default
> > > > > > values for HFP 1.7. But DBus agent can register HFP profile also for
> > > > > > version 1.5 or also 1.8.
> > > > > >
> > > > > > To which value should it be autoinitialized when remote device announce
> > > > > > that is in version 1.8 but does not specify features (= default for
> > > > > > version 1.8 should be used)? The only option which make sense is to not
> > > > > > autoinitiaze this value as version 1.8 was not released yet and now we
> > > > > > do not know what would be the default value.
> > > > >
> > > > > We should probably use the mininum version of the client and server,
> > > > > so if the device supports 1.8 but our agent is 1.5 it should default
> > > > > to 1.5, this actually would have to be done anyway because othewise we
> > > > > would make agents duplicating the logic of handling the features
> > > > > properly so we might as well try to consolidate this on the daemon and
> > > > > only expose the features that can actually be used in the session,
> > > > > that means the autoinitilize logic the to take into account both the
> > > > > device and agent version.
> > > >
> > > > And what would happen if both agent and remote device supports 1.8?
> > >
> > > Then 1.8 defaults are used, if the daemon don't support 1.8 then the
> > > agent cannot register for 1.8 to begin with.
> >
> > bluez daemon does not implement HFP profile. It is external DBus agent
> > which implements it. Bluez provides just some default values for SDP
> > records, but external DBus agent can provide own SDP values including
> > version number and feature list.
> 
> Yes and in order to construct a valid SDP record it needs to
> understand the version and features.

Bluez does not need to understand meaning of particular bits in features
u16 value. And currently it even does not understand it. bluez just
pass-through those bits from agent to SDP record. In bluez there is no
code for parsing or generating meaning of bits in feature.

> > So default version numberin bluez is meaningless for HFP implementation
> > in external DBus agent.
> >
> > > > When remote device do specified features, default one should be used.
> > > > So this cannot be solved with some default values in bluez.
> > >
> > > Like I said before, bluez controls the SDP record so it will need to
> > > understand what features are supported based on version, this is
> >
> > Why it needs to know it? It just export SDP features which agent can
> > provides on its own. Bluez is not doing more then providing SDP records.
> 
> Read above, we still need to construct a valid SDP record, this
> shouldn't be a big change compared to what you already have all we
> would be doing is:

Yes, constructing of valid SDP record is needed to done by bluez. But
agent API can provide: version, feature bits (and possibly channels).

> 1. version = MIN(remote_version, local_version)
> 2. features =  default_features(version)
> 3. features = get_features()

Problem is that AG features are different of HF features. And agent
provides via DBus API only its features (AG part). So bluez does not
have all information from agent to do mix of features supported by
remote side and client side. So with current bluez DBus API it is
impossible.

> 4. NewConnectiion(version, features)
> 
> Then all the agents out there would don't need to keep doing on their own.

Yes, if existing bluez API would force agents to provide all needed
informations (both local and remote feature bits), then bluez can
implement that mixing of features. But because bluez API is backward
compatible, current implementation does not support it, it is not
possible to implement. And so agents need to implement that feature bits
exchange by its own.

> > > exactly why we want the daemon to control the sdp record so we don't
> > > have the agents dealing version compatibility, etc.
> > >
> > > > > > > I that was
> > > > > > > really necessary Id just dump the whole record as a ServiceRecord
> > > > > > > option and leave it up to the client to parse it.
> > > > > > >
> > > > > > > > > > I can put these default values in profile-role specific macros e.g.:
> > > > > > > > > >
> > > > > > > > > > #define HFP_AG_DEFAULT_FEATURES 0x09
> > > > > > > > > > #define HFP_HF_DEFAULT_FEATURES 0x00
> > > > > > > > > > #define HSP_HS_DEFAULT_VOLCNTRL "false"
> > > > > > > > >
> > > > > > > > > Don't bother with default that are 0x00/false, we can assume this is
> > > > > > > > > implicit when allocating the memory everything is set to 0 so these
> > > > > > > > > defines wouldn't be needed in the first place.
> > > > > > > > >
> > > > > > > > > > Or do you prefer different names?
> > > > > > > > >
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > --
> > > > > > > > > Luiz Augusto von Dentz
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > --
> > > > > > > Luiz Augusto von Dentz
> > > > >
> > > > >
> > > > >
> > > > > --
> > > > > Luiz Augusto von Dentz
> > >
> > >
> > >
> > > --
> > > Luiz Augusto von Dentz
> 
> 
> 
> -- 
> Luiz Augusto von Dentz

[PATCH 2/3] src/profile: Export Remote Audio Volume Control SDP value for HSP HS role via first bit in features value

[Pali Rohár]

Remote Audio Volume Control property in SDP record for HSP HS role
indicates if device supports volume control.

It is required for DBus agents which implements audio part of HSP
profile to know if remote device supports volume control or not.

With this change bluez exports status of this SDP property via firt bit
in Features entry in NewConnection() DBus callback method, like for HFP
profile.

Signed-off-by: Pali Rohár <pali@kernel.org>
---
 src/profile.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/src/profile.c b/src/profile.c
index 884440408..3b7e08f26 100644
--- a/src/profile.c
+++ b/src/profile.c
@@ -923,10 +923,26 @@ static void append_prop(gpointer a, gpointer b)
 }
 
 static uint16_t get_supported_features(const sdp_record_t *rec,
-					bool *have_features)
+					const char *uuid, bool *have_features)
 {
 	sdp_data_t *data;
 
+	if (strcasecmp(uuid, HSP_AG_UUID) == 0) {
+		/* HSP AG role does not provide any features */
+		*have_features = false;
+		return 0;
+	} else if (strcasecmp(uuid, HSP_HS_UUID) == 0) {
+		/* HSP HS role provides Remote Audio Volume Control */
+		data = sdp_data_get(rec, SDP_ATTR_REMOTE_AUDIO_VOLUME_CONTROL);
+		if (!data || data->dtd != SDP_BOOL) {
+			*have_features = false;
+			return 0;
+		} else {
+			*have_features = true;
+			return data->val.int8 ? 0x1 : 0x0;
+		}
+	}
+
 	data = sdp_data_get(rec, SDP_ATTR_SUPPORTED_FEATURES);
 	if (!data || data->dtd != SDP_UINT16) {
 		*have_features = false;
@@ -979,7 +995,7 @@ static bool send_new_connection(struct ext_profile *ext, struct ext_io *conn)
 		rec = btd_device_get_record(conn->device, remote_uuid);
 		if (rec) {
 			conn->features = get_supported_features(rec,
-							&conn->have_features);
+					remote_uuid, &conn->have_features);
 			conn->version = get_profile_version(rec);
 		}
 	}
@@ -1596,7 +1612,7 @@ static void record_cb(sdp_list_t *recs, int err, gpointer user_data)
 		if (conn->psm == 0 && sdp_get_proto_desc(protos, OBEX_UUID))
 			conn->psm = get_goep_l2cap_psm(rec);
 
-		conn->features = get_supported_features(rec,
+		conn->features = get_supported_features(rec, ext->remote_uuid,
 							&conn->have_features);
 		conn->version = get_profile_version(rec);
 
-- 
2.20.1

[PATCH 3/3] src/profile: Add default SDP record for Headset role of HSP 1.2 profile with Erratum 3507

[Pali Rohár]

This would allow DBus agents to implement HS role of HSP profile.

Signed-off-by: Pali Rohár <pali@kernel.org>
---
 src/profile.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 64 insertions(+)

diff --git a/src/profile.c b/src/profile.c
index 3b7e08f26..09627fbbd 100644
--- a/src/profile.c
+++ b/src/profile.c
@@ -56,6 +56,7 @@
 
 #define DUN_DEFAULT_CHANNEL	1
 #define SPP_DEFAULT_CHANNEL	3
+#define HSP_HS_DEFAULT_CHANNEL	6
 #define HFP_HF_DEFAULT_CHANNEL	7
 #define OPP_DEFAULT_CHANNEL	9
 #define FTP_DEFAULT_CHANNEL	10
@@ -155,6 +156,49 @@
 		</attribute>						\
 	</record>"
 
+/* SDP record for Headset role of HSP 1.2 profile with Erratum 3507 */
+#define HSP_HS_RECORD							\
+	"<?xml version=\"1.0\" encoding=\"UTF-8\" ?>			\
+	<record>							\
+		<attribute id=\"0x0001\">				\
+			<sequence>					\
+				<uuid value=\"0x1108\" />		\
+				<uuid value=\"0x1131\" />		\
+				<uuid value=\"0x1203\" />		\
+			</sequence>					\
+		</attribute>						\
+		<attribute id=\"0x0004\">				\
+			<sequence>					\
+				<sequence>				\
+					<uuid value=\"0x0100\" />	\
+				</sequence>				\
+				<sequence>				\
+					<uuid value=\"0x0003\" />	\
+					<uint8 value=\"0x%02x\" />	\
+				</sequence>				\
+			</sequence>					\
+		</attribute>						\
+		<attribute id=\"0x0005\">				\
+			<sequence>					\
+				<uuid value=\"0x1002\" />		\
+			</sequence>					\
+		</attribute>						\
+		<attribute id=\"0x0009\">				\
+			<sequence>					\
+				<sequence>				\
+					<uuid value=\"0x1108\" />	\
+					<uint16 value=\"0x%04x\" />	\
+				</sequence>				\
+			</sequence>					\
+		</attribute>						\
+		<attribute id=\"0x0100\">				\
+			<text value=\"%s\" />				\
+		</attribute>						\
+		<attribute id=\"0x0302\">				\
+			<boolean value=\"%s\" />			\
+		</attribute>						\
+	</record>"
+
 #define HSP_AG_RECORD							\
 	"<?xml version=\"1.0\" encoding=\"UTF-8\" ?>			\
 	<record>							\
@@ -1789,6 +1833,16 @@ static char *get_hfp_ag_record(struct ext_profile *ext, struct ext_io *l2cap,
 				ext->have_features ? ext->features : 0x9);
 }
 
+static char *get_hsp_hs_record(struct ext_profile *ext, struct ext_io *l2cap,
+							struct ext_io *rfcomm)
+{
+	/* HSP 1.2: By default Remote Audio Volume Control is off */
+	return g_strdup_printf(HSP_HS_RECORD, rfcomm->chan, ext->version,
+				ext->name,
+				(ext->have_features && (ext->features & 0x1))
+				? "true" : "false");
+}
+
 static char *get_hsp_ag_record(struct ext_profile *ext, struct ext_io *l2cap,
 							struct ext_io *rfcomm)
 {
@@ -2012,6 +2066,16 @@ static struct default_settings {
 		.auto_connect	= true,
 		.get_record	= get_hfp_ag_record,
 		.version	= 0x0107,
+	}, {
+		.uuid		= HSP_HS_UUID,
+		.name		= "Headset unit",
+		.priority	= BTD_PROFILE_PRIORITY_HIGH,
+		.remote_uuid	= HSP_AG_UUID,
+		.channel	= HSP_HS_DEFAULT_CHANNEL,
+		.authorize	= true,
+		.auto_connect	= true,
+		.get_record	= get_hsp_hs_record,
+		.version	= 0x0102,
 	}, {
 		.uuid		= HSP_AG_UUID,
 		.name		= "Headset Voice gateway",
-- 
2.20.1

Re: [PATCH 3/3] src/profile: Add default SDP record for Headset role of HSP 1.2 profile with Erratum 3507

[Luiz Augusto von Dentz]

Hi Pali,

On Mon, Apr 13, 2020 at 9:25 AM Pali Rohár <pali@kernel.org> wrote:
>
> This would allow DBus agents to implement HS role of HSP profile.
>

We don't use Signed-off-by on userspace, other than that and the
introduction of have_features Im fine with the patches.

> Signed-off-by: Pali Rohár <pali@kernel.org>
> ---
>  src/profile.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 64 insertions(+)
>
> diff --git a/src/profile.c b/src/profile.c
> index 3b7e08f26..09627fbbd 100644
> --- a/src/profile.c
> +++ b/src/profile.c
> @@ -56,6 +56,7 @@
>
>  #define DUN_DEFAULT_CHANNEL    1
>  #define SPP_DEFAULT_CHANNEL    3
> +#define HSP_HS_DEFAULT_CHANNEL 6
>  #define HFP_HF_DEFAULT_CHANNEL 7
>  #define OPP_DEFAULT_CHANNEL    9
>  #define FTP_DEFAULT_CHANNEL    10
> @@ -155,6 +156,49 @@
>                 </attribute>                                            \
>         </record>"
>
> +/* SDP record for Headset role of HSP 1.2 profile with Erratum 3507 */
> +#define HSP_HS_RECORD                                                  \
> +       "<?xml version=\"1.0\" encoding=\"UTF-8\" ?>                    \
> +       <record>                                                        \
> +               <attribute id=\"0x0001\">                               \
> +                       <sequence>                                      \
> +                               <uuid value=\"0x1108\" />               \
> +                               <uuid value=\"0x1131\" />               \
> +                               <uuid value=\"0x1203\" />               \
> +                       </sequence>                                     \
> +               </attribute>                                            \
> +               <attribute id=\"0x0004\">                               \
> +                       <sequence>                                      \
> +                               <sequence>                              \
> +                                       <uuid value=\"0x0100\" />       \
> +                               </sequence>                             \
> +                               <sequence>                              \
> +                                       <uuid value=\"0x0003\" />       \
> +                                       <uint8 value=\"0x%02x\" />      \
> +                               </sequence>                             \
> +                       </sequence>                                     \
> +               </attribute>                                            \
> +               <attribute id=\"0x0005\">                               \
> +                       <sequence>                                      \
> +                               <uuid value=\"0x1002\" />               \
> +                       </sequence>                                     \
> +               </attribute>                                            \
> +               <attribute id=\"0x0009\">                               \
> +                       <sequence>                                      \
> +                               <sequence>                              \
> +                                       <uuid value=\"0x1108\" />       \
> +                                       <uint16 value=\"0x%04x\" />     \
> +                               </sequence>                             \
> +                       </sequence>                                     \
> +               </attribute>                                            \
> +               <attribute id=\"0x0100\">                               \
> +                       <text value=\"%s\" />                           \
> +               </attribute>                                            \
> +               <attribute id=\"0x0302\">                               \
> +                       <boolean value=\"%s\" />                        \
> +               </attribute>                                            \
> +       </record>"
> +
>  #define HSP_AG_RECORD                                                  \
>         "<?xml version=\"1.0\" encoding=\"UTF-8\" ?>                    \
>         <record>                                                        \
> @@ -1789,6 +1833,16 @@ static char *get_hfp_ag_record(struct ext_profile *ext, struct ext_io *l2cap,
>                                 ext->have_features ? ext->features : 0x9);
>  }
>
> +static char *get_hsp_hs_record(struct ext_profile *ext, struct ext_io *l2cap,
> +                                                       struct ext_io *rfcomm)
> +{
> +       /* HSP 1.2: By default Remote Audio Volume Control is off */
> +       return g_strdup_printf(HSP_HS_RECORD, rfcomm->chan, ext->version,
> +                               ext->name,
> +                               (ext->have_features && (ext->features & 0x1))
> +                               ? "true" : "false");
> +}
> +
>  static char *get_hsp_ag_record(struct ext_profile *ext, struct ext_io *l2cap,
>                                                         struct ext_io *rfcomm)
>  {
> @@ -2012,6 +2066,16 @@ static struct default_settings {
>                 .auto_connect   = true,
>                 .get_record     = get_hfp_ag_record,
>                 .version        = 0x0107,
> +       }, {
> +               .uuid           = HSP_HS_UUID,
> +               .name           = "Headset unit",
> +               .priority       = BTD_PROFILE_PRIORITY_HIGH,
> +               .remote_uuid    = HSP_AG_UUID,
> +               .channel        = HSP_HS_DEFAULT_CHANNEL,
> +               .authorize      = true,
> +               .auto_connect   = true,
> +               .get_record     = get_hsp_hs_record,
> +               .version        = 0x0102,
>         }, {
>                 .uuid           = HSP_AG_UUID,
>                 .name           = "Headset Voice gateway",
> --
> 2.20.1
>


-- 
Luiz Augusto von Dentz

Re: [PATCH 0/3] bluez: Export SDP "Remote audio volume control" item for HSP profile

[Luiz Augusto von Dentz]

Hi Pali,

On Mon, Apr 13, 2020 at 9:25 AM Pali Rohár <pali@kernel.org> wrote:
>
> This patch series fixes handling of zero value in feature list and
> provides Remote audio volume control support for HSP profile in both HS
> and AG roles.
>
> Luiz, you wrote that you do not have time to work on this, so I
> implemented it myself in this patch series. Could you please find time
> at least for reviewing and merging these patches? Thanks.
>
> Pali Rohár (3):
>   src/profile: Distinguish between zero-set HFP AG features and unset
>     HFP AG features
>   src/profile: Export Remote Audio Volume Control SDP value for HSP HS
>     role via first bit in features value
>   src/profile: Add default SDP record for Headset role of HSP 1.2
>     profile with Erratum 3507
>
>  src/profile.c | 112 ++++++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 104 insertions(+), 8 deletions(-)
>
> --
> 2.20.1

Ive make some changes to remove the need to a flag to detect when the
features should be added or not, it is now applied, thanks.

-- 
Luiz Augusto von Dentz

Re: [PATCH 0/3] bluez: Export SDP "Remote audio volume control" item for HSP profile

[Pali Rohár]

On Monday 13 April 2020 17:09:42 Luiz Augusto von Dentz wrote:
> Hi Pali,
> 
> On Mon, Apr 13, 2020 at 9:25 AM Pali Rohár <pali@kernel.org> wrote:
> >
> > This patch series fixes handling of zero value in feature list and
> > provides Remote audio volume control support for HSP profile in both HS
> > and AG roles.
> >
> > Luiz, you wrote that you do not have time to work on this, so I
> > implemented it myself in this patch series. Could you please find time
> > at least for reviewing and merging these patches? Thanks.
> >
> > Pali Rohár (3):
> >   src/profile: Distinguish between zero-set HFP AG features and unset
> >     HFP AG features
> >   src/profile: Export Remote Audio Volume Control SDP value for HSP HS
> >     role via first bit in features value
> >   src/profile: Add default SDP record for Headset role of HSP 1.2
> >     profile with Erratum 3507
> >
> >  src/profile.c | 112 ++++++++++++++++++++++++++++++++++++++++++++++----
> >  1 file changed, 104 insertions(+), 8 deletions(-)
> >
> > --
> > 2.20.1
> 
> Ive make some changes to remove the need to a flag to detect when the
> features should be added or not, it is now applied, thanks.

Ok, thank you!

In commit you did one mistake:

https://git.kernel.org/pub/scm/bluetooth/bluez.git/commit/?id=040bd56a948f4d1ecd6987cdf0ba51779dc0c02a

+		/* HSP AG role does not provide any features */
+		return 0;

It should return -ENODATA as AG role does not provide any features, so
has_features needs to be set to false.

Re: [PATCH 0/3] bluez: Export SDP "Remote audio volume control" item for HSP profile

[Pali Rohár]

Hello Luiz!

I have there another simple change for updating documentation:

diff --git a/doc/profile-api.txt b/doc/profile-api.txt
index ec18034a6..183c6c11a 100644
--- a/doc/profile-api.txt
+++ b/doc/profile-api.txt
@@ -16,10 +16,33 @@ Object path	/org/bluez
 			If an application disconnects from the bus all
 			its registered profiles will be removed.
 
+			Some predefined services:
+
+			HFP AG UUID: 0000111f-0000-1000-8000-00805f9b34fb
+
+				Default profile Version is 1.7, profile Features
+				is 0b001001 and RFCOMM channel is 13.
+				Authentication is required.
+
 			HFP HS UUID: 0000111e-0000-1000-8000-00805f9b34fb
 
-				Default RFCOMM channel is 6. And this requires
-				authentication.
+				Default profile Version is 1.7, profile Features
+				is 0b000000 and RFCOMM channel is 7.
+				Authentication is required.
+
+			HSP AG UUID: 00001112-0000-1000-8000-00805f9b34fb
+
+				Default profile Version is 1.2, RFCOMM channel
+				is 12 and Authentication is required. Does not
+				support any Features, option is ignored.
+
+			HSP HS UUID: 00001108-0000-1000-8000-00805f9b34fb
+
+				Default profile Version is 1.2, profile Features
+				is 0b0 and RFCOMM channel is 6. Authentication
+				is required. Features is one bit value, specify
+				capability of Remote Audio Volume Control
+				(by default turned off).
 
 			Available options:
 


There is a mistake in documentation that default rfcomm channel for HFP
HS profile is 6. But in reality default rfcomm channel for HFP is 7.
Channel 6 is default for HSP HS profile as can be seen in src/profile.c.

Re: [PATCH 0/3] bluez: Export SDP "Remote audio volume control" item for HSP profile

[Luiz Augusto von Dentz]

Hi Pali,

Can you send a proper patch adding these, have you changed the code
already to use channel 7?

On Tue, Apr 14, 2020 at 12:53 PM Pali Rohár <pali@kernel.org> wrote:
>
> Hello Luiz!
>
> I have there another simple change for updating documentation:
>
> diff --git a/doc/profile-api.txt b/doc/profile-api.txt
> index ec18034a6..183c6c11a 100644
> --- a/doc/profile-api.txt
> +++ b/doc/profile-api.txt
> @@ -16,10 +16,33 @@ Object path /org/bluez
>                         If an application disconnects from the bus all
>                         its registered profiles will be removed.
>
> +                       Some predefined services:
> +
> +                       HFP AG UUID: 0000111f-0000-1000-8000-00805f9b34fb
> +
> +                               Default profile Version is 1.7, profile Features
> +                               is 0b001001 and RFCOMM channel is 13.
> +                               Authentication is required.
> +
>                         HFP HS UUID: 0000111e-0000-1000-8000-00805f9b34fb
>
> -                               Default RFCOMM channel is 6. And this requires
> -                               authentication.
> +                               Default profile Version is 1.7, profile Features
> +                               is 0b000000 and RFCOMM channel is 7.
> +                               Authentication is required.
> +
> +                       HSP AG UUID: 00001112-0000-1000-8000-00805f9b34fb
> +
> +                               Default profile Version is 1.2, RFCOMM channel
> +                               is 12 and Authentication is required. Does not
> +                               support any Features, option is ignored.
> +
> +                       HSP HS UUID: 00001108-0000-1000-8000-00805f9b34fb
> +
> +                               Default profile Version is 1.2, profile Features
> +                               is 0b0 and RFCOMM channel is 6. Authentication
> +                               is required. Features is one bit value, specify
> +                               capability of Remote Audio Volume Control
> +                               (by default turned off).
>
>                         Available options:
>
>
>
> There is a mistake in documentation that default rfcomm channel for HFP
> HS profile is 6. But in reality default rfcomm channel for HFP is 7.
> Channel 6 is default for HSP HS profile as can be seen in src/profile.c.



-- 
Luiz Augusto von Dentz

[PATCH] doc: Update documentation for HSP and HFP profiles

[Pali Rohár]

Fix information about default rfcomm channel number in HFP HS role
(it is 7, not 6) and add documentation about default values.
---
 doc/profile-api.txt | 27 +++++++++++++++++++++++++--
 1 file changed, 25 insertions(+), 2 deletions(-)

diff --git a/doc/profile-api.txt b/doc/profile-api.txt
index ec18034..183c6c1 100644
--- a/doc/profile-api.txt
+++ b/doc/profile-api.txt
@@ -16,10 +16,33 @@ Object path	/org/bluez
 			If an application disconnects from the bus all
 			its registered profiles will be removed.
 
+			Some predefined services:
+
+			HFP AG UUID: 0000111f-0000-1000-8000-00805f9b34fb
+
+				Default profile Version is 1.7, profile Features
+				is 0b001001 and RFCOMM channel is 13.
+				Authentication is required.
+
 			HFP HS UUID: 0000111e-0000-1000-8000-00805f9b34fb
 
-				Default RFCOMM channel is 6. And this requires
-				authentication.
+				Default profile Version is 1.7, profile Features
+				is 0b000000 and RFCOMM channel is 7.
+				Authentication is required.
+
+			HSP AG UUID: 00001112-0000-1000-8000-00805f9b34fb
+
+				Default profile Version is 1.2, RFCOMM channel
+				is 12 and Authentication is required. Does not
+				support any Features, option is ignored.
+
+			HSP HS UUID: 00001108-0000-1000-8000-00805f9b34fb
+
+				Default profile Version is 1.2, profile Features
+				is 0b0 and RFCOMM channel is 6. Authentication
+				is required. Features is one bit value, specify
+				capability of Remote Audio Volume Control
+				(by default turned off).
 
 			Available options:
 
-- 
2.20.1

Re: [PATCH] doc: Update documentation for HSP and HFP profiles

[Luiz Augusto von Dentz]

Hi Pali,

On Tue, Apr 14, 2020 at 1:46 PM Pali Rohár <pali@kernel.org> wrote:
>
> Fix information about default rfcomm channel number in HFP HS role
> (it is 7, not 6) and add documentation about default values.
> ---
>  doc/profile-api.txt | 27 +++++++++++++++++++++++++--
>  1 file changed, 25 insertions(+), 2 deletions(-)
>
> diff --git a/doc/profile-api.txt b/doc/profile-api.txt
> index ec18034..183c6c1 100644
> --- a/doc/profile-api.txt
> +++ b/doc/profile-api.txt
> @@ -16,10 +16,33 @@ Object path /org/bluez
>                         If an application disconnects from the bus all
>                         its registered profiles will be removed.
>
> +                       Some predefined services:
> +
> +                       HFP AG UUID: 0000111f-0000-1000-8000-00805f9b34fb
> +
> +                               Default profile Version is 1.7, profile Features
> +                               is 0b001001 and RFCOMM channel is 13.
> +                               Authentication is required.
> +
>                         HFP HS UUID: 0000111e-0000-1000-8000-00805f9b34fb
>
> -                               Default RFCOMM channel is 6. And this requires
> -                               authentication.
> +                               Default profile Version is 1.7, profile Features
> +                               is 0b000000 and RFCOMM channel is 7.
> +                               Authentication is required.
> +
> +                       HSP AG UUID: 00001112-0000-1000-8000-00805f9b34fb
> +
> +                               Default profile Version is 1.2, RFCOMM channel
> +                               is 12 and Authentication is required. Does not
> +                               support any Features, option is ignored.
> +
> +                       HSP HS UUID: 00001108-0000-1000-8000-00805f9b34fb
> +
> +                               Default profile Version is 1.2, profile Features
> +                               is 0b0 and RFCOMM channel is 6. Authentication
> +                               is required. Features is one bit value, specify
> +                               capability of Remote Audio Volume Control
> +                               (by default turned off).
>
>                         Available options:
>
> --
> 2.20.1

Applied, thanks.

-- 
Luiz Augusto von Dentz