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

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

[Simon Fels]

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

---
 plugins/hfp_ag_bluez5.c | 105 ++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 101 insertions(+), 4 deletions(-)

diff --git a/plugins/hfp_ag_bluez5.c b/plugins/hfp_ag_bluez5.c
index ef8a048..a80875f 100644
--- a/plugins/hfp_ag_bluez5.c
+++ b/plugins/hfp_ag_bluez5.c
@@ -49,11 +49,42 @@
 #define HFP_AG_EXT_PROFILE_PATH   "/bluetooth/profile/hfp_ag"
 #define BT_ADDR_SIZE 18
 
+#define HFP16_AG_DRIVER		"hfp16-ag-driver"
+
 static guint modemwatch_id;
 static GList *modems;
 static GHashTable *sim_hash = NULL;
 static GHashTable *connection_hash;
 
+static int hfp16_card_probe(struct ofono_handsfree_card *card,
+					unsigned int vendor, void *data)
+{
+	return 0;
+}
+
+static void hfp16_card_remove(struct ofono_handsfree_card *card)
+{
+}
+
+static void hfp16_card_connect(struct ofono_handsfree_card *card,
+					ofono_handsfree_card_connect_cb_t cb,
+					void *data)
+{
+	ofono_handsfree_card_connect_sco(card);
+}
+
+static void hfp16_sco_connected_hint(struct ofono_handsfree_card *card)
+{
+}
+
+static struct ofono_handsfree_card_driver hfp16_ag_driver = {
+	.name			= HFP16_AG_DRIVER,
+	.probe			= hfp16_card_probe,
+	.remove			= hfp16_card_remove,
+	.connect		= hfp16_card_connect,
+	.sco_connected_hint	= hfp16_sco_connected_hint,
+};
+
 static void connection_destroy(gpointer data)
 {
 	int fd = GPOINTER_TO_INT(data);
@@ -74,11 +105,51 @@ static gboolean io_hup_cb(GIOChannel *io, GIOCondition cond, gpointer data)
 	return FALSE;
 }
 
+static int get_version(DBusMessageIter *iter, uint16_t *version)
+{
+	DBusMessageIter dict, entry, valiter;
+	const char *key;
+	uint16_t value;
+
+	/* Points to dict */
+	dbus_message_iter_recurse(iter, &dict);
+
+	/* For each entry in this dict */
+	while (dbus_message_iter_get_arg_type(&dict) != DBUS_TYPE_INVALID) {
+		/* I want to access the entry's contents */
+		dbus_message_iter_recurse(&dict, &entry);
+
+		if (dbus_message_iter_get_arg_type(&entry) != DBUS_TYPE_STRING)
+			return -EINVAL;
+
+		/* If the current key isn't "Version", keep looking */
+		dbus_message_iter_get_basic(&entry, &key);
+		if (!g_str_equal("Version", key)) {
+			dbus_message_iter_next(&dict);
+			continue;
+		}
+
+		dbus_message_iter_next(&entry);
+		if (dbus_message_iter_get_arg_type(&entry) != DBUS_TYPE_VARIANT)
+			return -EINVAL;
+
+		dbus_message_iter_recurse(&entry, &valiter);
+		dbus_message_iter_get_basic(&valiter, &value);
+
+		if (version)
+			*version = value;
+
+		return 0;
+	}
+
+	return -ENOENT;
+}
+
 static DBusMessage *profile_new_connection(DBusConnection *conn,
 						DBusMessage *msg, void *data)
 {
 	DBusMessageIter entry;
-	const char *device;
+	const char *device, *driver;
 	GIOChannel *io;
 	int fd, fd_dup;
 	struct sockaddr_rc saddr;
@@ -87,6 +158,7 @@ static DBusMessage *profile_new_connection(DBusConnection *conn,
 	struct ofono_modem *modem;
 	char local[BT_ADDR_SIZE], remote[BT_ADDR_SIZE];
 	struct ofono_handsfree_card *card;
+	uint16_t version = HFP_VERSION_1_5;
 	int err;
 
 	DBG("Profile handler NewConnection");
@@ -104,12 +176,22 @@ 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;
+	}
+
+	if (get_version(&entry, &version) < 0) {
+		close(fd);
+		goto invalid;
+	}
+
+	DBG("version: %hd", version);
 
 	/* Pick the first voicecall capable modem */
 	if (modems == NULL) {
@@ -165,9 +247,14 @@ static DBusMessage *profile_new_connection(DBusConnection *conn,
 						g_strdup(device), g_free);
 	g_io_channel_unref(io);
 
+	driver = NULL;
+
+	if (version >= HFP_VERSION_1_6)
+		driver = HFP16_AG_DRIVER;
+
 	card = ofono_handsfree_card_create(0,
 					OFONO_HANDSFREE_CARD_TYPE_GATEWAY,
-					NULL, NULL);
+					driver, NULL);
 
 	ofono_handsfree_card_set_local(card, local);
 	ofono_handsfree_card_set_remote(card, remote);
@@ -369,6 +456,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 +471,13 @@ static int hfp_ag_init(void)
 		return -EIO;
 	}
 
