[PATCH_v2 0/4] Intiates L2CAP Control Channel

[PATCH_v2 0/4] Intiates L2CAP Control Channel

[Ravi kumar Veeramally]

v2: Fixed Szymon's comments (provided utility to convert
  wrong HAL channel type defines, minor issues).

v1: Patchset is initial implementaion for MCL connection. It can
  be tested with PTS, register application in source or sink role
  and run source or sink L2CAP control channel tests. Initiate
  channel connection from haltest, only MCAP MCL gets connected.

Ravi kumar Veeramally (4):
  profiles/health/hdp: Fix memory leak in SDP record preparati     Sdp
    record preparation part is copied to android/health.c from    
    profiles/health/hdp_utils.c. Memory leak is noticed while    
    testing. Memory summay is from android daemon, but code snippet    
    is same. It is already fixed in android/health.c while submitting   
     android related patches.
  anrdoid/health: Fix wrong channel type defines
  android/health: Initial connect channel implementation
  android/health: Notify channel connection status

 android/health.c           | 451 ++++++++++++++++++++++++++++++++++++++++++++-
 profiles/health/hdp_util.c |   2 +
 2 files changed, 450 insertions(+), 3 deletions(-)

-- 
1.9.1

[PATCH_v2 1/4] profiles/health/hdp: Fix memory leak in SDP record preparati Sdp record preparation part is copied to android/health.c from profiles/health/hdp_utils.c. Memory leak is noticed while testing. Memory summay is from android daemon, but code snippet is same. It is already fixed in android/health.c while submitting android related patches.

[Ravi kumar Veeramally]

==12515== 286 (16 direct, 270 indirect) bytes in 1 blocks are definitely lost in loss record 158 of 165
==12515==    at 0x4C2AB80: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==12515==    by 0x44AC45: sdp_list_append (sdp.c:1743)
==12515==    by 0x4398F9: register_features (health.c:381)
==12515==    by 0x4091CC: queue_foreach (queue.c:219)
==12515==    by 0x43A31F: bt_health_mdep_cfg_data (health.c:398)
==12515==    by 0x418B50: cmd_watch_cb (ipc.c:144)
==12515==    by 0x4E7FCE4: g_main_context_dispatch (in /lib/x86_64-linux-gnu/libglib-2.0.so.0.4000.0)
==12515==    by 0x4E80047: ??? (in /lib/x86_64-linux-gnu/libglib-2.0.so.0.4000.0)
==12515==    by 0x4E80309: g_main_loop_run (in /lib/x86_64-linux-gnu/libglib-2.0.so.0.4000.0)
==12515==    by 0x4044F2: main (main.c:538)
---
 profiles/health/hdp_util.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/profiles/health/hdp_util.c b/profiles/health/hdp_util.c
index 58770b5..47c464e 100644
--- a/profiles/health/hdp_util.c
+++ b/profiles/health/hdp_util.c
@@ -652,6 +652,8 @@ static gboolean register_service_sup_features(GSList *app_list,
 		return FALSE;
 	}
 
+	sdp_list_free(sup_features, free_hdp_list);
+
 	return TRUE;
 }
 
-- 
1.9.1

Re: [PATCH_v2 1/4] profiles/health/hdp: Fix memory leak in SDP record preparati Sdp record preparation part is copied to android/health.c from profiles/health/hdp_utils.c. Memory leak is noticed while testing. Memory summay is from android daemon, but code snippet is same. It is already fixed in android/health.c while submitting android related patches.

[Szymon Janc]

Hi Ravi,

On Tuesday 17 of June 2014 01:02:47 Ravi kumar Veeramally wrote:
> ==12515== 286 (16 direct, 270 indirect) bytes in 1 blocks are definitely lost in loss record 158 of 165
> ==12515==    at 0x4C2AB80: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==12515==    by 0x44AC45: sdp_list_append (sdp.c:1743)
> ==12515==    by 0x4398F9: register_features (health.c:381)
> ==12515==    by 0x4091CC: queue_foreach (queue.c:219)
> ==12515==    by 0x43A31F: bt_health_mdep_cfg_data (health.c:398)
> ==12515==    by 0x418B50: cmd_watch_cb (ipc.c:144)
> ==12515==    by 0x4E7FCE4: g_main_context_dispatch (in /lib/x86_64-linux-gnu/libglib-2.0.so.0.4000.0)
> ==12515==    by 0x4E80047: ??? (in /lib/x86_64-linux-gnu/libglib-2.0.so.0.4000.0)
> ==12515==    by 0x4E80309: g_main_loop_run (in /lib/x86_64-linux-gnu/libglib-2.0.so.0.4000.0)
> ==12515==    by 0x4044F2: main (main.c:538)
> ---

There is something wrong with this patch. Subject is very long. Should that be
in commit message body?

>  profiles/health/hdp_util.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/profiles/health/hdp_util.c b/profiles/health/hdp_util.c
> index 58770b5..47c464e 100644
> --- a/profiles/health/hdp_util.c
> +++ b/profiles/health/hdp_util.c
> @@ -652,6 +652,8 @@ static gboolean register_service_sup_features(GSList *app_list,
>  		return FALSE;
>  	}
>  
> +	sdp_list_free(sup_features, free_hdp_list);
> +
>  	return TRUE;
>  }
>  
> 

-- 
Best regards, 
Szymon Janc

Re: [PATCH_v2 1/4] profiles/health/hdp: Fix memory leak in SDP record preparati Sdp record preparation part is copied to android/health.c from profiles/health/hdp_utils.c. Memory leak is noticed while testing. Memory summay is from android daemon, but code snippet is same. It is already fixed in android/health.c while submitting android related patches.

[Ravi kumar Veeramally]

Hi Szymon,

On 06/17/2014 12:29 PM, Szymon Janc wrote:
> Hi Ravi,
>
> On Tuesday 17 of June 2014 01:02:47 Ravi kumar Veeramally wrote:
>> ==12515== 286 (16 direct, 270 indirect) bytes in 1 blocks are definitely lost in loss record 158 of 165
>> ==12515==    at 0x4C2AB80: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
>> ==12515==    by 0x44AC45: sdp_list_append (sdp.c:1743)
>> ==12515==    by 0x4398F9: register_features (health.c:381)
>> ==12515==    by 0x4091CC: queue_foreach (queue.c:219)
>> ==12515==    by 0x43A31F: bt_health_mdep_cfg_data (health.c:398)
>> ==12515==    by 0x418B50: cmd_watch_cb (ipc.c:144)
>> ==12515==    by 0x4E7FCE4: g_main_context_dispatch (in /lib/x86_64-linux-gnu/libglib-2.0.so.0.4000.0)
>> ==12515==    by 0x4E80047: ??? (in /lib/x86_64-linux-gnu/libglib-2.0.so.0.4000.0)
>> ==12515==    by 0x4E80309: g_main_loop_run (in /lib/x86_64-linux-gnu/libglib-2.0.so.0.4000.0)
>> ==12515==    by 0x4044F2: main (main.c:538)
>> ---
> There is something wrong with this patch. Subject is very long. Should that be
> in commit message body?
   Oops, I will correct it.
>>   profiles/health/hdp_util.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/profiles/health/hdp_util.c b/profiles/health/hdp_util.c
>> index 58770b5..47c464e 100644
>> --- a/profiles/health/hdp_util.c
>> +++ b/profiles/health/hdp_util.c
>> @@ -652,6 +652,8 @@ static gboolean register_service_sup_features(GSList *app_list,
>>   		return FALSE;
>>   	}
>>   
>> +	sdp_list_free(sup_features, free_hdp_list);
>> +
>>   	return TRUE;
>>   }
>>   
>>
  Thanks,
  Ravi.

