2013-10-29 12:16:51

by Jakub Tyszkowski

Subject: [PATCH 1/6] android: Remove reduntant structure

---
android/adapter.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/android/adapter.c b/android/adapter.c
index 15b65e5..7e5c1a1 100644
--- a/android/adapter.c
+++ b/android/adapter.c
@@ -234,12 +234,11 @@ static void load_link_keys(GSList *keys)
{
struct mgmt_cp_load_link_keys *cp;
size_t key_len = g_slist_length(keys);
- struct mgmt_link_key_info *key;
size_t len;

DBG("");

- len = sizeof(*cp) + key_len * sizeof(*key);
+ len = sizeof(*cp) + key_len * sizeof(struct mgmt_link_key_info);
cp = g_malloc0(len);

cp->debug_keys = 0;
--
1.8.4.1

2013-10-29 13:18:13

by Jakub Tyszkowski

Subject: Re: [PATCH 5/6] android/hal: Add device state changed event handler

Hi Andrei,

I think the props packing part can be extracted and reused. Not sure
if other events will benefit from this.. probably just adapter props
event?
Thanks, I'll fix those.

On 29 October 2013 13:36, Andrei Emeltchenko
<[email protected]> wrote:
> Hi Jakub,
>
> On Tue, Oct 29, 2013 at 01:16:55PM +0100, Jakub Tyszkowski wrote:
>> This is used to report property change of already reported remote
>> device.
>>
>> ---
>> android/hal-bluetooth.c | 26 ++++++++++++++++++++++++++
>> 1 file changed, 26 insertions(+)
>>
>> diff --git a/android/hal-bluetooth.c b/android/hal-bluetooth.c
>> index 261ae85..0fef680 100644
>> --- a/android/hal-bluetooth.c
>> +++ b/android/hal-bluetooth.c
>> @@ -114,6 +114,29 @@ static void handle_device_found(void *buf)
>> bt_hal_cbacks->device_found_cb(ev->num_props, send_props);
>> }
>>
>> +static void handle_device_state_changed(void *buf)
>> +{
>> + uint8_t i;
>
> I've got comment myself that first we put variables with assignments.
>
>> + struct hal_ev_remote_device_props *ev = buf;
>> + bt_property_t send_props[ev->num_props];
>> + struct hal_property *prop = ev->props;
>> +
>> + if (!bt_hal_cbacks->remote_device_properties_cb)
>> + return;
>> +
>> + /* repack props */
>> + for (i = 0; i < ev->num_props; ++i) {
>> + send_props[i].type = prop->type;
>> + send_props[i].len = prop->len;
>> + send_props[i].val = prop->val;
>> +
>> + prop = (void *) prop + (sizeof(*prop) + prop->len);
>> + }
>
> I would put empty line here. This looks a bit more readable then
> handle_adapter_props_changed. Do you think those 2 might have reused code?
>
> Best regards
> Andrei Emeltchenko
>
>
>> + bt_hal_cbacks->remote_device_properties_cb(ev->status,
>> + (bt_bdaddr_t *)ev->bdaddr,
>> + ev->num_props, send_props);
>> +}
>> +
>> /* will be called from notification thread context */
>> void bt_notify_adapter(uint16_t opcode, void *buf, uint16_t len)
>> {
>> @@ -133,6 +156,9 @@ void bt_notify_adapter(uint16_t opcode, void *buf, uint16_t len)
>> case HAL_EV_DEVICE_FOUND:
>> handle_device_found(buf);
>> break;
>> + case HAL_EV_REMOTE_DEVICE_PROPS:
>> + handle_device_state_changed(buf);
>> + break;
>> default:
>> DBG("Unhandled callback opcode=0x%x", opcode);
>> break;
>> --
>> 1.8.4.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
>> the body of a message to [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html

2013-10-29 13:01:17

by Jakub Tyszkowski

Subject: Re: [PATCH 1/6] android: Remove reduntant structure

Hi Johan,

Please ignore this patch then. I'll fix this or Szymon will.

On 29 October 2013 13:28, Johan Hedberg <[email protected]> wrote:
> Hi Jakub,
>
> On Tue, Oct 29, 2013, Jakub Tyszkowski wrote:
>> ---
>> android/adapter.c | 3 +--
>> 1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/android/adapter.c b/android/adapter.c
>> index 15b65e5..7e5c1a1 100644
>> --- a/android/adapter.c
>> +++ b/android/adapter.c
>> @@ -234,12 +234,11 @@ static void load_link_keys(GSList *keys)
>> {
>> struct mgmt_cp_load_link_keys *cp;
>> size_t key_len = g_slist_length(keys);
>> - struct mgmt_link_key_info *key;
>> size_t len;
>>
>> DBG("");
>>
>> - len = sizeof(*cp) + key_len * sizeof(*key);
>> + len = sizeof(*cp) + key_len * sizeof(struct mgmt_link_key_info);
>> cp = g_malloc0(len);
>>
>> cp->debug_keys = 0;
>
> If the point of the keys list is to contain struct mgmt_link_key_info
> entries I'd rather fix this function to properly fill in the mgmt
> command with those since right now it's broken if the list contains any
> entries at all.
>
> Johan

2013-10-29 12:36:24

by Andrei Emeltchenko

Subject: Re: [PATCH 5/6] android/hal: Add device state changed event handler

Hi Jakub,

On Tue, Oct 29, 2013 at 01:16:55PM +0100, Jakub Tyszkowski wrote:
> This is used to report property change of already reported remote
> device.
>
> ---
> android/hal-bluetooth.c | 26 ++++++++++++++++++++++++++
> 1 file changed, 26 insertions(+)
>
> diff --git a/android/hal-bluetooth.c b/android/hal-bluetooth.c
> index 261ae85..0fef680 100644
> --- a/android/hal-bluetooth.c
> +++ b/android/hal-bluetooth.c
> @@ -114,6 +114,29 @@ static void handle_device_found(void *buf)
> bt_hal_cbacks->device_found_cb(ev->num_props, send_props);
> }
>
> +static void handle_device_state_changed(void *buf)
> +{
> + uint8_t i;

I've got comment myself that first we put variables with assignments.

> + struct hal_ev_remote_device_props *ev = buf;
> + bt_property_t send_props[ev->num_props];
> + struct hal_property *prop = ev->props;
> +
> + if (!bt_hal_cbacks->remote_device_properties_cb)
> + return;
> +
> + /* repack props */
> + for (i = 0; i < ev->num_props; ++i) {
> + send_props[i].type = prop->type;
> + send_props[i].len = prop->len;
> + send_props[i].val = prop->val;
> +
> + prop = (void *) prop + (sizeof(*prop) + prop->len);
> + }

I would put empty line here. This looks a bit more readable then
handle_adapter_props_changed. Do you think those 2 might have reused code?

Best regards
Andrei Emeltchenko


> + bt_hal_cbacks->remote_device_properties_cb(ev->status,
> + (bt_bdaddr_t *)ev->bdaddr,
> + ev->num_props, send_props);
> +}
> +
> /* will be called from notification thread context */
> void bt_notify_adapter(uint16_t opcode, void *buf, uint16_t len)
> {
> @@ -133,6 +156,9 @@ void bt_notify_adapter(uint16_t opcode, void *buf, uint16_t len)
> case HAL_EV_DEVICE_FOUND:
> handle_device_found(buf);
> break;
> + case HAL_EV_REMOTE_DEVICE_PROPS:
> + handle_device_state_changed(buf);
> + break;
> default:
> DBG("Unhandled callback opcode=0x%x", opcode);
> break;
> --
> 1.8.4.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html

2013-10-29 12:28:27

by Johan Hedberg

Subject: Re: [PATCH 1/6] android: Remove reduntant structure

Hi Jakub,

On Tue, Oct 29, 2013, Jakub Tyszkowski wrote:
> ---
> android/adapter.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/android/adapter.c b/android/adapter.c
> index 15b65e5..7e5c1a1 100644
> --- a/android/adapter.c
> +++ b/android/adapter.c
> @@ -234,12 +234,11 @@ static void load_link_keys(GSList *keys)
> {
> struct mgmt_cp_load_link_keys *cp;
> size_t key_len = g_slist_length(keys);
> - struct mgmt_link_key_info *key;
> size_t len;
>
> DBG("");
>
> - len = sizeof(*cp) + key_len * sizeof(*key);
> + len = sizeof(*cp) + key_len * sizeof(struct mgmt_link_key_info);
> cp = g_malloc0(len);
>
> cp->debug_keys = 0;

If the point of the keys list is to contain struct mgmt_link_key_info
entries I'd rather fix this function to properly fill in the mgmt
command with those since right now it's broken if the list contains any
entries at all.

Johan

2013-10-29 12:16:56

by Jakub Tyszkowski

Subject: [PATCH 6/6] android: Add missing discovery state definitions to IPC header

---
android/hal-msg.h | 3 +++
1 file changed, 3 insertions(+)

diff --git a/android/hal-msg.h b/android/hal-msg.h
index a4eb2a8..ec4b62a 100644
--- a/android/hal-msg.h
+++ b/android/hal-msg.h
@@ -348,6 +348,9 @@ struct hal_ev_device_found {
struct hal_property props[0];
} __attribute__((packed));

+#define HAL_DISCOVERY_STATE_STOPPED 0x00
+#define HAL_DISCOVERY_STATE_STARTED 0x01
+
#define HAL_EV_DISCOVERY_STATE_CHANGED 0x85
struct hal_ev_discovery_state_changed {
uint8_t state;
--
1.8.4.1

2013-10-29 12:16:55

by Jakub Tyszkowski

Subject: [PATCH 5/6] android/hal: Add device state changed event handler

This is used to report property change of already reported remote
device.

---
android/hal-bluetooth.c | 26 ++++++++++++++++++++++++++
1 file changed, 26 insertions(+)

diff --git a/android/hal-bluetooth.c b/android/hal-bluetooth.c
index 261ae85..0fef680 100644
--- a/android/hal-bluetooth.c
+++ b/android/hal-bluetooth.c
@@ -114,6 +114,29 @@ static void handle_device_found(void *buf)
bt_hal_cbacks->device_found_cb(ev->num_props, send_props);
}

+static void handle_device_state_changed(void *buf)
+{
+ uint8_t i;
+ struct hal_ev_remote_device_props *ev = buf;
+ bt_property_t send_props[ev->num_props];
+ struct hal_property *prop = ev->props;
+
+ if (!bt_hal_cbacks->remote_device_properties_cb)
+ return;
+
+ /* repack props */
+ for (i = 0; i < ev->num_props; ++i) {
+ send_props[i].type = prop->type;
+ send_props[i].len = prop->len;
+ send_props[i].val = prop->val;
+
+ prop = (void *) prop + (sizeof(*prop) + prop->len);
+ }
+ bt_hal_cbacks->remote_device_properties_cb(ev->status,
+ (bt_bdaddr_t *)ev->bdaddr,
+ ev->num_props, send_props);
+}
+
/* will be called from notification thread context */
void bt_notify_adapter(uint16_t opcode, void *buf, uint16_t len)
{
@@ -133,6 +156,9 @@ void bt_notify_adapter(uint16_t opcode, void *buf, uint16_t len)
case HAL_EV_DEVICE_FOUND:
handle_device_found(buf);
break;
+ case HAL_EV_REMOTE_DEVICE_PROPS:
+ handle_device_state_changed(buf);
+ break;
default:
DBG("Unhandled callback opcode=0x%x", opcode);
break;
--
1.8.4.1

2013-10-29 12:16:54

by Jakub Tyszkowski

Subject: [PATCH 4/6] android/hal: Add device found event handler

This is called when new remote device is found.

---
android/hal-bluetooth.c | 24 ++++++++++++++++++++++++
1 file changed, 24 insertions(+)

diff --git a/android/hal-bluetooth.c b/android/hal-bluetooth.c
index b13f4d7..261ae85 100644
--- a/android/hal-bluetooth.c
+++ b/android/hal-bluetooth.c
@@ -93,6 +93,27 @@ static void handle_discovery_state_changed(void *buf)
bt_hal_cbacks->discovery_state_changed_cb(ev->state);
}

+static void handle_device_found(void *buf)
+{
+ uint8_t i;
+ struct hal_ev_device_found *ev = buf;
+ bt_property_t send_props[ev->num_props];
+ struct hal_property *prop = ev->props;
+
+ if (!bt_hal_cbacks->device_found_cb)
+ return;
+
+ /* repack props */
+ for (i = 0; i < ev->num_props; ++i) {
+ send_props[i].type = prop->type;
+ send_props[i].len = prop->len;
+ send_props[i].val = prop->val;
+
+ prop = (void *) prop + (sizeof(*prop) + prop->len);
+ }
+ bt_hal_cbacks->device_found_cb(ev->num_props, send_props);
+}
+
/* will be called from notification thread context */
void bt_notify_adapter(uint16_t opcode, void *buf, uint16_t len)
{
@@ -109,6 +130,9 @@ void bt_notify_adapter(uint16_t opcode, void *buf, uint16_t len)
case HAL_EV_DISCOVERY_STATE_CHANGED:
handle_discovery_state_changed(buf);
break;
+ case HAL_EV_DEVICE_FOUND:
+ handle_device_found(buf);
+ break;
default:
DBG("Unhandled callback opcode=0x%x", opcode);
break;
--
1.8.4.1

2013-10-29 12:16:53

by Jakub Tyszkowski

Subject: [PATCH 3/6] android/hal: Add support for handling discovery state change event

---
android/hal-bluetooth.c | 11 +++++++++++
1 file changed, 11 insertions(+)

diff --git a/android/hal-bluetooth.c b/android/hal-bluetooth.c
index aa104ba..b13f4d7 100644
--- a/android/hal-bluetooth.c
+++ b/android/hal-bluetooth.c
@@ -85,6 +85,14 @@ static bool interface_ready(void)
return bt_hal_cbacks != NULL;
}

+static void handle_discovery_state_changed(void *buf)
+{
+ struct hal_ev_discovery_state_changed *ev = buf;
+
+ if (bt_hal_cbacks->discovery_state_changed_cb)
+ bt_hal_cbacks->discovery_state_changed_cb(ev->state);
+}
+
/* will be called from notification thread context */
void bt_notify_adapter(uint16_t opcode, void *buf, uint16_t len)
{
@@ -98,6 +106,9 @@ void bt_notify_adapter(uint16_t opcode, void *buf, uint16_t len)
case HAL_EV_ADAPTER_PROPS_CHANGED:
handle_adapter_props_changed(buf, len);
break;
+ case HAL_EV_DISCOVERY_STATE_CHANGED:
+ handle_discovery_state_changed(buf);
+ break;
default:
DBG("Unhandled callback opcode=0x%x", opcode);
break;
--
1.8.4.1

2013-10-29 12:16:52

by Jakub Tyszkowski

Subject: [PATCH 2/6] android/hal: Add support for start/cancel device discovery commands

---
android/hal-bluetooth.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/android/hal-bluetooth.c b/android/hal-bluetooth.c
index 5929fff..aa104ba 100644
--- a/android/hal-bluetooth.c
+++ b/android/hal-bluetooth.c
@@ -308,7 +308,9 @@ static int start_discovery(void)
if (!interface_ready())
return BT_STATUS_NOT_READY;

- return BT_STATUS_UNSUPPORTED;
+ return hal_ipc_cmd(HAL_SERVICE_ID_BLUETOOTH,
+ HAL_OP_START_DISCOVERY, 0, NULL, 0,
+ NULL, NULL);
}

static int cancel_discovery(void)
@@ -318,7 +320,9 @@ static int cancel_discovery(void)
if (!interface_ready())
return BT_STATUS_NOT_READY;

- return BT_STATUS_UNSUPPORTED;
+ return hal_ipc_cmd(HAL_SERVICE_ID_BLUETOOTH,
+ HAL_OP_CANCEL_DISCOVERY, 0, NULL, 0,
+ NULL, NULL);
}

static int create_bond(const bt_bdaddr_t *bd_addr)
--
1.8.4.1