On Fri, Aug 16, 2013 at 12:32 PM, Tanu Kaskinen
<tanu.kaskinen at linux.intel.com> wrote:
> On Tue, 2013-08-13 at 01:54 -0300, jprvita at gmail.com wrote:
>> From: Jo?o Paulo Rechi Vita <jprvita at openbossa.org>
>>
>> Create the pa_bluetooth_transport structure to store information about
>> the bluetooth transport and utility functions to manipulate this
>> structure. The acquire() and release() operations are function pointers
>> in the pa_bluetooth_transport structure to make possible for different
>> transport backends to provide different implementations of these
>> operations. Thre is also a userdata field for the transport backend
>> provide data for the acquire/release functions.
>> ---
>> src/modules/bluetooth/bluez5-util.c | 145 ++++++++++++++++++++++++++++++++++++
>> src/modules/bluetooth/bluez5-util.h | 43 +++++++++++
>> 2 files changed, 188 insertions(+)
>>
>> diff --git a/src/modules/bluetooth/bluez5-util.c b/src/modules/bluetooth/bluez5-util.c
>> index 1972a92..69eb759 100644
>> --- a/src/modules/bluetooth/bluez5-util.c
>> +++ b/src/modules/bluetooth/bluez5-util.c
>> @@ -36,6 +36,7 @@
>> #include "bluez5-util.h"
>>
>> #define BLUEZ_SERVICE "org.bluez"
>> +#define BLUEZ_MEDIA_TRANSPORT_INTERFACE BLUEZ_SERVICE ".MediaTransport1"
>>
>> struct pa_bluetooth_discovery {
>> PA_REFCNT_DECLARE;
>> @@ -45,8 +46,131 @@ struct pa_bluetooth_discovery {
>> bool filter_added;
>> pa_hook hooks[PA_BLUETOOTH_HOOK_MAX];
>> pa_hashmap *devices;
>> + pa_hashmap *transports;
>> };
>>
>> +pa_bluetooth_transport *pa_bluetooth_transport_new(pa_bluetooth_device *d, const char *owner, const char *path,
>> + pa_bluetooth_profile_t p, const uint8_t *config, size_t size) {
>
> Unaligned parameter list.
>
> There is some precedence for wrapping long parameter lists like this:
>
> pa_bluetooth_transport *pa_bluetooth_transport_new(
> pa_bluetooth_device *d,
> const char *owner,
> const char *path,
> pa_bluetooth_profile_t p,
> const uint8_t *config,
> size_t size) {
>
Ok. Maybe we should add this to the official coding style guidelines
in the wiki, although I personally dislike having the function
parameters aligned in the same level as the function implementation.
My preference would be to have them aligned after the opening
parenthesis.
>> + pa_bluetooth_transport *t;
>> +
>> + t = pa_xnew0(pa_bluetooth_transport, 1);
>> + t->device = d;
>> + t->owner = pa_xstrdup(owner);
>> + t->path = pa_xstrdup(path);
>> + t->profile = p;
>> + t->config_size = size;
>> +
>> + if (size > 0) {
>> + t->config = pa_xnew(uint8_t, size);
>> + memcpy(t->config, config, size);
>> + }
>> +
>> + pa_assert_se(pa_hashmap_put(d->discovery->transports, t->path, t) >= 0);
>> +
>> + return t;
>> +}
>> +
>> +static void transport_state_changed(pa_bluetooth_transport *t, pa_bluetooth_transport_state_t state) {
>> + bool old_any_connected;
>> +
>> + if (t->state == state)
>> + return;
>> +
>> + old_any_connected = pa_bluetooth_device_any_transport_connected(t->device);
>> +
>> + pa_log_debug("Transport %s state changed from %d to %d", t->path, t->state, state);
>
> Human-readable states, please.
>
Ok, forgot that :/
>> +
>> + t->state = state;
>> + pa_hook_fire(&t->device->discovery->hooks[PA_BLUETOOTH_HOOK_TRANSPORT_STATE_CHANGED], t);
>> +
>> + if (old_any_connected != pa_bluetooth_device_any_transport_connected(t->device))
>
> undefined reference to `pa_bluetooth_device_any_transport_connected'
>
It actually makes sense to squash the commit that implements
pa_bluetooth_device_any_transport_connected() on top of this one.
>> + pa_hook_fire(&t->device->discovery->hooks[PA_BLUETOOTH_HOOK_DEVICE_CONNECTION_CHANGED], t->device);
>> +}
>> +
>> +void pa_bluetooth_transport_put(pa_bluetooth_transport *t) {
>> + transport_state_changed(t, PA_BLUETOOTH_TRANSPORT_STATE_IDLE);
>> +}
>> +
>> +void pa_bluetooth_transport_free(pa_bluetooth_transport *t) {
>> + pa_assert(t);
>> +
>> + pa_hashmap_remove(t->device->discovery->transports, t->path);
>> + pa_xfree(t->owner);
>> + pa_xfree(t->path);
>> + pa_xfree(t->config);
>> + pa_xfree(t);
>> +}
>> +
>> +static int bluez5_transport_acquire_cb(pa_bluetooth_transport *t, bool optional, size_t *imtu, size_t *omtu) {
>> + DBusMessage *m, *r;
>> + DBusError err;
>> + int ret;
>> + uint16_t i, o;
>> + const char *method = optional ? "TryAcquire" : "Acquire";
>> +
>> + pa_assert(t);
>> + pa_assert(t->device);
>> + pa_assert(t->device->discovery);
>> +
>> + pa_assert_se(m = dbus_message_new_method_call(t->owner, t->path, BLUEZ_MEDIA_TRANSPORT_INTERFACE, method));
>> +
>> + dbus_error_init(&err);
>> +
>> + r = dbus_connection_send_with_reply_and_block(pa_dbus_connection_get(t->device->discovery->connection), m, -1, &err);
>> + if (!r) {
>> + if (optional && pa_streq(err.name, "org.bluez.Error.NotAvailable"))
>> + pa_log_info("Failed optional acquire of unavailable transport %s", t->path);
>> + else
>> + pa_log_error("Transport %s() failed for transport %s (%s)", method, t->path, err.message);
>> +
>> + dbus_error_free(&err);
>> + return -1;
>> + }
>> +
>> + if (!dbus_message_get_args(r, &err, DBUS_TYPE_UNIX_FD, &ret, DBUS_TYPE_UINT16, &i, DBUS_TYPE_UINT16, &o,
>> + DBUS_TYPE_INVALID)) {
>> + pa_log_error("Failed to parse %s() reply: %s", method, err.message);
>> + dbus_error_free(&err);
>> + ret = -1;
>> + goto finish;
>> + }
>> +
>> + if (imtu)
>> + *imtu = i;
>> +
>> + if (omtu)
>> + *omtu = o;
>> +
>> +finish:
>> + dbus_message_unref(r);
>> + return ret;
>> +}
>> +
>> +static void bluez5_transport_release_cb(pa_bluetooth_transport *t) {
>> + DBusMessage *m;
>> + DBusError err;
>> +
>> + pa_assert(t);
>> + pa_assert(t->device);
>> + pa_assert(t->device->discovery);
>> +
>> + dbus_error_init(&err);
>> +
>> + if (t->state <= PA_BLUETOOTH_TRANSPORT_STATE_IDLE) {
>> + pa_log_info("Transport %s auto-released by BlueZ or already released", t->path);
>> + return;
>> + }
>> +
>> + pa_assert_se(m = dbus_message_new_method_call(t->owner, t->path, BLUEZ_MEDIA_TRANSPORT_INTERFACE, "Release"));
>> + dbus_connection_send_with_reply_and_block(pa_dbus_connection_get(t->device->discovery->connection), m, -1, &err);
>> +
>> + if (dbus_error_is_set(&err)) {
>> + pa_log_error("Failed to release transport %s: %s", t->path, err.message);
>> + dbus_error_free(&err);
>> + } else
>> + pa_log_info("Transport %s released", t->path);
>> +}
>> +
>> static pa_bluetooth_device* device_create(pa_bluetooth_discovery *y, const char *path) {
>> pa_bluetooth_device *d;
>>
>> @@ -93,8 +217,23 @@ pa_bluetooth_device* pa_bluetooth_discovery_get_device_by_address(pa_bluetooth_d
>> }
>>
>> static void device_free(pa_bluetooth_device *d) {
>> + unsigned i;
>> +
>> pa_assert(d);
>>
>> + for (i = 0; i < PA_BLUETOOTH_PROFILE_COUNT; i++) {
>> + pa_bluetooth_transport *t;
>> +
>> + if (!(t = d->transports[i]))
>> + continue;
>> +
>> + d->transports[i] = NULL;
>> + transport_state_changed(t, PA_BLUETOOTH_TRANSPORT_STATE_DISCONNECTED);
>> + /* TODO: check if there is no risk of calling
>> + * transport_connection_changed_cb() when the transport is already freed */
>> + pa_bluetooth_transport_free(t);
>
> IMO pa_bluetooth_transport_free() should be called by the transport
> code, because the backend also creates the transport. The backend may
> need a callback for getting notified about the device going away. Or
> perhaps there should be pa_bluetooth_transport_kill(), with a kill()
> callback in the backend. This would be similar to how sink inputs are
> killed if the sink they're connected to goes away.
>
> If the backend is responsible for calling pa_bluetooth_transport_free(),
> the core should still ensure that t->device is set to NULL and that the
> transport is removed from discovery->transports (feel free to create
> pa_bluetooth_transport_unlink() for this purpose, if you want).
>
I'll answer this comment in the next message in this thread due to the
additional comment in that message.
> As for the TODO item, I don't know what it means. There is no
> transport_connection_changed_cb() function.
>
transport_connection_changed_cb() is the callback in
module-bluez5-device for the hook
PA_BLUETOOTH_HOOK_TRANSPORT_STATE_CHANGED, which is fired by the
transport_state_changed() call right before the TODO line. Depending
on the way hook callbacks are called by the mainloop t may be already
freed by the pa_bluetooth_transport_free() call after the TODO line.
>> + }
>> +
>> d->discovery = NULL;
>> pa_xfree(d->path);
>> pa_xfree(d->alias);
>> @@ -189,6 +328,7 @@ pa_bluetooth_discovery* pa_bluetooth_discovery_get(pa_core *c) {
>> PA_REFCNT_INIT(y);
>> y->core = c;
>> y->devices = pa_hashmap_new(pa_idxset_string_hash_func, pa_idxset_string_compare_func);
>> + y->transports = pa_hashmap_new(pa_idxset_string_hash_func, pa_idxset_string_compare_func);
>>
>> for (i = 0; i < PA_BLUETOOTH_HOOK_MAX; i++)
>> pa_hook_init(&y->hooks[i], y);
>> @@ -249,6 +389,11 @@ void pa_bluetooth_discovery_unref(pa_bluetooth_discovery *y) {
>> pa_hashmap_free(y->devices, NULL);
>> }
>>
>> + if (y->transports) {
>> + pa_assert(pa_hashmap_isempty(y->transports));
>> + pa_hashmap_free(y->transports, NULL);
>> + }
>> +
>> if (y->connection) {
>> pa_dbus_remove_matches(pa_dbus_connection_get(y->connection),
>> "type='signal',sender='org.freedesktop.DBus',interface='org.freedesktop.DBus',member='NameOwnerChanged'"
>> diff --git a/src/modules/bluetooth/bluez5-util.h b/src/modules/bluetooth/bluez5-util.h
>> index 18b20c4..c3f2f2f 100644
>> --- a/src/modules/bluetooth/bluez5-util.h
>> +++ b/src/modules/bluetooth/bluez5-util.h
>> @@ -24,14 +24,50 @@
>>
>> #include <pulsecore/core.h>
>>
>> +typedef struct pa_bluetooth_transport pa_bluetooth_transport;
>> typedef struct pa_bluetooth_device pa_bluetooth_device;
>> typedef struct pa_bluetooth_discovery pa_bluetooth_discovery;
>>
>> typedef enum pa_bluetooth_hook {
>> PA_BLUETOOTH_HOOK_DEVICE_CONNECTION_CHANGED, /* Call data: pa_bluetooth_device */
>> + PA_BLUETOOTH_HOOK_TRANSPORT_STATE_CHANGED, /* Call data: pa_bluetooth_transport */
>> PA_BLUETOOTH_HOOK_MAX
>> } pa_bluetooth_hook_t;
>>
>> +typedef enum profile {
>> + PROFILE_A2DP_SINK,
>> + PROFILE_A2DP_SOURCE,
>> + PROFILE_OFF
>
> Missing PA_BLUETOOTH_ prefix.
>
Ok.
>> +} pa_bluetooth_profile_t;
>> +#define PA_BLUETOOTH_PROFILE_COUNT PROFILE_OFF
>> +
>> +typedef enum pa_bluetooth_transport_state {
>> + PA_BLUETOOTH_TRANSPORT_STATE_DISCONNECTED,
>> + PA_BLUETOOTH_TRANSPORT_STATE_IDLE,
>> + PA_BLUETOOTH_TRANSPORT_STATE_PLAYING
>> +} pa_bluetooth_transport_state_t;
>> +
>> +typedef int (*pa_bluetooth_transport_acquire_cb)(pa_bluetooth_transport *t, bool optional, size_t *imtu, size_t *omtu);
>> +typedef void (*pa_bluetooth_transport_release_cb)(pa_bluetooth_transport *t);
>> +
>> +struct pa_bluetooth_transport {
>> + pa_bluetooth_device *device;
>> +
>> + char *owner;
>> + char *path;
>> + pa_bluetooth_profile_t profile;
>> +
>> + uint8_t codec;
>> + uint8_t *config;
>> + size_t config_size;
>> +
>> + pa_bluetooth_transport_state_t state;
>> +
>> + pa_bluetooth_transport_acquire_cb acquire;
>> + pa_bluetooth_transport_release_cb release;
>> + void *userdata;
>> +};
>> +
>> struct pa_bluetooth_device {
>> pa_bluetooth_discovery *discovery;
>>
>> @@ -43,8 +79,15 @@ struct pa_bluetooth_device {
>> char *remote;
>> char *local;
>> int class_of_device;
>> +
>> + pa_bluetooth_transport *transports[PA_BLUETOOTH_PROFILE_COUNT];
>> };
>>
>> +pa_bluetooth_transport *pa_bluetooth_transport_new(pa_bluetooth_device *d, const char *owner, const char *path,
>> + pa_bluetooth_profile_t p, const uint8_t *config, size_t size);
>
> Unaligned parameter list.
>
Same comment as the previous occurrence.
--
Jo?o Paulo Rechi Vita
http://about.me/jprvita
