[PATCH 0/4] Fix collision in AVCTP connection

[PATCH 0/4] Fix collision in AVCTP connection

[Marcin Kraglak]

This patch set corresponds to previous RFC with the same subject.
Now reconnection logic is moved to policy. Avrcp and avctp
modules are responsible for passing correct error to higher
layer, policy detects collision and set random timer for reconnection.
Comments are welcome

Marcin Kraglak (4):
  audio/avctp: Pass error argument to avctp_state_changed callback
  audio/avrcp: Pass error to session_destroy()
  audio/avctp: Cancel outgoing connection in case of conflict
  plugins/policy: Try reconnect Control/Target services

 plugins/policy.c         | 80 ++++++++++++++++++++++++++++++++++++++++++++++++
 profiles/audio/avctp.c   | 45 ++++++++++++++++-----------
 profiles/audio/avctp.h   |  2 +-
 profiles/audio/avrcp.c   | 11 ++++---
 profiles/audio/control.c |  3 +-
 5 files changed, 116 insertions(+), 25 deletions(-)

-- 
2.1.0

[PATCH 1/4] audio/avctp: Pass error argument to avctp_state_changed callback

[Marcin Kraglak]

This additional argument will help higher layer to recognize what action should be
taken in error condition.
---
 profiles/audio/avctp.c   | 38 ++++++++++++++++++++------------------
 profiles/audio/avctp.h   |  2 +-
 profiles/audio/avrcp.c   |  3 ++-
 profiles/audio/control.c |  3 ++-
 4 files changed, 25 insertions(+), 21 deletions(-)

diff --git a/profiles/audio/avctp.c b/profiles/audio/avctp.c
index 9f9f1c9..14b0266 100644
--- a/profiles/audio/avctp.c
+++ b/profiles/audio/avctp.c
@@ -573,7 +573,8 @@ static void avctp_disconnected(struct avctp *session)
 	g_free(session);
 }
 
-static void avctp_set_state(struct avctp *session, avctp_state_t new_state)
+static void avctp_set_state(struct avctp *session, avctp_state_t new_state,
+									int err)
 {
 	GSList *l;
 	avctp_state_t old_state = session->state;
@@ -586,7 +587,8 @@ static void avctp_set_state(struct avctp *session, avctp_state_t new_state)
 		if (cb->dev && cb->dev != session->device)
 			continue;
 
-		cb->cb(session->device, old_state, new_state, cb->user_data);
+		cb->cb(session->device, old_state, new_state, err,
+								cb->user_data);
 	}
 
 	switch (new_state) {
@@ -940,7 +942,7 @@ send:
 
 failed:
 	DBG("AVCTP Browsing: disconnected");
-	avctp_set_state(session, AVCTP_STATE_CONNECTED);
+	avctp_set_state(session, AVCTP_STATE_CONNECTED, 0);
 
 	if (session->browsing) {
 		avctp_channel_destroy(session->browsing);
@@ -1036,7 +1038,7 @@ done:
 
 failed:
 	DBG("AVCTP session %p got disconnected", session);
-	avctp_set_state(session, AVCTP_STATE_DISCONNECTED);
+	avctp_set_state(session, AVCTP_STATE_DISCONNECTED, -EIO);
 	return FALSE;
 }
 
@@ -1193,7 +1195,7 @@ static void avctp_connect_browsing_cb(GIOChannel *chan, GError *err,
 				G_IO_IN | G_IO_ERR | G_IO_HUP | G_IO_NVAL,
 				(GIOFunc) session_browsing_cb, session);
 
-	avctp_set_state(session, AVCTP_STATE_BROWSING_CONNECTED);
+	avctp_set_state(session, AVCTP_STATE_BROWSING_CONNECTED, 0);
 
 	/* Process any request that was pending the connection to complete */
 	if (browsing->process_id == 0 && !g_queue_is_empty(browsing->queue))
@@ -1202,7 +1204,7 @@ static void avctp_connect_browsing_cb(GIOChannel *chan, GError *err,
 	return;
 
 fail:
-	avctp_set_state(session, AVCTP_STATE_CONNECTED);
+	avctp_set_state(session, AVCTP_STATE_CONNECTED, 0);
 
 	if (session->browsing) {
 		avctp_channel_destroy(session->browsing);
@@ -1218,7 +1220,7 @@ static void avctp_connect_cb(GIOChannel *chan, GError *err, gpointer data)
 	GError *gerr = NULL;
 
 	if (err) {
-		avctp_set_state(session, AVCTP_STATE_DISCONNECTED);
+		avctp_set_state(session, AVCTP_STATE_DISCONNECTED, -EIO);
 		error("%s", err->message);
 		return;
 	}
@@ -1229,7 +1231,7 @@ static void avctp_connect_cb(GIOChannel *chan, GError *err, gpointer data)
 			BT_IO_OPT_IMTU, &omtu,
 			BT_IO_OPT_INVALID);
 	if (gerr) {
-		avctp_set_state(session, AVCTP_STATE_DISCONNECTED);
+		avctp_set_state(session, AVCTP_STATE_DISCONNECTED, -EIO);
 		error("%s", gerr->message);
 		g_error_free(gerr);
 		return;
@@ -1262,7 +1264,7 @@ static void avctp_connect_cb(GIOChannel *chan, GError *err, gpointer data)
 
 	init_uinput(session);
 
-	avctp_set_state(session, AVCTP_STATE_CONNECTED);
+	avctp_set_state(session, AVCTP_STATE_CONNECTED, 0);
 }
 
 static void auth_cb(DBusError *derr, void *user_data)
@@ -1279,7 +1281,7 @@ static void auth_cb(DBusError *derr, void *user_data)
 
 	if (derr && dbus_error_is_set(derr)) {
 		error("Access denied: %s", derr->message);
-		avctp_set_state(session, AVCTP_STATE_DISCONNECTED);
+		avctp_set_state(session, AVCTP_STATE_DISCONNECTED, -EIO);
 		return;
 	}
 
@@ -1287,7 +1289,7 @@ static void auth_cb(DBusError *derr, void *user_data)
 								NULL, &err)) {
 		error("bt_io_accept: %s", err->message);
 		g_error_free(err);
-		avctp_set_state(session, AVCTP_STATE_DISCONNECTED);
+		avctp_set_state(session, AVCTP_STATE_DISCONNECTED, -EIO);
 	}
 }
 
