[PATCH 37/56] bluetooth: Parse BlueZ 5 device properties

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Jul 25, 2013 at 1:44 AM, Jo?o Paulo Rechi Vita
<jprvita at gmail.com> wrote:
> On Mon, Jul 22, 2013 at 7:33 AM, Tanu Kaskinen
> <tanu.kaskinen at linux.intel.com> wrote:
>> On Fri, 2013-07-12 at 15:06 -0300, jprvita at gmail.com wrote:
>>> +static int parse_device_property(pa_bluetooth_device *d, DBusMessageIter *i, bool is_property_change) {
>>> +    const char *key;
>>> +    DBusMessageIter variant_i;
>>> +
>>> +    pa_assert(d);
>>> +
>>> +    key = pa_bluetooth_dbus_check_variant_property(i);
>>> +    if (key == NULL) {
>>> +        pa_log_error("Received invalid property for device %s", d->path);
>>> +        return -1;
>>> +    }
>>> +
>>> +    dbus_message_iter_recurse(i, &variant_i);
>>> +
>>> +    switch (dbus_message_iter_get_arg_type(&variant_i)) {
>>> +
>>> +        case DBUS_TYPE_STRING: {
>>> +            const char *value;
>>> +            dbus_message_iter_get_basic(&variant_i, &value);
>>> +
>>> +            if (pa_streq(key, "Alias")) {
>>> +                pa_xfree(d->alias);
>>> +                d->alias = pa_xstrdup(value);
>>> +            } else if (pa_streq(key, "Address")) {
>>> +                if (is_property_change) {
>>> +                    pa_log_error("Device property 'Address' expected to be constant but changed for %s", d->path);
>>> +                    return -1;
>>> +                }
>>> +
>>> +                if (d->address) {
>>> +                    pa_log_error("Device %s: Received a duplicate Address property.", d->path);
>>> +                    return -1;
>>> +                }
>>> +
>>> +                d->address = pa_xstrdup(value);
>>> +            }
>>> +
>>> +            pa_log_debug("%s: %s", key, value);
>>
>> Please use proper sentences in the log. Or is this a temporary log
>> message that you just forgot to remove from the final patch?
>>
>
> This is usually received when the device is created, together with a
> lot of other properties, so I think is better to minimize verbosity
> here. What I'll change to improve this scenario is add a log in the
> beginning of the function with the device path.
>
>>> +            break;
>>> +        }
>>> +
>>> +        case DBUS_TYPE_UINT32: {
>>> +            uint32_t value;
>>> +            dbus_message_iter_get_basic(&variant_i, &value);
>>> +
>>> +            if (pa_streq(key, "Class"))
>>> +                d->class_of_device = (int) value;
>>
>> Is the device allowed to change its class at runtime? If not, then there
>> should be a check for is_property_change like with Address.
>>
>
> I'm not sure, and BlueZ' documentation does't mention anything about
> that. Maybe Luiz have more information about that.
>

Looking into the code, the class of device can change on every new connection.

>>> +
>>> +            pa_log_debug("%s: %d", key, value);
>>> +            break;
>>> +        }
>>> +
>>> +        case DBUS_TYPE_ARRAY: {
>>> +            DBusMessageIter ai;
>>> +            dbus_message_iter_recurse(&variant_i, &ai);
>>> +
>>> +            if (dbus_message_iter_get_arg_type(&ai) == DBUS_TYPE_STRING && pa_streq(key, "UUIDs")) {
>>> +                while (dbus_message_iter_get_arg_type(&ai) != DBUS_TYPE_INVALID) {
>>> +                    pa_bluetooth_uuid *node;
>>> +                    const char *value;
>>> +
>>> +                    dbus_message_iter_get_basic(&ai, &value);
>>> +
>>> +                    if (pa_bluetooth_uuid_has(d->uuids, value)) {
>>> +                        dbus_message_iter_next(&ai);
>>> +                        continue;
>>> +                    }
>>> +
>>> +                    node = pa_bluetooth_uuid_new(value);
>>> +                    PA_LLIST_PREPEND(pa_bluetooth_uuid, d->uuids, node);
>>> +
>>> +                    pa_log_debug("%s: %s", key, value);
>>> +                    dbus_message_iter_next(&ai);
>>
>> This code only adds UUIDs to pa_bluetooth_device, never removes them. Is
>> this intentional? If so, I think a comment explaining the rationale
>> would be good.
>>
>> There aren't any hooks that would be fired when the properties (Alias,
>> Class, UUIDs) change. Perhaps you implement them in some later patches?
>>
>
> This is used to create card profiles on load of module-bluez5-device.
> It is possible (although not likely) that a device change the
> implemented UUIDs during the connection lifetime. Alias can change,
> but I think this should be handled in a separate patch (I've not
> written that yet).
>
>>> +                }
>>> +            }
>>> +
>>> +            break;
>>> +        }
>>> +    }
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static int parse_device_properties(pa_bluetooth_device *d, DBusMessageIter *i, bool is_property_change) {
>>> +    DBusMessageIter element_i;
>>> +    int ret = 0;
>>> +
>>> +    dbus_message_iter_recurse(i, &element_i);
>>> +
>>> +    while (dbus_message_iter_get_arg_type(&element_i) == DBUS_TYPE_DICT_ENTRY) {
>>> +        DBusMessageIter dict_i;
>>> +
>>> +        dbus_message_iter_recurse(&element_i, &dict_i);
>>> +
>>> +        if (parse_device_property(d, &dict_i, is_property_change) < 0)
>>> +            ret = -1;
>>> +
>>> +        dbus_message_iter_next(&element_i);
>>> +    }
>>> +
>>> +    if (!d->address || !d->alias/* || d->paired < 0 || d->trusted < 0*/) {
>>
>> If you don't plan to support the paired and trusted properties (which
>> would be a good plan IMO), please remove the commented out code.
>>
>
> Yes, I've forgot to remove that. There is no use for the Paired and
> Trusted properties in PulseAudio.
>
>>> +        pa_log_error("Non-optional information missing for device %s", d->path);
>>> +        d->device_info_valid = -1;
>>> +        return -1;
>>> +    }
>>> +
>>> +    d->device_info_valid = 1;
>>> +    return ret;
>>> +}
>>> +
>>>  static void register_endpoint_reply(DBusPendingCall *pending, void *userdata) {
>>>      DBusMessage *r;
>>>      pa_dbus_pending *p;
>>> @@ -423,6 +595,8 @@ static void parse_interfaces_and_properties(pa_bluetooth_discovery *y, DBusMessa
>>>              register_endpoint(y, path, A2DP_SOURCE_ENDPOINT, A2DP_SOURCE_UUID);
>>>              register_endpoint(y, path, A2DP_SINK_ENDPOINT, A2DP_SINK_UUID);
>>>          } else if (pa_streq(interface, BLUEZ_DEVICE_INTERFACE)) {
>>> +            pa_bluetooth_device *d;
>>> +
>>>              if (pa_bluetooth_discovery_get_device_by_path(y, path)) {
>>>                  pa_log_error("Found duplicated D-Bus path for device %s", path);
>>>                  return;
>>> @@ -430,8 +604,8 @@ static void parse_interfaces_and_properties(pa_bluetooth_discovery *y, DBusMessa
>>>
>>>              pa_log_debug("Device %s found", path);
>>>
>>> -            pa_bluetooth_discovery_create_device(y, path);
>>> -            /* TODO: parse device properties */
>>> +            d = pa_bluetooth_discovery_create_device(y, path);
>>> +            parse_device_properties(d, &iface_i, false);
>>
>> If parsing the device properties fails, we shouldn't create the device.
>>
>
> parse_device_properties() will fail even if the parsing of an optional
> property fails, but the device is still fully usable, so we should
> still create the device in this case. To differentiate between these
> two situations there is the device_info_valid field. Maybe we can
> review the necessity of the device_info_valid field, but since this
> affects the modules working logic and may introduce bugs I would
> prefer to change that in a latter moment to minimize the impact.
>
>>>          } else
>>>              pa_log_debug("Unknown interface %s found, skipping", interface);
>>>
>>> diff --git a/src/modules/bluetooth/bluez5-util.h b/src/modules/bluetooth/bluez5-util.h
>>> index 68c2dc0..b1a112f 100644
>>> --- a/src/modules/bluetooth/bluez5-util.h
>>> +++ b/src/modules/bluetooth/bluez5-util.h
>>> @@ -27,6 +27,7 @@
>>>  #define A2DP_SOURCE_UUID        "0000110a-0000-1000-8000-00805f9b34fb"
>>>  #define A2DP_SINK_UUID          "0000110b-0000-1000-8000-00805f9b34fb"
>>>
>>> +typedef struct pa_bluetooth_uuid pa_bluetooth_uuid;
>>
>> I don't see the point of a separate struct. The purpose seems to be only
>> to make it possible to save the UUIDs in a linked list, but using a
>> hashmap (as a set, i.e. the key and value are the same) would be much
>> simpler.
>>
>
> AFAIK that's the only purpose, so I think it makes sense to change to
> a hashmap as you suggested.
>
> --
> Jo?o Paulo Rechi Vita
> http://about.me/jprvita



-- 
Jo?o Paulo Rechi Vita
http://about.me/jprvita


[Index of Archives]     [Linux Audio Users]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux