| Message ID | 20200413162513.2221-2-pali@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | bluez: Export SDP "Remote audio volume control" item for HSP profile | expand |
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 >
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?
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?
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
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
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
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
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
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
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
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
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
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;
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(-)