+	err = ofono_handsfree_card_driver_register(&hfp16_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 +499,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(&hfp16_ag_driver);
+
 	g_hash_table_destroy(connection_hash);
 
 	g_list_free(modems);
-- 
2.5.0

[PATCH 2/2] hfp_ag_bluez5: add codec negotiation support

[Simon Fels]

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

For devices which have support for HFP version >= 1.6 we need to negotiate the
codec we use for audio data first before opening the SCO channel.
---
 include/emulator.h      |   6 ++
 plugins/hfp_ag_bluez5.c |  48 ++++++++++-
 src/emulator.c          | 208 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 260 insertions(+), 2 deletions(-)

diff --git a/include/emulator.h b/include/emulator.h
index 15dc61c..c377268 100644
--- a/include/emulator.h
+++ b/include/emulator.h
@@ -26,6 +26,7 @@
 extern "C" {
 #endif
 
+#include <stdint.h>
 #include <ofono/types.h>
 
 #define OFONO_EMULATOR_IND_BATTERY "battchg"
@@ -112,6 +113,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);
 
+void ofono_emulator_enable_codec_negotiation(struct ofono_emulator *em);
+ofono_bool_t ofono_emulator_codec_already_negotiated(struct ofono_emulator *em);
+ofono_bool_t ofono_emulator_codec_negotiation_supported(struct ofono_emulator *em);
+int ofono_emulator_start_codec_negotiation(struct ofono_emulator *em, uint8_t codec);
+
 #ifdef __cplusplus
 }
 #endif
diff --git a/plugins/hfp_ag_bluez5.c b/plugins/hfp_ag_bluez5.c
index a80875f..d0430a3 100644
--- a/plugins/hfp_ag_bluez5.c
+++ b/plugins/hfp_ag_bluez5.c
@@ -59,22 +59,60 @@ static GHashTable *connection_hash;
 static int hfp16_card_probe(struct ofono_handsfree_card *card,
 					unsigned int vendor, void *data)
 {
+	DBG("");
+
 	return 0;
 }
 
 static void hfp16_card_remove(struct ofono_handsfree_card *card)
 {
+	DBG("");
 }
 
 static void hfp16_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);
