Hi Luiz,
>>>>>>>>>>>>> This change adds an adapter Api to report the controller's
>>>>>>>>>>>>> widebandspeech enabled state.
>>>>>>>>>>>
>>>>>>>>>>> I wonder if this shouldn't be queried over SCO socket, or simple fail
>>>>>>>>>>> with BT_VOICE when using BT_VOICE_TRANSPARENT which is how what is
>>>>>>>>>>> normally used when using mSBC.
>>>>>>>>>>
>>>>>>>>>> I think there is value in both.
>>>>>>>>>
>>>>>>>>> Can you expand on that? I think this might generate confusion if the
>>>>>>>>> property indicates support for it but HFP implementation don't support
>>>>>>>>> it, since the later is usually implemented as a external profile so we
>>>>>>>>> don't have the features it may support, or perhaps the intention here
>>>>>>>>> is to actually indicate when it is in use?
>>>>>>>>
>>>>>>>> This is a signal that the adapter supports it and has everything
>>>>>>>> enabled to support it. Driver indicated it supports it and erroneous
>>>>>>>> data reporting was enabled. The profile has it's own state which may
>>>>>>>> indicate if msbc will be used, but this will be on a per connection
>>>>>>>> basis and is independent from this adapter property.
>>>>>>>>
>>>>>>>> The value in this property is to support diagnostic UX about
>>>>>>>> controller capabilities/state and also allow profiles that are
>>>>>>>> implemented outside of bluetoothd to see which codec it can attempt to
>>>>>>>> negotiate with the device.
>>>>>>>
>>>>>>> For diagnosic I think we would be better off with some dedicated
>>>>>>> interface to query this, as for the later the information we are
>>>>>>> giving does not actually tell anything about the codec support, which
>>>>>>> was part of my original argument that for the likes of HFP and other
>>>>>>> profiles using it it might not be enough and they still need to use
>>>>>>> BT_VOICE in order to enable the use of custom codecs, if you take
>>>>>>> ofono for example it does implement support for wideband speech
>>>>>>> already and it would completely disregard this property which can give
>>>>>>> the false impression that wideband speech cannot be activated when in
>>>>>>> fact it can, it just don't have erroneous data reporting enable, so
>>>>>>> perhaps we should indicate the actual adapter feature (e.g.
>>>>>>> ErrnoneousDataReporting) not the profile feature here, so the profile
>>>>>>> implementation can check weather this would disable use of wideband
>>>>>>> speech or not, futhermore we should probably report the errors back to
>>>>>>> the SCO socket or is that just for diagnostic and cannot be used to
>>>>>>> adjust the streaming?
>>>>>>
>>>>>> My original patch actually had this MGMT feature called erroneous data
>>>>>> reporting and Marcel argued against it. If you both agree, then I'm
>>>>>> happy to rename all of this to erroneous data reporting. We'd still
>>>>>> need some way for the driver support to be messaged some other way
>>>>>> though. see:
>>>>>>
>>>>>> if (id->driver_info & BTUSB_WIDEBAND_SPEECH)
>>>>>> set_bit(HCI_QUIRK_WIDEBAND_SPEECH_SUPPORTED, &hdev->quirks);
>>>
>>> So this will get a little bit confusing if we don't have a HFP
>>> implementation that actually does implement the profile parts to
>>> enable wideband speech so I wonder if we should rename it to the
>>> underline feature it enables, that said this sort of feature is
>>> probably easier to be queried over the socket itself rather than over
>>> D-Bus so it can be used in conjunction with the likes of BT_VOICE,
>>> also regarding the erroneous data should that be also enabled by the
>>> HFP profile, because depending on the platform it may not support
>>> wideband speech so enabling erroneous data automatically may result in
>>> artifacs, specially in cases where the codecs don't have error
>>> correction or data loss concealment to handle data with possible error
>>> and lost data respectively, btw we don't seem to be parsing the
>>> SCO/ESCO flags and even if we do this properly we need to find a way
>>> to notify them over the socket so things like plc works properly.
>>
>> This indeed makes sense, but we'll need to decide how erroneous data
>> reporting gets enabled. The SCO socket is a per connection scope
>> thing while erroneous data reporting is a controller level thing.
>
> The way I would have done this is to make use of recvmsg and then add
> the packet flags as msg_flags:
>
> recvmsg()
> The recvmsg() call uses a msghdr structure to minimize the
> number of directly supplied arguments. This structure is defined as
> follows in <sys/socket.h>:
>
> struct iovec { /* Scatter/gather array items */
> void *iov_base; /* Starting address */
> size_t iov_len; /* Number of bytes to transfer */
> };
>
> struct msghdr {
> void *msg_name; /* Optional address */
> socklen_t msg_namelen; /* Size of address */
> struct iovec *msg_iov; /* Scatter/gather array */
> size_t msg_iovlen; /* # elements in msg_iov */
> void *msg_control; /* Ancillary data, see below */
> size_t msg_controllen; /* Ancillary data buffer len */
> int msg_flags; /* Flags on received message */
> };
>
> But to make this backward compatible I would recommend adding a
> socketopt that enables this new behavior and in case there is more
> than one SCO socket those that did not use the sockopt should probably
> drop packets with error or lost data so they work as without erroneous
> data report since in that case the application will likely be using
> regular reads it won't be able to detect the use of msg_flags.
I think that I already send an example on how to do that a while ago. Don’t remember if that was a private email or addressed to the mailing list, but I remember showing how this can be done. Or I confused this with something similar :(
Regards
Marcel
