Re: [PATCH] android/sco: Add sco helper lib

[Szymon Janc <szymon.janc@tieto.com>]

Hi Łukasz,

On Monday 03 of November 2014 16:05:37 Lukasz Rymanowski wrote:
> This patch adds sco lib which gives means to create SCO object with
> API to create listening socket, connect and disconnect.
> 
> This is going to be common code for HFP HF and HFP GW.
> For now we support only one SCO at the time.
> 
> Before creating listening SCO socket, remember to register confirm
> and connect callbacks. Those are called on incoming connection.
> 
> Sco lib is not aware about voice settings. Those must be provided
> on bt_sco_connect or in confirm callback.
> 
> This patch also adds this lib to Android build.
> ---
>  android/Android.mk  |   1 +
>  android/Makefile.am |   1 +
>  android/sco.c       | 307 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  android/sco.h       |  52 +++++++++
>  4 files changed, 361 insertions(+)
>  create mode 100644 android/sco.c
>  create mode 100644 android/sco.h
> 
> diff --git a/android/Android.mk b/android/Android.mk
> index aefe41c..dbbcdb2 100644
> --- a/android/Android.mk
> +++ b/android/Android.mk
> @@ -56,6 +56,7 @@ LOCAL_SRC_FILES := \
>  	bluez/android/handsfree-client.c \
>  	bluez/android/gatt.c \
>  	bluez/android/health.c \
> +	bluez/android/sco.c \
>  	bluez/profiles/health/mcap.c \
>  	bluez/android/map-client.c \
>  	bluez/src/log.c \
> diff --git a/android/Makefile.am b/android/Makefile.am
> index 496fbc4..4edb007 100644
> --- a/android/Makefile.am
> +++ b/android/Makefile.am
> @@ -40,6 +40,7 @@ android_bluetoothd_SOURCES = android/main.c \
>  				android/avrcp.h android/avrcp.c \
>  				android/avrcp-lib.h android/avrcp-lib.c \
>  				android/socket.h android/socket.c \
> +				android/sco.h android/sco.c \
>  				android/pan.h android/pan.c \
>  				android/handsfree.h android/handsfree.c \
>  				android/handsfree-client.c android/handsfree-client.h \
> diff --git a/android/sco.c b/android/sco.c
> new file mode 100644
> index 0000000..ebb81ad
> --- /dev/null
> +++ b/android/sco.c
> @@ -0,0 +1,307 @@
> +/*
> + *
> + *  BlueZ - Bluetooth protocol stack for Linux
> + *
> + *  Copyright (C) 2014 Intel Corporation. All rights reserved.
> + *
> + *  This program is free software; you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License as published by
> + *  the Free Software Foundation; either version 2 of the License, or
> + *  (at your option) any later version.
> + *
> + *  This program is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *  GNU General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License
> + *  along with this program; if not, write to the Free Software
> + *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> + *
> + */
> +
> +#ifdef HAVE_CONFIG_H
> +#include <config.h>
> +#endif
> +
> +#include <stdlib.h>
> +#include <stdbool.h>
> +#include <errno.h>
> +
> +#include <glib.h>
> +
> +#include "lib/bluetooth.h"
> +#include "btio/btio.h"
> +#include "src/log.h"
> +
> +#include "sco.h"
> +
> +struct bt_sco {
> +	int ref_count;
> +
> +	GIOChannel *server_io;
> +
> +	GIOChannel *io;
> +	guint watch;
> +
> +	bdaddr_t local_addr;
> +	bdaddr_t remote_addr;
> +
> +	bt_sco_conn_confirm_cb_func_t confirm_cb;
> +	bt_sco_conn_cb_func_t connect_cb;
> +	bt_sco_disconn_cb_func_t disconnect_cb;
> +};
> +
> +/* We support only one sco for the moment */
> +static bool sco_in_use = false;
> +
> +struct bt_sco *bt_sco_new(const bdaddr_t *addr)
> +{
> +	struct bt_sco *sco = NULL;

This initialization is not needed.

> +
> +	if (sco_in_use)
> +		return NULL;

Put comment about single user here.

> +
> +	sco = g_try_new0(struct bt_sco, 1);
> +	if (!sco)
> +		return NULL;

Lets not use glib if it is not necessary, so new0() here.

> +
> +	bacpy(&sco->local_addr, addr);
> +
> +	sco_in_use  = true;
> +
> +	return bt_sco_ref(sco);
> +}
> +
> +struct bt_sco *bt_sco_ref(struct bt_sco *sco)
> +{
> +	if (!sco)
> +		return NULL;
> +
> +	__sync_fetch_and_add(&sco->ref_count, 1);
> +
> +	return sco;
> +}
> +
> +static void sco_free(struct bt_sco *sco)
> +{
> +	if (sco->server_io) {
> +		g_io_channel_shutdown(sco->server_io, TRUE, NULL);
> +		g_io_channel_unref(sco->server_io);
> +	}
> +
> +	if (sco->io) {
> +		g_io_channel_shutdown(sco->io, TRUE, NULL);
> +		g_io_channel_unref(sco->io);
> +	}
> +
> +	g_free(sco);
> +	sco_in_use  = false;
> +}
> +
> +void bt_sco_unref(struct bt_sco *sco)
> +{
> +	DBG("");
> +
> +	if (!sco)
> +		return;
> +
> +	if (__sync_sub_and_fetch(&sco->ref_count, 1))
> +		return;
> +
> +	sco_free(sco);
> +}
> +
> +static void clear_remote_address(struct bt_sco *sco)
> +{
> +	memset(&sco->remote_addr, 0, sizeof(bdaddr_t));
> +}
> +
> +static gboolean disconnect_watch(GIOChannel *chan, GIOCondition cond,
> +							gpointer user_data)
> +{
> +	struct bt_sco *sco = user_data;
> +
> +	g_io_channel_shutdown(sco->io, TRUE, NULL);
> +	g_io_channel_unref(sco->io);
> +	sco->io = NULL;
> +
> +	DBG("");
> +
> +	sco->watch = 0;
> +
> +	sco->disconnect_cb(&sco->remote_addr);
> +
> +	clear_remote_address(sco);
> +
> +	return FALSE;
> +}
> +
> +static void connect_sco_cb(GIOChannel *chan, GError *err, gpointer user_data)
> +{
> +	struct bt_sco *sco = user_data;
> +
> +	DBG("");
> +
> +	if (err) {
> +		error("sco: audio connect failed (%s)", err->message);
> +
> +		sco->connect_cb(SCO_STATUS_ERROR, &sco->remote_addr);

We should be checking if callbacks are set. If callback is missing then we
should probably just fail respective operation.

> +
> +		clear_remote_address(sco);
> +
> +		return;
> +	}
> +
> +	g_io_channel_set_close_on_unref(chan, TRUE);
> +
> +	sco->io = g_io_channel_ref(chan);
> +	sco->watch = g_io_add_watch(chan, G_IO_ERR | G_IO_HUP | G_IO_NVAL,
> +							disconnect_watch, sco);
> +
> +	sco->connect_cb(SCO_STATUS_OK, &sco->remote_addr);
> +}
> +
> +static void confirm_sco_cb(GIOChannel *chan, gpointer user_data)
> +{
> +	char address[18];
> +	bdaddr_t bdaddr;
> +	GError *err = NULL;
> +	struct bt_sco *sco = user_data;
> +	uint32_t voice_settings;

voice_settings is uint16 in btio.

> +
> +	DBG("");
> +
> +	bt_io_get(chan, &err,
> +			BT_IO_OPT_DEST, address,
> +			BT_IO_OPT_DEST_BDADDR, &bdaddr,
> +			BT_IO_OPT_INVALID);
> +	if (err) {
> +		error("sco: audio confirm failed (%s)", err->message);
> +		g_error_free(err);
> +		goto drop;
> +	}
> +
> +	if (!sco->confirm_cb(&bdaddr, &voice_settings)) {
> +		error("sco: audio connection from %s rejected", address);
> +		goto drop;
> +	}
> +
> +	bacpy(&sco->remote_addr, &bdaddr);
> +
> +	DBG("incoming SCO connection from %s, voice settings %d", address,

voice_settings is unsigned. (maybe print it as 0x%x ?)

> +								voice_settings);
> +
> +	err = NULL;
> +	bt_io_set(chan, &err, BT_IO_OPT_VOICE, voice_settings,
> +							BT_IO_OPT_INVALID);
> +	if (err) {
> +		error("sco: Could not set voice settings (%s)", err->message);
> +		g_error_free(err);
> +		goto drop;
> +	}
> +
> +	if (!bt_io_accept(chan, connect_sco_cb, sco, NULL, NULL)) {
> +		error("sco: failed to accept audio connection");
> +		goto drop;
> +	}
> +
> +	return;
> +
> +drop:
> +	g_io_channel_shutdown(chan, TRUE, NULL);
> +}
> +
> +bool bt_sco_listen(struct bt_sco *sco)
> +{
> +	GError *err = NULL;
> +
> +	sco->server_io = bt_io_listen(NULL, confirm_sco_cb, sco, NULL, &err,
> +							BT_IO_OPT_SOURCE_BDADDR,
> +							&sco->local_addr,
> +							BT_IO_OPT_INVALID);
> +	if (!sco->server_io) {
> +		error("sco: Failed to listen on SCO: %s", err->message);
> +		g_error_free(err);
> +		return false;
> +	}
> +
> +	return true;
> +}

