On Tue, 2017-02-14 at 14:33 +0100, Georg Chini wrote: > From: Wim Taymans <wim.taymans at gmail.com> > > This is a rebase of Wim Taymans patch to support the HSP headset role that has > somehow been forgotten. Original patch can be found at > https://lists.freedesktop.org/archives/pulseaudio-discuss/2015-February/023242.html You could mention your own name here. > Original commit message: > > In addition to the HSP Audio Gateway, also add support for the HeadSet > profile in the native bluetooth backend. With this profile you can use > pulseaudio as a headset. I think it would be better to use the term "role" here instead of "profile". > In the headset role, we create source and sink to receive and send the samples > from the gateway, respectively. The loopback device will automatically link > these to a sink and source for playback. What's "the loopback device"? I think you mean module-bluetooth-policy. Also, "for playback" doesn't seem very accurate description, when audio is going both directions. > Because this makes the source the > speaker and the sink the microphone, we need to reverse the roles of source > and sink compared to the gateway role. > > In the gateway role, adjusting the sink volume generates a +VGS command to set > the volume on the headset. Likewise, receiving AT+VGS updates the sink volume. > > In the headset role, receiving a +VGS should set the source volume and any > source volume changes should be reported back to the gateway with AT+VGS. > > --- > src/modules/bluetooth/backend-native.c | 236 ++++++++++++++++++++++----- > src/modules/bluetooth/module-bluez5-device.c | 32 +++- > 2 files changed, 222 insertions(+), 46 deletions(-) > #define BLUEZ_PROFILE_INTERFACE BLUEZ_SERVICE ".Profile1" > > #define HSP_AG_PROFILE "/Profile/HSPAGProfile" > +#define HSP_HS_PROFILE "/Profile/HSPHSProfile" > + > +#define HSP_HS_DEFAULT_CHANNEL 3 A comment would be nice here. What does "channel" mean here? Why is 3 a good default? > +static int bluez5_sco_listen(pa_bluetooth_transport *t) { The "bluez5_" prefix seems pointless. > + struct transport_data *trd = t->userdata; > + struct sockaddr_sco addr; > + int sock; > + > + sock = socket(PF_BLUETOOTH, SOCK_SEQPACKET | SOCK_NONBLOCK | SOCK_CLOEXEC, BTPROTO_SCO); > + if (sock < 0) { > + pa_log_error("socket(SEQPACKET, SCO) %s", pa_cstrerror(errno)); > + return -1; > + } > + > + /* Bind to local address */ > + memset(&addr, 0, sizeof(addr)); > + addr.sco_family = AF_BLUETOOTH; > + bacpy(&addr.sco_bdaddr, BDADDR_ANY); This looks like it won't work right if there are multiple bluetooth adapters. We're setting up the listening socket for a specific transport, which is associated with a specific adapter, so I think we should use the adapter address here. > + > + if (bind(sock, (struct sockaddr *) &addr, sizeof(addr)) < 0) { > + pa_log_error("bind(): %s", pa_cstrerror(errno)); > + goto fail_close; > + } > + > + pa_log_info ("doing listen"); > + if (listen(sock, 1) < 0) { > + pa_log_error("listen(): %s", pa_cstrerror(errno)); > + goto fail_close; > + } > + > + trd->sco_fd = sock; > + trd->sco_io = trd->mainloop->io_new(trd->mainloop, sock, PA_IO_EVENT_INPUT|PA_IO_EVENT_ERROR|PA_IO_EVENT_HANGUP, > + sco_io_callback, t); Errors and hangups don't need to be specified here, those events are always delivered. > @@ -220,6 +322,14 @@ static void register_profile(pa_bluetooth_backend *b, const char *profile, const > dbus_message_iter_append_basic(&i, DBUS_TYPE_STRING, &uuid); > dbus_message_iter_open_container(&i, DBUS_TYPE_ARRAY, DBUS_DICT_ENTRY_BEGIN_CHAR_AS_STRING DBUS_TYPE_STRING_AS_STRING > DBUS_TYPE_VARIANT_AS_STRING DBUS_DICT_ENTRY_END_CHAR_AS_STRING, &d); > + if (pa_streq (uuid, PA_BLUETOOTH_UUID_HSP_HS)) { > + autoconnect = 0; > + pa_dbus_append_basic_variant_dict_entry(&d, "AutoConnect", DBUS_TYPE_BOOLEAN, &autoconnect); Why do we disable autoconnecting? I'm not saying we should enable it, but I'd like to understand what this is about. > + chan = HSP_HS_DEFAULT_CHANNEL; > + pa_dbus_append_basic_variant_dict_entry(&d, "Channel", DBUS_TYPE_UINT16, &chan); > + version = 0x0102; > + pa_dbus_append_basic_variant_dict_entry(&d, "Version", DBUS_TYPE_UINT16, &version); Does 0x0102 mean HSP version 1.2? (A comment would be nice.) > + } > dbus_message_iter_close_container(&i, &d); > > send_and_add_to_pending(b, m, register_profile_reply, pa_xstrdup(profile)); > @@ -239,7 +349,7 @@ static void rfcomm_io_callback(pa_mainloop_api *io, pa_io_event *e, int fd, pa_i > if (events & PA_IO_EVENT_INPUT) { > char buf[512]; > ssize_t len; > - int gain; > + int gain, dummy, do_reply = FALSE; do_reply should be a bool. > > len = pa_read(fd, buf, 511, NULL); > if (len < 0) { > @@ -249,22 +359,31 @@ static void rfcomm_io_callback(pa_mainloop_api *io, pa_io_event *e, int fd, pa_i > buf[len] = 0; > pa_log_debug("RFCOMM << %s", buf); > > - if (sscanf(buf, "AT+VGS=%d", &gain) == 1) { > - t->speaker_gain = gain; > - pa_hook_fire(pa_bluetooth_discovery_hook(t->device->discovery, PA_BLUETOOTH_HOOK_TRANSPORT_SPEAKER_GAIN_CHANGED), t); > - } else if (sscanf(buf, "AT+VGM=%d", &gain) == 1) { > - t->microphone_gain = gain; > - pa_hook_fire(pa_bluetooth_discovery_hook(t->device->discovery, PA_BLUETOOTH_HOOK_TRANSPORT_MICROPHONE_GAIN_CHANGED), t); > + if (sscanf(buf, "AT+VGS=%d", &gain) == 1 || sscanf(buf, "\r\n+VGM=%d\r\n", &gain) == 1) { > + t->speaker_gain = gain; > + pa_hook_fire(pa_bluetooth_discovery_hook(t->device->discovery, PA_BLUETOOTH_HOOK_TRANSPORT_SPEAKER_GAIN_CHANGED), t); > + do_reply = TRUE; > + > + } else if (sscanf(buf, "AT+VGM=%d", &gain) == 1 || sscanf(buf, "\r\n+VGS=%d\r\n", &gain) == 1) { > + t->microphone_gain = gain; > + pa_hook_fire(pa_bluetooth_discovery_hook(t->device->discovery, PA_BLUETOOTH_HOOK_TRANSPORT_MICROPHONE_GAIN_CHANGED), t); > + do_reply = TRUE; > + } else if (sscanf(buf, "AT+CKPD=%d", &dummy) == 1) { What's AT+CKPD=? It would be good to have a comment that documents all the AT commands that appear in the code. Why don't we any more respond with OK to all unknown commands? Changing it can have implications regarding which headsets will work and which will not. I'd like to avoid unnecessary changes here. > @@ -372,12 +513,14 @@ static DBusMessage *profile_new_connection(DBusConnection *conn, DBusMessage *m, > t->set_speaker_gain = set_speaker_gain; > t->set_microphone_gain = set_microphone_gain; > > - trfc = pa_xnew0(struct transport_rfcomm, 1); > - trfc->rfcomm_fd = fd; > - trfc->mainloop = b->core->mainloop; > - trfc->rfcomm_io = trfc->mainloop->io_new(b->core->mainloop, fd, PA_IO_EVENT_INPUT|PA_IO_EVENT_HANGUP, > + trd = pa_xnew0(struct transport_data, 1); > + trd->rfcomm_fd = fd; > + trd->mainloop = b->core->mainloop; > + trd->rfcomm_io = trd->mainloop->io_new(b->core->mainloop, fd, PA_IO_EVENT_INPUT|PA_IO_EVENT_HANGUP|PA_IO_EVENT_ERROR, No need to specify the hangup and error events. > @@ -898,6 +901,13 @@ static int source_process_msg(pa_msgobject *o, int code, void *data, int64_t off > return 0; > } > > + case PA_SOURCE_MESSAGE_SET_SHARED_VOLUME: { > + if (u->profile == PA_BLUETOOTH_PROFILE_HEADSET_AUDIO_GATEWAY) { > + source_set_volume_cb (u->source); > + } > + break; > + } > + The message is processed in the IO thread, but source_set_volume_cb() is expected to be called from the main thread. Using the SET_SHARED_VOLUME is an unusual way to get volume change notifications anyway. I think we should set the set_volume() callback to get the volume change notifications. The documentation for set_volume() in sink.h says this (source.h seems to lack similarly detailed explanation): * If PA_SINK_DEFERRED_VOLUME is not set, then this is called from the * main thread. The callback implementation must set the hardware * volume according to s->real_volume. If the driver can't set the * hardware volume to the exact requested value, it has to update * s->real_volume and/or s->soft_volume so that they together * match the actual hardware volume that was set. I don't fully understand what that means (even though I think I wrote that myself). The last sentence looks a bit misleading. Here's how I think the volume should be handled: The callback should first read s->real_volume to get the target volume. Since HSP has only 17 volume levels, rounding should be applied like source_set_volume_cb() already does. When pulseaudio is in the headset role, there won't be any hardware volume applied, so s->soft_volume needs to set to the same value as s->real_volume. source_set_volume_cb() calls set_microphone_gain(), which is confusing when we're in the headset role. It seems to do the right thing, though. There should be comments both in source_set_volume_cb() and set_microphone_gain() to make it easy to figure out what's going on. (The same applies to the sink side, of course.) Similarly I think it would be good to have comments in the transport_speaker/microphone_gain_changed_cb() functions. > @@ -1272,7 +1287,7 @@ static int setup_transport(struct userdata *u) { > /* check if profile has a transport */ > t = u->device->transports[u->profile]; > if (!t || t->state <= PA_BLUETOOTH_TRANSPORT_STATE_DISCONNECTED) { > - pa_log_warn("Profile has no transport"); > + pa_log_warn("Profile %d has no transport", u->profile); Magic numbers in logs are frustrating to read. Please use pa_bluetooth_profile_to_string(). -- Tanu https://www.patreon.com/tanuk
