[PATCH 1/2 v3] emulator: add codec negotiation support

[PATCH 1/2 v3] emulator: add codec negotiation support

[Simon Fels]

[-- Attachment #1: Type: text/plain, Size: 8407 bytes --]

---
 include/emulator.h |   6 ++
 src/emulator.c     | 248 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 254 insertions(+)

diff --git a/include/emulator.h b/include/emulator.h
index 15dc61c..36153ef 100644
--- a/include/emulator.h
+++ b/include/emulator.h
@@ -112,6 +112,12 @@ void ofono_emulator_set_hf_indicator_active(struct ofono_emulator *em,
 void ofono_emulator_set_handsfree_card(struct ofono_emulator *em,
 					struct ofono_handsfree_card *card);
 
+typedef void (*ofono_emulator_codec_negotiation_cb)(int err, void *data);
+
+int ofono_emulator_start_codec_negotiation(struct ofono_emulator *em,
+						unsigned char codec,
+						ofono_emulator_codec_negotiation_cb cb, void *data);
+
 #ifdef __cplusplus
 }
 #endif
diff --git a/src/emulator.c b/src/emulator.c
index 626dec3..e16312e 100644
--- a/src/emulator.c
+++ b/src/emulator.c
@@ -27,6 +27,7 @@
 #include <string.h>
 #include <unistd.h>
 #include <stdbool.h>
+#include <errno.h>
 
 #include <glib.h>
 
@@ -39,6 +40,15 @@
 
 #define RING_TIMEOUT 3
 
+#define CVSD_OFFSET 0
+#define MSBC_OFFSET 1
+#define CODECS_COUNT (MSBC_OFFSET + 1)
+
+struct hfp_codec_info {
+	unsigned char type;
+	ofono_bool_t supported;
+};
+
 struct ofono_emulator {
 	struct ofono_atom *atom;
 	enum ofono_emulator_type type;
@@ -50,6 +60,13 @@ struct ofono_emulator {
 	guint callsetup_source;
 	int pns_id;
 	struct ofono_handsfree_card *card;
+	struct hfp_codec_info r_codecs[CODECS_COUNT];
+	unsigned char negotiated_codec;
+	unsigned char proposed_codec;
+	guint delay_sco;
+	ofono_emulator_codec_negotiation_cb codec_negotiation_cb;
+	void *codec_negotiation_data;
+	ofono_bool_t bac_received;
 	bool slc : 1;
 	unsigned int events_mode : 2;
 	bool events_ind : 1;
@@ -938,6 +955,168 @@ fail:
 	}
 }
 
+static void bac_cb(GAtServer *server, GAtServerRequestType type,
+			GAtResult *result, gpointer user_data)
+{
+	struct ofono_emulator *em = user_data;
+	GAtResultIter iter;
+	int val;
+
+	DBG("");
+
+	switch (type) {
+	case G_AT_SERVER_REQUEST_TYPE_SET:
+		g_at_result_iter_init(&iter, result);
+		g_at_result_iter_next(&iter, "");
+
+		/*
+		 * CVSD codec is mandatory and must come first.
+		 * See HFP v1.6 4.34.1
+		 */
+		if (g_at_result_iter_next_number(&iter, &val) == FALSE ||
+				val != HFP_CODEC_CVSD)
+			goto fail;
+
+		em->bac_received = TRUE;
+
+		em->negotiated_codec = 0;
+		em->r_codecs[CVSD_OFFSET].supported = TRUE;
+
+		while (g_at_result_iter_next_number(&iter, &val)) {
+			switch (val) {
+			case HFP_CODEC_MSBC:
+				em->r_codecs[MSBC_OFFSET].supported = TRUE;
+				break;
+			default:
+				DBG("Unsupported HFP codec %d", val);
+				break;
+			}
+		}
+
+		g_at_server_send_final(server, G_AT_SERVER_RESULT_OK);
+
+		/*
+		 * If we're currently in the process of selecting a codec
+		 * we have to restart that now
+		 */
+		if (em->proposed_codec)
+			ofono_emulator_start_codec_negotiation(em, 0, NULL, NULL);
+
+		break;
+
+	default:
+fail:
+		DBG("Process AT+BAC failed");
+		g_at_server_send_final(server, G_AT_SERVER_RESULT_ERROR);
+		break;
+	}
+}
+
+static void finish_codec_negotiation(struct ofono_emulator *em,
+			int err)
+{
+	if (em->codec_negotiation_cb == NULL)
+		return;
+
+	em->codec_negotiation_cb(err, em->codec_negotiation_data);
+
+	em->codec_negotiation_cb = NULL;
+	em->codec_negotiation_data = NULL;
+}
+
+static void connect_sco(struct ofono_emulator *em)
+{
+	int err;
+
+	DBG("");
+
+	em->delay_sco = 0;
+
+	err = ofono_handsfree_card_connect_sco(em->card);
+	if (err == 0) {
+		finish_codec_negotiation(em, 0);
+		return;
+	}
+
+	/* If we have another codec we can try then lets do that */
+	if (em->negotiated_codec != HFP_CODEC_CVSD) {
+		ofono_emulator_start_codec_negotiation(em, HFP_CODEC_CVSD,
+							em->codec_negotiation_cb,
+							em->codec_negotiation_data);
+		return;
+	}
+
+	finish_codec_negotiation(em, -EIO);
+}
+
+static void bcs_cb(GAtServer *server, GAtServerRequestType type,
+			GAtResult *result, gpointer user_data)
+{
+	struct ofono_emulator *em = user_data;
+	GAtResultIter iter;
+	int val;
+
+	switch (type) {
+	case G_AT_SERVER_REQUEST_TYPE_SET:
+		g_at_result_iter_init(&iter, result);
+		g_at_result_iter_next(&iter, "");
+
+		if (!g_at_result_iter_next_number(&iter, &val))
+			break;
+
+		if (em->proposed_codec != val) {
+			em->proposed_codec = 0;
+			break;
+		}
+
+		em->proposed_codec = 0;
+		em->negotiated_codec = val;
+
+		DBG("negotiated codec %d", val);
+
+		if (em->card != NULL)
+			ofono_handsfree_card_set_codec(em->card,
+							em->negotiated_codec);
+
+		g_at_server_send_final(server, G_AT_SERVER_RESULT_OK);
+
+		connect_sco(em);
+
+		return;
+	default:
+		break;
+	}
+
+	finish_codec_negotiation(em, -EIO);
+
+	g_at_server_send_final(server, G_AT_SERVER_RESULT_ERROR);
+}
+
+static void bcc_cb(GAtServer *server, GAtServerRequestType type,
+			GAtResult *result, gpointer user_data)
+{
+	struct ofono_emulator *em = user_data;
+
+	switch (type) {
+	case G_AT_SERVER_REQUEST_TYPE_COMMAND_ONLY:
+
+		g_at_server_send_final(server, G_AT_SERVER_RESULT_OK);
+
+		if (!em->negotiated_codec) {
+			ofono_emulator_start_codec_negotiation(em, 0, NULL, NULL);
+			return;
+		}
+
+		connect_sco(em);
+
+		return;
+	default:
+		break;
+	}
+
+	g_at_server_send_final(server, G_AT_SERVER_RESULT_ERROR);
+}
+
 static void emulator_add_indicator(struct ofono_emulator *em, const char* name,
 					int min, int max, int dflt,
 					gboolean mandatory)
@@ -1047,6 +1226,9 @@ void ofono_emulator_register(struct ofono_emulator *em, int fd)
 		g_at_server_register(em->server, "+BIA", bia_cb, em, NULL);
 		g_at_server_register(em->server, "+BIND", bind_cb, em, NULL);
 		g_at_server_register(em->server, "+BIEV", biev_cb, em, NULL);
+		g_at_server_register(em->server, "+BAC", bac_cb, em, NULL);
+		g_at_server_register(em->server, "+BCC", bcc_cb, em, NULL);
+		g_at_server_register(em->server, "+BCS", bcs_cb, em, NULL);
 	}
 
 	__ofono_atom_register(em->atom, emulator_unregister);