@@ -1352,7 +1354,7 @@ static void avctp_control_confirm(struct avctp *session, GIOChannel *chan,
 		return;
 	}
 
-	avctp_set_state(session, AVCTP_STATE_CONNECTING);
+	avctp_set_state(session, AVCTP_STATE_CONNECTING, 0);
 	session->control = avctp_channel_create(session, chan, NULL);
 
 	src = btd_adapter_get_address(device_get_adapter(dev));
@@ -1369,7 +1371,7 @@ static void avctp_control_confirm(struct avctp *session, GIOChannel *chan,
 	return;
 
 drop:
-	avctp_set_state(session, AVCTP_STATE_DISCONNECTED);
+	avctp_set_state(session, AVCTP_STATE_DISCONNECTED, -EIO);
 }
 
 static void avctp_browsing_confirm(struct avctp *session, GIOChannel *chan,
@@ -1385,7 +1387,7 @@ static void avctp_browsing_confirm(struct avctp *session, GIOChannel *chan,
 
 	if (bt_io_accept(chan, avctp_connect_browsing_cb, session, NULL,
 								&err)) {
-		avctp_set_state(session, AVCTP_STATE_BROWSING_CONNECTING);
+		avctp_set_state(session, AVCTP_STATE_BROWSING_CONNECTING, 0);
 		return;
 	}
 
@@ -1975,7 +1977,7 @@ struct avctp *avctp_connect(struct btd_device *device)
 	if (session->state > AVCTP_STATE_DISCONNECTED)
 		return session;
 
-	avctp_set_state(session, AVCTP_STATE_CONNECTING);
+	avctp_set_state(session, AVCTP_STATE_CONNECTING, 0);
 
 	src = btd_adapter_get_address(session->server->adapter);
 
@@ -1987,7 +1989,7 @@ struct avctp *avctp_connect(struct btd_device *device)
 				BT_IO_OPT_PSM, AVCTP_CONTROL_PSM,
 				BT_IO_OPT_INVALID);
 	if (err) {
-		avctp_set_state(session, AVCTP_STATE_DISCONNECTED);
+		avctp_set_state(session, AVCTP_STATE_DISCONNECTED, -EIO);
 		error("%s", err->message);
 		g_error_free(err);
 		return NULL;
@@ -2012,7 +2014,7 @@ int avctp_connect_browsing(struct avctp *session)
 	if (session->browsing != NULL)
 		return 0;
 
-	avctp_set_state(session, AVCTP_STATE_BROWSING_CONNECTING);
+	avctp_set_state(session, AVCTP_STATE_BROWSING_CONNECTING, 0);
 
 	src = btd_adapter_get_address(session->server->adapter);
 
@@ -2042,7 +2044,7 @@ void avctp_disconnect(struct avctp *session)
 	if (session->state == AVCTP_STATE_DISCONNECTED)
 		return;
 
-	avctp_set_state(session, AVCTP_STATE_DISCONNECTED);
+	avctp_set_state(session, AVCTP_STATE_DISCONNECTED, -EIO);
 }
 
 struct avctp *avctp_get(struct btd_device *device)
diff --git a/profiles/audio/avctp.h b/profiles/audio/avctp.h
index 05fceb4..6c19ce4 100644
--- a/profiles/audio/avctp.h
+++ b/profiles/audio/avctp.h
@@ -122,7 +122,7 @@ typedef enum {
 typedef void (*avctp_state_cb) (struct btd_device *dev,
 				avctp_state_t old_state,
 				avctp_state_t new_state,
-				void *user_data);
+				int err, void *user_data);
 
 typedef bool (*avctp_passthrough_cb) (struct avctp *session,
 					uint8_t op, bool pressed,
diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
index a67b81e..dc57770 100644
--- a/profiles/audio/avrcp.c
+++ b/profiles/audio/avrcp.c
@@ -3569,7 +3569,8 @@ static struct avrcp *session_create(struct avrcp_server *server,
 }
 
 static void state_changed(struct btd_device *device, avctp_state_t old_state,
-				avctp_state_t new_state, void *user_data)
+					avctp_state_t new_state, int err,
+					void *user_data)
 {
 	struct avrcp_server *server;
 	struct avrcp *session;
diff --git a/profiles/audio/control.c b/profiles/audio/control.c
index 9110b0f..4db5b92 100644
--- a/profiles/audio/control.c
+++ b/profiles/audio/control.c
@@ -71,7 +71,8 @@ struct control {
 };
 
 static void state_changed(struct btd_device *dev, avctp_state_t old_state,
-				avctp_state_t new_state, void *user_data)
+					avctp_state_t new_state, int err,
+					void *user_data)
 {
 	struct control *control = user_data;
 	DBusConnection *conn = btd_get_dbus_connection();
-- 
2.1.0

[PATCH 2/4] audio/avrcp: Pass error to session_destroy()

[Marcin Kraglak]

It will allow us to invoke btd_service_connecting_complete() or
btd_service_disconnecting_complete() with correct error code.
---
 profiles/audio/avrcp.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
index dc57770..414ee25 100644
--- a/profiles/audio/avrcp.c
+++ b/profiles/audio/avrcp.c
@@ -3507,7 +3507,7 @@ static void target_destroy(struct avrcp *session)
 	g_free(target);
 }
 
-static void session_destroy(struct avrcp *session)
+static void session_destroy(struct avrcp *session, int err)
 {
 	struct avrcp_server *server = session->server;
 	struct btd_service *service;
@@ -3519,7 +3519,7 @@ static void session_destroy(struct avrcp *session)
 	service = btd_device_get_service(session->dev, AVRCP_TARGET_UUID);
 	if (service != NULL) {
 		if (session->control_id == 0)
-			btd_service_connecting_complete(service, -EIO);
+			btd_service_connecting_complete(service, err);
 		else
 			btd_service_disconnecting_complete(service, 0);
 	}
@@ -3527,7 +3527,7 @@ static void session_destroy(struct avrcp *session)
 	service = btd_device_get_service(session->dev, AVRCP_REMOTE_UUID);
 	if (service != NULL) {
 		if (session->control_id == 0)
-			btd_service_connecting_complete(service, -EIO);
+			btd_service_connecting_complete(service, err);
 		else
 			btd_service_disconnecting_complete(service, 0);
 	}
@@ -3586,7 +3586,7 @@ static void state_changed(struct btd_device *device, avctp_state_t old_state,
 		if (session == NULL)
 			break;
 
-		session_destroy(session);
+		session_destroy(session, err);
 
 		break;
 	case AVCTP_STATE_CONNECTING:
-- 
2.1.0

[PATCH 3/4] audio/avctp: Cancel outgoing connection in case of conflict

[Marcin Kraglak]

According to AVRCP Spec 1.5 AVCTP channel should be closed if both
sides try establish connection at the same time.
---
 profiles/audio/avctp.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/profiles/audio/avctp.c b/profiles/audio/avctp.c
index 14b0266..22bf35b 100644
--- a/profiles/audio/avctp.c
+++ b/profiles/audio/avctp.c
@@ -1351,6 +1351,13 @@ static void avctp_control_confirm(struct avctp *session, GIOChannel *chan,
 	if (session->control != NULL) {
 		error("Control: Refusing unexpected connect");
 		g_io_channel_shutdown(chan, TRUE, NULL);
+
+		/*
+		 * Close AVCTP channel if remote tried connect
+		 * at the same time
+		 * AVRCP SPEC V1.5 4.1.1 Connection Establishment
+		 */
+		avctp_set_state(session, AVCTP_STATE_DISCONNECTED, -EAGAIN);
 		return;
 	}
 
-- 
2.1.0

[PATCH 4/4] plugins/policy: Try reconnect Control/Target services

[Marcin Kraglak]

If state of Control/Remote services changed from CONNECTING to
DISCONNECTED, and error is set to -EAGAIN, set random timer and try
reconnect. This approach is described in AVRCP Spec 1.5 4.1.1:
"If both devices open an AVCTP channel at the same time both channels
shall be closed and each device shall wait a random time
(not more than 1 s and not less than 100ms) and then try to open
the AVCTP channel again."
---
 plugins/policy.c | 80 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 80 insertions(+)

diff --git a/plugins/policy.c b/plugins/policy.c
index 9cbf146..306a7a3 100644
--- a/plugins/policy.c
+++ b/plugins/policy.c
@@ -50,6 +50,16 @@
 #define SINK_RETRY_TIMEOUT SOURCE_RETRY_TIMEOUT
 #define SOURCE_RETRIES 1
 #define SINK_RETRIES SOURCE_RETRIES
+#define CT_RETRIES 1
+#define TG_RETRIES CT_RETRIES
+
+/*
+ * AVRCP Spec 1.5 says that reconnect timeout after connection collision
+ * should be random value between 100 ms and 1000 ms. To improve user experience
+ * we linit MAX value to 500 ms
+ */
+#define RECONNECT_MAX_TIMEOUT 500
+#define RECONNECT_MIN_TIMEOUT 100
 
 /* Tracking of remote services to be auto-reconnected upon link loss */
 
@@ -82,7 +92,9 @@ struct policy_data {
 	guint sink_timer;
 	uint8_t sink_retries;
 	guint ct_timer;
+	uint8_t ct_retries;
 	guint tg_timer;
+	uint8_t tg_retries;
 };
 
 static void policy_connect(struct policy_data *data,
@@ -111,6 +123,7 @@ static gboolean policy_connect_ct(gpointer user_data)
 	struct btd_service *service;
 
 	data->ct_timer = 0;
+	data->ct_retries++;
 
 	service = btd_device_get_service(data->dev, AVRCP_REMOTE_UUID);
 	if (service != NULL)
@@ -128,6 +141,23 @@ static void policy_set_ct_timer(struct policy_data *data)
 						policy_connect_ct, data);
 }
 
+static void policy_set_random_ct_timer(struct policy_data *data)
+{
+	GRand *rand;
+	int timeout;
+
+	rand = g_rand_new();
+	timeout = g_rand_int_range(rand, RECONNECT_MIN_TIMEOUT,
+							RECONNECT_MAX_TIMEOUT);
+
+	if (data->ct_timer > 0)
+		g_source_remove(data->ct_timer);
+
+	data->ct_timer = g_timeout_add(timeout, policy_connect_ct, data);
+
+	g_rand_free(rand);
+}
+
 static struct policy_data *find_data(struct btd_device *dev)
 {
 	GSList *l;
@@ -273,6 +303,7 @@ static gboolean policy_connect_tg(gpointer user_data)
 	struct btd_service *service;
 
 	data->tg_timer = 0;
+	data->tg_retries++;
 
 	service = btd_device_get_service(data->dev, AVRCP_TARGET_UUID);
 	if (service != NULL)
@@ -291,6 +322,23 @@ static void policy_set_tg_timer(struct policy_data *data)
 							data);
 }
 
+static void policy_set_random_tg_timer(struct policy_data *data)
+{
+	GRand *rand;
+	int timeout;
+
+	rand = g_rand_new();
+	timeout = g_rand_int_range(rand, RECONNECT_MIN_TIMEOUT,
+							RECONNECT_MAX_TIMEOUT);
+
+	if (data->tg_timer > 0)
+		g_source_remove(data->tg_timer);
+
+	data->tg_timer = g_timeout_add(timeout, policy_connect_tg, data);
+
+	g_rand_free(rand);
+}
+
 static gboolean policy_connect_source(gpointer user_data)
 {
 	struct policy_data *data = user_data;
@@ -401,6 +449,22 @@ static void controller_cb(struct btd_service *service,
 		}
 		break;
 	case BTD_SERVICE_STATE_DISCONNECTED:
+		if (old_state == BTD_SERVICE_STATE_CONNECTING) {
+			int err = btd_service_get_error(service);
+
+			if (err == -EAGAIN) {
+				if (data->ct_retries < CT_RETRIES)
+					policy_set_random_ct_timer(data);
+				else
+					data->ct_retries = 0;
+				break;
+			} else if (data->ct_timer > 0) {
+				g_source_remove(data->ct_timer);
+				data->ct_timer = 0;
+			}
+		} else if (old_state == BTD_SERVICE_STATE_CONNECTED) {
+			data->ct_retries = 0;
+		}
 		break;
 	case BTD_SERVICE_STATE_CONNECTING:
 		break;
@@ -434,6 +498,22 @@ static void target_cb(struct btd_service *service,
 		}
 		break;
 	case BTD_SERVICE_STATE_DISCONNECTED:
+		if (old_state == BTD_SERVICE_STATE_CONNECTING) {
+			int err = btd_service_get_error(service);
+
+			if (err == -EAGAIN) {
+				if (data->tg_retries < TG_RETRIES)
+					policy_set_random_tg_timer(data);
+				else
+					data->tg_retries = 0;
+				break;
+			} else if (data->tg_timer > 0) {
+				g_source_remove(data->tg_timer);
+				data->tg_timer = 0;
+			}
+		} else if (old_state == BTD_SERVICE_STATE_CONNECTED) {
+			data->tg_retries = 0;
+		}
 		break;
 	case BTD_SERVICE_STATE_CONNECTING:
 		break;
-- 
2.1.0

Re: [PATCH 4/4] plugins/policy: Try reconnect Control/Target services

[Luiz Augusto von Dentz]

Hi Marcin,

On Wed, Mar 11, 2015 at 8:11 PM, Marcin Kraglak
<marcin.kraglak@tieto.com> wrote:
> If state of Control/Remote services changed from CONNECTING to
> DISCONNECTED, and error is set to -EAGAIN, set random timer and try
> reconnect. This approach is described in AVRCP Spec 1.5 4.1.1:
> "If both devices open an AVCTP channel at the same time both channels
> shall be closed and each device shall wait a random time
> (not more than 1 s and not less than 100ms) and then try to open
> the AVCTP channel again."
> ---
>  plugins/policy.c | 80 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 80 insertions(+)
>
> diff --git a/plugins/policy.c b/plugins/policy.c
> index 9cbf146..306a7a3 100644
> --- a/plugins/policy.c
> +++ b/plugins/policy.c
> @@ -50,6 +50,16 @@
>  #define SINK_RETRY_TIMEOUT SOURCE_RETRY_TIMEOUT
>  #define SOURCE_RETRIES 1
>  #define SINK_RETRIES SOURCE_RETRIES
> +#define CT_RETRIES 1
> +#define TG_RETRIES CT_RETRIES
> +
> +/*
> + * AVRCP Spec 1.5 says that reconnect timeout after connection collision
> + * should be random value between 100 ms and 1000 ms. To improve user experience
> + * we linit MAX value to 500 ms
> + */
> +#define RECONNECT_MAX_TIMEOUT 500
> +#define RECONNECT_MIN_TIMEOUT 100

Im not sure we should use sub seconds values, g_timeout_add_seconds
allows group wake-up whereas g_timeout_add doesn't (see
g_timeout_add_seconds_full documentation), so perhaps going with 1
sec. for reconnect is fine here since by design g_timeout_add_seconds
will not be precise so you have some randomness already.

>  /* Tracking of remote services to be auto-reconnected upon link loss */
>
> @@ -82,7 +92,9 @@ struct policy_data {
>         guint sink_timer;
>         uint8_t sink_retries;
>         guint ct_timer;
> +       uint8_t ct_retries;
>         guint tg_timer;
> +       uint8_t tg_retries;
>  };
>
>  static void policy_connect(struct policy_data *data,
> @@ -111,6 +123,7 @@ static gboolean policy_connect_ct(gpointer user_data)
>         struct btd_service *service;
>
>         data->ct_timer = 0;
> +       data->ct_retries++;
>
>         service = btd_device_get_service(data->dev, AVRCP_REMOTE_UUID);
>         if (service != NULL)
> @@ -128,6 +141,23 @@ static void policy_set_ct_timer(struct policy_data *data)
>                                                 policy_connect_ct, data);
>  }
>
> +static void policy_set_random_ct_timer(struct policy_data *data)
> +{
> +       GRand *rand;
> +       int timeout;
> +
> +       rand = g_rand_new();
> +       timeout = g_rand_int_range(rand, RECONNECT_MIN_TIMEOUT,
> +                                                       RECONNECT_MAX_TIMEOUT);
> +
> +       if (data->ct_timer > 0)
> +               g_source_remove(data->ct_timer);
> +
> +       data->ct_timer = g_timeout_add(timeout, policy_connect_ct, data);
> +
> +       g_rand_free(rand);
> +}
> +
>  static struct policy_data *find_data(struct btd_device *dev)
>  {
>         GSList *l;
> @@ -273,6 +303,7 @@ static gboolean policy_connect_tg(gpointer user_data)
>         struct btd_service *service;
>
>         data->tg_timer = 0;
> +       data->tg_retries++;
>
>         service = btd_device_get_service(data->dev, AVRCP_TARGET_UUID);
>         if (service != NULL)
> @@ -291,6 +322,23 @@ static void policy_set_tg_timer(struct policy_data *data)
>                                                         data);
>  }
>
> +static void policy_set_random_tg_timer(struct policy_data *data)
> +{
> +       GRand *rand;
> +       int timeout;
> +
> +       rand = g_rand_new();
> +       timeout = g_rand_int_range(rand, RECONNECT_MIN_TIMEOUT,
> +                                                       RECONNECT_MAX_TIMEOUT);
> +
> +       if (data->tg_timer > 0)
> +               g_source_remove(data->tg_timer);
> +
> +       data->tg_timer = g_timeout_add(timeout, policy_connect_tg, data);
> +
> +       g_rand_free(rand);
> +}
> +
>  static gboolean policy_connect_source(gpointer user_data)
>  {
>         struct policy_data *data = user_data;
> @@ -401,6 +449,22 @@ static void controller_cb(struct btd_service *service,
>                 }
>                 break;
>         case BTD_SERVICE_STATE_DISCONNECTED:
> +               if (old_state == BTD_SERVICE_STATE_CONNECTING) {
> +                       int err = btd_service_get_error(service);
> +
> +                       if (err == -EAGAIN) {
> +                               if (data->ct_retries < CT_RETRIES)
> +                                       policy_set_random_ct_timer(data);
> +                               else
> +                                       data->ct_retries = 0;
> +                               break;
> +                       } else if (data->ct_timer > 0) {
> +                               g_source_remove(data->ct_timer);
> +                               data->ct_timer = 0;
> +                       }
> +               } else if (old_state == BTD_SERVICE_STATE_CONNECTED) {
> +                       data->ct_retries = 0;
> +               }
>                 break;
>         case BTD_SERVICE_STATE_CONNECTING:
>                 break;
> @@ -434,6 +498,22 @@ static void target_cb(struct btd_service *service,
>                 }
>                 break;
>         case BTD_SERVICE_STATE_DISCONNECTED:
> +               if (old_state == BTD_SERVICE_STATE_CONNECTING) {
> +                       int err = btd_service_get_error(service);
> +
> +                       if (err == -EAGAIN) {
> +                               if (data->tg_retries < TG_RETRIES)
> +                                       policy_set_random_tg_timer(data);
> +                               else
> +                                       data->tg_retries = 0;
> +                               break;
> +                       } else if (data->tg_timer > 0) {
> +                               g_source_remove(data->tg_timer);
> +                               data->tg_timer = 0;
> +                       }
> +               } else if (old_state == BTD_SERVICE_STATE_CONNECTED) {
> +                       data->tg_retries = 0;
> +               }
>                 break;
>         case BTD_SERVICE_STATE_CONNECTING:
>                 break;
> --
> 2.1.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Luiz Augusto von Dentz

Re: [PATCH 4/4] plugins/policy: Try reconnect Control/Target services

[Marcin Kraglak]

Hi Luiz,

On 13 March 2015 at 09:42, Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote:
> Hi Marcin,
>
> On Wed, Mar 11, 2015 at 8:11 PM, Marcin Kraglak
> <marcin.kraglak@tieto.com> wrote:
>> If state of Control/Remote services changed from CONNECTING to
>> DISCONNECTED, and error is set to -EAGAIN, set random timer and try
>> reconnect. This approach is described in AVRCP Spec 1.5 4.1.1:
>> "If both devices open an AVCTP channel at the same time both channels
>> shall be closed and each device shall wait a random time
>> (not more than 1 s and not less than 100ms) and then try to open
>> the AVCTP channel again."
>> ---
>>  plugins/policy.c | 80 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 80 insertions(+)
>>
>> diff --git a/plugins/policy.c b/plugins/policy.c
>> index 9cbf146..306a7a3 100644
>> --- a/plugins/policy.c
>> +++ b/plugins/policy.c
>> @@ -50,6 +50,16 @@
>>  #define SINK_RETRY_TIMEOUT SOURCE_RETRY_TIMEOUT
>>  #define SOURCE_RETRIES 1
>>  #define SINK_RETRIES SOURCE_RETRIES
>> +#define CT_RETRIES 1
>> +#define TG_RETRIES CT_RETRIES
>> +
>> +/*
>> + * AVRCP Spec 1.5 says that reconnect timeout after connection collision
>> + * should be random value between 100 ms and 1000 ms. To improve user experience
>> + * we linit MAX value to 500 ms
>> + */
>> +#define RECONNECT_MAX_TIMEOUT 500
>> +#define RECONNECT_MIN_TIMEOUT 100
>
> Im not sure we should use sub seconds values, g_timeout_add_seconds
> allows group wake-up whereas g_timeout_add doesn't (see
> g_timeout_add_seconds_full documentation), so perhaps going with 1
> sec. for reconnect is fine here since by design g_timeout_add_seconds
> will not be precise so you have some randomness already.

