[PATCH_v2 0/7] Initial implementation for MCAP data channel

[PATCH_v2 0/7] Initial implementation for MCAP data channel

[Ravi kumar Veeramally]

v2: Few cleanups and support for streaming data channel.

v1: Patchset implements initial MCAP MDL creation and connection. On
    successful MDL (reliable) connection fd will be passed to HAL.

PTS tests:

TC_SRC_CC_BV_03_C:

hl register_application bluez-android Bluez  bluez-hdp health-device-profile 1 BTHL_MDEP_ROLE_SOURCE 4100 BTHL_CHANNEL_TYPE_RELIABLE pulse-oximeter

hl connect_channel 1 00:1b:dc:05:b5:54 0

TC_SRC_CC_BV_07_C:

hl register_application bluez-android Bluez  bluez-hdp health-device-profile 2 BTHL_MDEP_ROLE_SOURCE 4100 BTHL_CHANNEL_TYPE_RELIABLE pulse-oximeter  BTHL_MDEP_ROLE_SOURCE 4100 BTHL_CHANNEL_TYPE_STREAMING pulse-oximeter

hl connect_channel 1 00:1b:dc:05:b5:54 0
hl connect_channel 1 00:1b:dc:05:b5:54 1


TC_SNK_CC_BV_04_C:
TC_SNK_HCT_BV_01_I:

hl register_application bluez-android Bluez  bluez-hdp health-device-profile 1 BTHL_MDEP_ROLE_SINK 4100 BTHL_CHANNEL_TYPE_RELIABLE pulse-oximeter

hl connect_channel 1 00:1b:dc:05:b5:54 0


TC_SNK_CC_BV_08_C:

hl register_application bluez-android Bluez  bluez-hdp health-device-profile 2 BTHL_MDEP_ROLE_SINK 4100 BTHL_CHANNEL_TYPE_RELIABLE pulse-oximeter  BTHL_MDEP_ROLE_SINK 4100 BTHL_CHANNEL_TYPE_STREAMING pulse-oximeter

hl connect_channel 1 00:1b:dc:05:b5:54 0
hl connect_channel 1 00:1b:dc:05:b5:54 1


Ravi kumar Veeramally (7):
  android/health: Fix queue creation for mdeps and devices
  android/health: Check if device and channel already exists or not
  android/health: Cache remote mdep id on channel connect request
  android/health: Notify channel status on channel destroy call
  android/health: Create and connect MDL
  android/health: Implement mcap_mdl_deleted_cb
  anrdroid/client/health: Cache fd and close on channel disconnection

 android/client/if-hl.c |   9 ++
 android/health.c       | 419 +++++++++++++++++++++++++++++++++++++++++--------
 2 files changed, 363 insertions(+), 65 deletions(-)

-- 
1.9.1

[PATCH_v2 1/7] android/health: Fix queue creation for mdeps and devices

[Ravi kumar Veeramally]

Create queue for mdeps, devices and channels when creating app
and device struct. It is simpler to read code than on demand
creation.
---
 android/health.c | 33 +++++++++++++--------------------
 1 file changed, 13 insertions(+), 20 deletions(-)

diff --git a/android/health.c b/android/health.c
index 0462e99..7553467 100644
--- a/android/health.c
+++ b/android/health.c
@@ -678,6 +678,14 @@ static struct health_app *create_health_app(const char *app_name,
 			goto fail;
 	}
 
+	app->mdeps = queue_new();
+	if (!app->mdeps)
+		goto fail;
+
+	app->devices = queue_new();
+	if (!app->devices)
+		goto fail;
+
 	return app;
 
 fail:
@@ -784,14 +792,6 @@ static void bt_health_mdep_cfg_data(const void *buf, uint16_t len)
 		memcpy(mdep->descr, cmd->descr, cmd->descr_len);
 	}
 
-	if (app->num_of_mdep > 0 && !app->mdeps) {
-		app->mdeps = queue_new();
-		if (!app->mdeps) {
-			status = HAL_STATUS_FAILED;
-			goto fail;
-		}
-	}
-
 	if (!queue_push_tail(app->mdeps, mdep)) {
 		status = HAL_STATUS_FAILED;
 		goto fail;
@@ -1102,6 +1102,11 @@ static struct health_device *create_device(uint16_t app_id, const uint8_t *addr)
 
 	android2bdaddr(addr, &dev->dst);
 	dev->app_id = app_id;
+	dev->channels = queue_new();
+	if (!dev->channels) {
+		free_health_device(dev);
+		return NULL;
+	}
 
 	return dev;
 }
@@ -1159,21 +1164,9 @@ static void bt_health_connect_channel(const void *buf, uint16_t len)
 
 	channel->dev = dev;
 
-	if (!app->devices) {
-		app->devices = queue_new();
-		if (!app->devices)
-			goto fail;
-	}
-
 	if (!queue_push_tail(app->devices, dev))
 		goto fail;
 
-	if (!dev->channels) {
-		dev->channels = queue_new();
-		if (!dev->channels)
-			goto fail;
-	}
-
 	if (!queue_push_tail(dev->channels, channel)) {
 		queue_remove(app->devices, dev);
 		goto fail;
-- 
1.9.1

[PATCH_v2 2/7] android/health: Check if device and channel already exists or not

[Ravi kumar Veeramally]

On channel connect request, check if device is already exists or not.
Also check if channel is already created for remote device or not.
---
 android/health.c | 83 ++++++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 59 insertions(+), 24 deletions(-)

diff --git a/android/health.c b/android/health.c
index 7553467..083ab1e 100644
--- a/android/health.c
+++ b/android/health.c
@@ -213,6 +213,22 @@ static void send_channel_state_notify(struct health_channel *channel,
 					sizeof(ev), &ev, fd);
 }
 