@@ -1101,6 +1283,7 @@ struct ofono_emulator *ofono_emulator_create(struct ofono_modem *modem,
 	em->l_features |= HFP_AG_FEATURE_ENHANCED_CALL_CONTROL;
 	em->l_features |= HFP_AG_FEATURE_EXTENDED_RES_CODE;
 	em->l_features |= HFP_AG_FEATURE_HF_INDICATORS;
+	em->l_features |= HFP_AG_FEATURE_CODEC_NEGOTIATION;
 	em->events_mode = 3;	/* default mode is forwarding events */
 	em->cmee_mode = 0;	/* CME ERROR disabled by default */
 
@@ -1476,3 +1659,68 @@ void ofono_emulator_set_handsfree_card(struct ofono_emulator *em,
 
 	em->card = card;
 }
+
+static unsigned char select_codec(struct ofono_emulator *em)
+{
+	if (ofono_handsfree_audio_has_wideband() &&
+			em->r_codecs[MSBC_OFFSET].supported)
+		return HFP_CODEC_MSBC;
+
+	/* CVSD is mandatory for both sides */
+	return HFP_CODEC_CVSD;
+}
+
+int ofono_emulator_start_codec_negotiation(struct ofono_emulator *em,
+						unsigned char codec,
+						ofono_emulator_codec_negotiation_cb cb, void *data)
+{
+	char buf[64];
+	unsigned char selected_codec;
+
+	if (em == NULL)
+		return -EINVAL;
+
+	if (cb != NULL && em->codec_negotiation_cb != NULL)
+		return -EALREADY;
+
+	if (!em->bac_received || em->negotiated_codec > 0) {
+		/*
+		 * Report we're done even if we don't have done any
+		 * negotiation as the other side may have to clean up.
+		 */
+		cb(0, data);
+
+		/*
+		 * If we didn't received any +BAC during the SLC setup the
+		 * remote side doesn't support codec negotiation and we can
+		 * directly connect our card. Otherwise if we got +BAC and
+		 * already have a negotiated codec we can proceed here
+		 * without doing any negotiation again.
+		 */
+		ofono_handsfree_card_connect_sco(em->card);
+
+		return 0;
+	}
+
+	if (codec > 0) {
+		selected_codec = codec;
+		goto done;
+	}
+
+	selected_codec = select_codec(em);
+	if (!selected_codec) {
+		DBG("Failed to selected HFP codec");
+		return -EINVAL;
+	}
+
+done:
+	em->proposed_codec = selected_codec;
+
+	em->codec_negotiation_cb = cb;
+	em->codec_negotiation_data = data;
+
+	snprintf(buf, 64, "+BCS: %d", selected_codec);
+	g_at_server_send_unsolicited(em->server, buf);
+
+	return 0;
+}
-- 
2.5.0

[PATCH 2/2 v3] hfp_ag_bluez5: use codec negotiation

[Simon Fels]

[-- Attachment #1: Type: text/plain, Size: 2402 bytes --]

---
 plugins/hfp_ag_bluez5.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 55 insertions(+), 1 deletion(-)

diff --git a/plugins/hfp_ag_bluez5.c b/plugins/hfp_ag_bluez5.c
index 3aca792..c5abff0 100644
--- a/plugins/hfp_ag_bluez5.c
+++ b/plugins/hfp_ag_bluez5.c
@@ -38,6 +38,11 @@
 #include <ofono/modem.h>
 #include <ofono/handsfree-audio.h>
 
+typedef struct GAtChat GAtChat;
+typedef struct GAtResult GAtResult;
+
+#include "drivers/atmodem/atutil.h"
+
 #include "hfp.h"
 #include "bluez5.h"
 #include "bluetooth.h"
@@ -69,12 +74,59 @@ static void hfp_card_remove(struct ofono_handsfree_card *card)
 	DBG("");
 }
 
+static void codec_negotiation_done_cb(int err, void *data)
+{
+	struct cb_data *cbd = data;
+	ofono_handsfree_card_connect_cb_t cb = cbd->cb;
+
+	DBG("err %d", err);
+
+	if (err < 0) {
+		CALLBACK_WITH_FAILURE(cb, cbd->data);
+		goto done;
+	}
+
+	/*
+	 * We don't have anything to do at this point as when the
+	 * codec negotiation succeeded the emulator internally
+	 * already triggered the SCO connection setup of the
+	 * handsfree card which also takes over the processing
+	 * of the pending dbus message
+	 */
+
+done:
+	g_free(cbd);
+}
+
 static void hfp_card_connect(struct ofono_handsfree_card *card,
 					ofono_handsfree_card_connect_cb_t cb,
 					void *data)
 {
+	int err;
+	struct ofono_emulator *em = ofono_handsfree_card_get_data(card);
+	struct cb_data *cbd;
+
 	DBG("");
-	ofono_handsfree_card_connect_sco(card);
+
+	cbd = cb_data_new(cb, data);
+
+	/*
+	 * The emulator core will take care if the remote side supports
+	 * codec negotiation or not.
+	 */
+	err = ofono_emulator_start_codec_negotiation(em, 0,
+					codec_negotiation_done_cb, cbd);
+	if (err < 0) {
+		CALLBACK_WITH_FAILURE(cb, data);
+
+		g_free(cbd);
+		return;
+	}
+
+	/*
+	 * We hand over to the emulator core here to establish the
+	 * SCO connection once the codec is negotiated
+	 * */
 }
 
 static void hfp_sco_connected_hint(struct ofono_handsfree_card *card)
@@ -208,6 +260,8 @@ static DBusMessage *profile_new_connection(DBusConnection *conn,
 					OFONO_HANDSFREE_CARD_TYPE_GATEWAY,
 					HFP_AG_DRIVER, em);
 
+	ofono_handsfree_card_set_data(card, em);
+
 	ofono_handsfree_card_set_local(card, local);
 	ofono_handsfree_card_set_remote(card, remote);
 
-- 
2.5.0

Re: [PATCH 1/2 v3] emulator: add codec negotiation support

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 10823 bytes --]