There is no way to stop listening other than unreferencing sco so maybe
this should be done inside new()?

> +
> +bool bt_sco_connect(struct bt_sco *sco, const bdaddr_t *addr,
> +							uint16_t voice_settings)
> +{
> +	GIOChannel *io;
> +	GError *gerr = NULL;
> +
> +	DBG("");

So we allow multiple SCO connections?

> +
> +	io = bt_io_connect(connect_sco_cb, sco, NULL, &gerr,
> +				BT_IO_OPT_SOURCE_BDADDR, &sco->local_addr,
> +				BT_IO_OPT_DEST_BDADDR, addr,
> +				BT_IO_OPT_VOICE, voice_settings,
> +				BT_IO_OPT_INVALID);
> +
> +	if (!io) {
> +		error("sco: unable to connect audio: %s", gerr->message);
> +		g_error_free(gerr);
> +		return false;
> +	}
> +
> +	g_io_channel_unref(io);
> +
> +	bacpy(&sco->remote_addr, addr);
> +
> +	return true;

I think we should be able to cancel outgoing connection.

> +}
> +
> +void bt_sco_disconnect(struct bt_sco *sco)
> +{
> +	if (sco->watch) {
> +		g_source_remove(sco->watch);
> +		sco->watch = 0;
> +	}
> +
> +	g_io_channel_shutdown(sco->io, TRUE, NULL);
> +	g_io_channel_unref(sco->io);
> +	sco->io = NULL;

This will crash if sco is not connected. (and there is no api to check if it is).

> +
> +	clear_remote_address(sco);
> +}
> +
> +bool bt_sco_get_fd_and_mtu(struct bt_sco *sco, int *fd, uint16_t *mtu)
> +{
> +	GError *err;
> +
> +	if (!sco->io || !fd || !mtu)
> +		return false;
> +
> +	err = NULL;
> +	if (!bt_io_get(sco->io, &err, BT_IO_OPT_MTU, mtu, BT_IO_OPT_INVALID)) {
> +			error("Unable to get MTU: %s\n", err->message);
> +			g_clear_error(&err);
> +			return false;
> +		}
> +
> +	*fd = g_io_channel_unix_get_fd(sco->io);
> +
> +	return true;
> +}
> +
> +void bt_sco_set_confirm_cb(struct bt_sco *sco,
> +					bt_sco_conn_confirm_cb_func_t func)
> +{
> +	sco->confirm_cb = func;
> +}
> +
> +void bt_sco_set_connect_cb(struct bt_sco *sco, bt_sco_conn_cb_func_t func)
> +{
> +	sco->connect_cb = func;
> +}
> +
> +void bt_sco_set_disconnect_cb(struct bt_sco *sco,
> +						bt_sco_disconn_cb_func_t func)
> +{
> +	sco->disconnect_cb = func;
> +}
> diff --git a/android/sco.h b/android/sco.h
> new file mode 100644
> index 0000000..491e633
> --- /dev/null
> +++ b/android/sco.h
> @@ -0,0 +1,52 @@
> +/*
> + *
> + *  BlueZ - Bluetooth protocol stack for Linux
> + *
> + *  Copyright (C) 2014  Intel Corporation. All rights reserved.
> + *
> + *
> + *  This library is free software; you can redistribute it and/or
> + *  modify it under the terms of the GNU Lesser General Public
> + *  License as published by the Free Software Foundation; either
> + *  version 2.1 of the License, or (at your option) any later version.
> + *
> + *  This library is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + *  Lesser General Public License for more details.
> + *
> + *  You should have received a copy of the GNU Lesser General Public
> + *  License along with this library; if not, write to the Free Software
> + *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> + *
> + */
> +
> +enum sco_status {
> +	SCO_STATUS_OK,
> +	SCO_STATUS_ERROR,
> +};
> +
> +struct bt_sco;
> +
> +struct bt_sco *bt_sco_new(const bdaddr_t *adapter_addr);

