Hi Szymon,
On 06/16/2014 11:23 AM, Szymon Janc wrote:
Hi Ravi,
On Thursday 12 of June 2014 16:10:14 Ravi kumar Veeramally wrote:
---
android/health.c | 188 +++++++++++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 182 insertions(+), 6 deletions(-)
diff --git a/android/health.c b/android/health.c
index 655d9f9..a117390 100644
--- a/android/health.c
+++ b/android/health.c
@@ -35,6 +35,8 @@
#include "lib/sdp.h"
#include "lib/sdp_lib.h"
#include "src/log.h"
+#include "src/shared/util.h"
+#include "src/shared/queue.h"
#include "hal-msg.h"
#include "ipc-common.h"
@@ -45,21 +47,193 @@
static bdaddr_t adapter_addr;
static struct ipc *hal_ipc = NULL;
+static struct queue *apps = NULL;
-static void bt_health_register_app(const void *buf, uint16_t len)
+struct mdep_cfg {
+ uint8_t role;
+ uint16_t data_type;
+ uint8_t channel_type;
+ char *descr;
+
+ uint8_t id; /* mdep id */
+};
+
+struct health_app {
+ char *app_name;
+ char *provider_name;
+ char *service_name;
+ char *service_descr;
+ uint8_t num_of_mdep;
+ struct queue *mdeps;
+
+ uint8_t id; /* app id */
+};
+
+static void free_mdep_cfg(void *data)
{
- DBG("Not implemented");
+ struct mdep_cfg *cfg = data;
- ipc_send_rsp(hal_ipc, HAL_SERVICE_ID_HEALTH, HAL_OP_HEALTH_REG_APP,
- HAL_STATUS_UNSUPPORTED);
+ if (!cfg)
+ return;
+
+ free(cfg->descr);
+ free(cfg);
+}
+
+static void free_health_app(void *data)
+{
+ struct health_app *app = data;
+
+ if (!app)
+ return;
+
+ free(app->app_name);
+ free(app->provider_name);
+ free(app->service_name);
+ free(app->service_descr);
+ queue_destroy(app->mdeps, free_mdep_cfg);
+ free(app);
+}
+
+static bool app_by_app_id(const void *data, const void *user_data)
+{
+ const struct health_app *app = data;
+ uint16_t app_id = PTR_TO_INT(user_data);
+
+ return app->id == app_id;
+}
+
+static void bt_health_register_app(const void *buf, uint16_t buf_len)
+{
+ const struct hal_cmd_health_reg_app *cmd = buf;
+ struct hal_rsp_health_reg_app rsp;
+ struct health_app *app;
+ uint16_t off, len;
+
+ DBG("");
+
+ app = new0(struct health_app, 1);
Check if allocation succeed.
Ok.
+ app->id = queue_length(apps) + 1;
+ app->num_of_mdep = cmd->num_of_mdep;
+
+ off = 0;
+
+ if (cmd->provider_name_off)
+ len = cmd->provider_name_off;
+ else if (cmd->service_name_off)
+ len = cmd->service_name_off;
+ else if (cmd->service_descr_off)
+ len = cmd->service_descr_off;
+ else
+ len = cmd->len;
Offsets can be only increased so length is always next offset - current offset
ie.
app_name_len = provider_name_off - app_name_off
provider_name_len = service_name_off - provider_name_off;
etc.
Also how about factoring this to helper ie. create_health_app() ?
struct health_app *create_health_app(const uint8_t *app_name, uint16_t app_len,
const uint8_t *provider_name, uint16_t provider_len, ...., int mdeps)
and then just call it like:
app_name = cmd->data + cmd->app_name_off;
app_len = cmd->provider_name_off - cmd->app_name_off;
....
app = create_health_app(app_name, app_len, ....);
if (!app) {
status = HAL_STATUS_FAILED;
goto failed;
}
This should make code more readable.
Ok, make sense.
+
+ app->app_name = malloc0(len);
Check if allocation succeed.
Ok.
+ memcpy(app->app_name, cmd->data, len);
+ off += len;
+
+ if (cmd->provider_name_off) {
You should be checking for length, not offset here.
Ok.
+ if (cmd->service_name_off)
+ len = cmd->service_name_off - cmd->provider_name_off;
+ else if (cmd->service_descr_off)
+ len = cmd->service_descr_off - cmd->provider_name_off;
+ else
+ len = cmd->len - cmd->provider_name_off;
+
+ app->provider_name = malloc0(len);
+ memcpy(app->provider_name, cmd->data + off, len);
+ off += len;
+ } else {
+ app->provider_name = NULL;
Else is not needed as struct is already zeroed.
Ok.
+ }
+
+ if (cmd->service_name_off) {
+ if (cmd->service_descr_off)
+ len = cmd->service_descr_off - cmd->service_name_off;
+ else
+ len = cmd->len - cmd->service_name_off;
+
+ app->service_name = malloc0(len);
+ memcpy(app->service_name, cmd->data + off, len);
+ off += len;
+ } else {
+ app->service_name = NULL;
+ }
+
+ if (cmd->service_descr_off) {
+ len = cmd->len - cmd->service_descr_off;
+
+ app->service_descr = malloc0(len);
+ memcpy(app->service_descr, cmd->data + off, len);
+ off += len;
+ } else {
+ app->service_descr = NULL;
+ }
+
+ if (app->num_of_mdep > 0)
+ app->mdeps = queue_new();
I'd always allocate queue for sanity. It will just be empty if no mdeps.
Ok.
+
+ rsp.app_id = app->id;
+
+ if (!queue_push_tail(apps, app)) {
+ free_health_app(app);
+ ipc_send_rsp(hal_ipc, HAL_SERVICE_ID_HEALTH,
+ HAL_OP_HEALTH_REG_APP, HAL_STATUS_FAILED);
+ return;
+ }
+
+ ipc_send_rsp_full(hal_ipc, HAL_SERVICE_ID_HEALTH, HAL_OP_HEALTH_REG_APP,
+ sizeof(rsp), &rsp, -1);
}
static void bt_health_mdep_cfg_data(const void *buf, uint16_t len)
{
- DBG("Not implemented");
+ const struct hal_cmd_health_mdep *cmd = buf;
+ struct health_app *app;
+ struct mdep_cfg *mdep;
+ uint8_t status;
+
+ DBG("");
+
+ app = queue_find(apps, app_by_app_id, INT_TO_PTR(cmd->app_id));
+ if (!app) {
+ status = HAL_STATUS_INVALID;
+ goto fail;
+ }
+
+ mdep = new0(struct mdep_cfg, 1);
+ mdep->role = cmd->role;
+ mdep->data_type = cmd->data_type;
+ mdep->channel_type = cmd->channel_type;
+ mdep->id = queue_length(app->mdeps) + 1;
+ if (cmd->descr_len > 0) {
+ mdep->descr = malloc0(cmd->descr_len);
+ memcpy(mdep->descr, cmd->descr, cmd->descr_len);
+ }
+
+ if (!queue_push_tail(app->mdeps, mdep)) {
+ free_mdep_cfg(mdep);
+ status = HAL_STATUS_FAILED;
+ goto fail;
+ }
+
+ if (app->num_of_mdep != queue_length(app->mdeps)) {
+ status = HAL_STATUS_SUCCESS;
+ goto end;
+ }
+
+ /* TODO: Create MCAP instance and prepare SDP profile */
+ status = HAL_STATUS_SUCCESS;
+
+fail:
+ if (status != HAL_STATUS_SUCCESS) {
Don't do that. If this is failed label make it handle fail case only.
ie
ipc_send_rsp(.., SUCCESS);
return;
failed:
clean_ip();
ips_send_rsp(..., status);
Further code is in coming patches, I thought it would be good to put
TODO comments. Anyway I will change in next version.
+ queue_remove(apps, app);
+ free_health_app(app);
+ }
+
+end:
ipc_send_rsp(hal_ipc, HAL_SERVICE_ID_HEALTH, HAL_OP_HEALTH_MDEP,
- HAL_STATUS_UNSUPPORTED);
+ status);
}
static void bt_health_unregister_app(const void *buf, uint16_t len)
@@ -111,6 +285,7 @@ bool bt_health_register(struct ipc *ipc, const bdaddr_t *addr, uint8_t mode)
bacpy(&adapter_addr, addr);
hal_ipc = ipc;
+ apps = queue_new();
For sanity, you should check if this succeed.
Ok.
ipc_register(hal_ipc, HAL_SERVICE_ID_HEALTH, cmd_handlers,
G_N_ELEMENTS(cmd_handlers));
@@ -121,6 +296,7 @@ void bt_health_unregister(void)
{
DBG("");
+ queue_destroy(apps, free_health_app);
ipc_unregister(hal_ipc, HAL_SERVICE_ID_HEALTH);
hal_ipc = NULL;
}
Thanks,
Ravi.
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
