Hi Pali,
On Mon, Apr 13, 2020 at 10:17 AM Pali Rohár <pali@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxx> 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