Name this local_addr, also declarations and definitions should be consistent
ie. in sco.c you name this just addr

> +
> +struct bt_sco *bt_sco_ref(struct bt_sco *sco);
> +void bt_sco_unref(struct bt_sco *sco);
> +
> +bool bt_sco_listen(struct bt_sco *sco);
> +bool bt_sco_connect(struct bt_sco *sco, const bdaddr_t *remote_addr,
> +						uint16_t voice_settings);
> +void bt_sco_disconnect(struct bt_sco *sco);
> +bool bt_sco_get_fd_and_mtu(struct bt_sco *sco, int *fd, uint16_t *mtu);
> +

Callbacks should have user_data (and possibly destroy handler for it), this is
to avoid relaying on static data (or loopups) in callers.

> +typedef bool (*bt_sco_conn_confirm_cb_func_t) (const bdaddr_t *remote_addr,
> +						uint32_t *voice_settings);

name this bt_sco_confirm_func_t, voice settings should be uint16.

> +typedef void (*bt_sco_conn_cb_func_t) (enum sco_status status,
> +							const bdaddr_t *addr);

bt_sco_conn_func_t,  also lets keep addr first parameter.

> +typedef void (*bt_sco_disconn_cb_func_t) (const bdaddr_t *addr);
> +
> +void bt_sco_set_confirm_cb(struct bt_sco *sco,
> +					bt_sco_conn_confirm_cb_func_t func);
> +void bt_sco_set_connect_cb(struct bt_sco *sco, bt_sco_conn_cb_func_t func);
> +void bt_sco_set_disconnect_cb(struct bt_sco *sco,
> +						bt_sco_disconn_cb_func_t func);
> 

Also in general I think this lib should be a bit more robust in case of invalid
calls, like checking for valid sco, cb presence etc. (similar to what we have
shared/).

-- 
Best regards, 
Szymon Janc