On Wed, 2016-08-24 at 16:17 +0300, Tanu Kaskinen wrote:
> On Tue, 2016-08-23 at 08:37 -0400, James Bottomley wrote:
> > On Mon, 2016-08-22 at 15:12 +0300, Tanu Kaskinen wrote:
> > > and AT+CHLD=?.
> >
> > This one is a bit weird: in theory the response should be call hold
> > states, but we didn't show any call held indicators, so none (i.e.
> > just respond OK) is the correct response. I actually experimented
> > with showing some and the headset crashed.
>
> Ok. I couldn't find a statement from the HFP spec about how "we don't
> support any of the +CHLD values" should be reported, but judging from
> the 3GPP 27.007 spec, it looks like the response to AT+CHLD=? is
> optional.
Actually, if you've found spec support from only responding OK, I'll
rework the code to do that rather than making up a pointless indicator.
The slight problem is what happens to the mandatory AT+CHLD?
negotiation because I can see some headsets not sending that if we
don't support any indicators, but I think I can cope with that in the
sequence.
> AT+CIND=? is pretty similar in that we'd rather not pretend to
> support any indicators, but the 3GPP spec doesn't mark the reply as
> optional in that case, and the list of indicators has to have at
> least one entry. However, "if MT is not currently reachable, +CME
> ERROR: <err> is returned" - maybe returning +CME ERROR for AT+CIND=?
> would make more sense?
I'd really rather make up an indicator than return error, because I bet
error in the initialisation sequence isn't usual for headsets.
> > > > > Now that I've read some of the HFP spec, I believe there are
> > > > > other commands (possibly many of them) that we may encounter
> > > > > too after the initial setup, for example AT+CMER. How did
> > > > > these two commands end up being handled specially? Did your
> > > > > headset not work without this code?
> > > >
> > > > We handle AT+CMER. The v1.6 version of the standard contains a
> > > > diagram which shows the minimum dialog you need. It's
> > > > basically BRSF, two CINDS and a CMER. Once the CMER is sent
> > > > and replied to, you can establish audio. The other end may
> > > > optionally send a AT+CHLD=? but we can just reply OK to that.
> > > > I actually theorise that just replying OK to this entire
> > > > negotiation sequence is fine because it indicates a minimal
> > > > but functional audio gateway, so in theory, the negotiation is
> > > > an optional best practice because the original headset code
> > > > will just reply OK on its own.
> > >
> > > We don't handle AT+CMER, except as part of the initial setup
> > > sequence. Further AT+CMER commands get "ERROR" as reply.
> >
> > We should only ever get one. Actually 3,0,0,1 to enable event
> > reporting.
>
> Why should we only ever get one? Why should the headset never send
> 3,0,0,0 to disable event reporting? (If you're going to change the
> code to respond OK to everything, this question becomes moot, but
> still I'm somewhat interested in how we can make assumptions about
> what commands headsets will or will not use.)
The latest code will just respond OK to this ... it doesn't matter
because we never send event indicators, so whether the headset wants
them on or off is irrelevant to us.
> > > What makes AT+BTRH? special in that we should reply "OK" to it
> > > and "ERROR" to all other commands?
> >
> > Actually, I think we should just reply OK to all commands (a bit
> > like HFP). There's no harm and we don't alter any state.
>
> That sounds good, if it removes the need for special handling for a
> headset-specific set of commands.
Yes, see below ... I'll post the complete set as a v3, but this is what
I currently have.
> > > > > One possibility for avoiding manual configuration in HFP-only
> > > > > use cases would be to add some logic to automatically
> > > > > register the HFP profile when we notice that a HFP-only
> > > > > headset is being used.
> > > >
> > > > It would still obscure HSP, though, if you're switching
> > > > headsets without reloading the modules.
> > >
> > > True. However, that would be a problem only if
> > >
> > > - the user actively uses multiple headsets
> > > - one of the headsets supports both HSP and HFP, and one supports
> > > only HFP
> > > - the HSP+HFP headset works only with HSP
> > >
> > > Seems very marginal case to me, and enabling HFP by default would
> > > anyway break the HSP+HFP headset.
> >
> > Yes, I agree, lets just do it and see if there's a problem. I
> > suspect all phones negotiate for HFP first (android certainly
> > does), so we'd just be doing what every headset expects.
>
> I'm not sure if you understood what I meant. I suggested not
> registering HFP support until a HFP-only headset appears. Your
> response sounds more like you thought that I was suggesting enabling
> HFP by default as your patches currently do, but I'm not sure, maybe
> "let's just do it" means that you'll happily write the additional
> logic of adding HFP support only after pulseaudio encounters an HFP
> -only headset.
I can do it either way. Right at the moment, for HSP/HFP systems it
doesn't matter. However, as soon as I get the mSBC code working, it
will because only HFP can do wideband audio.
I was really thinking that if it "just works" most people with HFP/HSP
headsets would want to use HFP to get the coded, so what currently
happens looks OK in that context.
James
---
diff --git a/src/modules/bluetooth/backend-native.c b/src/modules/bluetooth/backend-native.c
index 546762a..0b3adbc 100644
--- a/src/modules/bluetooth/backend-native.c
+++ b/src/modules/bluetooth/backend-native.c
@@ -50,6 +50,45 @@ struct transport_rfcomm {
pa_mainloop_api *mainloop;
};
+struct hfp_config {
+ uint32_t capabilities;
+ int state;
+};
+
+/*
+ * the separate hansfree headset (HF) and Audio Gateway (AG) features
+ */
+enum hfp_hf_features {
+ HFP_HF_EC_NR = 0,
+ HFP_HF_CALL_WAITING = 1,
+ HFP_HF_CLI = 2,
+ HFP_HF_VR = 3,
+ HFP_HF_RVOL = 4,
+ HFP_HF_ESTATUS = 5,
+ HFP_HF_ECALL = 6,
+ HFP_HF_CODECS = 7,
+};
+
+enum hfp_ag_features {
+ HFP_AG_THREE_WAY = 0,
+ HFP_AG_EC_NR = 1,
+ HFP_AG_VR = 2,
+ HFP_AG_RING = 3,
+ HFP_AG_NUM_TAG = 4,
+ HFP_AG_REJECT = 5,
+ HFP_AG_ESTATUS = 6,
+ HFP_AG_ECALL = 7,
+ HFP_AG_EERR = 8,
+ HFP_AG_CODECS = 9,
+};
+
+/* gateway features we support */
+static uint32_t hfp_features =
+ /* LG HBS900 reboots if it doesn't see this */
+ (1 << HFP_AG_THREE_WAY) |
+ /* HFP 1.6 requires this */
+ (1 << HFP_AG_ESTATUS );
+
#define BLUEZ_SERVICE "org.bluez"
#define BLUEZ_MEDIA_TRANSPORT_INTERFACE BLUEZ_SERVICE ".MediaTransport1"
@@ -101,6 +140,27 @@ static pa_dbus_pending* send_and_add_to_pending(pa_bluetooth_backend *backend, D
return p;
}
+static void rfcomm_write(int fd, const char *str)
+{
+ size_t len;
+ char buf[512];
+
+ pa_log_debug("RFCOMM >> %s", str);
+ sprintf(buf, "\r\n%s\r\n", str);
+ len = write(fd, buf, strlen(buf));
+
+ if (len != strlen(buf))
+ pa_log_error("RFCOMM write error: %s", pa_cstrerror(errno));
+}
+
+static void hfp_send_features(int fd)
+{
+ char buf[512];
+
+ sprintf(buf, "+BRSF: %d", hfp_features);
+ rfcomm_write(fd, buf);
+}
+
static int bluez5_sco_acquire_cb(pa_bluetooth_transport *t, bool optional, size_t *imtu, size_t *omtu) {
pa_bluetooth_device *d = t->device;
struct sockaddr_sco addr;
@@ -216,6 +276,55 @@ static void register_profile(pa_bluetooth_backend *b, const char *profile, const
send_and_add_to_pending(b, m, register_profile_reply, pa_xstrdup(profile));
}
+static void transport_put(pa_bluetooth_transport *t)
+{
+ pa_bluetooth_transport_put(t);
+
+ pa_log_debug("Transport %s available for profile %s", t->path, pa_bluetooth_profile_to_string(t->profile));
+}
+
+static int hfp_rfcomm_handle(int fd, pa_bluetooth_transport *t, const char *buf)
+{
+ struct hfp_config *c = t->config;
+ int val;
+
+ /* stateful negotiation */
+ if (c->state == 0 && sscanf(buf, "AT+BRSF=%d", &val) == 1) {
+ c->capabilities = val;
+ pa_log_info("HFP capabilities returns 0x%x", val);
+ hfp_send_features(fd);
+ c->state = 1;
+ return 0;
+ } else if (c->state == 1 && pa_startswith(buf, "AT+CIND=?")) {
+ /* we declare minimal no indicators */
+ c->state = 2;
+ return 0;
+ } else if ((c->state == 1 || c->state == 2) && pa_startswith(buf, "AT+CIND?")) {
+ c->state = 3;
+ return 0;
+ } else if (c->state == 3 && pa_startswith(buf, "AT+CMER=")) {
+ rfcomm_write(fd, "OK");
+ c->state = 4;
+ transport_put(t);
+ return 1;
+ }
+
+ /* if we get here, negotiation should be complete */
+ if (c->state != 4) {
+ pa_log_error("HFP negotiation failed in state %d with inbound %s\n",
+ c->state, buf);
+ rfcomm_write(fd, "ERROR");
+ return 1;
+ }
+
+ /*
+ * once we're fully connected, just reply OK to everything
+ * it will just be the headset sending the occasional status
+ * update, but we process only the ones we care about
+ */
+ return 0;
+}
+
static void rfcomm_io_callback(pa_mainloop_api *io, pa_io_event *e, int fd, pa_io_event_flags_t events, void *userdata) {
pa_bluetooth_transport *t = userdata;
@@ -246,16 +355,11 @@ static void rfcomm_io_callback(pa_mainloop_api *io, pa_io_event *e, int fd, pa_i
} 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);
+ } else if (t->config) { /* t->config is only non-null for hfp profile */
+ if (hfp_rfcomm_handle(fd, t, buf) == 1)
+ return;
}
-
- pa_log_debug("RFCOMM >> OK");
-
- len = write(fd, "\r\nOK\r\n", 6);
-
- /* we ignore any errors, it's not critical and real errors should
- * be caught with the HANGUP and ERROR events handled above */
- if (len < 0)
- pa_log_error("RFCOMM write error: %s", pa_cstrerror(errno));
+ rfcomm_write(fd, "OK");
}
return;
@@ -365,7 +469,7 @@ static DBusMessage *profile_new_connection(DBusConnection *conn, DBusMessage *m,
p = PA_BLUETOOTH_PROFILE_HSP_HS;
pathfd = pa_sprintf_malloc ("%s/fd%d", path, fd);
- t = pa_bluetooth_transport_new(d, sender, pathfd, p, NULL, 0);
+ t = pa_bluetooth_transport_new(d, sender, pathfd, p, NULL, is_hfp ? sizeof(struct hfp_config) : 0);
pa_xfree(pathfd);
t->acquire = bluez5_sco_acquire_cb;
@@ -381,9 +485,8 @@ static DBusMessage *profile_new_connection(DBusConnection *conn, DBusMessage *m,
rfcomm_io_callback, t);
t->userdata = trfc;
- pa_bluetooth_transport_put(t);
-
- pa_log_debug("Transport %s available for profile %s", t->path, pa_bluetooth_profile_to_string(t->profile));
+ if (!is_hfp)
+ transport_put(t);
pa_assert_se(r = dbus_message_new_method_return(m));
diff --git a/src/modules/bluetooth/bluez5-util.c b/src/modules/bluetooth/bluez5-util.c
index 09201d4..cfce89c 100644
--- a/src/modules/bluetooth/bluez5-util.c
+++ b/src/modules/bluetooth/bluez5-util.c
@@ -150,7 +150,10 @@ pa_bluetooth_transport *pa_bluetooth_transport_new(pa_bluetooth_device *d, const
if (size > 0) {
t->config = pa_xnew(uint8_t, size);
- memcpy(t->config, config, size);
+ if (config)
+ memcpy(t->config, config, size);
+ else
+ memset(t->config, 0, size);
}
return t;
diff --git a/src/modules/bluetooth/bluez5-util.h b/src/modules/bluetooth/bluez5-util.h
index 5c41d30..9460aad 100644
--- a/src/modules/bluetooth/bluez5-util.h
+++ b/src/modules/bluetooth/bluez5-util.h
@@ -73,7 +73,7 @@ struct pa_bluetooth_transport {
pa_bluetooth_profile_t profile;
uint8_t codec;
- uint8_t *config;
+ void *config;
size_t config_size;
uint16_t microphone_gain;