That's fine with me, I'll change it to 1 sec timeout.
>
>>  /* Tracking of remote services to be auto-reconnected upon link loss */
>>
>> @@ -82,7 +92,9 @@ struct policy_data {
>>         guint sink_timer;
>>         uint8_t sink_retries;
>>         guint ct_timer;
>> +       uint8_t ct_retries;
>>         guint tg_timer;
>> +       uint8_t tg_retries;
>>  };
>>
>>  static void policy_connect(struct policy_data *data,
>> @@ -111,6 +123,7 @@ static gboolean policy_connect_ct(gpointer user_data)
>>         struct btd_service *service;
>>
>>         data->ct_timer = 0;
>> +       data->ct_retries++;
>>
>>         service = btd_device_get_service(data->dev, AVRCP_REMOTE_UUID);
>>         if (service != NULL)
>> @@ -128,6 +141,23 @@ static void policy_set_ct_timer(struct policy_data *data)
>>                                                 policy_connect_ct, data);
>>  }
>>
>> +static void policy_set_random_ct_timer(struct policy_data *data)
>> +{
>> +       GRand *rand;
>> +       int timeout;
>> +
>> +       rand = g_rand_new();
>> +       timeout = g_rand_int_range(rand, RECONNECT_MIN_TIMEOUT,
>> +                                                       RECONNECT_MAX_TIMEOUT);
>> +
>> +       if (data->ct_timer > 0)
>> +               g_source_remove(data->ct_timer);
>> +
>> +       data->ct_timer = g_timeout_add(timeout, policy_connect_ct, data);
>> +
>> +       g_rand_free(rand);
>> +}
>> +
>>  static struct policy_data *find_data(struct btd_device *dev)
>>  {
>>         GSList *l;
>> @@ -273,6 +303,7 @@ static gboolean policy_connect_tg(gpointer user_data)
>>         struct btd_service *service;
>>
>>         data->tg_timer = 0;
>> +       data->tg_retries++;
>>
>>         service = btd_device_get_service(data->dev, AVRCP_TARGET_UUID);
>>         if (service != NULL)
>> @@ -291,6 +322,23 @@ static void policy_set_tg_timer(struct policy_data *data)
>>                                                         data);
>>  }
>>
>> +static void policy_set_random_tg_timer(struct policy_data *data)
>> +{
>> +       GRand *rand;
>> +       int timeout;
>> +
>> +       rand = g_rand_new();
>> +       timeout = g_rand_int_range(rand, RECONNECT_MIN_TIMEOUT,
>> +                                                       RECONNECT_MAX_TIMEOUT);
>> +
>> +       if (data->tg_timer > 0)
>> +               g_source_remove(data->tg_timer);
>> +
>> +       data->tg_timer = g_timeout_add(timeout, policy_connect_tg, data);
>> +
>> +       g_rand_free(rand);
>> +}
>> +
>>  static gboolean policy_connect_source(gpointer user_data)
>>  {
>>         struct policy_data *data = user_data;
>> @@ -401,6 +449,22 @@ static void controller_cb(struct btd_service *service,
>>                 }
>>                 break;
>>         case BTD_SERVICE_STATE_DISCONNECTED:
>> +               if (old_state == BTD_SERVICE_STATE_CONNECTING) {
>> +                       int err = btd_service_get_error(service);
>> +
>> +                       if (err == -EAGAIN) {
>> +                               if (data->ct_retries < CT_RETRIES)
>> +                                       policy_set_random_ct_timer(data);
>> +                               else
>> +                                       data->ct_retries = 0;
>> +                               break;
>> +                       } else if (data->ct_timer > 0) {
>> +                               g_source_remove(data->ct_timer);
>> +                               data->ct_timer = 0;
>> +                       }
>> +               } else if (old_state == BTD_SERVICE_STATE_CONNECTED) {
>> +                       data->ct_retries = 0;
>> +               }
>>                 break;
>>         case BTD_SERVICE_STATE_CONNECTING:
>>                 break;
>> @@ -434,6 +498,22 @@ static void target_cb(struct btd_service *service,
>>                 }
>>                 break;
>>         case BTD_SERVICE_STATE_DISCONNECTED:
>> +               if (old_state == BTD_SERVICE_STATE_CONNECTING) {
>> +                       int err = btd_service_get_error(service);
>> +
>> +                       if (err == -EAGAIN) {
>> +                               if (data->tg_retries < TG_RETRIES)
>> +                                       policy_set_random_tg_timer(data);
>> +                               else
>> +                                       data->tg_retries = 0;
>> +                               break;
>> +                       } else if (data->tg_timer > 0) {
>> +                               g_source_remove(data->tg_timer);
>> +                               data->tg_timer = 0;
>> +                       }
>> +               } else if (old_state == BTD_SERVICE_STATE_CONNECTED) {
>> +                       data->tg_retries = 0;
>> +               }
>>                 break;
>>         case BTD_SERVICE_STATE_CONNECTING:
>>                 break;
>> --
>> 2.1.0
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>
>
> --
> Luiz Augusto von Dentz



-- 
BR
Marcin Kraglak

Re: [PATCH 0/4] Fix collision in AVCTP connection

[Luiz Augusto von Dentz]

Hi Marcin,

On Wed, Mar 11, 2015 at 8:10 PM, Marcin Kraglak
<marcin.kraglak@tieto.com> wrote:
> This patch set corresponds to previous RFC with the same subject.
> Now reconnection logic is moved to policy. Avrcp and avctp
> modules are responsible for passing correct error to higher
> layer, policy detects collision and set random timer for reconnection.
> Comments are welcome
>
> Marcin Kraglak (4):
>   audio/avctp: Pass error argument to avctp_state_changed callback
>   audio/avrcp: Pass error to session_destroy()
>   audio/avctp: Cancel outgoing connection in case of conflict
>   plugins/policy: Try reconnect Control/Target services
>
>  plugins/policy.c         | 80 ++++++++++++++++++++++++++++++++++++++++++++++++
>  profiles/audio/avctp.c   | 45 ++++++++++++++++-----------
>  profiles/audio/avctp.h   |  2 +-
>  profiles/audio/avrcp.c   | 11 ++++---
>  profiles/audio/control.c |  3 +-
>  5 files changed, 116 insertions(+), 25 deletions(-)
>
> --
> 2.1.0

Applied, thanks.


-- 
Luiz Augusto von Dentz