[PATCH_v2 2/4] anrdoid/health: Fix wrong channel type defines

[Ravi kumar Veeramally]

Enums in bt_hl.h are defined in this order BTHL_CHANNEL_TYPE_RELIABLE,
BTHL_CHANNEL_TYPE_STREAMING, BTHL_CHANNEL_TYPE_ANY. But HDP_SPEC_V11(3.4)
has no-preference(any)-0, reliable-1, stream-2, reserved(0x03-0xff).
Providing utility to solve this.
---
 android/health.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/android/health.c b/android/health.c
index 7e2c5d6..5a87089 100644
--- a/android/health.c
+++ b/android/health.c
@@ -52,6 +52,10 @@
 #define HDP_VERSION			0x0101
 #define DATA_EXCHANGE_SPEC_11073	0x01
 
+#define CHANNEL_TYPE_ANY       0x00
+#define CHANNEL_TYPE_RELIABLE  0x01
+#define CHANNEL_TYPE_STREAM    0x02
+
 static bdaddr_t adapter_addr;
 static struct ipc *hal_ipc = NULL;
 static struct queue *apps = NULL;
@@ -639,6 +643,18 @@ fail:
 							HAL_STATUS_FAILED);
 }
 
+static uint8_t android2channel_type(uint8_t type)
+{
+	switch (type) {
+	case HAL_HEALTH_CHANNEL_TYPE_RELIABLE:
+		return CHANNEL_TYPE_RELIABLE;
+	case HAL_HEALTH_CHANNEL_TYPE_STREAMING:
+		return CHANNEL_TYPE_STREAM;
+	}
+
+	return CHANNEL_TYPE_ANY;
+}
+
 static void bt_health_mdep_cfg_data(const void *buf, uint16_t len)
 {
 	const struct hal_cmd_health_mdep *cmd = buf;
@@ -662,7 +678,7 @@ static void bt_health_mdep_cfg_data(const void *buf, uint16_t len)
 
 	mdep->role = cmd->role;
 	mdep->data_type = cmd->data_type;
-	mdep->channel_type = cmd->channel_type;
+	mdep->channel_type = android2channel_type(cmd->channel_type);
 	mdep->id = queue_length(app->mdeps) + 1;
 
 	if (cmd->descr_len > 0) {
-- 
1.9.1

Re: [PATCH_v2 2/4] anrdoid/health: Fix wrong channel type defines

[Szymon Janc]

Hi Ravi,

On Tuesday 17 of June 2014 01:02:48 Ravi kumar Veeramally wrote:
> Enums in bt_hl.h are defined in this order BTHL_CHANNEL_TYPE_RELIABLE,
> BTHL_CHANNEL_TYPE_STREAMING, BTHL_CHANNEL_TYPE_ANY. But HDP_SPEC_V11(3.4)
> has no-preference(any)-0, reliable-1, stream-2, reserved(0x03-0xff).
> Providing utility to solve this.
> ---
>  android/health.c | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/android/health.c b/android/health.c
> index 7e2c5d6..5a87089 100644
> --- a/android/health.c
> +++ b/android/health.c
> @@ -52,6 +52,10 @@
>  #define HDP_VERSION			0x0101
>  #define DATA_EXCHANGE_SPEC_11073	0x01
>  
> +#define CHANNEL_TYPE_ANY       0x00
> +#define CHANNEL_TYPE_RELIABLE  0x01
> +#define CHANNEL_TYPE_STREAM    0x02
> +

Isn't that already defined in hdp_types.h ?

>  static bdaddr_t adapter_addr;
>  static struct ipc *hal_ipc = NULL;
>  static struct queue *apps = NULL;
> @@ -639,6 +643,18 @@ fail:
>  							HAL_STATUS_FAILED);
>  }
>  
> +static uint8_t android2channel_type(uint8_t type)
> +{
> +	switch (type) {
> +	case HAL_HEALTH_CHANNEL_TYPE_RELIABLE:
> +		return CHANNEL_TYPE_RELIABLE;
> +	case HAL_HEALTH_CHANNEL_TYPE_STREAMING:
> +		return CHANNEL_TYPE_STREAM;
> +	}
> +
> +	return CHANNEL_TYPE_ANY;

Move this under default case.

> +}
> +
>  static void bt_health_mdep_cfg_data(const void *buf, uint16_t len)
>  {
>  	const struct hal_cmd_health_mdep *cmd = buf;
> @@ -662,7 +678,7 @@ static void bt_health_mdep_cfg_data(const void *buf, uint16_t len)
>  
>  	mdep->role = cmd->role;
>  	mdep->data_type = cmd->data_type;
> -	mdep->channel_type = cmd->channel_type;
> +	mdep->channel_type = android2channel_type(cmd->channel_type);
>  	mdep->id = queue_length(app->mdeps) + 1;
>  
>  	if (cmd->descr_len > 0) {
> 

-- 
Best regards, 
Szymon Janc

Re: [PATCH_v2 2/4] anrdoid/health: Fix wrong channel type defines

[Ravi kumar Veeramally]

Hi Szymon,

On 06/17/2014 12:29 PM, Szymon Janc wrote:
> Hi Ravi,
>
> On Tuesday 17 of June 2014 01:02:48 Ravi kumar Veeramally wrote:
>> Enums in bt_hl.h are defined in this order BTHL_CHANNEL_TYPE_RELIABLE,
>> BTHL_CHANNEL_TYPE_STREAMING, BTHL_CHANNEL_TYPE_ANY. But HDP_SPEC_V11(3.4)
>> has no-preference(any)-0, reliable-1, stream-2, reserved(0x03-0xff).
>> Providing utility to solve this.
>> ---
>>   android/health.c | 18 +++++++++++++++++-
>>   1 file changed, 17 insertions(+), 1 deletion(-)
>>
>> diff --git a/android/health.c b/android/health.c
>> index 7e2c5d6..5a87089 100644
>> --- a/android/health.c
>> +++ b/android/health.c
>> @@ -52,6 +52,10 @@
>>   #define HDP_VERSION			0x0101
>>   #define DATA_EXCHANGE_SPEC_11073	0x01
>>   
>> +#define CHANNEL_TYPE_ANY       0x00
>> +#define CHANNEL_TYPE_RELIABLE  0x01
>> +#define CHANNEL_TYPE_STREAM    0x02
>> +
> Isn't that already defined in hdp_types.h ?
   Android is not using hdp_types.h .
>>   static bdaddr_t adapter_addr;
>>   static struct ipc *hal_ipc = NULL;
>>   static struct queue *apps = NULL;
>> @@ -639,6 +643,18 @@ fail:
>>   							HAL_STATUS_FAILED);
>>   }
>>   
>> +static uint8_t android2channel_type(uint8_t type)
>> +{
>> +	switch (type) {
>> +	case HAL_HEALTH_CHANNEL_TYPE_RELIABLE:
>> +		return CHANNEL_TYPE_RELIABLE;
>> +	case HAL_HEALTH_CHANNEL_TYPE_STREAMING:
>> +		return CHANNEL_TYPE_STREAM;
>> +	}
>> +
>> +	return CHANNEL_TYPE_ANY;
> Move this under default case.
   Ok.
>> +}
>> +
>>   static void bt_health_mdep_cfg_data(const void *buf, uint16_t len)
>>   {
>>   	const struct hal_cmd_health_mdep *cmd = buf;
>> @@ -662,7 +678,7 @@ static void bt_health_mdep_cfg_data(const void *buf, uint16_t len)
>>   
>>   	mdep->role = cmd->role;
>>   	mdep->data_type = cmd->data_type;
>> -	mdep->channel_type = cmd->channel_type;
>> +	mdep->channel_type = android2channel_type(cmd->channel_type);
>>   	mdep->id = queue_length(app->mdeps) + 1;
>>   
>>   	if (cmd->descr_len > 0) {
>>
Thanks,
Ravi.

[PATCH_v2 3/4] android/health: Initial connect channel implementation

[Ravi kumar Veeramally]

Fetches remote sdp record and initiates MCL connection.
---
 android/health.c | 415 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 413 insertions(+), 2 deletions(-)

diff --git a/android/health.c b/android/health.c
index 5a87089..94154c3 100644
--- a/android/health.c
+++ b/android/health.c
@@ -36,9 +36,13 @@
 #include "lib/bluetooth.h"
 #include "lib/sdp.h"
 #include "lib/sdp_lib.h"
+#include "lib/uuid.h"
+#include "lib/l2cap.h"
 #include "src/log.h"
 #include "src/shared/util.h"
 #include "src/shared/queue.h"
+#include "src/uuid-helper.h"
+#include "src/sdp-client.h"
 
 #include "hal-msg.h"
 #include "ipc-common.h"
@@ -72,6 +76,28 @@ struct mdep_cfg {
 	uint8_t id; /* mdep id */
 };
 
+struct health_device {
+	bdaddr_t dst;
+	uint16_t app_id;
+
+	struct mcap_mcl *mcl;
+	bool mcl_conn;
+
+	struct queue *channels;     /* data channels */
+
+	uint16_t ccpsm;
+	uint16_t dcpsm;
+};
+
+struct health_channel {
+	uint8_t mdep_id;
+	uint8_t type;
+
+	struct health_device *dev;
+
+	uint16_t id; /* channel id */
+};
+
 struct health_app {
 	char *app_name;
 	char *provider_name;
@@ -81,8 +107,54 @@ struct health_app {
 	struct queue *mdeps;
 
 	uint16_t id; /* app id */
+	struct queue *devices;
 };
 
+static void free_health_channel(void *data)
+{
+	struct health_channel *channel = data;
+
+	if (!channel)
+		return;
+
+	free(channel);
+}
+
+static void destroy_channel(void *data)
+{
+	struct health_channel *channel = data;
+
+	if (!channel)
+		return;
+
+	/* TODO: Notify channel connection status DESTROYED */
+	queue_remove(channel->dev->channels, channel);
+	free_health_channel(channel);
+}
+
+static void unref_mcl(struct health_device *dev)
+{
+	if (!dev && !dev->mcl)
+		return;
+
+	mcap_close_mcl(dev->mcl, FALSE);
+	mcap_mcl_unref(dev->mcl);
+	dev->mcl = NULL;
+	dev->mcl_conn = FALSE;
+}
+
+static void free_health_device(void *data)
+{
+	struct health_device *dev = data;
+
+	if (!dev)
+		return;
+
+	unref_mcl(dev);
+	queue_destroy(dev->channels, free_health_channel);
+	free(dev);
+}
+
 static void free_mdep_cfg(void *data)
 {
 	struct mdep_cfg *cfg = data;
@@ -106,6 +178,7 @@ static void free_health_app(void *data)
 	free(app->service_name);
 	free(app->service_descr);
 	queue_destroy(app->mdeps, free_mdep_cfg);
+	queue_destroy(app->devices, free_health_device);
 	free(app);
 }
 
@@ -130,6 +203,14 @@ static bool mdep_by_mdep_role(const void *data, const void *user_data)
 	return mdep->role == role;
 }
 
+static bool mdep_by_mdep_id(const void *data, const void *user_data)
+{
+	const struct mdep_cfg *mdep = data;
+	uint16_t mdep_id = PTR_TO_INT(user_data);
+
+	return mdep->id == mdep_id;
+}
+
 static bool app_by_app_id(const void *data, const void *user_data)
 {
 	const struct health_app *app = data;
@@ -760,12 +841,342 @@ static void bt_health_unregister_app(const void *buf, uint16_t len)
 				HAL_OP_HEALTH_UNREG_APP, HAL_STATUS_SUCCESS);
 }
 
+static int get_prot_desc_entry(sdp_data_t *entry, int type, guint16 *val)
+{
+	sdp_data_t *iter;
+	int proto;
+
+	if (!entry || !SDP_IS_SEQ(entry->dtd))
+		return -1;
+
+	iter = entry->val.dataseq;
+	if (!(iter->dtd & SDP_UUID_UNSPEC))
+		return -1;
+	proto = sdp_uuid_to_proto(&iter->val.uuid);
+	if (proto != type)
+		return -1;
+
+	if (!val)
+		return 0;
+
+	iter = iter->next;
+	if (iter->dtd != SDP_UINT16)
+		return -1;
+
+	*val = iter->val.uint16;
+
+	return 0;
+}
+
+static int get_prot_desc_list(const sdp_record_t *rec, uint16_t *psm,
+							uint16_t *version)
+{
+	sdp_data_t *pdl, *p0, *p1;
+
+	if (!psm && !version)
+		return -1;
+
+	pdl = sdp_data_get(rec, SDP_ATTR_PROTO_DESC_LIST);
+	if (!pdl || !SDP_IS_SEQ(pdl->dtd))
+		return -1;
+
+	p0 = pdl->val.dataseq;
+	if (get_prot_desc_entry(p0, L2CAP_UUID, psm) < 0)
+		return -1;
+
+	p1 = p0->next;
+	if (get_prot_desc_entry(p1, MCAP_CTRL_UUID, version) < 0)
+		return -1;
+
+	return 0;
+}
+
+static int get_ccpsm(sdp_list_t *recs, uint16_t *ccpsm)
+{
+	sdp_list_t *l;
+
+	for (l = recs; l; l = l->next) {
+		sdp_record_t *rec = l->data;
+
+		if (!get_prot_desc_list(rec, ccpsm, NULL))
+			return 0;
+	}
+
+	return -1;
+}
+
+static int get_add_prot_desc_list(const sdp_record_t *rec, uint16_t *psm)
+{
+	sdp_data_t *pdl, *p0, *p1;
+
+	if (!psm)
+		return -1;
+
+	pdl = sdp_data_get(rec, SDP_ATTR_ADD_PROTO_DESC_LIST);
+	if (!pdl || pdl->dtd != SDP_SEQ8)
+		return -1;
+
+	pdl = pdl->val.dataseq;
+	if (pdl->dtd != SDP_SEQ8)
+		return -1;
+
+	p0 = pdl->val.dataseq;
+
+	if (get_prot_desc_entry(p0, L2CAP_UUID, psm) < 0)
+		return -1;
+
+	p1 = p0->next;
+	if (get_prot_desc_entry(p1, MCAP_DATA_UUID, NULL) < 0)
+		return -1;
+
+	return 0;
+}
+
+static int get_dcpsm(sdp_list_t *recs, uint16_t *dcpsm)
+{
+	sdp_list_t *l;
+
+	for (l = recs; l; l = l->next) {
+		sdp_record_t *rec = l->data;
+
+		if (!get_add_prot_desc_list(rec, dcpsm))
+			return 0;
+	}
+
+	return -1;
+}
+
+static void mcap_mdl_connected_cb(struct mcap_mdl *mdl, void *data)
+{
+	DBG("Not Implemeneted");
+}
+
+static void mcap_mdl_closed_cb(struct mcap_mdl *mdl, void *data)
+{
+	DBG("Not Implemeneted");
+}
+
+static void mcap_mdl_deleted_cb(struct mcap_mdl *mdl, void *data)
+{
+	DBG("Not Implemeneted");
+}
+
+static void mcap_mdl_aborted_cb(struct mcap_mdl *mdl, void *data)
+{
+	DBG("Not Implemeneted");
+}
+
+static void mcap_mdl_conn_req_cb(struct mcap_mcl *mcl, uint8_t mdepid,
+				uint16_t mdlid, uint8_t *conf, void *data)
+{
+	DBG("Not Implemeneted");
+}
+
+static void mcap_mdl_reconn_req_cb(struct mcap_mdl *mdl, void *data)
+{
+	DBG("Not Implemeneted");
+}
+
+static void create_mcl_cb(struct mcap_mcl *mcl, GError *err, gpointer data)
+{
+	struct health_channel *channel = data;
+	gboolean ret;
+	GError *gerr = NULL;
+
+	DBG("");
+
+	if (err) {
+		error("error creating MCL : %s", err->message);
+		goto fail;
+	}
+
+	if (!channel->dev->mcl)
+		channel->dev->mcl = mcap_mcl_ref(mcl);
+
+	channel->dev->mcl_conn = TRUE;
+	DBG("MCL connected");
+
+	ret = mcap_mcl_set_cb(channel->dev->mcl, channel, &gerr,
+			MCAP_MDL_CB_CONNECTED, mcap_mdl_connected_cb,
+			MCAP_MDL_CB_CLOSED, mcap_mdl_closed_cb,
+			MCAP_MDL_CB_DELETED, mcap_mdl_deleted_cb,
+			MCAP_MDL_CB_ABORTED, mcap_mdl_aborted_cb,
+			MCAP_MDL_CB_REMOTE_CONN_REQ, mcap_mdl_conn_req_cb,
+			MCAP_MDL_CB_REMOTE_RECONN_REQ, mcap_mdl_reconn_req_cb,
+			MCAP_MDL_CB_INVALID);
+	if (!ret) {
+		error("error setting mdl callbacks on mcl");
+		goto fail;
+	}
+
+	/* TODO : create mdl */
+	return;
+
+fail:
+	destroy_channel(channel);
+
+	if (err)
+		g_error_free(err);
+}
+
+static void search_cb(sdp_list_t *recs, int err, gpointer data)
+{
+	struct health_channel *channel = data;
+	GError *gerr = NULL;
+
+	DBG("");
+
+	if (err < 0 || !recs) {
+		error("Error getting remote SDP records");
+		goto fail;
+	}
+
+	if (get_ccpsm(recs, &channel->dev->ccpsm) < 0) {
+		error("Can't get remote PSM for control channel");
+		goto fail;
+	}
+
+	if (get_dcpsm(recs, &channel->dev->dcpsm) < 0) {
+		error("Can't get remote PSM for data channel");
+		goto fail;
+	}
+
+	if (!mcap_create_mcl(mcap, &channel->dev->dst, channel->dev->ccpsm,
+					create_mcl_cb, channel, NULL, &gerr)) {
+		error("error creating mcl %s", gerr->message);
+		goto fail;
+	}
+
+	/* TODO: send channel state CONNECTING */
+	return;
+
+fail:
+	/* TODO: send channel state DESTROYED*/
+
+	queue_remove(channel->dev->channels, channel);
+	free_health_channel(channel);
+
+	if (gerr)
+		g_error_free(gerr);
+}
+
+static int connect_mcl(struct health_channel *channel)
+{
+	uuid_t uuid;
+
+	DBG("");
+
+	bt_string2uuid(&uuid, HDP_UUID);
+
+	return bt_search_service(&adapter_addr, &channel->dev->dst, &uuid,
+						search_cb, channel, NULL, 0);
+}
+
+static struct health_device *create_device(uint16_t app_id,
+						const uint8_t addr[])
+{
+	struct health_device *dev;
+
+	dev = new0(struct health_device, 1);
+	if (!dev)
+		return NULL;
+
+	android2bdaddr(&addr, &dev->dst);
+	dev->app_id = app_id;
+
+	return dev;
+}
+
+static struct health_channel *create_channel(uint16_t app_id,
+						uint8_t mdep_index)
+{
+	struct health_app *app;
+	struct mdep_cfg *mdep;
+	struct health_channel *channel;
+	uint8_t index;
+	static unsigned int channel_id = 1;
+
+	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;
+
+	channel = new0(struct health_channel, 1);
+	if (!channel)
+		return NULL;
+
+	channel->mdep_id = mdep_index;
+	channel->type = mdep->channel_type;
+	channel->id = channel_id++;
+
+	return channel;
+}
+
 static void bt_health_connect_channel(const void *buf, uint16_t len)
 {
-	DBG("Not implemented");
+	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);
+	if (!channel)
+		goto fail;
+
+	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;
+	}
+
+	if (connect_mcl(channel) < 0) {
+		error("error retrieving HDP SDP record");
+		queue_remove(app->devices, dev);
+		goto fail;
+	}
+
+	rsp.channel_id = channel->id;
+	ipc_send_rsp_full(hal_ipc, HAL_SERVICE_ID_HEALTH,
+				HAL_OP_HEALTH_CONNECT_CHANNEL,
+				sizeof(rsp), &rsp, -1);
+	return;
+
+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_UNSUPPORTED);
+			HAL_OP_HEALTH_CONNECT_CHANNEL, HAL_STATUS_FAILED);
 }
 
 static void bt_health_destroy_channel(const void *buf, uint16_t len)