+static bool dev_by_addr(const void *data, const void *user_data)
+{
+	const struct health_device *dev = data;
+	const bdaddr_t *addr = user_data;
+
+	return !bacmp(&dev->dst, addr);
+}
+
+static bool channel_by_mdep_id(const void *data, const void *user_data)
+{
+	const struct health_channel *channel = data;
+	uint16_t mdep_id = PTR_TO_INT(user_data);
+
+	return channel->mdep_id == mdep_id;
+}
+
 static bool mdep_by_mdep_role(const void *data, const void *user_data)
 {
 	const struct mdep_cfg *mdep = data;
@@ -1094,25 +1110,44 @@ static int connect_mcl(struct health_channel *channel)
 
 static struct health_device *create_device(uint16_t app_id, const uint8_t *addr)
 {
+	struct health_app *app;
 	struct health_device *dev;
+	bdaddr_t bdaddr;
 
+	app = queue_find(apps, app_by_app_id, INT_TO_PTR(app_id));
+	if (!app)
+		return NULL;
+
+	android2bdaddr(addr, &bdaddr);
+	dev = queue_find(app->devices, dev_by_addr, &bdaddr);
+	if (dev)
+		return dev;
+
+	/* create device and push it to devices queue */
 	dev = new0(struct health_device, 1);
 	if (!dev)
 		return NULL;
 
 	android2bdaddr(addr, &dev->dst);
 	dev->app_id = app_id;
+
 	dev->channels = queue_new();
 	if (!dev->channels) {
 		free_health_device(dev);
 		return NULL;
 	}
 
+	if (!queue_push_tail(app->devices, dev)) {
+		free_health_device(dev);
+		return NULL;
+	}
+
 	return dev;
 }
 
 static struct health_channel *create_channel(uint16_t app_id,
-						uint8_t mdep_index)
+						uint8_t mdep_index,
+						struct health_device *dev)
 {
 	struct health_app *app;
 	struct mdep_cfg *mdep;
@@ -1120,22 +1155,37 @@ static struct health_channel *create_channel(uint16_t app_id,
 	uint8_t index;
 	static unsigned int channel_id = 1;
 
+	if (!dev)
+		return NULL;
+
+	index = mdep_index + 1;
+	channel = queue_find(dev->channels, channel_by_mdep_id,
+							INT_TO_PTR(index));
+	if (channel)
+		return channel;
+
 	app = queue_find(apps, app_by_app_id, INT_TO_PTR(app_id));
 	if (!app)
 		return NULL;
 
-	index = mdep_index + 1;
 	mdep = queue_find(app->mdeps, mdep_by_mdep_id, INT_TO_PTR(index));
 	if (!mdep)
 		return NULL;
 
+	/* create channel and push it to device */
 	channel = new0(struct health_channel, 1);
 	if (!channel)
 		return NULL;
 
-	channel->mdep_id = mdep_index;
+	channel->mdep_id = mdep->id;
 	channel->type = mdep->channel_type;
 	channel->id = channel_id++;
+	channel->dev = dev;
+
+	if (!queue_push_tail(dev->channels, channel)) {
+		free_health_channel(channel);
+		return NULL;
+	}
 
 	return channel;
 }
@@ -1144,38 +1194,24 @@ static void bt_health_connect_channel(const void *buf, uint16_t len)
 {
 	const struct hal_cmd_health_connect_channel *cmd = buf;
 	struct hal_rsp_health_connect_channel rsp;
-	struct health_app *app;
 	struct health_device *dev = NULL;
 	struct health_channel *channel = NULL;
 
 	DBG("");
 
-	app = queue_find(apps, app_by_app_id, INT_TO_PTR(cmd->app_id));
-	if (!app)
-		goto fail;
-
 	dev = create_device(cmd->app_id, cmd->bdaddr);
 	if (!dev)
 		goto fail;
 
-	channel = create_channel(cmd->app_id, cmd->mdep_index);
+	channel = create_channel(cmd->app_id, cmd->mdep_index, dev);
 	if (!channel)
 		goto fail;
 
-	channel->dev = dev;
-
-	if (!queue_push_tail(app->devices, dev))
-		goto fail;
-
-	if (!queue_push_tail(dev->channels, channel)) {
-		queue_remove(app->devices, dev);
-		goto fail;
-	}
-
-	if (connect_mcl(channel) < 0) {
-		error("error retrieving HDP SDP record");
-		queue_remove(app->devices, dev);
-		goto fail;
+	if (!dev->mcl) {
+		if (connect_mcl(channel) < 0) {
+			error("error retrieving HDP SDP record");
+			goto fail;
+		}
 	}
 
 	rsp.channel_id = channel->id;
@@ -1186,7 +1222,6 @@ static void bt_health_connect_channel(const void *buf, uint16_t len)
 
 fail:
 	free_health_channel(channel);
-	free_health_device(dev);
 	ipc_send_rsp(hal_ipc, HAL_SERVICE_ID_HEALTH,
 			HAL_OP_HEALTH_CONNECT_CHANNEL, HAL_STATUS_FAILED);
 }
-- 
1.9.1

[PATCH_v2 3/7] android/health: Cache remote mdep id on channel connect request

[Ravi kumar Veeramally]

Remote mdep is required to initiate MD_CREATE_MDL_REQ request.
---
 android/health.c | 124 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 123 insertions(+), 1 deletion(-)

diff --git a/android/health.c b/android/health.c
index 083ab1e..506c859 100644
--- a/android/health.c
+++ b/android/health.c
@@ -95,6 +95,11 @@ struct health_channel {
 
 	struct health_device *dev;
 
+	uint8_t remote_mdep;
+	struct mcap_mdl *mdl;
+	bool mdl_conn;
+	uint16_t mdl_id; /* MDL ID */
+
 	uint16_t id; /* channel id */
 };
 
@@ -1011,6 +1016,111 @@ static void mcap_mdl_reconn_req_cb(struct mcap_mdl *mdl, void *data)
 	DBG("Not Implemeneted");
 }
 
+static bool check_role(uint8_t rec_role, uint8_t app_role)
+{
+	if ((rec_role == HAL_HEALTH_MDEP_ROLE_SINK &&
+			app_role == HAL_HEALTH_MDEP_ROLE_SOURCE) ||
+			(rec_role == HAL_HEALTH_MDEP_ROLE_SOURCE &&
+			app_role == HAL_HEALTH_MDEP_ROLE_SINK))
+		return true;
+
+	return false;
+}
+
+static bool get_mdep_from_rec(const sdp_record_t *rec, uint8_t role,
+						uint16_t d_type, uint8_t *mdep)
+{
+	sdp_data_t *list, *feat;
+
+	if (!mdep)
+		return false;
+
+	list = sdp_data_get(rec, SDP_ATTR_SUPPORTED_FEATURES_LIST);
+	if (!list || !SDP_IS_SEQ(list->dtd))
+		return false;
+
+	for (feat = list->val.dataseq; feat; feat = feat->next) {
+		sdp_data_t *data_type, *mdepid, *role_t;
+
+		if (!SDP_IS_SEQ(feat->dtd))
+			continue;
+
+		mdepid = feat->val.dataseq;
+		if (!mdepid)
+			continue;
+
+		data_type = mdepid->next;
+		if (!data_type)
+			continue;
+
+		role_t = data_type->next;
+		if (!role_t)
+			continue;
+
+		if (data_type->dtd != SDP_UINT16 || mdepid->dtd != SDP_UINT8 ||
+						role_t->dtd != SDP_UINT8)
+			continue;
+
+		if (data_type->val.uint16 != d_type ||
+					!check_role(role_t->val.uint8, role))
+			continue;
+
+		*mdep = mdepid->val.uint8;
+
+		return true;
+	}
+
+	return false;
+}
+
+static void get_mdep_cb(sdp_list_t *recs, int err, gpointer user_data)
+{
+	struct health_channel *channel = user_data;
+	struct health_app *app;
+	struct mdep_cfg *mdep;
+	uint8_t mdep_id;
+
+	if (err < 0 || !recs) {
+		error("error getting remote SDP records");
+		goto fail;
+	}
+
+	app = queue_find(apps, app_by_app_id, INT_TO_PTR(channel->dev->app_id));
+	if (!app)
+		goto fail;
+
+	mdep = queue_find(app->mdeps, mdep_by_mdep_id,
+						INT_TO_PTR(channel->mdep_id));
+	if (!mdep)
+		goto fail;
+
+	if (!get_mdep_from_rec(recs->data, mdep->role, mdep->data_type,
+								&mdep_id)) {
+		error("no matching MDEP found");
+		goto fail;
+	}
+
+	channel->remote_mdep = mdep_id;
+
+	/* TODO : create mdl */
+	return;
+
+fail:
+	destroy_channel(channel);
+}
+
+static int get_mdep(struct health_channel *channel)
+{
+	uuid_t uuid;
+
+	DBG("");
+
+	bt_string2uuid(&uuid, HDP_UUID);
+
+	return bt_search_service(&adapter_addr, &channel->dev->dst, &uuid,
+						get_mdep_cb, channel, NULL, 0);
+}
+
 static void create_mcl_cb(struct mcap_mcl *mcl, GError *err, gpointer data)
 {
 	struct health_channel *channel = data;
@@ -1047,7 +1157,11 @@ static void create_mcl_cb(struct mcap_mcl *mcl, GError *err, gpointer data)
 		goto fail;
 	}
 
-	/* TODO : create mdl */
+	if (get_mdep(channel) < 0) {
+		error("error getting remote MDEP from SDP record");
+		goto fail;
+	}
+
 	return;
 
 fail:
@@ -1212,6 +1326,14 @@ static void bt_health_connect_channel(const void *buf, uint16_t len)
 			error("error retrieving HDP SDP record");
 			goto fail;
 		}