Hi Simon,

On 10/20/2015 08:01 AM, Simon Fels wrote:
> ---
>   include/emulator.h |   6 ++
>   src/emulator.c     | 248 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 254 insertions(+)
>

Just a couple of small nitpicks below:

> diff --git a/include/emulator.h b/include/emulator.h
> index 15dc61c..36153ef 100644
> --- a/include/emulator.h
> +++ b/include/emulator.h
> @@ -112,6 +112,12 @@ void ofono_emulator_set_hf_indicator_active(struct ofono_emulator *em,
>   void ofono_emulator_set_handsfree_card(struct ofono_emulator *em,
>   					struct ofono_handsfree_card *card);
>
> +typedef void (*ofono_emulator_codec_negotiation_cb)(int err, void *data);
> +
> +int ofono_emulator_start_codec_negotiation(struct ofono_emulator *em,
> +						unsigned char codec,
> +						ofono_emulator_codec_negotiation_cb cb, void *data);

Please make sure not to go over 80 chars / line.  Even if you need to 
indent arguments 2-4 less than what is required by our style guidelines.

> +
>   #ifdef __cplusplus
>   }
>   #endif
> diff --git a/src/emulator.c b/src/emulator.c
> index 626dec3..e16312e 100644
> --- a/src/emulator.c
> +++ b/src/emulator.c
> @@ -27,6 +27,7 @@
>   #include <string.h>
>   #include <unistd.h>
>   #include <stdbool.h>
> +#include <errno.h>
>
>   #include <glib.h>
>
> @@ -39,6 +40,15 @@
>
>   #define RING_TIMEOUT 3
>
> +#define CVSD_OFFSET 0
> +#define MSBC_OFFSET 1
> +#define CODECS_COUNT (MSBC_OFFSET + 1)
> +
> +struct hfp_codec_info {
> +	unsigned char type;
> +	ofono_bool_t supported;
> +};
> +
>   struct ofono_emulator {
>   	struct ofono_atom *atom;
>   	enum ofono_emulator_type type;
> @@ -50,6 +60,13 @@ struct ofono_emulator {
>   	guint callsetup_source;
>   	int pns_id;
>   	struct ofono_handsfree_card *card;
> +	struct hfp_codec_info r_codecs[CODECS_COUNT];
> +	unsigned char negotiated_codec;
> +	unsigned char proposed_codec;
> +	guint delay_sco;

Is this still needed?

> +	ofono_emulator_codec_negotiation_cb codec_negotiation_cb;
> +	void *codec_negotiation_data;
> +	ofono_bool_t bac_received;
>   	bool slc : 1;
>   	unsigned int events_mode : 2;
>   	bool events_ind : 1;
> @@ -938,6 +955,168 @@ fail:
>   	}
>   }
>
> +static void bac_cb(GAtServer *server, GAtServerRequestType type,
> +			GAtResult *result, gpointer user_data)
> +{
> +	struct ofono_emulator *em = user_data;
> +	GAtResultIter iter;
> +	int val;
> +
> +	DBG("");
> +
> +	switch (type) {
> +	case G_AT_SERVER_REQUEST_TYPE_SET:
> +		g_at_result_iter_init(&iter, result);
> +		g_at_result_iter_next(&iter, "");
> +
> +		/*
> +		 * CVSD codec is mandatory and must come first.
> +		 * See HFP v1.6 4.34.1
> +		 */
> +		if (g_at_result_iter_next_number(&iter, &val) == FALSE ||
> +				val != HFP_CODEC_CVSD)
> +			goto fail;
> +
> +		em->bac_received = TRUE;
> +
> +		em->negotiated_codec = 0;
> +		em->r_codecs[CVSD_OFFSET].supported = TRUE;
> +
> +		while (g_at_result_iter_next_number(&iter, &val)) {
> +			switch (val) {
> +			case HFP_CODEC_MSBC:
> +				em->r_codecs[MSBC_OFFSET].supported = TRUE;
> +				break;
> +			default:
> +				DBG("Unsupported HFP codec %d", val);
> +				break;
> +			}
> +		}
> +
> +		g_at_server_send_final(server, G_AT_SERVER_RESULT_OK);
> +
> +		/*
> +		 * If we're currently in the process of selecting a codec
> +		 * we have to restart that now
> +		 */
> +		if (em->proposed_codec)
> +			ofono_emulator_start_codec_negotiation(em, 0, NULL, NULL);
> +
> +		break;
> +
> +	default:
> +fail:
> +		DBG("Process AT+BAC failed");
> +		g_at_server_send_final(server, G_AT_SERVER_RESULT_ERROR);

In theory our codec-negotiation procedure might have failed, do we want 
to call the callback here as well?

> +		break;
> +	}
> +}
> +
> +static void finish_codec_negotiation(struct ofono_emulator *em,
> +			int err)
> +{
> +	if (em->codec_negotiation_cb == NULL)
> +		return;
> +
> +	em->codec_negotiation_cb(err, em->codec_negotiation_data);
> +
> +	em->codec_negotiation_cb = NULL;
> +	em->codec_negotiation_data = NULL;
> +}
> +
> +static void connect_sco(struct ofono_emulator *em)
> +{
> +	int err;
> +
> +	DBG("");
> +
> +	em->delay_sco = 0;

What does this do?

> +
> +	err = ofono_handsfree_card_connect_sco(em->card);

ofono_handsfree_card_connect_sco function does not check for em->card 
being NULL.

> +	if (err == 0) {
> +		finish_codec_negotiation(em, 0);
> +		return;
> +	}
> +
> +	/* If we have another codec we can try then lets do that */
> +	if (em->negotiated_codec != HFP_CODEC_CVSD) {
> +		ofono_emulator_start_codec_negotiation(em, HFP_CODEC_CVSD,
> +							em->codec_negotiation_cb,
> +							em->codec_negotiation_data);

Over 80 characters here again.  Indent less if you need to.

ofono_handsfree_card_connect_sco won't really fail unless the kernel is 
not configured properly.  I'm unsure of the practical utility of this 
logic.  Would it be simpler to mark the wideband / negotiated codec as 
unsupported and simply error out here?

> +		return;
> +	}
> +
> +	finish_codec_negotiation(em, -EIO);
> +}
> +
> +static void bcs_cb(GAtServer *server, GAtServerRequestType type,
> +			GAtResult *result, gpointer user_data)
> +{
> +	struct ofono_emulator *em = user_data;
> +	GAtResultIter iter;
> +	int val;
> +
> +	switch (type) {
> +	case G_AT_SERVER_REQUEST_TYPE_SET:
> +		g_at_result_iter_init(&iter, result);
> +		g_at_result_iter_next(&iter, "");
> +
> +		if (!g_at_result_iter_next_number(&iter, &val))
> +			break;
> +
> +		if (em->proposed_codec != val) {
> +			em->proposed_codec = 0;
> +			break;
> +		}
> +
> +		em->proposed_codec = 0;
> +		em->negotiated_codec = val;
> +
> +		DBG("negotiated codec %d", val);
> +
> +		if (em->card != NULL)
> +			ofono_handsfree_card_set_codec(em->card,
> +							em->negotiated_codec);
> +
> +		g_at_server_send_final(server, G_AT_SERVER_RESULT_OK);
> +
> +		connect_sco(em);
> +
> +		return;
> +	default:
> +		break;
> +	}
> +
> +	finish_codec_negotiation(em, -EIO);
> +
> +	g_at_server_send_final(server, G_AT_SERVER_RESULT_ERROR);
> +}
> +
> +static void bcc_cb(GAtServer *server, GAtServerRequestType type,
> +			GAtResult *result, gpointer user_data)
> +{
> +	struct ofono_emulator *em = user_data;
> +
> +	switch (type) {
> +	case G_AT_SERVER_REQUEST_TYPE_COMMAND_ONLY:
> +

No need for an empty line here.

> +		g_at_server_send_final(server, G_AT_SERVER_RESULT_OK);
> +
> +		if (!em->negotiated_codec) {
> +			ofono_emulator_start_codec_negotiation(em, 0, NULL, NULL);

Over 80 characters / line here.

It seems we would have a potential race condition here.  Lets say HF 
sends AT+BCC, we call ofono_emulator_start_codec_negotiation.  Right 
after, the AG calls HandsfreeCard.Connect.  This will result in two +BCS 
unsolicited notifications being sent out.

We might want to put in a guard where we don't send another +BCS 
notification until the HF has sent us back a +BCS or +BAC command. 
Also, in the case of HF initiated connections, it probably makes sense 
to reject any immediately subsequent attempts to establish the audio 
connection by the AG side with an -EALREADY.

> +			return;
> +		}
> +
> +		connect_sco(em);
> +
> +		return;
> +	default:
> +		break;
> +	}
> +
> +	g_at_server_send_final(server, G_AT_SERVER_RESULT_ERROR);
> +}
> +
>   static void emulator_add_indicator(struct ofono_emulator *em, const char* name,
>   					int min, int max, int dflt,
>   					gboolean mandatory)
> @@ -1047,6 +1226,9 @@ void ofono_emulator_register(struct ofono_emulator *em, int fd)
>   		g_at_server_register(em->server, "+BIA", bia_cb, em, NULL);
>   		g_at_server_register(em->server, "+BIND", bind_cb, em, NULL);
>   		g_at_server_register(em->server, "+BIEV", biev_cb, em, NULL);
> +		g_at_server_register(em->server, "+BAC", bac_cb, em, NULL);
> +		g_at_server_register(em->server, "+BCC", bcc_cb, em, NULL);
> +		g_at_server_register(em->server, "+BCS", bcs_cb, em, NULL);
>   	}
>
>   	__ofono_atom_register(em->atom, emulator_unregister);
> @@ -1101,6 +1283,7 @@ struct ofono_emulator *ofono_emulator_create(struct ofono_modem *modem,
>   	em->l_features |= HFP_AG_FEATURE_ENHANCED_CALL_CONTROL;
>   	em->l_features |= HFP_AG_FEATURE_EXTENDED_RES_CODE;
>   	em->l_features |= HFP_AG_FEATURE_HF_INDICATORS;
> +	em->l_features |= HFP_AG_FEATURE_CODEC_NEGOTIATION;
>   	em->events_mode = 3;	/* default mode is forwarding events */
>   	em->cmee_mode = 0;	/* CME ERROR disabled by default */
>
> @@ -1476,3 +1659,68 @@ void ofono_emulator_set_handsfree_card(struct ofono_emulator *em,
>
>   	em->card = card;
>   }
> +
> +static unsigned char select_codec(struct ofono_emulator *em)
> +{
> +	if (ofono_handsfree_audio_has_wideband() &&
> +			em->r_codecs[MSBC_OFFSET].supported)
> +		return HFP_CODEC_MSBC;
> +
> +	/* CVSD is mandatory for both sides */
> +	return HFP_CODEC_CVSD;
> +}
> +
> +int ofono_emulator_start_codec_negotiation(struct ofono_emulator *em,
> +						unsigned char codec,

What is the theoretical purpose of this codec parameter?  Is the intent 
to only use it for the fallback use case in connect_sco() above?  If so, 
then we probably need to handle it some other way and remove this 
parameter from the public API.

> +						ofono_emulator_codec_negotiation_cb cb, void *data)
> +{
> +	char buf[64];
> +	unsigned char selected_codec;
> +
> +	if (em == NULL)
> +		return -EINVAL;
> +
> +	if (cb != NULL && em->codec_negotiation_cb != NULL)
> +		return -EALREADY;
> +
> +	if (!em->bac_received || em->negotiated_codec > 0) {
> +		/*
> +		 * Report we're done even if we don't have done any
> +		 * negotiation as the other side may have to clean up.
> +		 */
> +		cb(0, data);
> +
> +		/*
> +		 * If we didn't received any +BAC during the SLC setup the
> +		 * remote side doesn't support codec negotiation and we can
> +		 * directly connect our card. Otherwise if we got +BAC and
> +		 * already have a negotiated codec we can proceed here
> +		 * without doing any negotiation again.
> +		 */
> +		ofono_handsfree_card_connect_sco(em->card);
> +
> +		return 0;
> +	}
> +
> +	if (codec > 0) {
> +		selected_codec = codec;
> +		goto done;
> +	}
> +
> +	selected_codec = select_codec(em);
> +	if (!selected_codec) {
> +		DBG("Failed to selected HFP codec");
> +		return -EINVAL;
> +	}
> +
> +done:
> +	em->proposed_codec = selected_codec;
> +
> +	em->codec_negotiation_cb = cb;
> +	em->codec_negotiation_data = data;
> +
> +	snprintf(buf, 64, "+BCS: %d", selected_codec);
> +	g_at_server_send_unsolicited(em->server, buf);
> +
> +	return 0;
> +}
>

Regards,
-Denis

Re: [PATCH 1/2 v3] emulator: add codec negotiation support

[Simon Fels]

[-- Attachment #1: Type: text/plain, Size: 10422 bytes --]

Hey Denis,

> Please make sure not to go over 80 chars / line.  Even if you need to
> indent arguments 2-4 less than what is required by our style guidelines.

vim shows me I am at 76 chars with having 4 spaces per tab. So how many 
spaces do you take per tab to come over 80 chars? 8 spaces?

>> +
>>   #ifdef __cplusplus
>>   }
>>   #endif
>> diff --git a/src/emulator.c b/src/emulator.c
>> index 626dec3..e16312e 100644
>> --- a/src/emulator.c
>> +++ b/src/emulator.c
>> @@ -27,6 +27,7 @@
>>   #include <string.h>
>>   #include <unistd.h>
>>   #include <stdbool.h>
>> +#include <errno.h>
>>
>>   #include <glib.h>
>>
>> @@ -39,6 +40,15 @@
>>
>>   #define RING_TIMEOUT 3
>>
>> +#define CVSD_OFFSET 0
>> +#define MSBC_OFFSET 1
>> +#define CODECS_COUNT (MSBC_OFFSET + 1)
>> +
>> +struct hfp_codec_info {
>> +    unsigned char type;
>> +    ofono_bool_t supported;
>> +};
>> +
>>   struct ofono_emulator {
>>       struct ofono_atom *atom;
>>       enum ofono_emulator_type type;
>> @@ -50,6 +60,13 @@ struct ofono_emulator {
>>       guint callsetup_source;
>>       int pns_id;
>>       struct ofono_handsfree_card *card;
>> +    struct hfp_codec_info r_codecs[CODECS_COUNT];
>> +    unsigned char negotiated_codec;
>> +    unsigned char proposed_codec;
>> +    guint delay_sco;
>
> Is this still needed?
>
>> +    ofono_emulator_codec_negotiation_cb codec_negotiation_cb;
>> +    void *codec_negotiation_data;
>> +    ofono_bool_t bac_received;
>>       bool slc : 1;
>>       unsigned int events_mode : 2;
>>       bool events_ind : 1;
>> @@ -938,6 +955,168 @@ fail:
>>       }
>>   }
>>
>> +static void bac_cb(GAtServer *server, GAtServerRequestType type,
>> +            GAtResult *result, gpointer user_data)
>> +{
>> +    struct ofono_emulator *em = user_data;
>> +    GAtResultIter iter;
>> +    int val;
>> +
>> +    DBG("");
>> +
>> +    switch (type) {
>> +    case G_AT_SERVER_REQUEST_TYPE_SET:
>> +        g_at_result_iter_init(&iter, result);
>> +        g_at_result_iter_next(&iter, "");
>> +
>> +        /*
>> +         * CVSD codec is mandatory and must come first.
>> +         * See HFP v1.6 4.34.1
>> +         */
>> +        if (g_at_result_iter_next_number(&iter, &val) == FALSE ||
>> +                val != HFP_CODEC_CVSD)
>> +            goto fail;
>> +
>> +        em->bac_received = TRUE;
>> +
>> +        em->negotiated_codec = 0;
>> +        em->r_codecs[CVSD_OFFSET].supported = TRUE;
>> +
>> +        while (g_at_result_iter_next_number(&iter, &val)) {
>> +            switch (val) {
>> +            case HFP_CODEC_MSBC:
>> +                em->r_codecs[MSBC_OFFSET].supported = TRUE;
>> +                break;
>> +            default:
>> +                DBG("Unsupported HFP codec %d", val);
>> +                break;
>> +            }
>> +        }
>> +
>> +        g_at_server_send_final(server, G_AT_SERVER_RESULT_OK);
>> +
>> +        /*
>> +         * If we're currently in the process of selecting a codec
>> +         * we have to restart that now
>> +         */
>> +        if (em->proposed_codec)
>> +            ofono_emulator_start_codec_negotiation(em, 0, NULL, NULL);
>> +
>> +        break;
>> +
>> +    default:
>> +fail:
>> +        DBG("Process AT+BAC failed");
>> +        g_at_server_send_final(server, G_AT_SERVER_RESULT_ERROR);
>
> In theory our codec-negotiation procedure might have failed, do we want
> to call the callback here as well?
>
>> +        break;
>> +    }
>> +}
>> +
>> +static void finish_codec_negotiation(struct ofono_emulator *em,
>> +            int err)
>> +{
>> +    if (em->codec_negotiation_cb == NULL)
>> +        return;
>> +
>> +    em->codec_negotiation_cb(err, em->codec_negotiation_data);
>> +
>> +    em->codec_negotiation_cb = NULL;
>> +    em->codec_negotiation_data = NULL;
>> +}
>> +
>> +static void connect_sco(struct ofono_emulator *em)
>> +{
>> +    int err;
>> +
>> +    DBG("");
>> +
>> +    em->delay_sco = 0;
>
> What does this do?
>
>> +
>> +    err = ofono_handsfree_card_connect_sco(em->card);
>
> ofono_handsfree_card_connect_sco function does not check for em->card
> being NULL.
>
>> +    if (err == 0) {
>> +        finish_codec_negotiation(em, 0);
>> +        return;
>> +    }
>> +
>> +    /* If we have another codec we can try then lets do that */
>> +    if (em->negotiated_codec != HFP_CODEC_CVSD) {
>> +        ofono_emulator_start_codec_negotiation(em, HFP_CODEC_CVSD,
>> +                            em->codec_negotiation_cb,
>> +                            em->codec_negotiation_data);
>
> Over 80 characters here again.  Indent less if you need to.
>
> ofono_handsfree_card_connect_sco won't really fail unless the kernel is
> not configured properly.  I'm unsure of the practical utility of this
> logic.  Would it be simpler to mark the wideband / negotiated codec as
> unsupported and simply error out here?

This is the fallback to the mandatory codec. According to the spec this 
one must be supported by both sides so if we have selected a different 
one before we fallback to what must work but negotiate that first before 
actually using it. Sure, if we end up here then something in our system 
is wrong as when we say we support a codec we should be able to use it 
without further problems. We can go and drop this if you think we don't 
need it.

>> +        return;
>> +    }
>> +
>> +    finish_codec_negotiation(em, -EIO);
>> +}
>> +
>> +static void bcs_cb(GAtServer *server, GAtServerRequestType type,
>> +            GAtResult *result, gpointer user_data)
>> +{
>> +    struct ofono_emulator *em = user_data;
>> +    GAtResultIter iter;
>> +    int val;
>> +
>> +    switch (type) {
>> +    case G_AT_SERVER_REQUEST_TYPE_SET:
>> +        g_at_result_iter_init(&iter, result);
>> +        g_at_result_iter_next(&iter, "");
>> +
>> +        if (!g_at_result_iter_next_number(&iter, &val))
>> +            break;
>> +
>> +        if (em->proposed_codec != val) {
>> +            em->proposed_codec = 0;
>> +            break;
>> +        }
>> +
>> +        em->proposed_codec = 0;
>> +        em->negotiated_codec = val;
>> +
>> +        DBG("negotiated codec %d", val);
>> +
>> +        if (em->card != NULL)
>> +            ofono_handsfree_card_set_codec(em->card,
>> +                            em->negotiated_codec);
>> +
>> +        g_at_server_send_final(server, G_AT_SERVER_RESULT_OK);
>> +
>> +        connect_sco(em);
>> +
>> +        return;
>> +    default:
>> +        break;
>> +    }
>> +
>> +    finish_codec_negotiation(em, -EIO);
>> +
>> +    g_at_server_send_final(server, G_AT_SERVER_RESULT_ERROR);
>> +}
>> +
>> +static void bcc_cb(GAtServer *server, GAtServerRequestType type,
>> +            GAtResult *result, gpointer user_data)
>> +{
>> +    struct ofono_emulator *em = user_data;
>> +
>> +    switch (type) {
>> +    case G_AT_SERVER_REQUEST_TYPE_COMMAND_ONLY:
>> +
>
> No need for an empty line here.
>
>> +        g_at_server_send_final(server, G_AT_SERVER_RESULT_OK);
>> +
>> +        if (!em->negotiated_codec) {
>> +            ofono_emulator_start_codec_negotiation(em, 0, NULL, NULL);
>
> Over 80 characters / line here.
>
> It seems we would have a potential race condition here.  Lets say HF
> sends AT+BCC, we call ofono_emulator_start_codec_negotiation.  Right
> after, the AG calls HandsfreeCard.Connect.  This will result in two +BCS
> unsolicited notifications being sent out.
>
> We might want to put in a guard where we don't send another +BCS
> notification until the HF has sent us back a +BCS or +BAC command. Also,
> in the case of HF initiated connections, it probably makes sense to
> reject any immediately subsequent attempts to establish the audio
> connection by the AG side with an -EALREADY.

Good point. Let me add proper guards.

>> +            return;
>> +        }
>> +
>> +        connect_sco(em);
>> +
>> +        return;
>> +    default:
>> +        break;
>> +    }
>> +
>> +    g_at_server_send_final(server, G_AT_SERVER_RESULT_ERROR);
>> +}
>> +
>>   static void emulator_add_indicator(struct ofono_emulator *em, const
>> char* name,
>>                       int min, int max, int dflt,
>>                       gboolean mandatory)
>> @@ -1047,6 +1226,9 @@ void ofono_emulator_register(struct
>> ofono_emulator *em, int fd)
>>           g_at_server_register(em->server, "+BIA", bia_cb, em, NULL);
>>           g_at_server_register(em->server, "+BIND", bind_cb, em, NULL);
>>           g_at_server_register(em->server, "+BIEV", biev_cb, em, NULL);
>> +        g_at_server_register(em->server, "+BAC", bac_cb, em, NULL);
>> +        g_at_server_register(em->server, "+BCC", bcc_cb, em, NULL);
>> +        g_at_server_register(em->server, "+BCS", bcs_cb, em, NULL);
>>       }
>>
>>       __ofono_atom_register(em->atom, emulator_unregister);
>> @@ -1101,6 +1283,7 @@ struct ofono_emulator
>> *ofono_emulator_create(struct ofono_modem *modem,
>>       em->l_features |= HFP_AG_FEATURE_ENHANCED_CALL_CONTROL;
>>       em->l_features |= HFP_AG_FEATURE_EXTENDED_RES_CODE;
>>       em->l_features |= HFP_AG_FEATURE_HF_INDICATORS;
>> +    em->l_features |= HFP_AG_FEATURE_CODEC_NEGOTIATION;
>>       em->events_mode = 3;    /* default mode is forwarding events */
>>       em->cmee_mode = 0;    /* CME ERROR disabled by default */
>>
>> @@ -1476,3 +1659,68 @@ void ofono_emulator_set_handsfree_card(struct
>> ofono_emulator *em,
>>
>>       em->card = card;
>>   }
>> +
>> +static unsigned char select_codec(struct ofono_emulator *em)
>> +{
>> +    if (ofono_handsfree_audio_has_wideband() &&
>> +            em->r_codecs[MSBC_OFFSET].supported)
>> +        return HFP_CODEC_MSBC;
>> +
>> +    /* CVSD is mandatory for both sides */
>> +    return HFP_CODEC_CVSD;
>> +}
>> +
>> +int ofono_emulator_start_codec_negotiation(struct ofono_emulator *em,
>> +                        unsigned char codec,
>
> What is the theoretical purpose of this codec parameter?  Is the intent
> to only use it for the fallback use case in connect_sco() above?  If so,
> then we probably need to handle it some other way and remove this
> parameter from the public API.

Yes, that was the reason. Will drop that.

regards,
Simon

Re: [PATCH 1/2 v3] emulator: add codec negotiation support

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 6407 bytes --]

Hi Simon,

On 10/21/2015 02:42 AM, Simon Fels wrote:
> Hey Denis,
>
>> Please make sure not to go over 80 chars / line.  Even if you need to
>> indent arguments 2-4 less than what is required by our style guidelines.
>
> vim shows me I am at 76 chars with having 4 spaces per tab. So how many
> spaces do you take per tab to come over 80 chars? 8 spaces?
>

We use kernel coding style, so yes, 8 spaces.

>>> +
>>>   #ifdef __cplusplus
>>>   }
>>>   #endif
>>> diff --git a/src/emulator.c b/src/emulator.c
>>> index 626dec3..e16312e 100644
>>> --- a/src/emulator.c
>>> +++ b/src/emulator.c
>>> @@ -27,6 +27,7 @@
>>>   #include <string.h>
>>>   #include <unistd.h>
>>>   #include <stdbool.h>
>>> +#include <errno.h>
>>>
>>>   #include <glib.h>
>>>
>>> @@ -39,6 +40,15 @@
>>>
>>>   #define RING_TIMEOUT 3
>>>
>>> +#define CVSD_OFFSET 0
>>> +#define MSBC_OFFSET 1
>>> +#define CODECS_COUNT (MSBC_OFFSET + 1)
>>> +
>>> +struct hfp_codec_info {
>>> +    unsigned char type;
>>> +    ofono_bool_t supported;
>>> +};
>>> +
>>>   struct ofono_emulator {
>>>       struct ofono_atom *atom;
>>>       enum ofono_emulator_type type;
>>> @@ -50,6 +60,13 @@ struct ofono_emulator {
>>>       guint callsetup_source;
>>>       int pns_id;
>>>       struct ofono_handsfree_card *card;
>>> +    struct hfp_codec_info r_codecs[CODECS_COUNT];
>>> +    unsigned char negotiated_codec;
>>> +    unsigned char proposed_codec;
>>> +    guint delay_sco;
>>
>> Is this still needed?
>>
>>> +    ofono_emulator_codec_negotiation_cb codec_negotiation_cb;
>>> +    void *codec_negotiation_data;
>>> +    ofono_bool_t bac_received;
>>>       bool slc : 1;
>>>       unsigned int events_mode : 2;
>>>       bool events_ind : 1;
>>> @@ -938,6 +955,168 @@ fail:
>>>       }
>>>   }
>>>
>>> +static void bac_cb(GAtServer *server, GAtServerRequestType type,
>>> +            GAtResult *result, gpointer user_data)
>>> +{
>>> +    struct ofono_emulator *em = user_data;
>>> +    GAtResultIter iter;
>>> +    int val;
>>> +
>>> +    DBG("");
>>> +
>>> +    switch (type) {
>>> +    case G_AT_SERVER_REQUEST_TYPE_SET:
>>> +        g_at_result_iter_init(&iter, result);
>>> +        g_at_result_iter_next(&iter, "");
>>> +
>>> +        /*
>>> +         * CVSD codec is mandatory and must come first.
>>> +         * See HFP v1.6 4.34.1
>>> +         */
>>> +        if (g_at_result_iter_next_number(&iter, &val) == FALSE ||
>>> +                val != HFP_CODEC_CVSD)
>>> +            goto fail;
>>> +
>>> +        em->bac_received = TRUE;
>>> +
>>> +        em->negotiated_codec = 0;
>>> +        em->r_codecs[CVSD_OFFSET].supported = TRUE;
>>> +
>>> +        while (g_at_result_iter_next_number(&iter, &val)) {
>>> +            switch (val) {
>>> +            case HFP_CODEC_MSBC:
>>> +                em->r_codecs[MSBC_OFFSET].supported = TRUE;
>>> +                break;
>>> +            default:
>>> +                DBG("Unsupported HFP codec %d", val);
>>> +                break;
>>> +            }
>>> +        }
>>> +
>>> +        g_at_server_send_final(server, G_AT_SERVER_RESULT_OK);
>>> +
>>> +        /*
>>> +         * If we're currently in the process of selecting a codec
>>> +         * we have to restart that now
>>> +         */
>>> +        if (em->proposed_codec)
>>> +            ofono_emulator_start_codec_negotiation(em, 0, NULL, NULL);
>>> +
>>> +        break;
>>> +
>>> +    default:
>>> +fail:
>>> +        DBG("Process AT+BAC failed");
>>> +        g_at_server_send_final(server, G_AT_SERVER_RESULT_ERROR);
>>
>> In theory our codec-negotiation procedure might have failed, do we want
>> to call the callback here as well?
>>
>>> +        break;
>>> +    }
>>> +}
>>> +
>>> +static void finish_codec_negotiation(struct ofono_emulator *em,
>>> +            int err)
>>> +{
>>> +    if (em->codec_negotiation_cb == NULL)
>>> +        return;
>>> +
>>> +    em->codec_negotiation_cb(err, em->codec_negotiation_data);
>>> +
>>> +    em->codec_negotiation_cb = NULL;
>>> +    em->codec_negotiation_data = NULL;
>>> +}
>>> +
>>> +static void connect_sco(struct ofono_emulator *em)
>>> +{
>>> +    int err;
>>> +
>>> +    DBG("");
>>> +
>>> +    em->delay_sco = 0;
>>
>> What does this do?
>>
>>> +
>>> +    err = ofono_handsfree_card_connect_sco(em->card);
>>
>> ofono_handsfree_card_connect_sco function does not check for em->card
>> being NULL.
>>
>>> +    if (err == 0) {
>>> +        finish_codec_negotiation(em, 0);
>>> +        return;
>>> +    }
>>> +
>>> +    /* If we have another codec we can try then lets do that */
>>> +    if (em->negotiated_codec != HFP_CODEC_CVSD) {
>>> +        ofono_emulator_start_codec_negotiation(em, HFP_CODEC_CVSD,
>>> +                            em->codec_negotiation_cb,
>>> +                            em->codec_negotiation_data);
>>
>> Over 80 characters here again.  Indent less if you need to.
>>
>> ofono_handsfree_card_connect_sco won't really fail unless the kernel is
>> not configured properly.  I'm unsure of the practical utility of this
>> logic.  Would it be simpler to mark the wideband / negotiated codec as
>> unsupported and simply error out here?
>
> This is the fallback to the mandatory codec. According to the spec this
> one must be supported by both sides so if we have selected a different
> one before we fallback to what must work but negotiate that first before
> actually using it. Sure, if we end up here then something in our system
> is wrong as when we say we support a codec we should be able to use it
> without further problems. We can go and drop this if you think we don't
> need it.

The spec says:
"If an (e)SCO link cannot be established according to the parameters 
required for the selected codec (e.g., basebands negotiation fails for a 
link with re-transmission although a wide band codec has been selected), 
the Codec Connection establishment procedure shall be re-started by the 
AG with the purpose of selecting a codec that can be used. The mandatory 
narrow band Codec (CVSD) shall be used before the AG gives up trying to 
establish a Codec connection."

In practice, the only failure we can detect here is if setsockopt for 
BT_VOICE fails inside apply_codec_settings.  But I'm okay keeping this 
logic...

Regards,
-Denis

Re: [PATCH 1/2 v3] emulator: add codec negotiation support

[Simon Fels]

[-- Attachment #1: Type: text/plain, Size: 6658 bytes --]

On 21.10.2015 14:57, Denis Kenzior wrote:
> Hi Simon,
>
> On 10/21/2015 02:42 AM, Simon Fels wrote:
>> Hey Denis,
>>
>>> Please make sure not to go over 80 chars / line.  Even if you need to
>>> indent arguments 2-4 less than what is required by our style guidelines.
>>
>> vim shows me I am at 76 chars with having 4 spaces per tab. So how many
>> spaces do you take per tab to come over 80 chars? 8 spaces?
>>
>
> We use kernel coding style, so yes, 8 spaces.
>
>>>> +
>>>>   #ifdef __cplusplus
>>>>   }
>>>>   #endif
>>>> diff --git a/src/emulator.c b/src/emulator.c
>>>> index 626dec3..e16312e 100644
>>>> --- a/src/emulator.c
>>>> +++ b/src/emulator.c
>>>> @@ -27,6 +27,7 @@
>>>>   #include <string.h>
>>>>   #include <unistd.h>
>>>>   #include <stdbool.h>
>>>> +#include <errno.h>
>>>>
>>>>   #include <glib.h>
>>>>
>>>> @@ -39,6 +40,15 @@
>>>>
>>>>   #define RING_TIMEOUT 3
>>>>
>>>> +#define CVSD_OFFSET 0
>>>> +#define MSBC_OFFSET 1
>>>> +#define CODECS_COUNT (MSBC_OFFSET + 1)
>>>> +
>>>> +struct hfp_codec_info {
>>>> +    unsigned char type;
>>>> +    ofono_bool_t supported;
>>>> +};
>>>> +
>>>>   struct ofono_emulator {
>>>>       struct ofono_atom *atom;
>>>>       enum ofono_emulator_type type;
>>>> @@ -50,6 +60,13 @@ struct ofono_emulator {
>>>>       guint callsetup_source;
>>>>       int pns_id;
>>>>       struct ofono_handsfree_card *card;
>>>> +    struct hfp_codec_info r_codecs[CODECS_COUNT];
>>>> +    unsigned char negotiated_codec;
>>>> +    unsigned char proposed_codec;
>>>> +    guint delay_sco;
>>>
>>> Is this still needed?
>>>
>>>> +    ofono_emulator_codec_negotiation_cb codec_negotiation_cb;
>>>> +    void *codec_negotiation_data;
>>>> +    ofono_bool_t bac_received;
>>>>       bool slc : 1;
>>>>       unsigned int events_mode : 2;
>>>>       bool events_ind : 1;
>>>> @@ -938,6 +955,168 @@ fail:
>>>>       }
>>>>   }
>>>>
>>>> +static void bac_cb(GAtServer *server, GAtServerRequestType type,
>>>> +            GAtResult *result, gpointer user_data)
>>>> +{
>>>> +    struct ofono_emulator *em = user_data;
>>>> +    GAtResultIter iter;
>>>> +    int val;
>>>> +
>>>> +    DBG("");
>>>> +
>>>> +    switch (type) {
>>>> +    case G_AT_SERVER_REQUEST_TYPE_SET:
>>>> +        g_at_result_iter_init(&iter, result);
>>>> +        g_at_result_iter_next(&iter, "");
>>>> +
>>>> +        /*
>>>> +         * CVSD codec is mandatory and must come first.
>>>> +         * See HFP v1.6 4.34.1
>>>> +         */
>>>> +        if (g_at_result_iter_next_number(&iter, &val) == FALSE ||
>>>> +                val != HFP_CODEC_CVSD)
>>>> +            goto fail;
>>>> +
>>>> +        em->bac_received = TRUE;
>>>> +
>>>> +        em->negotiated_codec = 0;
>>>> +        em->r_codecs[CVSD_OFFSET].supported = TRUE;
>>>> +
>>>> +        while (g_at_result_iter_next_number(&iter, &val)) {
>>>> +            switch (val) {
>>>> +            case HFP_CODEC_MSBC:
>>>> +                em->r_codecs[MSBC_OFFSET].supported = TRUE;
>>>> +                break;
>>>> +            default:
>>>> +                DBG("Unsupported HFP codec %d", val);
>>>> +                break;
>>>> +            }
>>>> +        }
>>>> +
>>>> +        g_at_server_send_final(server, G_AT_SERVER_RESULT_OK);
>>>> +
>>>> +        /*
>>>> +         * If we're currently in the process of selecting a codec
>>>> +         * we have to restart that now
>>>> +         */
>>>> +        if (em->proposed_codec)
>>>> +            ofono_emulator_start_codec_negotiation(em, 0, NULL, NULL);
>>>> +
>>>> +        break;
>>>> +
>>>> +    default:
>>>> +fail:
>>>> +        DBG("Process AT+BAC failed");
>>>> +        g_at_server_send_final(server, G_AT_SERVER_RESULT_ERROR);
>>>
>>> In theory our codec-negotiation procedure might have failed, do we want
>>> to call the callback here as well?
>>>
>>>> +        break;
>>>> +    }
>>>> +}
>>>> +
>>>> +static void finish_codec_negotiation(struct ofono_emulator *em,
>>>> +            int err)
>>>> +{
>>>> +    if (em->codec_negotiation_cb == NULL)
>>>> +        return;
>>>> +
>>>> +    em->codec_negotiation_cb(err, em->codec_negotiation_data);
>>>> +
>>>> +    em->codec_negotiation_cb = NULL;
>>>> +    em->codec_negotiation_data = NULL;
>>>> +}
>>>> +
>>>> +static void connect_sco(struct ofono_emulator *em)
>>>> +{
>>>> +    int err;
>>>> +
>>>> +    DBG("");
>>>> +
>>>> +    em->delay_sco = 0;
>>>
>>> What does this do?
>>>
>>>> +
>>>> +    err = ofono_handsfree_card_connect_sco(em->card);
>>>
>>> ofono_handsfree_card_connect_sco function does not check for em->card
>>> being NULL.
>>>
>>>> +    if (err == 0) {
>>>> +        finish_codec_negotiation(em, 0);
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    /* If we have another codec we can try then lets do that */
>>>> +    if (em->negotiated_codec != HFP_CODEC_CVSD) {
>>>> +        ofono_emulator_start_codec_negotiation(em, HFP_CODEC_CVSD,
>>>> +                            em->codec_negotiation_cb,
>>>> +                            em->codec_negotiation_data);
>>>
>>> Over 80 characters here again.  Indent less if you need to.
>>>
>>> ofono_handsfree_card_connect_sco won't really fail unless the kernel is
>>> not configured properly.  I'm unsure of the practical utility of this
>>> logic.  Would it be simpler to mark the wideband / negotiated codec as
>>> unsupported and simply error out here?
>>
>> This is the fallback to the mandatory codec. According to the spec this
>> one must be supported by both sides so if we have selected a different
>> one before we fallback to what must work but negotiate that first before
>> actually using it. Sure, if we end up here then something in our system
>> is wrong as when we say we support a codec we should be able to use it
>> without further problems. We can go and drop this if you think we don't
>> need it.
>
> The spec says:
> "If an (e)SCO link cannot be established according to the parameters
> required for the selected codec (e.g., basebands negotiation fails for a
> link with re-transmission although a wide band codec has been selected),
> the Codec Connection establishment procedure shall be re-started by the
> AG with the purpose of selecting a codec that can be used. The mandatory
> narrow band Codec (CVSD) shall be used before the AG gives up trying to
> establish a Codec connection."
>
> In practice, the only failure we can detect here is if setsockopt for
> BT_VOICE fails inside apply_codec_settings.  But I'm okay keeping this
> logic...

Ok.

regards,
Simon