[pulseaudio-discuss] [PATCH 26/56] bluetooth: Create pa_bluetooth_transport for BlueZ 5 support
Tanu Kaskinen
tanu.kaskinen at linux.intel.com
Thu Jul 18 08:31:03 PDT 2013
On Fri, 2013-07-12 at 15:06 -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 | 130 ++++++++++++++++++++++++++++++++++++
> src/modules/bluetooth/bluez5-util.h | 44 ++++++++++++
> 2 files changed, 174 insertions(+)
>
> diff --git a/src/modules/bluetooth/bluez5-util.c b/src/modules/bluetooth/bluez5-util.c
> index 87aaa37..6a94d50 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,117 @@ 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, int size,
> + pa_bluetooth_transport_acquire_cb acquire,
> + pa_bluetooth_transport_release_cb release, void *userdata) {
> + 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;
> + t->acquire = acquire;
> + t->release = release;
> + t->userdata = userdata;
> +
> + if (size > 0) {
> + t->config = pa_xnew(uint8_t, size);
> + memcpy(t->config, config, size);
> + }
> +
> + t->state = PA_BLUETOOTH_TRANSPORT_STATE_IDLE;
> +
> + pa_assert_se(pa_hashmap_put(d->discovery->transports, t->path, t) >= 0);
> +
> + return t;
> +}
> +
> +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(pa_bluetooth_transport *t, bool optional, size_t *imtu, size_t *omtu) {
This is meant to be used as an acquire callback, right? Please put "_cb"
at the end of the function name. Same for bluez5_transport_release.
> + 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(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);
In an earlier log message there is a mention of auto-releasing by BlueZ.
I don't know exactly what it means, but it sounds like BlueZ can release
the transport too, in which case there is a race condition: if BlueZ
releases a transport at the same time when this code is running, our
Release() call will be received when the transport is already released,
which I presume will result in an error. If that is not a real error
condition, perhaps the log level should be info instead of error.
> + dbus_error_free(&err);
> + } else
> + pa_log_info("Transport %s released", t->path);
> +}
> +
> static pa_bluetooth_device* pa_bluetooth_discovery_create_device(pa_bluetooth_discovery *y, const char *path) {
> pa_bluetooth_device *d;
>
> @@ -92,8 +202,22 @@ pa_bluetooth_device* pa_bluetooth_discovery_get_device_by_address(pa_bluetooth_d
> }
>
> static void pa_bluetooth_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;
> + t->state = PA_BLUETOOTH_TRANSPORT_STATE_DISCONNECTED;
> + pa_hook_fire(&d->discovery->hooks[PA_BLUETOOTH_HOOK_TRANSPORT_STATE_CHANGED], t);
Can the transport state be DISCONNECTED already before we set it here?
If it can, please fire the hook only if the state actually changes.
> + pa_bluetooth_transport_free(t);
> + }
> +
> d->discovery = NULL;
> pa_xfree(d->path);
> pa_xfree(d->alias);
> @@ -194,6 +318,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);
> @@ -254,6 +379,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 a205b45..82953a7 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
> +} 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);
If these typedefs are only used in the pa_bluetooth_transport
definition, I don't think they bring any benefit, and they also make it
harder to check what the function signature is (the first place to look
is the struct definition, and from there you have to go to the typedef -
it would be easier if the signature was visible already in the struct
definition).
Hmm, now I see that the typedefs are used also in the transport_new()
function parameters, where they certainly are very useful. However, I
would prefer you to follow the usual subclassing pattern in PulseAudio:
the backend calls foo_new() and then sets the callbacks and the userdata
pointer (and then calls foo_put(), if such function exists).
> +
> +struct pa_bluetooth_transport {
> + pa_bluetooth_device *device;
> +
> + char *owner;
> + char *path;
> + pa_bluetooth_profile_t profile;
> +
> + uint8_t codec;
> + uint8_t *config;
> + int config_size;
I think the convention is to use size_t for byte array sizes.
> +
> + 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,16 @@ struct pa_bluetooth_device {
> char *alias;
> char *address;
> 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, int size,
size_t here also.
--
Tanu
More information about the pulseaudio-discuss
mailing list