+
+	DBG("");
+
+	if (ofono_emulator_codec_negotiation_supported(em) &&
+		!ofono_emulator_codec_already_negotiated(em)) {
+
+		struct ofono_error error;
+		error.type = OFONO_ERROR_TYPE_NO_ERROR;
+		error.error = 0;
+
+		err = ofono_emulator_start_codec_negotiation(em, 0);
+		if (err < 0) {
+			error.type = OFONO_ERROR_TYPE_FAILURE;
+			error.error = err;
+			cb(&error, data);
+			return;
+		}
+
+		/* We hand over to the emulator core here to establish the
+		 * SCO connection once the codec is negotiated */
+		cb(&error, data);
+
+		return;
+	}
+
+	/*
+	 * If any side (remote or local) doesn't support codec negotiation or
+	 * if the codec was already negotiated before we can directly proceed
+	 * with establishing the SCO connection. Calling connect_sco()
+	 * hands the connection responsibility to the core, so no need
+	 * to call the callback
+	 */
 	ofono_handsfree_card_connect_sco(card);
 }
 
 static void hfp16_sco_connected_hint(struct ofono_handsfree_card *card)
 {
+	DBG("");
 }
 
 static struct ofono_handsfree_card_driver hfp16_ag_driver = {
@@ -249,12 +287,18 @@ static DBusMessage *profile_new_connection(DBusConnection *conn,
 
 	driver = NULL;
 
-	if (version >= HFP_VERSION_1_6)
+	if (version >= HFP_VERSION_1_6) {
 		driver = HFP16_AG_DRIVER;
+		/* Codec negotiation between HF and AG is only supported
+		 * starting with HFP 1.6  */
+		ofono_emulator_enable_codec_negotiation(em);
+	}
 
 	card = ofono_handsfree_card_create(0,
 					OFONO_HANDSFREE_CARD_TYPE_GATEWAY,
-					driver, NULL);
+					driver, em);
+
+	ofono_handsfree_card_set_data(card, em);
 
 	ofono_handsfree_card_set_local(card, local);
 	ofono_handsfree_card_set_remote(card, remote);
diff --git a/src/emulator.c b/src/emulator.c
index 626dec3..58095e9 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>
 
@@ -50,6 +51,10 @@ struct ofono_emulator {
 	guint callsetup_source;
 	int pns_id;
 	struct ofono_handsfree_card *card;
+	int r_codecs;
+	uint8_t negotiated_codec;
+	uint8_t proposed_codec;
+	guint delay_sco;
 	bool slc : 1;
 	unsigned int events_mode : 2;
 	bool events_ind : 1;
@@ -938,6 +943,140 @@ 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:
+		if (!ofono_emulator_codec_negotiation_supported(em)) {
+			DBG("Codec negotiation is not supported from our side");
+			goto fail;
+		}
+
+		em->r_codecs = 0;
+		em->negotiated_codec = 0;
+
+		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->r_codecs |= HFP_CODEC_CVSD;
+
+		while (g_at_result_iter_next_number(&iter, &val))
+			em->r_codecs |= val;
+
+		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);
+
+		break;
+
+	default:
+fail:
+		DBG("Process AT+BAC failed");
+		g_at_server_send_final(server, G_AT_SERVER_RESULT_ERROR);
+		break;
+	}
+}
+
+static gboolean connect_sco_delayed(gpointer user_data)
+{
+	struct ofono_emulator *em = user_data;
+	int err;
+
+	DBG("");
+
+	em->delay_sco = 0;
+
+	err = ofono_handsfree_card_connect_sco(em->card);
+	if (err == 0)
+		return FALSE;
+
+	/* 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);
+
+	return FALSE;
+}
+
+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;
+
+		g_at_server_send_final(server, G_AT_SERVER_RESULT_OK);
+
+		if (!em->delay_sco)
+			em->delay_sco = g_idle_add(connect_sco_delayed, em);
+
+		return;
+	default:
+		break;
+	}
+
+	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:
+		if (!ofono_emulator_codec_negotiation_supported(em))
+			break;
+
+		g_at_server_send_final(server, G_AT_SERVER_RESULT_OK);
+
+		if (!em->negotiated_codec) {
+			ofono_emulator_start_codec_negotiation(em, 0);
+			return;
+		}
+
+		if (!em->delay_sco)
+			em->delay_sco = g_idle_add(connect_sco_delayed, 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 +1186,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);
@@ -1476,3 +1618,69 @@ void ofono_emulator_set_handsfree_card(struct ofono_emulator *em,
 
 	em->card = card;
 }
+
+void ofono_emulator_enable_codec_negotiation(struct ofono_emulator *em)
+{
+	em->l_features |= HFP_AG_FEATURE_CODEC_NEGOTIATION;
+}
+
+ofono_bool_t ofono_emulator_codec_negotiation_supported(struct ofono_emulator *em)
+{
+	if (em->card == NULL)
+		return FALSE;
+
+	return (em->l_features & HFP_AG_FEATURE_CODEC_NEGOTIATION) &&
+		(em->r_features & HFP_HF_FEATURE_CODEC_NEGOTIATION);
+}
+
+static uint8_t select_codec(struct ofono_emulator *em)
+{
+	if (em == NULL || em->card == NULL)
+		return 0;
+
+	if (ofono_handsfree_audio_has_wideband() &&
+		em->r_codecs & HFP_CODEC_MSBC)
+		return HFP_CODEC_MSBC;
+
+	if (!(em->r_codecs & HFP_CODEC_CVSD))
+		return 0;
+
+	/* CVSD is mandatory for both sides */
+	return HFP_CODEC_CVSD;
+}
+
+ofono_bool_t ofono_emulator_codec_already_negotiated(struct ofono_emulator *em)
+{
+	if (em == NULL)
+		return FALSE;
+
+	return em->negotiated_codec;
+}
+
+int ofono_emulator_start_codec_negotiation(struct ofono_emulator *em, uint8_t codec)
+{
+	char buf[64];
+	uint8_t selected_codec;
+
+	if (em == NULL || em->card == NULL)
+		return -EINVAL;
+
+	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;
+
+	snprintf(buf, 64, "+BCS: %d", selected_codec);
+	g_at_server_send_unsolicited(em->server, buf);
+
+	return 0;
+}
-- 
2.5.0

