[PATCH 1/3 v2] hfp_ag_bluez5: Add initial handsfree audio driver

[PATCH 1/3 v2] hfp_ag_bluez5: Add initial handsfree audio driver

[Simon Fels]

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

---
 plugins/hfp_ag_bluez5.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 52 insertions(+), 3 deletions(-)

diff --git a/plugins/hfp_ag_bluez5.c b/plugins/hfp_ag_bluez5.c
index ef8a048..3aca792 100644
--- a/plugins/hfp_ag_bluez5.c
+++ b/plugins/hfp_ag_bluez5.c
@@ -49,11 +49,47 @@
 #define HFP_AG_EXT_PROFILE_PATH   "/bluetooth/profile/hfp_ag"
 #define BT_ADDR_SIZE 18
 
+#define HFP_AG_DRIVER		"hfp-ag-driver"
+
 static guint modemwatch_id;
 static GList *modems;
 static GHashTable *sim_hash = NULL;
 static GHashTable *connection_hash;
 
+static int hfp_card_probe(struct ofono_handsfree_card *card,
+					unsigned int vendor, void *data)
+{
+	DBG("");
+
+	return 0;
+}
+
+static void hfp_card_remove(struct ofono_handsfree_card *card)
+{
+	DBG("");
+}
+
+static void hfp_card_connect(struct ofono_handsfree_card *card,
+					ofono_handsfree_card_connect_cb_t cb,
+					void *data)
+{
+	DBG("");
+	ofono_handsfree_card_connect_sco(card);
+}
+
+static void hfp_sco_connected_hint(struct ofono_handsfree_card *card)
+{
+	DBG("");
+}
+
+static struct ofono_handsfree_card_driver hfp_ag_driver = {
+	.name			= HFP_AG_DRIVER,
+	.probe			= hfp_card_probe,
+	.remove			= hfp_card_remove,
+	.connect		= hfp_card_connect,
+	.sco_connected_hint	= hfp_sco_connected_hint,
+};
+
 static void connection_destroy(gpointer data)
 {
 	int fd = GPOINTER_TO_INT(data);
@@ -104,12 +140,15 @@ static DBusMessage *profile_new_connection(DBusConnection *conn,
 		goto invalid;
 
 	dbus_message_iter_get_basic(&entry, &fd);
-	dbus_message_iter_next(&entry);
 
 	if (fd < 0)
 		goto invalid;
 
-	DBG("%s", device);
+	dbus_message_iter_next(&entry);
+	if (dbus_message_iter_get_arg_type(&entry) != DBUS_TYPE_ARRAY) {
+		close(fd);
+		goto invalid;
+	}
 
 	/* Pick the first voicecall capable modem */
 	if (modems == NULL) {
@@ -167,7 +206,7 @@ static DBusMessage *profile_new_connection(DBusConnection *conn,
 
 	card = ofono_handsfree_card_create(0,
 					OFONO_HANDSFREE_CARD_TYPE_GATEWAY,
-					NULL, NULL);
+					HFP_AG_DRIVER, em);
 
 	ofono_handsfree_card_set_local(card, local);
 	ofono_handsfree_card_set_remote(card, remote);
@@ -369,6 +408,7 @@ static void call_modemwatch(struct ofono_modem *modem, void *user)
 static int hfp_ag_init(void)
 {
 	DBusConnection *conn = ofono_dbus_get_connection();
+	int err;
 
 	if (DBUS_TYPE_UNIX_FD < 0)
 		return -EBADF;
@@ -383,6 +423,13 @@ static int hfp_ag_init(void)
 		return -EIO;
 	}
 
+	err = ofono_handsfree_card_driver_register(&hfp_ag_driver);
+	if (err < 0) {
+		g_dbus_unregister_interface(conn, HFP_AG_EXT_PROFILE_PATH,
+						BLUEZ_PROFILE_INTERFACE);
+		return err;
+	}
+
 	sim_hash = g_hash_table_new(g_direct_hash, g_direct_equal);
 
 	modemwatch_id = __ofono_modemwatch_add(modem_watch, NULL, NULL);
@@ -404,6 +451,8 @@ static void hfp_ag_exit(void)
 	g_dbus_unregister_interface(conn, HFP_AG_EXT_PROFILE_PATH,
 						BLUEZ_PROFILE_INTERFACE);
 
+	ofono_handsfree_card_driver_unregister(&hfp_ag_driver);
+
 	g_hash_table_destroy(connection_hash);
 
 	g_list_free(modems);
-- 
2.5.0

[PATCH 2/3 v2] emulator: add codec negotiation support

[Simon Fels]

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

---
 include/emulator.h |   5 ++
 src/emulator.c     | 244 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 249 insertions(+)

diff --git a/include/emulator.h b/include/emulator.h
index 15dc61c..ea5ef30 100644
--- a/include/emulator.h
+++ b/include/emulator.h
@@ -112,6 +112,11 @@ 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..6a89769 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_recevied;
 	bool slc : 1;
 	unsigned int events_mode : 2;
 	bool events_ind : 1;
@@ -938,6 +955,170 @@ 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:
+
+		em->bac_recevied = TRUE;
+
+		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->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 +1228,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 +1285,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 +1661,62 @@ void ofono_emulator_set_handsfree_card(struct ofono_emulator *em,
 
 	em->card = card;
 }
+
+static unsigned char select_codec(struct ofono_emulator *em)
+{
+	if (em == NULL || em->card == NULL)
+		return 0;
+
+	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 || em->card == NULL)
+		return -EINVAL;
+
+	if (em->codec_negotiation_cb != NULL)
+		return -EALREADY;
+
+	if (!em->bac_recevied) {
+		/*
+		 * 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.
+		 */
+		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 3/3 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 2/3 v2] emulator: add codec negotiation support

[Denis Kenzior]

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

Hi Simon,

On 10/16/2015 12:59 AM, Simon Fels wrote:
> ---
>   include/emulator.h |   5 ++
>   src/emulator.c     | 244 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 249 insertions(+)
>
> diff --git a/include/emulator.h b/include/emulator.h
> index 15dc61c..ea5ef30 100644
> --- a/include/emulator.h
> +++ b/include/emulator.h
> @@ -112,6 +112,11 @@ 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);
> +

Try not to go over the 80 char / line limit.

>   #ifdef __cplusplus
>   }
>   #endif
> diff --git a/src/emulator.c b/src/emulator.c
> index 626dec3..6a89769 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_recevied;

fix spelling please, bac_received.

>   	bool slc : 1;
>   	unsigned int events_mode : 2;
>   	bool events_ind : 1;
> @@ -938,6 +955,170 @@ 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:
> +

No need for an empty line here

> +		em->bac_recevied = TRUE;

You might want to set this later, once the parsing has succeeded.

> +
> +		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->negotiated_codec = 0;
> +		em->r_codecs[CVSD_OFFSET].supported = TRUE;
> +
> +		while (g_at_result_iter_next_number(&iter, &val)) {
> +

No need for an empty line here

> +			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);

Isn't this going to fail with an EALREADY if the plugin called this method?

> +
> +		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);