-- 
1.9.1

Re: [PATCH_v2 3/4] android/health: Initial connect channel implementation

[Szymon Janc]

Hi Ravi,

On Tuesday 17 of June 2014 01:02:49 Ravi kumar Veeramally wrote:
> Fetches remote sdp record and initiates MCL connection.
> ---
>  android/health.c | 415 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 413 insertions(+), 2 deletions(-)
> 
> diff --git a/android/health.c b/android/health.c
> index 5a87089..94154c3 100644
> --- a/android/health.c
> +++ b/android/health.c
> @@ -36,9 +36,13 @@
>  #include "lib/bluetooth.h"
>  #include "lib/sdp.h"
>  #include "lib/sdp_lib.h"
> +#include "lib/uuid.h"
> +#include "lib/l2cap.h"
>  #include "src/log.h"
>  #include "src/shared/util.h"
>  #include "src/shared/queue.h"
> +#include "src/uuid-helper.h"
> +#include "src/sdp-client.h"
>  
>  #include "hal-msg.h"
>  #include "ipc-common.h"
> @@ -72,6 +76,28 @@ struct mdep_cfg {
>  	uint8_t id; /* mdep id */
>  };
>  
> +struct health_device {
> +	bdaddr_t dst;
> +	uint16_t app_id;
> +
> +	struct mcap_mcl *mcl;
> +	bool mcl_conn;
> +
> +	struct queue *channels;     /* data channels */
> +
> +	uint16_t ccpsm;
> +	uint16_t dcpsm;
> +};
> +
> +struct health_channel {
> +	uint8_t mdep_id;
> +	uint8_t type;
> +
> +	struct health_device *dev;
> +
> +	uint16_t id; /* channel id */
> +};
> +
>  struct health_app {
>  	char *app_name;
>  	char *provider_name;
> @@ -81,8 +107,54 @@ struct health_app {
>  	struct queue *mdeps;
>  
>  	uint16_t id; /* app id */
> +	struct queue *devices;
>  };
>  
> +static void free_health_channel(void *data)
> +{
> +	struct health_channel *channel = data;
> +
> +	if (!channel)
> +		return;
> +
> +	free(channel);
> +}
> +
> +static void destroy_channel(void *data)
> +{
> +	struct health_channel *channel = data;
> +
> +	if (!channel)
> +		return;
> +
> +	/* TODO: Notify channel connection status DESTROYED */
> +	queue_remove(channel->dev->channels, channel);
> +	free_health_channel(channel);
> +}
> +
> +static void unref_mcl(struct health_device *dev)
> +{
> +	if (!dev && !dev->mcl)

Shouldn't this be "if (!dev || !dev->mcl)" ?


> +		return;
> +
> +	mcap_close_mcl(dev->mcl, FALSE);
> +	mcap_mcl_unref(dev->mcl);
> +	dev->mcl = NULL;
> +	dev->mcl_conn = FALSE;

mlc_conn is bool, use 'false'.

> +}
> +
> +static void free_health_device(void *data)
> +{
> +	struct health_device *dev = data;
> +
> +	if (!dev)
> +		return;
> +
> +	unref_mcl(dev);
> +	queue_destroy(dev->channels, free_health_channel);
> +	free(dev);
> +}
> +
>  static void free_mdep_cfg(void *data)
>  {
>  	struct mdep_cfg *cfg = data;
> @@ -106,6 +178,7 @@ static void free_health_app(void *data)
>  	free(app->service_name);
>  	free(app->service_descr);
>  	queue_destroy(app->mdeps, free_mdep_cfg);
> +	queue_destroy(app->devices, free_health_device);
>  	free(app);
>  }
>  
> @@ -130,6 +203,14 @@ static bool mdep_by_mdep_role(const void *data, const void *user_data)
>  	return mdep->role == role;
>  }
>  
> +static bool mdep_by_mdep_id(const void *data, const void *user_data)
> +{
> +	const struct mdep_cfg *mdep = data;
> +	uint16_t mdep_id = PTR_TO_INT(user_data);
> +
> +	return mdep->id == mdep_id;
> +}
> +
>  static bool app_by_app_id(const void *data, const void *user_data)
>  {
>  	const struct health_app *app = data;
> @@ -760,12 +841,342 @@ static void bt_health_unregister_app(const void *buf, uint16_t len)
>  				HAL_OP_HEALTH_UNREG_APP, HAL_STATUS_SUCCESS);
>  }
>  
> +static int get_prot_desc_entry(sdp_data_t *entry, int type, guint16 *val)
> +{
> +	sdp_data_t *iter;
> +	int proto;
> +
> +	if (!entry || !SDP_IS_SEQ(entry->dtd))
> +		return -1;
> +
> +	iter = entry->val.dataseq;
> +	if (!(iter->dtd & SDP_UUID_UNSPEC))
> +		return -1;

Add empty line here.

> +	proto = sdp_uuid_to_proto(&iter->val.uuid);
> +	if (proto != type)
> +		return -1;
> +
> +	if (!val)
> +		return 0;
> +
> +	iter = iter->next;
> +	if (iter->dtd != SDP_UINT16)
> +		return -1;
> +
> +	*val = iter->val.uint16;
> +
> +	return 0;
> +}
> +
> +static int get_prot_desc_list(const sdp_record_t *rec, uint16_t *psm,
> +							uint16_t *version)
> +{
> +	sdp_data_t *pdl, *p0, *p1;
> +
> +	if (!psm && !version)
> +		return -1;
> +
> +	pdl = sdp_data_get(rec, SDP_ATTR_PROTO_DESC_LIST);
> +	if (!pdl || !SDP_IS_SEQ(pdl->dtd))
> +		return -1;
> +
> +	p0 = pdl->val.dataseq;
> +	if (get_prot_desc_entry(p0, L2CAP_UUID, psm) < 0)
> +		return -1;
> +
> +	p1 = p0->next;
> +	if (get_prot_desc_entry(p1, MCAP_CTRL_UUID, version) < 0)
> +		return -1;
> +
> +	return 0;
> +}
> +
> +static int get_ccpsm(sdp_list_t *recs, uint16_t *ccpsm)
> +{
> +	sdp_list_t *l;
> +
> +	for (l = recs; l; l = l->next) {
> +		sdp_record_t *rec = l->data;
> +
> +		if (!get_prot_desc_list(rec, ccpsm, NULL))
> +			return 0;
> +	}
> +
> +	return -1;
> +}
> +
> +static int get_add_prot_desc_list(const sdp_record_t *rec, uint16_t *psm)
> +{
> +	sdp_data_t *pdl, *p0, *p1;
> +
> +	if (!psm)
> +		return -1;
> +
> +	pdl = sdp_data_get(rec, SDP_ATTR_ADD_PROTO_DESC_LIST);
> +	if (!pdl || pdl->dtd != SDP_SEQ8)
> +		return -1;
> +
> +	pdl = pdl->val.dataseq;
> +	if (pdl->dtd != SDP_SEQ8)
> +		return -1;
> +
> +	p0 = pdl->val.dataseq;
> +
> +	if (get_prot_desc_entry(p0, L2CAP_UUID, psm) < 0)
> +		return -1;
> +
> +	p1 = p0->next;
> +	if (get_prot_desc_entry(p1, MCAP_DATA_UUID, NULL) < 0)
> +		return -1;
> +
> +	return 0;
> +}
> +
> +static int get_dcpsm(sdp_list_t *recs, uint16_t *dcpsm)
> +{
> +	sdp_list_t *l;
> +
> +	for (l = recs; l; l = l->next) {
> +		sdp_record_t *rec = l->data;
> +
> +		if (!get_add_prot_desc_list(rec, dcpsm))
> +			return 0;
> +	}
> +
> +	return -1;
> +}
> +
> +static void mcap_mdl_connected_cb(struct mcap_mdl *mdl, void *data)
> +{
> +	DBG("Not Implemeneted");
> +}
> +
> +static void mcap_mdl_closed_cb(struct mcap_mdl *mdl, void *data)
> +{
> +	DBG("Not Implemeneted");
> +}
> +
> +static void mcap_mdl_deleted_cb(struct mcap_mdl *mdl, void *data)
> +{
> +	DBG("Not Implemeneted");
> +}
> +
> +static void mcap_mdl_aborted_cb(struct mcap_mdl *mdl, void *data)
> +{
> +	DBG("Not Implemeneted");
> +}
> +
> +static void mcap_mdl_conn_req_cb(struct mcap_mcl *mcl, uint8_t mdepid,
> +				uint16_t mdlid, uint8_t *conf, void *data)
> +{
> +	DBG("Not Implemeneted");
> +}
> +
> +static void mcap_mdl_reconn_req_cb(struct mcap_mdl *mdl, void *data)
> +{
> +	DBG("Not Implemeneted");
> +}
> +
> +static void create_mcl_cb(struct mcap_mcl *mcl, GError *err, gpointer data)
> +{
> +	struct health_channel *channel = data;
> +	gboolean ret;
> +	GError *gerr = NULL;
> +
> +	DBG("");
> +
> +	if (err) {
> +		error("error creating MCL : %s", err->message);
> +		goto fail;
> +	}
> +
> +	if (!channel->dev->mcl)
> +		channel->dev->mcl = mcap_mcl_ref(mcl);
> +
> +	channel->dev->mcl_conn = TRUE;

bool, use 'true'.

> +	DBG("MCL connected");
> +
> +	ret = mcap_mcl_set_cb(channel->dev->mcl, channel, &gerr,
> +			MCAP_MDL_CB_CONNECTED, mcap_mdl_connected_cb,
> +			MCAP_MDL_CB_CLOSED, mcap_mdl_closed_cb,
> +			MCAP_MDL_CB_DELETED, mcap_mdl_deleted_cb,
> +			MCAP_MDL_CB_ABORTED, mcap_mdl_aborted_cb,
> +			MCAP_MDL_CB_REMOTE_CONN_REQ, mcap_mdl_conn_req_cb,
> +			MCAP_MDL_CB_REMOTE_RECONN_REQ, mcap_mdl_reconn_req_cb,
> +			MCAP_MDL_CB_INVALID);
> +	if (!ret) {
> +		error("error setting mdl callbacks on mcl");

gerr should be checked and freed here.

> +		goto fail;
> +	}
> +
> +	/* TODO : create mdl */
> +	return;
> +
> +fail:
> +	destroy_channel(channel);
> +
> +	if (err)
> +		g_error_free(err);

Are you sure this is correct? Wouldn't this error will be free by glib
when call returns?

> +}
> +
> +static void search_cb(sdp_list_t *recs, int err, gpointer data)
> +{
> +	struct health_channel *channel = data;
> +	GError *gerr = NULL;
> +
> +	DBG("");
> +
> +	if (err < 0 || !recs) {
> +		error("Error getting remote SDP records");
> +		goto fail;
> +	}
> +
> +	if (get_ccpsm(recs, &channel->dev->ccpsm) < 0) {
> +		error("Can't get remote PSM for control channel");
> +		goto fail;
> +	}
> +
> +	if (get_dcpsm(recs, &channel->dev->dcpsm) < 0) {
> +		error("Can't get remote PSM for data channel");
> +		goto fail;
> +	}
> +
> +	if (!mcap_create_mcl(mcap, &channel->dev->dst, channel->dev->ccpsm,
> +					create_mcl_cb, channel, NULL, &gerr)) {
> +		error("error creating mcl %s", gerr->message);
> +		goto fail;
> +	}
> +
> +	/* TODO: send channel state CONNECTING */
> +	return;
> +
> +fail:
> +	/* TODO: send channel state DESTROYED*/
> +
> +	queue_remove(channel->dev->channels, channel);
> +	free_health_channel(channel);
> +
> +	if (gerr)
> +		g_error_free(gerr);

I'd free error before jumping to fail: label.

> +}
> +
> +static int connect_mcl(struct health_channel *channel)
> +{
> +	uuid_t uuid;
> +
> +	DBG("");
> +
> +	bt_string2uuid(&uuid, HDP_UUID);
> +
> +	return bt_search_service(&adapter_addr, &channel->dev->dst, &uuid,
> +						search_cb, channel, NULL, 0);
> +}
> +
> +static struct health_device *create_device(uint16_t app_id,
> +						const uint8_t addr[])
> +{
> +	struct health_device *dev;
> +
> +	dev = new0(struct health_device, 1);
> +	if (!dev)
> +		return NULL;
> +
> +	android2bdaddr(&addr, &dev->dst);
> +	dev->app_id = app_id;
> +
> +	return dev;
> +}
> +
> +static struct health_channel *create_channel(uint16_t app_id,
> +						uint8_t mdep_index)
> +{
> +	struct health_app *app;
> +	struct mdep_cfg *mdep;
> +	struct health_channel *channel;
> +	uint8_t index;
> +	static unsigned int channel_id = 1;
> +
> +	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;
> +
> +	channel = new0(struct health_channel, 1);
> +	if (!channel)
> +		return NULL;
> +
> +	channel->mdep_id = mdep_index;
> +	channel->type = mdep->channel_type;
> +	channel->id = channel_id++;
> +
> +	return channel;
> +}
> +
>  static void bt_health_connect_channel(const void *buf, uint16_t len)
>  {
> -	DBG("Not implemented");
> +	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;

Should we check if device was already created ie if it is already
connected?

> +
> +	channel = create_channel(cmd->app_id, cmd->mdep_index);
> +	if (!channel)
> +		goto fail;
> +
> +	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;
> +	}
> +
> +	if (connect_mcl(channel) < 0) {
> +		error("error retrieving HDP SDP record");
> +		queue_remove(app->devices, dev);
> +		goto fail;
> +	}
> +
> +	rsp.channel_id = channel->id;
> +	ipc_send_rsp_full(hal_ipc, HAL_SERVICE_ID_HEALTH,
> +				HAL_OP_HEALTH_CONNECT_CHANNEL,
> +				sizeof(rsp), &rsp, -1);
> +	return;
> +
> +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_UNSUPPORTED);
> +			HAL_OP_HEALTH_CONNECT_CHANNEL, HAL_STATUS_FAILED);
>  }
>  
>  static void bt_health_destroy_channel(const void *buf, uint16_t len)
> 