Re: [PATCH 2/2] hfp_ag_bluez5: add codec negotiation support

[Denis Kenzior]

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

Hi Simon,

On 10/09/2015 05:05 AM, Simon Fels wrote:
> For devices which have support for HFP version >= 1.6 we need to negotiate the
> codec we use for audio data first before opening the SCO channel.
> ---
>   include/emulator.h      |   6 ++
>   plugins/hfp_ag_bluez5.c |  48 ++++++++++-
>   src/emulator.c          | 208 ++++++++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 260 insertions(+), 2 deletions(-)

In general we prefer to separate patches per directory.  See HACKING, 
Submitting Patches section.

>
> diff --git a/include/emulator.h b/include/emulator.h
> index 15dc61c..c377268 100644
> --- a/include/emulator.h
> +++ b/include/emulator.h
> @@ -26,6 +26,7 @@
>   extern "C" {
>   #endif
>
> +#include <stdint.h>

For some long lost historic reasons, we've resisted adding stdint to our 
headers.

>   #include <ofono/types.h>
>
>   #define OFONO_EMULATOR_IND_BATTERY "battchg"
> @@ -112,6 +113,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);
>
> +void ofono_emulator_enable_codec_negotiation(struct ofono_emulator *em);

Do we really need this?  It might be simpler to just always support 
codec negotiation if the AG emulator is created with version >= 1.6

> +ofono_bool_t ofono_emulator_codec_already_negotiated(struct ofono_emulator *em);
> +ofono_bool_t ofono_emulator_codec_negotiation_supported(struct ofono_emulator *em);
> +int ofono_emulator_start_codec_negotiation(struct ofono_emulator *em, uint8_t codec);

For reasons stated above, lets change uint8_t to unsigned char.

