On Mon, 2012-11-12 at 16:19 +0100, Fr?d?ric Danis wrote:
> With BlueZ 5 volume updates for HFP will go through transport interface.
>
> Note that this is backward compatible.
> ---
> src/modules/bluetooth/bluetooth-util.c | 62 +++++++++++++++----
> src/modules/bluetooth/bluetooth-util.h | 4 ++
> src/modules/bluetooth/module-bluetooth-device.c | 75 +++++++++++++++++++++++
> 3 files changed, 130 insertions(+), 11 deletions(-)
>
> diff --git a/src/modules/bluetooth/bluetooth-util.c b/src/modules/bluetooth/bluetooth-util.c
> index 272b6ce..c0d5287 100644
> --- a/src/modules/bluetooth/bluetooth-util.c
> +++ b/src/modules/bluetooth/bluetooth-util.c
> @@ -754,21 +754,49 @@ int pa_bluetooth_transport_parse_property(pa_bluetooth_transport *t, DBusMessage
>
> dbus_message_iter_recurse(i, &variant_i);
>
> - switch (dbus_message_iter_get_arg_type(&variant_i)) {
> + if (pa_streq(key, "NREC")) {
> + dbus_bool_t value;
>
> - case DBUS_TYPE_BOOLEAN: {
> + if (dbus_message_iter_get_arg_type(&variant_i) != DBUS_TYPE_BOOLEAN) {
> + pa_log("Property value not a boolean.");
This is a bit generic. There could be multiple places where this is
printed (this seems to be the only place currently, though). I suggest
"'NREC' property value not a boolean." I know that there are other
places with similarly generic log messages, but I don't think those
messages are worth copying, even in the name of consistency.
Same comment applies to the other new log messages.
> + return -1;
> + }
>
> - dbus_bool_t value;
> - dbus_message_iter_get_basic(&variant_i, &value);
> + dbus_message_iter_get_basic(&variant_i, &value);
>
> - if (pa_streq(key, "NREC") && t->nrec != value) {
> - t->nrec = value;
> - pa_log_debug("Transport %s: Property 'NREC' changed to %s.", t->path, t->nrec ? "True" : "False");
> - pa_hook_fire(&t->hooks[PA_BLUETOOTH_TRANSPORT_HOOK_NREC_CHANGED], NULL);
> - }
> + if (t->nrec != value) {
> + t->nrec = value;
> + pa_log_debug("Transport %s: Property 'NREC' changed to %s.", t->path, t->nrec ? "True" : "False");
> + pa_hook_fire(&t->hooks[PA_BLUETOOTH_TRANSPORT_HOOK_NREC_CHANGED], NULL);
> + }
> + } else if(pa_streq(key,"OutputGain")) {
Spaces missing after "if" and "key,"
> + uint16_t value;
>
> - break;
> - }
> + if (dbus_message_iter_get_arg_type(&variant_i) != DBUS_TYPE_UINT16) {
> + pa_log("Property value not an uint16.");
> + return -1;
> + }
> +
> + dbus_message_iter_get_basic(&variant_i, &value);
> +
> + if (t->output_gain != value) {
> + t->output_gain = value;
> + pa_hook_fire(&t->hooks[PA_BLUETOOTH_TRANSPORT_HOOK_OUTPUT_GAIN_CHANGED], NULL);
I'd like to have a debug log message about the changed value similar to
what the NREC handler prints.
> + }
> + } else if(pa_streq(key,"InputGain")) {
More missing spaces.
> + uint16_t value;
> +
> + if (dbus_message_iter_get_arg_type(&variant_i) != DBUS_TYPE_UINT16) {
> + pa_log("Property value not an uint16.");
> + return -1;
> + }
> +
> + dbus_message_iter_get_basic(&variant_i, &value);
> +
> + if (t->input_gain != value) {
> + t->input_gain = value;
> + pa_hook_fire(&t->hooks[PA_BLUETOOTH_TRANSPORT_HOOK_INPUT_GAIN_CHANGED], NULL);
I suggest a debug log message here too.
> +static void send_property_update(struct userdata *u, const char *name, int type, void *data)
> +{
> + DBusMessage *m;
> + DBusMessageIter iter;
> +
> + pa_assert_se(m = dbus_message_new_method_call("org.bluez", u->transport->path, "org.bluez.MediaTransport", "SetProperty"));
I guess this will have to be adapted to Luiz's change that changed the
transport message destination? If so, I can do that. Or, I can also make
my "next" branch public, so you could develop against that if you
prefer. The reason why the branch is not currently public is that I want
to keep rebasing it on master, but if that's not a problem for you, I
can make it public.
> + dbus_message_iter_init_append(m, &iter);
> + pa_assert_se(dbus_message_iter_append_basic(&iter, DBUS_TYPE_STRING, &name));
> + pa_dbus_append_basic_variant(&iter, DBUS_TYPE_UINT16, data);
I think your intention was to use the "type" argument here instead of
hardcoding DBUS_TYPE_UINT16.
> @@ -1468,6 +1486,7 @@ static void source_set_volume_cb(pa_source *s) {
> pa_volume_t volume;
> struct userdata *u;
> char *k;
> + const char *name = "InputGain";
>
> pa_assert(s);
> pa_assert(s->core);
> @@ -1497,6 +1516,8 @@ static void source_set_volume_cb(pa_source *s) {
> pa_assert_se(dbus_message_append_args(m, DBUS_TYPE_UINT16, &gain, DBUS_TYPE_INVALID));
> pa_assert_se(dbus_connection_send(pa_dbus_connection_get(u->connection), m, NULL));
> dbus_message_unref(m);
> +
> + send_property_update(u, name, DBUS_TYPE_UINT16, &gain);
Hmm, I would prefer there to be functions in bluetooth-util for setting
transport properties. And preferably one function per property (e.g.
pa_bluetooth_transport_set_input_gain()) rather than a generic "set
property" function. My idea is that bluetooth-util would take care of
the dbus messaging as much as possible.
> @@ -1748,6 +1807,9 @@ static int add_sink(struct userdata *u) {
> k = pa_sprintf_malloc("bluetooth-device@%p", (void*) u->sink);
> pa_shared_set(u->core, k, u);
> pa_xfree(k);
> +
> + if (u->transport && !u->hsp.output_gain_slot)
There is pa_assert(u->transport) in the beginning of the function, no
need to check it here.
> + u->hsp.output_gain_slot = pa_hook_connect(&u->transport->hooks[PA_BLUETOOTH_TRANSPORT_HOOK_OUTPUT_GAIN_CHANGED], PA_HOOK_NORMAL, (pa_hook_cb_t) output_gain_changed_cb, u);
> }
>
> return 0;
> @@ -1832,6 +1894,9 @@ static int add_source(struct userdata *u) {
> k = pa_sprintf_malloc("bluetooth-device@%p", (void*) u->source);
> pa_shared_set(u->core, k, u);
> pa_xfree(k);
> +
> + if (u->transport && !u->hsp.input_gain_slot)
Same comment as above.
--
Tanu