+	} else {
+		/* data channel is already connected */
+		if (channel->mdl && channel->mdl_conn)
+			goto fail;
+
+		/* create mdl if it does not exists */
+		if (!channel->mdl && get_mdep(channel) < 0)
+			goto fail;
 	}
 
 	rsp.channel_id = channel->id;
-- 
1.9.1

[PATCH_v2 4/7] android/health: Notify channel status on channel destroy call

[Ravi kumar Veeramally]

---
 android/health.c | 64 ++++++++++++++++++++++++++++----------------------------
 1 file changed, 32 insertions(+), 32 deletions(-)

diff --git a/android/health.c b/android/health.c
index 506c859..fff999d 100644
--- a/android/health.c
+++ b/android/health.c
@@ -115,6 +115,37 @@ struct health_app {
 	struct queue *devices;
 };
 
+static void send_app_reg_notify(struct health_app *app, uint8_t state)
+{
+	struct hal_ev_health_app_reg_state ev;
+
+	DBG("");
+
+	ev.id = app->id;
+	ev.state = state;
+
+	ipc_send_notif(hal_ipc, HAL_SERVICE_ID_HEALTH,
+				HAL_EV_HEALTH_APP_REG_STATE, sizeof(ev), &ev);
+}
+
+static void send_channel_state_notify(struct health_channel *channel,
+						uint8_t state, int fd)
+{
+	struct hal_ev_health_channel_state ev;
+
+	DBG("");
+
+	bdaddr2android(&channel->dev->dst, ev.bdaddr);
+	ev.app_id = channel->dev->app_id;
+	ev.mdep_index = channel->mdep_id;
+	ev.channel_id = channel->id;
+	ev.channel_state = state;
+
+	ipc_send_notif_with_fd(hal_ipc, HAL_SERVICE_ID_HEALTH,
+					HAL_EV_HEALTH_CHANNEL_STATE,
+					sizeof(ev), &ev, fd);
+}
+
 static void free_health_channel(void *data)
 {
 	struct health_channel *channel = data;
@@ -132,7 +163,7 @@ static void destroy_channel(void *data)
 	if (!channel)
 		return;
 
-	/* TODO: Notify channel connection status DESTROYED */
+	send_channel_state_notify(channel, HAL_HEALTH_CHANNEL_DESTROYED, -1);
 	queue_remove(channel->dev->channels, channel);
 	free_health_channel(channel);
 }
@@ -187,37 +218,6 @@ static void free_health_app(void *data)
 	free(app);
 }
 
-static void send_app_reg_notify(struct health_app *app, uint8_t state)
-{
-	struct hal_ev_health_app_reg_state ev;
-
-	DBG("");
-
-	ev.id = app->id;
-	ev.state = state;
-
-	ipc_send_notif(hal_ipc, HAL_SERVICE_ID_HEALTH,
-				HAL_EV_HEALTH_APP_REG_STATE, sizeof(ev), &ev);
-}
-
-static void send_channel_state_notify(struct health_channel *channel,
-						uint8_t state, int fd)
-{
-	struct hal_ev_health_channel_state ev;
-
-	DBG("");
-
-	bdaddr2android(&channel->dev->dst, ev.bdaddr);
-	ev.app_id = channel->dev->app_id;
-	ev.mdep_index = channel->mdep_id;
-	ev.channel_id = channel->id;
-	ev.channel_state = state;
-
-	ipc_send_notif_with_fd(hal_ipc, HAL_SERVICE_ID_HEALTH,
-					HAL_EV_HEALTH_CHANNEL_STATE,
-					sizeof(ev), &ev, fd);
-}
-
 static bool dev_by_addr(const void *data, const void *user_data)
 {
 	const struct health_device *dev = data;
-- 
1.9.1

[PATCH_v2 5/7] android/health: Create and connect MDL

[Ravi kumar Veeramally]

Request for md_create_mdl and on successful response
connect mdl and pass fd. First data channel should be reliable
data channel.
---
 android/health.c | 122 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 120 insertions(+), 2 deletions(-)

diff --git a/android/health.c b/android/health.c
index fff999d..ec5a413 100644
--- a/android/health.c
+++ b/android/health.c
@@ -87,6 +87,7 @@ struct health_device {
 
 	uint16_t ccpsm;
 	uint16_t dcpsm;
+	bool fr; /* first reliable channel */
 };
 
 struct health_channel {
@@ -146,6 +147,16 @@ static void send_channel_state_notify(struct health_channel *channel,
 					sizeof(ev), &ev, fd);
 }
 
+static void unref_mdl(struct health_channel *channel)
+{
+	if (!channel || !channel->mdl)
+		return;
+
+	mcap_mdl_unref(channel->mdl);
+	channel->mdl = NULL;
+	channel->mdl_conn = false;
+}
+
 static void free_health_channel(void *data)
 {
 	struct health_channel *channel = data;
@@ -153,6 +164,7 @@ static void free_health_channel(void *data)
 	if (!channel)
 		return;
 
+	unref_mdl(channel);
 	free(channel);
 }
 
@@ -1016,6 +1028,93 @@ static void mcap_mdl_reconn_req_cb(struct mcap_mdl *mdl, void *data)
 	DBG("Not Implemeneted");
 }
 