> +
>   #ifdef __cplusplus
>   }
>   #endif
> diff --git a/plugins/hfp_ag_bluez5.c b/plugins/hfp_ag_bluez5.c
> index a80875f..d0430a3 100644
> --- a/plugins/hfp_ag_bluez5.c
> +++ b/plugins/hfp_ag_bluez5.c
> @@ -59,22 +59,60 @@ static GHashTable *connection_hash;
>   static int hfp16_card_probe(struct ofono_handsfree_card *card,
>   					unsigned int vendor, void *data)
>   {
> +	DBG("");
> +

This probably belongs in the previous patch

>   	return 0;
>   }
>
>   static void hfp16_card_remove(struct ofono_handsfree_card *card)
>   {
> +	DBG("");

As above

>   }
>
>   static void hfp16_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);
> +
> +	DBG("");
> +
> +	if (ofono_emulator_codec_negotiation_supported(em) &&
> +		!ofono_emulator_codec_already_negotiated(em)) {
> +

This isn't our style.  Please refer to doc/coding-style.txt item M4

> +		struct ofono_error error;
> +		error.type = OFONO_ERROR_TYPE_NO_ERROR;
> +		error.error = 0;
> +

If you include drivers/atmodem/atutil.h you can skip this and...

> +		err = ofono_emulator_start_codec_negotiation(em, 0);
> +		if (err < 0) {
> +			error.type = OFONO_ERROR_TYPE_FAILURE;
> +			error.error = err;
> +			cb(&error, data);

Use the nifty CALLBACK_WITH_FAILURE here and ...

> +			return;
> +		}
> +
> +		/* We hand over to the emulator core here to establish the
> +		 * SCO connection once the codec is negotiated */

For our comment style, see doc/coding-style.txt item M2

> +		cb(&error, data);
> +

CALLBACK_WITH_SUCCESS here

> +		return;
> +	}
> +
> +	/*
> +	 * If any side (remote or local) doesn't support codec negotiation or
> +	 * if the codec was already negotiated before we can directly proceed
> +	 * with establishing the SCO connection. Calling connect_sco()
> +	 * hands the connection responsibility to the core, so no need
> +	 * to call the callback
> +	 */
>   	ofono_handsfree_card_connect_sco(card);
>   }
>
>   static void hfp16_sco_connected_hint(struct ofono_handsfree_card *card)
>   {
> +	DBG("");

Put this along with Patch 1.

>   }
>
>   static struct ofono_handsfree_card_driver hfp16_ag_driver = {
> @@ -249,12 +287,18 @@ static DBusMessage *profile_new_connection(DBusConnection *conn,
>
>   	driver = NULL;
>
> -	if (version >= HFP_VERSION_1_6)
> +	if (version >= HFP_VERSION_1_6) {
>   		driver = HFP16_AG_DRIVER;
> +		/* Codec negotiation between HF and AG is only supported
> +		 * starting with HFP 1.6  */
> +		ofono_emulator_enable_codec_negotiation(em);

As mentioned before, we probably should just always support this by 
default.  Especially since you need to also turn on the SDP WideBand 
feature bit as well, which is more or less static.

> +	}
>
>   	card = ofono_handsfree_card_create(0,
>   					OFONO_HANDSFREE_CARD_TYPE_GATEWAY,
> -					driver, NULL);
> +					driver, em);
> +
> +	ofono_handsfree_card_set_data(card, em);
>
>   	ofono_handsfree_card_set_local(card, local);
>   	ofono_handsfree_card_set_remote(card, remote);
> diff --git a/src/emulator.c b/src/emulator.c
> index 626dec3..58095e9 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>
>
> @@ -50,6 +51,10 @@ struct ofono_emulator {
>   	guint callsetup_source;
>   	int pns_id;
>   	struct ofono_handsfree_card *card;
> +	int r_codecs;
> +	uint8_t negotiated_codec;
> +	uint8_t proposed_codec;
> +	guint delay_sco;
>   	bool slc : 1;
>   	unsigned int events_mode : 2;
>   	bool events_ind : 1;
> @@ -938,6 +943,140 @@ 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:
> +		if (!ofono_emulator_codec_negotiation_supported(em)) {
> +			DBG("Codec negotiation is not supported from our side");

The function call depends on both the AG & HF feature bits.  Also, the 
spec notes that:

"The AG shall initiate a Codec Connection only if the HF has indicated 
support for the Codec Negotiation feature and has sent at least one 
AT+BAC on the current service level connection. When selecting which 
codec to use for a codec connection, the AG shall use the information on 
codecs available in the HF as provided in the most recently received 
AT+BAC."

So the ofono_emulator_codec_negotiation_supported() should take that 
fact into account.

> +			goto fail;
> +		}
> +
> +		em->r_codecs = 0;
> +		em->negotiated_codec = 0;

Might be safer to set these only after successfully parsing the command

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

See doc/coding-style.txt item M2

> +		if (g_at_result_iter_next_number(&iter, &val) == FALSE ||
> +			val != HFP_CODEC_CVSD)

item M4

> +			goto fail;
> +
> +		em->r_codecs |= HFP_CODEC_CVSD;
> +
> +		while (g_at_result_iter_next_number(&iter, &val))
> +			em->r_codecs |= val;

Codecs are just 8-bit ids.  So this isn't really safe.

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

Item M2

> +		if (em->proposed_codec)
> +			ofono_emulator_start_codec_negotiation(em, 0);
> +
> +		break;
> +
> +	default:
> +fail:
> +		DBG("Process AT+BAC failed");
> +		g_at_server_send_final(server, G_AT_SERVER_RESULT_ERROR);
> +		break;
> +	}
> +}
> +
> +static gboolean connect_sco_delayed(gpointer user_data)
> +{
> +	struct ofono_emulator *em = user_data;
> +	int err;
> +
> +	DBG("");
> +
> +	em->delay_sco = 0;
> +
> +	err = ofono_handsfree_card_connect_sco(em->card);
> +	if (err == 0)
> +		return FALSE;
> +
> +	/* 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);
> +
> +	return FALSE;

Is calling this from inside g_idle_add really necessary?  card_connect 
is using Non-Blocking connect anyway.

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

Any pending D-Bus requests might be never replied to here...  According 
to the spec this situation is impossible, but still...

> +			break;
> +		}
> +
> +		em->proposed_codec = 0;
> +		em->negotiated_codec = val;

Do you want to call ofono_handsfree_card_set_codec() here?

> +
> +		g_at_server_send_final(server, G_AT_SERVER_RESULT_OK);
> +
> +		if (!em->delay_sco)
> +			em->delay_sco = g_idle_add(connect_sco_delayed, em);
> +
> +		return;
> +	default:
> +		break;
> +	}
> +
> +	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:
> +		if (!ofono_emulator_codec_negotiation_supported(em))
> +			break;
> +
> +		g_at_server_send_final(server, G_AT_SERVER_RESULT_OK);
> +
> +		if (!em->negotiated_codec) {
> +			ofono_emulator_start_codec_negotiation(em, 0);
> +			return;
> +		}
> +
> +		if (!em->delay_sco)
> +			em->delay_sco = g_idle_add(connect_sco_delayed, 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 +1186,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);
> @@ -1476,3 +1618,69 @@ void ofono_emulator_set_handsfree_card(struct ofono_emulator *em,
>
>   	em->card = card;
>   }
> +
> +void ofono_emulator_enable_codec_negotiation(struct ofono_emulator *em)
> +{
> +	em->l_features |= HFP_AG_FEATURE_CODEC_NEGOTIATION;
> +}
> +
> +ofono_bool_t ofono_emulator_codec_negotiation_supported(struct ofono_emulator *em)
> +{
> +	if (em->card == NULL)
> +		return FALSE;

What does the card have to do with this? Proceed with codec negotiation 
regardless and just don't connect SCO if there's no audio card.

> +
> +	return (em->l_features & HFP_AG_FEATURE_CODEC_NEGOTIATION) &&
> +		(em->r_features & HFP_HF_FEATURE_CODEC_NEGOTIATION);
> +}
> +
> +static uint8_t select_codec(struct ofono_emulator *em)
> +{
> +	if (em == NULL || em->card == NULL)
> +		return 0;
> +
> +	if (ofono_handsfree_audio_has_wideband() &&
> +		em->r_codecs & HFP_CODEC_MSBC)

Item M4

> +		return HFP_CODEC_MSBC;
> +
> +	if (!(em->r_codecs & HFP_CODEC_CVSD))
> +		return 0;
> +

Why is this check necessary?

> +	/* CVSD is mandatory for both sides */
> +	return HFP_CODEC_CVSD;
> +}
> +
> +ofono_bool_t ofono_emulator_codec_already_negotiated(struct ofono_emulator *em)
> +{
> +	if (em == NULL)
> +		return FALSE;
> +
> +	return em->negotiated_codec;
> +}
> +
> +int ofono_emulator_start_codec_negotiation(struct ofono_emulator *em, uint8_t codec)
> +{
> +	char buf[64];
> +	uint8_t selected_codec;
> +
> +	if (em == NULL || em->card == NULL)
> +		return -EINVAL;
> +
> +	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;
> +
> +	snprintf(buf, 64, "+BCS: %d", selected_codec);
> +	g_at_server_send_unsolicited(em->server, buf);
> +
> +	return 0;
> +}
>

