Return-Path: MIME-Version: 1.0 In-Reply-To: References: <1425024072-6259-1-git-send-email-marcin.kraglak@tieto.com> <1425024072-6259-2-git-send-email-marcin.kraglak@tieto.com> <1432936.7iWJrZ6TN0@uw000953> Date: Mon, 2 Mar 2015 07:59:00 +0100 Message-ID: Subject: Re: [RFC] audio/avctp: Accept incoming connection in case of colission From: Marcin Kraglak To: Luiz Augusto von Dentz Cc: Szymon Janc , "linux-bluetooth@vger.kernel.org" Content-Type: text/plain; charset=UTF-8 Sender: linux-bluetooth-owner@vger.kernel.org List-ID: Hi Luiz, Why do you want to move it to policy? As I understand policy is a way we want to improve user-experience, such connecting to default profiles etc., it's about channel establishment. On 27 February 2015 at 15:07, Luiz Augusto von Dentz wrote: > Hi Szymon, > > On Fri, Feb 27, 2015 at 12:37 PM, Szymon Janc wrote: >> Hi Marcin, >> >> On Friday 27 of February 2015 09:01:12 Marcin Kraglak wrote: >>> Accept incoming connection to Control Channel in case of colission. >>> If accepting connection won't succeed, try reconnect. >>> There is no need to request authorization while user has been connecting >>> to service in meantime. >>> Issue was found during tests with NOKIA BH-505 and Motorola HT-820. >>> --- >>> profiles/audio/avctp.c | 38 ++++++++++++++++++++++++++++++++++++-- >>> 1 file changed, 36 insertions(+), 2 deletions(-) >>> >>> diff --git a/profiles/audio/avctp.c b/profiles/audio/avctp.c >>> index 57071b5..8d457f4 100644 >>> --- a/profiles/audio/avctp.c >>> +++ b/profiles/audio/avctp.c >>> @@ -203,6 +203,7 @@ struct avctp { >>> uint8_t key_quirks[256]; >>> struct key_pressed key; >>> bool initiator; >>> + bool reconnect; >>> }; >>> >>> struct avctp_passthrough_handler { >>> @@ -1219,11 +1220,22 @@ static void avctp_connect_cb(GIOChannel *chan, GError *err, gpointer data) >>> GError *gerr = NULL; >>> >>> if (err) { >>> + struct btd_device *device = session->device; >>> + bool reconnect = session->reconnect; >>> + >>> avctp_set_state(session, AVCTP_STATE_DISCONNECTED); >>> error("%s", err->message); >>> + >>> + if (!reconnect) >>> + return; >>> + >>> + DBG("Control: Attempting to reconnect"); >>> + avctp_connect(device); >>> return; >>> } >>> >>> + session->reconnect = false; >>> + >>> bt_io_get(chan, &gerr, >>> BT_IO_OPT_DEST, &address, >>> BT_IO_OPT_IMTU, &imtu, >>> @@ -1346,10 +1358,32 @@ static void avctp_control_confirm(struct avctp *session, GIOChannel *chan, >>> { >>> const bdaddr_t *src; >>> const bdaddr_t *dst; >>> + GError *err = NULL; >>> >>> if (session->control != NULL) { >>> - error("Control: Refusing unexpected connect"); >>> - g_io_channel_shutdown(chan, TRUE, NULL); >>> + if (session->state != AVCTP_STATE_CONNECTING) { >>> + error("Control: Refusing unexpected connect"); >>> + g_io_channel_shutdown(chan, TRUE, NULL); >>> + return; >>> + } >>> + >>> + /* Cancel outgoing connection and accept incoming */ >>> + DBG("Control: Accepting unexpected connect from remote"); >>> + g_io_channel_shutdown(session->control->io, TRUE, NULL); >>> + g_io_channel_unref(session->control->io); >>> + session->control->io = NULL; >>> + >>> + if (!bt_io_accept(chan, avctp_connect_cb, session, NULL, >>> + &err)) { >>> + error("bt_io_accept: %s", err->message); >>> + g_error_free(err); >>> + avctp_set_state(session, AVCTP_STATE_DISCONNECTED); >>> + return; >>> + } >>> + >>> + session->reconnect = true; >>> + session->control->io = g_io_channel_ref(chan); >>> + >>> return; >>> } >> >> I think we should go with random timer as required by AVRCP specification >> >> p.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. If it is known which >> device is the master that device can re-try at once." > > That and it is probably a better idea to leave this to policy to > re-try as it is already doing for A2DP. > > > -- > Luiz Augusto von Dentz -- BR Marcin Kraglak