+static void connect_mdl_cb(struct mcap_mdl *mdl, GError *gerr, gpointer data)
+{
+	struct health_channel *channel = data;
+	int fd;
+
+	DBG("");
+
+	if (gerr) {
+		error("error connecting to MDL %s", gerr->message);
+		goto fail;
+	}
+
+	fd = mcap_mdl_get_fd(channel->mdl);
+	if (fd < 0) {
+		error("error retrieving fd");
+		goto fail;
+	}
+
+	DBG("MDL connected");
+	channel->mdl_conn = true;
+
+	/* first data channel should be reliable data channel */
+	if (!channel->dev->fr)
+		if (channel->type == CHANNEL_TYPE_RELIABLE)
+			channel->dev->fr = true;
+
+	send_channel_state_notify(channel, HAL_HEALTH_CHANNEL_CONNECTED, fd);
+
+	return;
+
+fail:
+	/* TODO: mcap_mdl_abort */
+	destroy_channel(channel);
+}
+
+static void create_mdl_cb(struct mcap_mdl *mdl, uint8_t type, GError *gerr,
+								gpointer data)
+{
+	struct health_channel *channel = data;
+	uint8_t mode;
+	GError *err = NULL;
+
+	DBG("");
+	if (gerr) {
+		error("error creating MDL %s", gerr->message);
+		goto fail;
+	}
+
+	if (channel->type == CHANNEL_TYPE_ANY && type != CHANNEL_TYPE_ANY)
+		channel->type = type;
+
+	/*
+	 * if requested channel type is not same as preferred
+	 * channel type from remote device, then abort the connection.
+	 */
+	if (channel->type != type) {
+		/* TODO: abort mdl */
+		error("abort, channel-type requested %d, preferred %d not same",
+							channel->type, type);
+		goto fail;
+	}
+
+	if (!channel->mdl)
+		channel->mdl = mcap_mdl_ref(mdl);
+
+	channel->type = type;
+	channel->mdl_id = mcap_mdl_get_mdlid(mdl);
+
+	if (channel->type == CHANNEL_TYPE_RELIABLE)
+		mode = L2CAP_MODE_ERTM;
+	else
+		mode = L2CAP_MODE_STREAMING;
+
+	if (!mcap_connect_mdl(channel->mdl, mode, channel->dev->dcpsm,
+						connect_mdl_cb, channel,
+						NULL, &err)) {
+		error("error connecting to mdl");
+		g_error_free(err);
+		goto fail;
+	}
+
+	return;
+
+fail:
+	destroy_channel(channel);
+}
+
 static bool check_role(uint8_t rec_role, uint8_t app_role)
 {
 	if ((rec_role == HAL_HEALTH_MDEP_ROLE_SINK &&
@@ -1078,7 +1177,8 @@ static void get_mdep_cb(sdp_list_t *recs, int err, gpointer user_data)
 	struct health_channel *channel = user_data;
 	struct health_app *app;
 	struct mdep_cfg *mdep;
-	uint8_t mdep_id;
+	uint8_t mdep_id, type;
+	GError *gerr = NULL;
 
 	if (err < 0 || !recs) {
 		error("error getting remote SDP records");
@@ -1102,7 +1202,18 @@ static void get_mdep_cb(sdp_list_t *recs, int err, gpointer user_data)
 
 	channel->remote_mdep = mdep_id;
 
-	/* TODO : create mdl */
+	if (mdep->role == HAL_HEALTH_MDEP_ROLE_SOURCE)
+		type = channel->type;
+	else
+		type = CHANNEL_TYPE_ANY;
+
+	if (!mcap_create_mdl(channel->dev->mcl, channel->remote_mdep,
+				type, create_mdl_cb, channel, NULL, &gerr)) {
+		error("error creating mdl %s", gerr->message);
+		g_error_free(gerr);
+		goto fail;
+	}
+
 	return;
 
 fail:
@@ -1321,6 +1432,13 @@ static void bt_health_connect_channel(const void *buf, uint16_t len)
 	if (!channel)
 		goto fail;
 
+	if (!dev->fr) {
+		if (channel->type != CHANNEL_TYPE_RELIABLE) {
+			error("error, first data shannel should be reliable");
+			goto fail;
+		}
+	}
+
 	if (!dev->mcl) {
 		if (connect_mcl(channel) < 0) {
 			error("error retrieving HDP SDP record");
-- 
1.9.1

[PATCH_v2 6/7] android/health: Implement mcap_mdl_deleted_cb

[Ravi kumar Veeramally]

mcap_mdl_deleted_cb will be called if remote device sends MDL_DELETE_REQ.
Free channel data and notify channel is destroyed.
---
 android/health.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/android/health.c b/android/health.c
index ec5a413..b379ee1 100644
--- a/android/health.c
+++ b/android/health.c
@@ -1007,9 +1007,30 @@ static void mcap_mdl_closed_cb(struct mcap_mdl *mdl, void *data)
 	DBG("Not Implemeneted");
 }
 
+static void notify_channel(void *data, void *user_data)
+{
+	struct health_channel *channel = data;
+
+	send_channel_state_notify(channel, HAL_HEALTH_CHANNEL_DESTROYED, -1);
+}
+
 static void mcap_mdl_deleted_cb(struct mcap_mdl *mdl, void *data)
 {
-	DBG("Not Implemeneted");
+	struct health_channel *channel = data;
+	struct health_device *dev;
+
+	DBG("");
+
+	dev = channel->dev;
+	/* mdl == NULL means, delete all mdls */
+	if (!mdl) {
+		queue_foreach(dev->channels, notify_channel, NULL);
+		queue_destroy(dev->channels, free_health_channel);
+		dev->channels = NULL;
+		return;
+	}
+
+	destroy_channel(channel);
 }
 
 static void mcap_mdl_aborted_cb(struct mcap_mdl *mdl, void *data)
-- 
1.9.1

[PATCH_v2 7/7] anrdroid/client/health: Cache fd and close on channel disconnection

[Ravi kumar Veeramally]

Cache fd and close them on channel disconnect or destroy notification.
When running PTS tests it is expecting to close all data channels before
exiting test case.
---
 android/client/if-hl.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/android/client/if-hl.c b/android/client/if-hl.c
index 557a205..5940526 100644
--- a/android/client/if-hl.c
+++ b/android/client/if-hl.c
@@ -17,6 +17,7 @@
 
 #include<stdio.h>
 #include<ctype.h>
+#include<unistd.h>
 
 #include<hardware/bluetooth.h>
 #include<hardware/bt_hl.h>
@@ -52,6 +53,7 @@ SINTMAP(bthl_channel_state_t, -1, "(unknown)")
 ENDMAP
 
 const bthl_interface_t *if_hl = NULL;
+static int fd_list[256] = {-1};
 
 static void app_reg_state_cb(int app_id, bthl_app_reg_state_t state)
 {
@@ -69,6 +71,13 @@ static void channel_state_cb(int app_id, bt_bdaddr_t *bd_addr,
 			"channel_id=%d channel_state=%s fd=%d\n", __func__,
 			app_id, bt_bdaddr_t2str(bd_addr, addr), mdep_cfg_index,
 			channel_id, bthl_channel_state_t2str(state), fd);
+
+	if (state == BTHL_CONN_STATE_CONNECTED)
+		fd_list[channel_id] = fd;
+
+	if (state == BTHL_CONN_STATE_DISCONNECTED ||
+			state == BTHL_CONN_STATE_DESTROYED)
+		close(fd_list[channel_id]);
 }
 
 static bthl_callbacks_t hl_cbacks = {
-- 
1.9.1

Re: [PATCH_v2 7/7] anrdroid/client/health: Cache fd and close on channel disconnection

[Sebastian Chlad]

Hi Ravi,

I was checking your patches against latest PTS and it seems I was able
to PASS some additional HDP tests.

Since patched are not upstreamed we should still run tests once git
tree is updated.

BR,
Sebastian

On 20 June 2014 14:23, Ravi kumar Veeramally
<ravikumar.veeramally@linux.intel.com> wrote:
> Cache fd and close them on channel disconnect or destroy notification.
> When running PTS tests it is expecting to close all data channels before
> exiting test case.
> ---
>  android/client/if-hl.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/android/client/if-hl.c b/android/client/if-hl.c
> index 557a205..5940526 100644
> --- a/android/client/if-hl.c
> +++ b/android/client/if-hl.c
> @@ -17,6 +17,7 @@
>
>  #include<stdio.h>
>  #include<ctype.h>
> +#include<unistd.h>
>
>  #include<hardware/bluetooth.h>
>  #include<hardware/bt_hl.h>
> @@ -52,6 +53,7 @@ SINTMAP(bthl_channel_state_t, -1, "(unknown)")
>  ENDMAP
>
>  const bthl_interface_t *if_hl = NULL;
> +static int fd_list[256] = {-1};
>
>  static void app_reg_state_cb(int app_id, bthl_app_reg_state_t state)
>  {
> @@ -69,6 +71,13 @@ static void channel_state_cb(int app_id, bt_bdaddr_t *bd_addr,
>                         "channel_id=%d channel_state=%s fd=%d\n", __func__,
>                         app_id, bt_bdaddr_t2str(bd_addr, addr), mdep_cfg_index,
>                         channel_id, bthl_channel_state_t2str(state), fd);
> +
> +       if (state == BTHL_CONN_STATE_CONNECTED)
> +               fd_list[channel_id] = fd;
> +
> +       if (state == BTHL_CONN_STATE_DISCONNECTED ||
> +                       state == BTHL_CONN_STATE_DESTROYED)
> +               close(fd_list[channel_id]);
>  }
>
>  static bthl_callbacks_t hl_cbacks = {
> --
> 1.9.1
>
> --
> 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



-- 
Seb/

Re: [PATCH_v2 1/7] android/health: Fix queue creation for mdeps and devices

[Szymon Janc]

Hi Ravi,

On Friday 20 of June 2014 15:23:30 Ravi kumar Veeramally wrote:
> Create queue for mdeps, devices and channels when creating app
> and device struct. It is simpler to read code than on demand
> creation.
> ---
>  android/health.c | 33 +++++++++++++--------------------
>  1 file changed, 13 insertions(+), 20 deletions(-)
> 
> diff --git a/android/health.c b/android/health.c
> index 0462e99..7553467 100644
> --- a/android/health.c
> +++ b/android/health.c
> @@ -678,6 +678,14 @@ static struct health_app *create_health_app(const char
> *app_name, goto fail;
>  	}
> 
> +	app->mdeps = queue_new();
> +	if (!app->mdeps)
> +		goto fail;
> +
> +	app->devices = queue_new();
> +	if (!app->devices)
> +		goto fail;
> +
>  	return app;
> 
>  fail:
> @@ -784,14 +792,6 @@ static void bt_health_mdep_cfg_data(const void *buf,
> uint16_t len) memcpy(mdep->descr, cmd->descr, cmd->descr_len);
>  	}
> 
> -	if (app->num_of_mdep > 0 && !app->mdeps) {
> -		app->mdeps = queue_new();
> -		if (!app->mdeps) {
> -			status = HAL_STATUS_FAILED;
> -			goto fail;
> -		}
> -	}
> -
>  	if (!queue_push_tail(app->mdeps, mdep)) {
>  		status = HAL_STATUS_FAILED;
>  		goto fail;
> @@ -1102,6 +1102,11 @@ static struct health_device *create_device(uint16_t
> app_id, const uint8_t *addr)
> 
>  	android2bdaddr(addr, &dev->dst);
>  	dev->app_id = app_id;
> +	dev->channels = queue_new();
> +	if (!dev->channels) {
> +		free_health_device(dev);
> +		return NULL;
> +	}
> 
>  	return dev;
>  }
> @@ -1159,21 +1164,9 @@ static void bt_health_connect_channel(const void
> *buf, uint16_t len)
> 
>  	channel->dev = dev;
> 
> -	if (!app->devices) {
> -		app->devices = queue_new();
> -		if (!app->devices)
> -			goto fail;
> -	}
> -
>  	if (!queue_push_tail(app->devices, dev))
>  		goto fail;
> 
> -	if (!dev->channels) {
> -		dev->channels = queue_new();
> -		if (!dev->channels)
> -			goto fail;
> -	}
> -
>  	if (!queue_push_tail(dev->channels, channel)) {
>  		queue_remove(app->devices, dev);
>  		goto fail;

This patch is now applied, thanks.

-- 
BR
Szymon Janc

Re: [PATCH_v2 2/7] android/health: Check if device and channel already exists or not

[Szymon Janc]

Hi Ravi,

On Friday 20 of June 2014 15:23:31 Ravi kumar Veeramally wrote:
> On channel connect request, check if device is already exists or not.
> Also check if channel is already created for remote device or not.
> ---
>  android/health.c | 83
> ++++++++++++++++++++++++++++++++++++++++---------------- 1 file changed, 59
> insertions(+), 24 deletions(-)
> 
> diff --git a/android/health.c b/android/health.c
> index 7553467..083ab1e 100644
> --- a/android/health.c
> +++ b/android/health.c
> @@ -213,6 +213,22 @@ static void send_channel_state_notify(struct
> health_channel *channel, sizeof(ev), &ev, fd);
>  }
> 
> +static bool dev_by_addr(const void *data, const void *user_data)
> +{
> +	const struct health_device *dev = data;
> +	const bdaddr_t *addr = user_data;
> +
> +	return !bacmp(&dev->dst, addr);
> +}
> +
> +static bool channel_by_mdep_id(const void *data, const void *user_data)
> +{
> +	const struct health_channel *channel = data;
> +	uint16_t mdep_id = PTR_TO_INT(user_data);
> +
> +	return channel->mdep_id == mdep_id;
> +}
> +
>  static bool mdep_by_mdep_role(const void *data, const void *user_data)
>  {
>  	const struct mdep_cfg *mdep = data;
> @@ -1094,25 +1110,44 @@ static int connect_mcl(struct health_channel
> *channel)
> 
>  static struct health_device *create_device(uint16_t app_id, const uint8_t
> *addr) {
> +	struct health_app *app;
>  	struct health_device *dev;
> +	bdaddr_t bdaddr;
> 
> +	app = queue_find(apps, app_by_app_id, INT_TO_PTR(app_id));
> +	if (!app)
> +		return NULL;
> +
> +	android2bdaddr(addr, &bdaddr);
> +	dev = queue_find(app->devices, dev_by_addr, &bdaddr);
> +	if (dev)
> +		return dev;
> +

I'd rather have create_device() to just create device and add wrapper 
get_device() that would call find + create when needed .

> +	/* create device and push it to devices queue */
>  	dev = new0(struct health_device, 1);
>  	if (!dev)
>  		return NULL;
> 
>  	android2bdaddr(addr, &dev->dst);
>  	dev->app_id = app_id;
> +
>  	dev->channels = queue_new();
>  	if (!dev->channels) {
>  		free_health_device(dev);
>  		return NULL;
>  	}
> 
> +	if (!queue_push_tail(app->devices, dev)) {
> +		free_health_device(dev);
> +		return NULL;
> +	}
> +
>  	return dev;
>  }
> 
>  static struct health_channel *create_channel(uint16_t app_id,
> -						uint8_t mdep_index)
> +						uint8_t mdep_index,
> +						struct health_device *dev)
>  {
>  	struct health_app *app;
>  	struct mdep_cfg *mdep;
> @@ -1120,22 +1155,37 @@ static struct health_channel
> *create_channel(uint16_t app_id, uint8_t index;
>  	static unsigned int channel_id = 1;
> 
> +	if (!dev)
> +		return NULL;
> +
> +	index = mdep_index + 1;
> +	channel = queue_find(dev->channels, channel_by_mdep_id,
> +							INT_TO_PTR(index));
> +	if (channel)
> +		return channel;

Same here.

> +
>  	app = queue_find(apps, app_by_app_id, INT_TO_PTR(app_id));
>  	if (!app)
>  		return NULL;
> 
> -	index = mdep_index + 1;
>  	mdep = queue_find(app->mdeps, mdep_by_mdep_id, INT_TO_PTR(index));
>  	if (!mdep)
>  		return NULL;
> 
> +	/* create channel and push it to device */
>  	channel = new0(struct health_channel, 1);
>  	if (!channel)
>  		return NULL;
> 
> -	channel->mdep_id = mdep_index;
> +	channel->mdep_id = mdep->id;
>  	channel->type = mdep->channel_type;
>  	channel->id = channel_id++;
> +	channel->dev = dev;
> +
> +	if (!queue_push_tail(dev->channels, channel)) {
> +		free_health_channel(channel);
> +		return NULL;
> +	}
> 
>  	return channel;
>  }
> @@ -1144,38 +1194,24 @@ static void bt_health_connect_channel(const void
> *buf, uint16_t len) {
>  	const struct hal_cmd_health_connect_channel *cmd = buf;
>  	struct hal_rsp_health_connect_channel rsp;
> -	struct health_app *app;
>  	struct health_device *dev = NULL;
>  	struct health_channel *channel = NULL;
> 
>  	DBG("");
> 
> -	app = queue_find(apps, app_by_app_id, INT_TO_PTR(cmd->app_id));
> -	if (!app)
> -		goto fail;
> -
>  	dev = create_device(cmd->app_id, cmd->bdaddr);
>  	if (!dev)
>  		goto fail;
> 
> -	channel = create_channel(cmd->app_id, cmd->mdep_index);
> +	channel = create_channel(cmd->app_id, cmd->mdep_index, dev);
>  	if (!channel)
>  		goto fail;
> 
> -	channel->dev = dev;
> -
> -	if (!queue_push_tail(app->devices, dev))
> -		goto fail;
> -
> -	if (!queue_push_tail(dev->channels, channel)) {
> -		queue_remove(app->devices, dev);
> -		goto fail;
> -	}
> -
> -	if (connect_mcl(channel) < 0) {
> -		error("error retrieving HDP SDP record");
> -		queue_remove(app->devices, dev);
> -		goto fail;
> +	if (!dev->mcl) {
> +		if (connect_mcl(channel) < 0) {
> +			error("error retrieving HDP SDP record");
> +			goto fail;
> +		}
>  	}
> 
>  	rsp.channel_id = channel->id;
> @@ -1186,7 +1222,6 @@ static void bt_health_connect_channel(const void *buf,
> uint16_t len)
> 
>  fail:
>  	free_health_channel(channel);
> -	free_health_device(dev);

So command failed and yet new device is on app->devices list?

>  	ipc_send_rsp(hal_ipc, HAL_SERVICE_ID_HEALTH,
>  			HAL_OP_HEALTH_CONNECT_CHANNEL, HAL_STATUS_FAILED);
>  }


Also a general note: please prefix all info() and error() messages with 
"health: ".

-- 
BR
Szymon Janc

Re: [PATCH_v2 5/7] android/health: Create and connect MDL

[Szymon Janc]

Hi Ravi,

On Friday 20 of June 2014 15:23:34 Ravi kumar Veeramally wrote:
> Request for md_create_mdl and on successful response
> connect mdl and pass fd. First data channel should be reliable
> data channel.
> ---
>  android/health.c | 122
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 120
> insertions(+), 2 deletions(-)
> 
> diff --git a/android/health.c b/android/health.c
> index fff999d..ec5a413 100644
> --- a/android/health.c
> +++ b/android/health.c
> @@ -87,6 +87,7 @@ struct health_device {
> 
>  	uint16_t ccpsm;
>  	uint16_t dcpsm;
> +	bool fr; /* first reliable channel */

Is this really needed? Couldn't this be decided based on eg. dev->channels 
queue length?

>  };
> 
>  struct health_channel {
> @@ -146,6 +147,16 @@ static void send_channel_state_notify(struct
> health_channel *channel, sizeof(ev), &ev, fd);
>  }
> 
> +static void unref_mdl(struct health_channel *channel)
> +{
> +	if (!channel || !channel->mdl)
> +		return;
> +
> +	mcap_mdl_unref(channel->mdl);
> +	channel->mdl = NULL;
> +	channel->mdl_conn = false;
> +}
> +
>  static void free_health_channel(void *data)
>  {
>  	struct health_channel *channel = data;
> @@ -153,6 +164,7 @@ static void free_health_channel(void *data)
>  	if (!channel)
>  		return;
> 
> +	unref_mdl(channel);
>  	free(channel);
>  }
> 
> @@ -1016,6 +1028,93 @@ static void mcap_mdl_reconn_req_cb(struct mcap_mdl
> *mdl, void *data) DBG("Not Implemeneted");
>  }
> 
> +static void connect_mdl_cb(struct mcap_mdl *mdl, GError *gerr, gpointer
> data) +{
> +	struct health_channel *channel = data;
> +	int fd;
> +
> +	DBG("");
> +
> +	if (gerr) {
> +		error("error connecting to MDL %s", gerr->message);
> +		goto fail;
> +	}
> +
> +	fd = mcap_mdl_get_fd(channel->mdl);
> +	if (fd < 0) {
> +		error("error retrieving fd");
> +		goto fail;
> +	}
> +
> +	DBG("MDL connected");
> +	channel->mdl_conn = true;
> +
> +	/* first data channel should be reliable data channel */
> +	if (!channel->dev->fr)
> +		if (channel->type == CHANNEL_TYPE_RELIABLE)
> +			channel->dev->fr = true;

If first isn't reliable shouldn't this be an error?

> +
> +	send_channel_state_notify(channel, HAL_HEALTH_CHANNEL_CONNECTED, fd);
> +
> +	return;
> +
> +fail:
> +	/* TODO: mcap_mdl_abort */
> +	destroy_channel(channel);
> +}
> +
> +static void create_mdl_cb(struct mcap_mdl *mdl, uint8_t type, GError *gerr,
> +								gpointer data)
> +{
> +	struct health_channel *channel = data;
> +	uint8_t mode;
> +	GError *err = NULL;
> +
> +	DBG("");
> +	if (gerr) {
> +		error("error creating MDL %s", gerr->message);
> +		goto fail;
> +	}
> +
> +	if (channel->type == CHANNEL_TYPE_ANY && type != CHANNEL_TYPE_ANY)
> +		channel->type = type;
> +
> +	/*
> +	 * if requested channel type is not same as preferred
> +	 * channel type from remote device, then abort the connection.
> +	 */
> +	if (channel->type != type) {
> +		/* TODO: abort mdl */
> +		error("abort, channel-type requested %d, preferred %d not same",
> +							channel->type, type);
> +		goto fail;
> +	}
> +
> +	if (!channel->mdl)
> +		channel->mdl = mcap_mdl_ref(mdl);
> +
> +	channel->type = type;
> +	channel->mdl_id = mcap_mdl_get_mdlid(mdl);
> +
> +	if (channel->type == CHANNEL_TYPE_RELIABLE)
> +		mode = L2CAP_MODE_ERTM;
> +	else
> +		mode = L2CAP_MODE_STREAMING;
> +
> +	if (!mcap_connect_mdl(channel->mdl, mode, channel->dev->dcpsm,
> +						connect_mdl_cb, channel,
> +						NULL, &err)) {
> +		error("error connecting to mdl");
> +		g_error_free(err);
> +		goto fail;
> +	}
> +
> +	return;
> +
> +fail:
> +	destroy_channel(channel);
> +}
> +
>  static bool check_role(uint8_t rec_role, uint8_t app_role)
>  {
>  	if ((rec_role == HAL_HEALTH_MDEP_ROLE_SINK &&
> @@ -1078,7 +1177,8 @@ static void get_mdep_cb(sdp_list_t *recs, int err,
> gpointer user_data) struct health_channel *channel = user_data;
>  	struct health_app *app;
>  	struct mdep_cfg *mdep;
> -	uint8_t mdep_id;
> +	uint8_t mdep_id, type;
> +	GError *gerr = NULL;
> 
>  	if (err < 0 || !recs) {
>  		error("error getting remote SDP records");
> @@ -1102,7 +1202,18 @@ static void get_mdep_cb(sdp_list_t *recs, int err,
> gpointer user_data)
> 
>  	channel->remote_mdep = mdep_id;
> 
> -	/* TODO : create mdl */
> +	if (mdep->role == HAL_HEALTH_MDEP_ROLE_SOURCE)
> +		type = channel->type;
> +	else
> +		type = CHANNEL_TYPE_ANY;
> +
> +	if (!mcap_create_mdl(channel->dev->mcl, channel->remote_mdep,
> +				type, create_mdl_cb, channel, NULL, &gerr)) {
> +		error("error creating mdl %s", gerr->message);
> +		g_error_free(gerr);
> +		goto fail;
> +	}
> +
>  	return;
> 
>  fail:
> @@ -1321,6 +1432,13 @@ static void bt_health_connect_channel(const void
> *buf, uint16_t len) if (!channel)
>  		goto fail;
> 
> +	if (!dev->fr) {
> +		if (channel->type != CHANNEL_TYPE_RELIABLE) {
> +			error("error, first data shannel should be reliable");
> +			goto fail;
> +		}
> +	}
> +
>  	if (!dev->mcl) {
>  		if (connect_mcl(channel) < 0) {
>  			error("error retrieving HDP SDP record");

-- 
BR
Szymon Janc

Re: [PATCH_v2 2/7] android/health: Check if device and channel already exists or not

[Ravi kumar Veeramally]

Hi Szymon,

On 06/21/2014 04:49 PM, Szymon Janc wrote:
> Hi Ravi,
>
> On Friday 20 of June 2014 15:23:31 Ravi kumar Veeramally wrote:
>> On channel connect request, check if device is already exists or not.
>> Also check if channel is already created for remote device or not.
>> ---
>>   android/health.c | 83
>> ++++++++++++++++++++++++++++++++++++++++---------------- 1 file changed, 59
>> insertions(+), 24 deletions(-)
>>
>> diff --git a/android/health.c b/android/health.c
>> index 7553467..083ab1e 100644
>> --- a/android/health.c
>> +++ b/android/health.c
>> @@ -213,6 +213,22 @@ static void send_channel_state_notify(struct
>> health_channel *channel, sizeof(ev), &ev, fd);
>>   }
>>
>> +static bool dev_by_addr(const void *data, const void *user_data)
>> +{
>> +	const struct health_device *dev = data;
>> +	const bdaddr_t *addr = user_data;
>> +
>> +	return !bacmp(&dev->dst, addr);
>> +}
>> +
>> +static bool channel_by_mdep_id(const void *data, const void *user_data)
>> +{
>> +	const struct health_channel *channel = data;
>> +	uint16_t mdep_id = PTR_TO_INT(user_data);
>> +
>> +	return channel->mdep_id == mdep_id;
>> +}
>> +
>>   static bool mdep_by_mdep_role(const void *data, const void *user_data)
>>   {
>>   	const struct mdep_cfg *mdep = data;
>> @@ -1094,25 +1110,44 @@ static int connect_mcl(struct health_channel
>> *channel)
>>
>>   static struct health_device *create_device(uint16_t app_id, const uint8_t
>> *addr) {
>> +	struct health_app *app;
>>   	struct health_device *dev;
>> +	bdaddr_t bdaddr;
>>
>> +	app = queue_find(apps, app_by_app_id, INT_TO_PTR(app_id));
>> +	if (!app)
>> +		return NULL;
>> +
>> +	android2bdaddr(addr, &bdaddr);
>> +	dev = queue_find(app->devices, dev_by_addr, &bdaddr);
>> +	if (dev)
>> +		return dev;
>> +
> I'd rather have create_device() to just create device and add wrapper
> get_device() that would call find + create when needed .
   Ok.
>> +	/* create device and push it to devices queue */
>>   	dev = new0(struct health_device, 1);
>>   	if (!dev)
>>   		return NULL;
>>
>>   	android2bdaddr(addr, &dev->dst);
>>   	dev->app_id = app_id;
>> +
>>   	dev->channels = queue_new();
>>   	if (!dev->channels) {
>>   		free_health_device(dev);
>>   		return NULL;
>>   	}
>>
>> +	if (!queue_push_tail(app->devices, dev)) {
>> +		free_health_device(dev);
>> +		return NULL;
>> +	}
>> +
>>   	return dev;
>>   }
>>
>>   static struct health_channel *create_channel(uint16_t app_id,
>> -						uint8_t mdep_index)
>> +						uint8_t mdep_index,
>> +						struct health_device *dev)
>>   {
>>   	struct health_app *app;
>>   	struct mdep_cfg *mdep;
>> @@ -1120,22 +1155,37 @@ static struct health_channel
>> *create_channel(uint16_t app_id, uint8_t index;
>>   	static unsigned int channel_id = 1;
>>
>> +	if (!dev)
>> +		return NULL;
>> +
>> +	index = mdep_index + 1;
>> +	channel = queue_find(dev->channels, channel_by_mdep_id,
>> +							INT_TO_PTR(index));
>> +	if (channel)
>> +		return channel;
> Same here.
  Ok.
>> +
>>   	app = queue_find(apps, app_by_app_id, INT_TO_PTR(app_id));
>>   	if (!app)
>>   		return NULL;
>>
>> -	index = mdep_index + 1;
>>   	mdep = queue_find(app->mdeps, mdep_by_mdep_id, INT_TO_PTR(index));
>>   	if (!mdep)
>>   		return NULL;
>>
>> +	/* create channel and push it to device */
>>   	channel = new0(struct health_channel, 1);
>>   	if (!channel)
>>   		return NULL;
>>
>> -	channel->mdep_id = mdep_index;
>> +	channel->mdep_id = mdep->id;
>>   	channel->type = mdep->channel_type;
>>   	channel->id = channel_id++;
>> +	channel->dev = dev;
>> +
>> +	if (!queue_push_tail(dev->channels, channel)) {
>> +		free_health_channel(channel);
>> +		return NULL;
>> +	}
>>
>>   	return channel;
>>   }
>> @@ -1144,38 +1194,24 @@ static void bt_health_connect_channel(const void
>> *buf, uint16_t len) {
>>   	const struct hal_cmd_health_connect_channel *cmd = buf;
>>   	struct hal_rsp_health_connect_channel rsp;
>> -	struct health_app *app;
>>   	struct health_device *dev = NULL;
>>   	struct health_channel *channel = NULL;
>>
>>   	DBG("");
>>
>> -	app = queue_find(apps, app_by_app_id, INT_TO_PTR(cmd->app_id));
>> -	if (!app)
>> -		goto fail;
>> -
>>   	dev = create_device(cmd->app_id, cmd->bdaddr);
>>   	if (!dev)
>>   		goto fail;
>>
>> -	channel = create_channel(cmd->app_id, cmd->mdep_index);
>> +	channel = create_channel(cmd->app_id, cmd->mdep_index, dev);
>>   	if (!channel)
>>   		goto fail;
>>
>> -	channel->dev = dev;
>> -
>> -	if (!queue_push_tail(app->devices, dev))
>> -		goto fail;
>> -
>> -	if (!queue_push_tail(dev->channels, channel)) {
>> -		queue_remove(app->devices, dev);
>> -		goto fail;
>> -	}
>> -
>> -	if (connect_mcl(channel) < 0) {
>> -		error("error retrieving HDP SDP record");
>> -		queue_remove(app->devices, dev);
>> -		goto fail;
>> +	if (!dev->mcl) {
>> +		if (connect_mcl(channel) < 0) {
>> +			error("error retrieving HDP SDP record");
>> +			goto fail;
>> +		}
>>   	}
>>
>>   	rsp.channel_id = channel->id;
>> @@ -1186,7 +1222,6 @@ static void bt_health_connect_channel(const void *buf,
>> uint16_t len)
>>
>>   fail:
>>   	free_health_channel(channel);
>> -	free_health_device(dev);
> So command failed and yet new device is on app->devices list?
   yes, connect_channel api will be called to connect multiple data channels
   on same device. i.e. if first data channel is already connected and 
second
  channel connection fails, freeing whole device is bug.
>>   	ipc_send_rsp(hal_ipc, HAL_SERVICE_ID_HEALTH,
>>   			HAL_OP_HEALTH_CONNECT_CHANNEL, HAL_STATUS_FAILED);
>>   }
>
> Also a general note: please prefix all info() and error() messages with
> "health: ".
>
   OK.

Thanks,
Ravi.

Re: [PATCH_v2 5/7] android/health: Create and connect MDL

[Ravi kumar Veeramally]

Hi Szymon,

On 06/21/2014 11:19 PM, Szymon Janc wrote:
> Hi Ravi,
>
> On Friday 20 of June 2014 15:23:34 Ravi kumar Veeramally wrote:
>> Request for md_create_mdl and on successful response
>> connect mdl and pass fd. First data channel should be reliable
>> data channel.
>> ---
>>   android/health.c | 122
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 120
>> insertions(+), 2 deletions(-)
>>
>> diff --git a/android/health.c b/android/health.c
>> index fff999d..ec5a413 100644
>> --- a/android/health.c
>> +++ b/android/health.c
>> @@ -87,6 +87,7 @@ struct health_device {
>>
>>   	uint16_t ccpsm;
>>   	uint16_t dcpsm;
>> +	bool fr; /* first reliable channel */
> Is this really needed? Couldn't this be decided based on eg. dev->channels
> queue length?
   Yes, make sense.
>>   };
>>
>>   struct health_channel {
>> @@ -146,6 +147,16 @@ static void send_channel_state_notify(struct
>> health_channel *channel, sizeof(ev), &ev, fd);
>>   }
>>
>> +static void unref_mdl(struct health_channel *channel)
>> +{
>> +	if (!channel || !channel->mdl)
>> +		return;
>> +
>> +	mcap_mdl_unref(channel->mdl);
>> +	channel->mdl = NULL;
>> +	channel->mdl_conn = false;
>> +}
>> +
>>   static void free_health_channel(void *data)
>>   {
>>   	struct health_channel *channel = data;
>> @@ -153,6 +164,7 @@ static void free_health_channel(void *data)
>>   	if (!channel)
>>   		return;
>>
>> +	unref_mdl(channel);
>>   	free(channel);
>>   }
>>
>> @@ -1016,6 +1028,93 @@ static void mcap_mdl_reconn_req_cb(struct mcap_mdl
>> *mdl, void *data) DBG("Not Implemeneted");
>>   }
>>
>> +static void connect_mdl_cb(struct mcap_mdl *mdl, GError *gerr, gpointer
>> data) +{
>> +	struct health_channel *channel = data;
>> +	int fd;
>> +
>> +	DBG("");
>> +
>> +	if (gerr) {
>> +		error("error connecting to MDL %s", gerr->message);
>> +		goto fail;
>> +	}
>> +
>> +	fd = mcap_mdl_get_fd(channel->mdl);
>> +	if (fd < 0) {
>> +		error("error retrieving fd");
>> +		goto fail;
>> +	}
>> +
>> +	DBG("MDL connected");
>> +	channel->mdl_conn = true;
>> +
>> +	/* first data channel should be reliable data channel */
>> +	if (!channel->dev->fr)
>> +		if (channel->type == CHANNEL_TYPE_RELIABLE)
>> +			channel->dev->fr = true;
> If first isn't reliable shouldn't this be an error?
  yes, I will add it.
>> +
>> +	send_channel_state_notify(channel, HAL_HEALTH_CHANNEL_CONNECTED, fd);
>> +
>> +	return;
>> +
>> +fail:
>> +	/* TODO: mcap_mdl_abort */
>> +	destroy_channel(channel);
>> +}
>> +
>> +static void create_mdl_cb(struct mcap_mdl *mdl, uint8_t type, GError *gerr,
>> +								gpointer data)
>> +{
>> +	struct health_channel *channel = data;
>> +	uint8_t mode;
>> +	GError *err = NULL;
>> +
>> +	DBG("");
>> +	if (gerr) {
>> +		error("error creating MDL %s", gerr->message);
>> +		goto fail;
>> +	}
>> +
>> +	if (channel->type == CHANNEL_TYPE_ANY && type != CHANNEL_TYPE_ANY)
>> +		channel->type = type;
>> +
>> +	/*
>> +	 * if requested channel type is not same as preferred
>> +	 * channel type from remote device, then abort the connection.
>> +	 */
>> +	if (channel->type != type) {
>> +		/* TODO: abort mdl */
>> +		error("abort, channel-type requested %d, preferred %d not same",
>> +							channel->type, type);
>> +		goto fail;
>> +	}
>> +
>> +	if (!channel->mdl)
>> +		channel->mdl = mcap_mdl_ref(mdl);
>> +
>> +	channel->type = type;
>> +	channel->mdl_id = mcap_mdl_get_mdlid(mdl);
>> +
>> +	if (channel->type == CHANNEL_TYPE_RELIABLE)
>> +		mode = L2CAP_MODE_ERTM;
>> +	else
>> +		mode = L2CAP_MODE_STREAMING;
>> +
>> +	if (!mcap_connect_mdl(channel->mdl, mode, channel->dev->dcpsm,
>> +						connect_mdl_cb, channel,
>> +						NULL, &err)) {
>> +		error("error connecting to mdl");
>> +		g_error_free(err);
>> +		goto fail;
>> +	}
>> +
>> +	return;
>> +
>> +fail:
>> +	destroy_channel(channel);
>> +}
>> +
>>   static bool check_role(uint8_t rec_role, uint8_t app_role)
>>   {
>>   	if ((rec_role == HAL_HEALTH_MDEP_ROLE_SINK &&
>> @@ -1078,7 +1177,8 @@ static void get_mdep_cb(sdp_list_t *recs, int err,
>> gpointer user_data) struct health_channel *channel = user_data;
>>   	struct health_app *app;
>>   	struct mdep_cfg *mdep;
>> -	uint8_t mdep_id;
>> +	uint8_t mdep_id, type;
>> +	GError *gerr = NULL;
>>
>>   	if (err < 0 || !recs) {
>>   		error("error getting remote SDP records");
>> @@ -1102,7 +1202,18 @@ static void get_mdep_cb(sdp_list_t *recs, int err,
>> gpointer user_data)
>>
>>   	channel->remote_mdep = mdep_id;
>>
>> -	/* TODO : create mdl */
>> +	if (mdep->role == HAL_HEALTH_MDEP_ROLE_SOURCE)
>> +		type = channel->type;
>> +	else
>> +		type = CHANNEL_TYPE_ANY;
>> +
>> +	if (!mcap_create_mdl(channel->dev->mcl, channel->remote_mdep,
>> +				type, create_mdl_cb, channel, NULL, &gerr)) {
>> +		error("error creating mdl %s", gerr->message);
>> +		g_error_free(gerr);
>> +		goto fail;
>> +	}
>> +
>>   	return;
>>
>>   fail:
>> @@ -1321,6 +1432,13 @@ static void bt_health_connect_channel(const void
>> *buf, uint16_t len) if (!channel)
>>   		goto fail;
>>
>> +	if (!dev->fr) {
>> +		if (channel->type != CHANNEL_TYPE_RELIABLE) {
>> +			error("error, first data shannel should be reliable");
>> +			goto fail;
>> +		}
>> +	}
>> +
>>   	if (!dev->mcl) {
>>   		if (connect_mcl(channel) < 0) {
>>   			error("error retrieving HDP SDP record");
Thanks,
Ravi.