Regards,
-Denis

Re: [PATCH 2/2] hfp_ag_bluez5: add codec negotiation support

[Simon Fels]

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

Hey Denis,

thanks for your review!

> In general we prefer to separate patches per directory.  See HACKING,
> Submitting Patches section.

Will fix that.

> Do we really need this?  It might be simpler to just always support
> codec negotiation if the AG emulator is created with version >= 1.6

We can do that but then have to pass the HFP version we're going to run 
with ofono_emulator_create.

> If you include drivers/atmodem/atutil.h you can skip this and...

I thought about this before but that also requires me to include 
gatchat/gatresult.h and gatchat/gatchat.h ... Best would be we move 
those defines to a common place like src/common.h or so.

> As mentioned before, we probably should just always support this by
> default.  Especially since you need to also turn on the SDP WideBand
> feature bit as well, which is more or less static.

I agree with that but we should still bind this to the profile version.

> The function call depends on both the AG & HF feature bits.  Also, the
> spec notes that:
>
> "The AG shall initiate a Codec Connection only if the HF has indicated
> support for the Codec Negotiation feature and has sent at least one
> AT+BAC on the current service level connection. When selecting which
> codec to use for a codec connection, the AG shall use the information on
> codecs available in the HF as provided in the most recently received
> AT+BAC."
>
> So the ofono_emulator_codec_negotiation_supported() should take that
> fact into account.