-- 
Best regards, 
Szymon Janc

Re: [PATCH_v2 3/4] android/health: Initial connect channel implementation

[Ravi kumar Veeramally]

Hi Szymon,

On 06/17/2014 01:16 PM, Szymon Janc wrote:
> Hi Ravi,
>
> On Tuesday 17 of June 2014 01:02:49 Ravi kumar Veeramally wrote:
>> Fetches remote sdp record and initiates MCL connection.
>> ---
>>   android/health.c | 415 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 413 insertions(+), 2 deletions(-)
>>
>> diff --git a/android/health.c b/android/health.c
>> index 5a87089..94154c3 100644
>> --- a/android/health.c
>> +++ b/android/health.c
>> @@ -36,9 +36,13 @@
>>   #include "lib/bluetooth.h"
>>   #include "lib/sdp.h"
>>   #include "lib/sdp_lib.h"
>> +#include "lib/uuid.h"
>> +#include "lib/l2cap.h"
>>   #include "src/log.h"
>>   #include "src/shared/util.h"
>>   #include "src/shared/queue.h"
>> +#include "src/uuid-helper.h"
>> +#include "src/sdp-client.h"
>>   
>>   #include "hal-msg.h"
>>   #include "ipc-common.h"
>> @@ -72,6 +76,28 @@ struct mdep_cfg {
>>   	uint8_t id; /* mdep id */
>>   };
>>   
>> +struct health_device {
>> +	bdaddr_t dst;
>> +	uint16_t app_id;
>> +
>> +	struct mcap_mcl *mcl;
>> +	bool mcl_conn;
>> +
>> +	struct queue *channels;     /* data channels */
>> +
>> +	uint16_t ccpsm;
>> +	uint16_t dcpsm;
>> +};
>> +
>> +struct health_channel {
>> +	uint8_t mdep_id;
>> +	uint8_t type;
>> +
>> +	struct health_device *dev;
>> +
>> +	uint16_t id; /* channel id */
>> +};
>> +
>>   struct health_app {
>>   	char *app_name;
>>   	char *provider_name;
>> @@ -81,8 +107,54 @@ struct health_app {
>>   	struct queue *mdeps;
>>   
>>   	uint16_t id; /* app id */
>> +	struct queue *devices;
>>   };
>>   
>> +static void free_health_channel(void *data)
>> +{
>> +	struct health_channel *channel = data;
>> +
>> +	if (!channel)
>> +		return;
>> +
>> +	free(channel);
>> +}
>> +
>> +static void destroy_channel(void *data)
>> +{
>> +	struct health_channel *channel = data;
>> +
>> +	if (!channel)
>> +		return;
>> +
>> +	/* TODO: Notify channel connection status DESTROYED */
>> +	queue_remove(channel->dev->channels, channel);
>> +	free_health_channel(channel);
>> +}
>> +
>> +static void unref_mcl(struct health_device *dev)
>> +{
>> +	if (!dev && !dev->mcl)
> Shouldn't this be "if (!dev || !dev->mcl)" ?
   yes.
>
>
>> +		return;
>> +
>> +	mcap_close_mcl(dev->mcl, FALSE);
>> +	mcap_mcl_unref(dev->mcl);
>> +	dev->mcl = NULL;
>> +	dev->mcl_conn = FALSE;
> mlc_conn is bool, use 'false'.
  Ok.
>
>> +}
>> +
>> +static void free_health_device(void *data)
>> +{
>> +	struct health_device *dev = data;
>> +
>> +	if (!dev)
>> +		return;
>> +
>> +	unref_mcl(dev);
>> +	queue_destroy(dev->channels, free_health_channel);
>> +	free(dev);
>> +}
>> +
>>   static void free_mdep_cfg(void *data)
>>   {
>>   	struct mdep_cfg *cfg = data;
>> @@ -106,6 +178,7 @@ static void free_health_app(void *data)
>>   	free(app->service_name);
>>   	free(app->service_descr);
>>   	queue_destroy(app->mdeps, free_mdep_cfg);
>> +	queue_destroy(app->devices, free_health_device);
>>   	free(app);
>>   }
>>   
>> @@ -130,6 +203,14 @@ static bool mdep_by_mdep_role(const void *data, const void *user_data)
>>   	return mdep->role == role;
>>   }
>>   
>> +static bool mdep_by_mdep_id(const void *data, const void *user_data)
>> +{
>> +	const struct mdep_cfg *mdep = data;
>> +	uint16_t mdep_id = PTR_TO_INT(user_data);
>> +
>> +	return mdep->id == mdep_id;
>> +}
>> +
>>   static bool app_by_app_id(const void *data, const void *user_data)
>>   {
>>   	const struct health_app *app = data;
>> @@ -760,12 +841,342 @@ static void bt_health_unregister_app(const void *buf, uint16_t len)
>>   				HAL_OP_HEALTH_UNREG_APP, HAL_STATUS_SUCCESS);
>>   }
>>   
>> +static int get_prot_desc_entry(sdp_data_t *entry, int type, guint16 *val)
>> +{
>> +	sdp_data_t *iter;
>> +	int proto;
>> +
>> +	if (!entry || !SDP_IS_SEQ(entry->dtd))
>> +		return -1;
>> +
>> +	iter = entry->val.dataseq;
>> +	if (!(iter->dtd & SDP_UUID_UNSPEC))
>> +		return -1;
> Add empty line here.
   Ok.
>
>> +	proto = sdp_uuid_to_proto(&iter->val.uuid);
>> +	if (proto != type)
>> +		return -1;
>> +
>> +	if (!val)
>> +		return 0;
>> +
>> +	iter = iter->next;
>> +	if (iter->dtd != SDP_UINT16)
>> +		return -1;
>> +
>> +	*val = iter->val.uint16;
>> +
>> +	return 0;
>> +}
>> +
>> +static int get_prot_desc_list(const sdp_record_t *rec, uint16_t *psm,
>> +							uint16_t *version)
>> +{
>> +	sdp_data_t *pdl, *p0, *p1;
>> +
>> +	if (!psm && !version)
>> +		return -1;
>> +
>> +	pdl = sdp_data_get(rec, SDP_ATTR_PROTO_DESC_LIST);
>> +	if (!pdl || !SDP_IS_SEQ(pdl->dtd))
>> +		return -1;
>> +
>> +	p0 = pdl->val.dataseq;
>> +	if (get_prot_desc_entry(p0, L2CAP_UUID, psm) < 0)
>> +		return -1;
>> +
>> +	p1 = p0->next;
>> +	if (get_prot_desc_entry(p1, MCAP_CTRL_UUID, version) < 0)
>> +		return -1;
>> +
>> +	return 0;
>> +}
>> +
>> +static int get_ccpsm(sdp_list_t *recs, uint16_t *ccpsm)
>> +{
>> +	sdp_list_t *l;
>> +
>> +	for (l = recs; l; l = l->next) {
>> +		sdp_record_t *rec = l->data;
>> +
>> +		if (!get_prot_desc_list(rec, ccpsm, NULL))
>> +			return 0;
>> +	}
>> +
>> +	return -1;
>> +}
>> +
>> +static int get_add_prot_desc_list(const sdp_record_t *rec, uint16_t *psm)
>> +{
>> +	sdp_data_t *pdl, *p0, *p1;
>> +
>> +	if (!psm)
>> +		return -1;
>> +
>> +	pdl = sdp_data_get(rec, SDP_ATTR_ADD_PROTO_DESC_LIST);
>> +	if (!pdl || pdl->dtd != SDP_SEQ8)
>> +		return -1;
>> +
>> +	pdl = pdl->val.dataseq;
>> +	if (pdl->dtd != SDP_SEQ8)
>> +		return -1;
>> +
>> +	p0 = pdl->val.dataseq;
>> +
>> +	if (get_prot_desc_entry(p0, L2CAP_UUID, psm) < 0)
>> +		return -1;
>> +
>> +	p1 = p0->next;
>> +	if (get_prot_desc_entry(p1, MCAP_DATA_UUID, NULL) < 0)
>> +		return -1;
>> +
>> +	return 0;
>> +}
>> +
>> +static int get_dcpsm(sdp_list_t *recs, uint16_t *dcpsm)
>> +{
>> +	sdp_list_t *l;
>> +
>> +	for (l = recs; l; l = l->next) {
>> +		sdp_record_t *rec = l->data;
>> +
>> +		if (!get_add_prot_desc_list(rec, dcpsm))
>> +			return 0;
>> +	}
>> +
>> +	return -1;
>> +}
>> +
>> +static void mcap_mdl_connected_cb(struct mcap_mdl *mdl, void *data)
>> +{
>> +	DBG("Not Implemeneted");
>> +}
>> +
>> +static void mcap_mdl_closed_cb(struct mcap_mdl *mdl, void *data)
>> +{
>> +	DBG("Not Implemeneted");
>> +}
>> +
>> +static void mcap_mdl_deleted_cb(struct mcap_mdl *mdl, void *data)
>> +{
>> +	DBG("Not Implemeneted");
>> +}
>> +
>> +static void mcap_mdl_aborted_cb(struct mcap_mdl *mdl, void *data)
>> +{
>> +	DBG("Not Implemeneted");
>> +}
>> +
>> +static void mcap_mdl_conn_req_cb(struct mcap_mcl *mcl, uint8_t mdepid,
>> +				uint16_t mdlid, uint8_t *conf, void *data)
>> +{
>> +	DBG("Not Implemeneted");
>> +}
>> +
>> +static void mcap_mdl_reconn_req_cb(struct mcap_mdl *mdl, void *data)
>> +{
>> +	DBG("Not Implemeneted");
>> +}
>> +
>> +static void create_mcl_cb(struct mcap_mcl *mcl, GError *err, gpointer data)
>> +{
>> +	struct health_channel *channel = data;
>> +	gboolean ret;
>> +	GError *gerr = NULL;
>> +
>> +	DBG("");
>> +
>> +	if (err) {
>> +		error("error creating MCL : %s", err->message);
>> +		goto fail;
>> +	}
>> +
>> +	if (!channel->dev->mcl)
>> +		channel->dev->mcl = mcap_mcl_ref(mcl);
>> +
>> +	channel->dev->mcl_conn = TRUE;
> bool, use 'true'.
   Ok.
>
>> +	DBG("MCL connected");
>> +
>> +	ret = mcap_mcl_set_cb(channel->dev->mcl, channel, &gerr,
>> +			MCAP_MDL_CB_CONNECTED, mcap_mdl_connected_cb,
>> +			MCAP_MDL_CB_CLOSED, mcap_mdl_closed_cb,
>> +			MCAP_MDL_CB_DELETED, mcap_mdl_deleted_cb,
>> +			MCAP_MDL_CB_ABORTED, mcap_mdl_aborted_cb,
>> +			MCAP_MDL_CB_REMOTE_CONN_REQ, mcap_mdl_conn_req_cb,
>> +			MCAP_MDL_CB_REMOTE_RECONN_REQ, mcap_mdl_reconn_req_cb,
>> +			MCAP_MDL_CB_INVALID);
>> +	if (!ret) {
>> +		error("error setting mdl callbacks on mcl");
> gerr should be checked and freed here.
  ok.
>
>> +		goto fail;
>> +	}
>> +
>> +	/* TODO : create mdl */
>> +	return;
>> +
>> +fail:
>> +	destroy_channel(channel);
>> +
>> +	if (err)
>> +		g_error_free(err);
> Are you sure this is correct? Wouldn't this error will be free by glib
> when call returns?
  mcap library is not designed in that way. It sets error on error condition
and returns. Not performing any cleanup operations like you mentioned.
>> +}
>> +
>> +static void search_cb(sdp_list_t *recs, int err, gpointer data)
>> +{
>> +	struct health_channel *channel = data;
>> +	GError *gerr = NULL;
>> +
>> +	DBG("");
>> +
>> +	if (err < 0 || !recs) {
>> +		error("Error getting remote SDP records");
>> +		goto fail;
>> +	}
>> +
>> +	if (get_ccpsm(recs, &channel->dev->ccpsm) < 0) {
>> +		error("Can't get remote PSM for control channel");
>> +		goto fail;
>> +	}
>> +
>> +	if (get_dcpsm(recs, &channel->dev->dcpsm) < 0) {
>> +		error("Can't get remote PSM for data channel");
>> +		goto fail;
>> +	}
>> +
>> +	if (!mcap_create_mcl(mcap, &channel->dev->dst, channel->dev->ccpsm,
>> +					create_mcl_cb, channel, NULL, &gerr)) {
>> +		error("error creating mcl %s", gerr->message);
>> +		goto fail;
>> +	}
>> +
>> +	/* TODO: send channel state CONNECTING */
>> +	return;
>> +
>> +fail:
>> +	/* TODO: send channel state DESTROYED*/
>> +
>> +	queue_remove(channel->dev->channels, channel);
>> +	free_health_channel(channel);
>> +
>> +	if (gerr)
>> +		g_error_free(gerr);
> I'd free error before jumping to fail: label.
  Ok.
>
>> +}
>> +
>> +static int connect_mcl(struct health_channel *channel)
>> +{
>> +	uuid_t uuid;
>> +
>> +	DBG("");
>> +
>> +	bt_string2uuid(&uuid, HDP_UUID);
>> +
>> +	return bt_search_service(&adapter_addr, &channel->dev->dst, &uuid,
>> +						search_cb, channel, NULL, 0);
>> +}
>> +
>> +static struct health_device *create_device(uint16_t app_id,
>> +						const uint8_t addr[])
>> +{
>> +	struct health_device *dev;
>> +
>> +	dev = new0(struct health_device, 1);
>> +	if (!dev)
>> +		return NULL;
>> +
>> +	android2bdaddr(&addr, &dev->dst);
>> +	dev->app_id = app_id;
>> +
>> +	return dev;
>> +}
>> +
>> +static struct health_channel *create_channel(uint16_t app_id,
>> +						uint8_t mdep_index)
>> +{
>> +	struct health_app *app;
>> +	struct mdep_cfg *mdep;
>> +	struct health_channel *channel;
>> +	uint8_t index;
>> +	static unsigned int channel_id = 1;
>> +
>> +	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;
>> +
>> +	channel = new0(struct health_channel, 1);
>> +	if (!channel)
>> +		return NULL;
>> +
>> +	channel->mdep_id = mdep_index;
>> +	channel->type = mdep->channel_type;
>> +	channel->id = channel_id++;
>> +
>> +	return channel;
>> +}
>> +
>>   static void bt_health_connect_channel(const void *buf, uint16_t len)
>>   {
>> -	DBG("Not implemented");
>> +	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;
> Should we check if device was already created ie if it is already
> connected?
  Yeap, we should check. That is lot more stuff. Have to check
  if device is already exists, if exists if it has mcl, if mcl is connected,
then first data channel is reliable or not. It is just an initial 
implementation,
thing will done step by step.
>> +
>> +	channel = create_channel(cmd->app_id, cmd->mdep_index);
>> +	if (!channel)
>> +		goto fail;
>> +
>> +	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;
>> +	}
>> +
>> +	if (connect_mcl(channel) < 0) {
>> +		error("error retrieving HDP SDP record");
>> +		queue_remove(app->devices, dev);
>> +		goto fail;
>> +	}
>> +
>> +	rsp.channel_id = channel->id;
>> +	ipc_send_rsp_full(hal_ipc, HAL_SERVICE_ID_HEALTH,
>> +				HAL_OP_HEALTH_CONNECT_CHANNEL,
>> +				sizeof(rsp), &rsp, -1);
>> +	return;
>> +
>> +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_UNSUPPORTED);
>> +			HAL_OP_HEALTH_CONNECT_CHANNEL, HAL_STATUS_FAILED);
>>   }
>>   
>>   static void bt_health_destroy_channel(const void *buf, uint16_t len)
>>
Thanks,
Ravi.

[PATCH_v2 4/4] android/health: Notify channel connection status

[Ravi kumar Veeramally]

---
 android/health.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/android/health.c b/android/health.c
index 94154c3..e833cc3 100644
--- a/android/health.c
+++ b/android/health.c
@@ -195,6 +195,24 @@ static void send_app_reg_notify(struct health_app *app, uint8_t state)
 				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 mdep_by_mdep_role(const void *data, const void *user_data)
 {
 	const struct mdep_cfg *mdep = data;
@@ -1047,11 +1065,11 @@ static void search_cb(sdp_list_t *recs, int err, gpointer data)
 		goto fail;
 	}
 
-	/* TODO: send channel state CONNECTING */
+	send_channel_state_notify(channel, HAL_HEALTH_CHANNEL_CONNECTING, -1);
 	return;
 
 fail:
-	/* TODO: send channel state DESTROYED*/
+	send_channel_state_notify(channel, HAL_HEALTH_CHANNEL_DESTROYED, -1);
 
 	queue_remove(channel->dev->channels, channel);
 	free_health_channel(channel);
-- 
1.9.1