Won't this result in EALREADY?

> +		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);
> +			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 +1228,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 +1285,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 +1661,62 @@ void ofono_emulator_set_handsfree_card(struct ofono_emulator *em,
>
>   	em->card = card;
>   }
> +
> +static unsigned char select_codec(struct ofono_emulator *em)
> +{
> +	if (em == NULL || em->card == NULL)
> +		return 0;
> +

In general, for non-public API functions, there's no need to check for 
the passed in arguments being NULL.  That just hides bugs.  Our motto 
is: "Crash early, crash often".

The em->card check doesn't seem to be necessary.  We should be able to 
perform codec-negotiation even if we have no audio card.

> +	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 || em->card == NULL)
> +		return -EINVAL;

See above about em->card

> +
> +	if (em->codec_negotiation_cb != NULL)
> +		return -EALREADY;
> +
> +	if (!em->bac_recevied) {
> +		/*
> +		 * 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.
> +		 */
> +		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 3/3 v3] hfp_ag_bluez5: use codec negotiation

[Denis Kenzior]

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

Hi Simon,

<snip>

 >   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);

In the case of HF not supporting codec negotiation, you leak cbd here...

> +	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
> +	 * */
>   }
>

Regards,
-Denis

Re: [PATCH 1/3 v2] hfp_ag_bluez5: Add initial handsfree audio driver

[Denis Kenzior]

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

Hi Simon,

On 10/16/2015 12:59 AM, Simon Fels wrote:
> ---
>   plugins/hfp_ag_bluez5.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++---
>   1 file changed, 52 insertions(+), 3 deletions(-)
>

Applied, thanks.

Regards,
-Denis