Will fix that.

> Is calling this from inside g_idle_add really necessary?  card_connect
> is using Non-Blocking connect anyway.

The idea was to ensure that the OK always goes out first before we open 
the SCO connection. Will add a comment to make this a bit more visible.


> Any pending D-Bus requests might be never replied to here...  According
> to the spec this situation is impossible, but still...

We already replied at this point! There are two cases where we end up in 
bcs_cb.

1. We trigger codec negotiation from hfp16_card_connect with a call to 
ofono_emulator_start_codec_negotation. If that call succeeds we reply to 
org.ofono.HandsfreeAudioCard.Connect and also when it fails we do. We 
don't bind the success of the connect call here to the codec negotiation.

2. When the remote side sends us the AT+BCC command we start the codec 
negotiation and the same as mentioned in 1. applies.

However as I am thinking right now about this is not correct. The 
documentation of the org.ofono.HandsfreeAudioCard.Connect method says:

  Attempts to establish the SCO audio connection.
  The Agent NewConnection() method will be called
  whenever the SCO audio link has been established.  If
  the audio connection could not be established, this
  method will return an error.

So we have to wait for codec negotiation to finish before we can reply.

Will fix that too.

> Do you want to call ofono_handsfree_card_set_codec() here?

Done.

> What does the card have to do with this? Proceed with codec negotiation
> regardless and just don't connect SCO if there's no audio card.

Done.

> Why is this check necessary?

Can be dropped as CVSD is mandatory even if codec negotiation has failed.

regards,
Simon

Re: [PATCH 2/2] hfp_ag_bluez5: add codec negotiation support

[Denis Kenzior]

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

Hi Simon,

On 10/13/2015 05:05 AM, Simon Fels wrote:
>> Do we really need this?  It might be simpler to just always support
>> codec negotiation if the AG emulator is created with version >= 1.6
>
> We can do that but then have to pass the HFP version we're going to run
> with ofono_emulator_create.
>

We don't really need to do that.  Just always enable codec negotiation 
support.  We register the SDP Record with version 1.7 anyway.  That is 
static and not a per-connection setting.

If the HF doesn't send as a AT+BAC during the SLC setup, then we don't 
do codec negotiation.

>> If you include drivers/atmodem/atutil.h you can skip this and...
>
> I thought about this before but that also requires me to include
> gatchat/gatresult.h and gatchat/gatchat.h ... Best would be we move
> those defines to a common place like src/common.h or so.
>

Ah, forgot the AG plugin doesn't use GAtChat.  Nevermind then.

>> Is calling this from inside g_idle_add really necessary?  card_connect
>> is using Non-Blocking connect anyway.
>
> The idea was to ensure that the OK always goes out first before we open
> the SCO connection. Will add a comment to make this a bit more visible.
>

GAtServer uses non-blocking writes, so you're not really guaranteeing it 
this way.  And the spec mentions no specific order either.

>
>> Any pending D-Bus requests might be never replied to here...  According
>> to the spec this situation is impossible, but still...
>
> We already replied at this point! There are two cases where we end up in
> bcs_cb.
>
> 1. We trigger codec negotiation from hfp16_card_connect with a call to
> ofono_emulator_start_codec_negotation. If that call succeeds we reply to
> org.ofono.HandsfreeAudioCard.Connect and also when it fails we do. We
> don't bind the success of the connect call here to the codec negotiation.
>

Yes, you're right.  I just re-read the implementation again.  However, 
I'm thinking that the D-Bus method should be replied to only after the 
SCO link has been opened / failed.

> 2. When the remote side sends us the AT+BCC command we start the codec
> negotiation and the same as mentioned in 1. applies.

In this case there's no D-Bus method.  The remote HF is asking for the 
codec connection.  In case we hit this condition the process simply stalls.

>
> However as I am thinking right now about this is not correct. The
> documentation of the org.ofono.HandsfreeAudioCard.Connect method says:
>
>   Attempts to establish the SCO audio connection.
>   The Agent NewConnection() method will be called
>   whenever the SCO audio link has been established.  If
>   the audio connection could not be established, this
>   method will return an error.
>
> So we have to wait for codec negotiation to finish before we can reply.
>

Agreed.

> Will fix that too.
>

Regards,
-Denis