---
android/pan.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/android/pan.c b/android/pan.c
index 2a11f46..fba86b8 100644
--- a/android/pan.c
+++ b/android/pan.c
@@ -58,7 +58,8 @@ static uint8_t bt_pan_connect(struct hal_cmd_pan_connect *cmd, uint16_t len)
return HAL_STATUS_FAILED;
}
-static uint8_t bt_pan_disconnect(struct hal_cmd_pan_connect *cmd, uint16_t len)
+static uint8_t bt_pan_disconnect(struct hal_cmd_pan_disconnect *cmd,
+ uint16_t len)
{
DBG("Not Implemented");
--
1.8.3.2
Hi Johan,
On 11/19/2013 03:35 PM, Johan Hedberg wrote:
> Hi Ravi,
>
> On Tue, Nov 19, 2013, Ravi kumar Veeramally wrote:
>> ---
>> android/hidhost.c | 10 ++++++++++
>> 1 file changed, 10 insertions(+)
>>
>> diff --git a/android/hidhost.c b/android/hidhost.c
>> index 05a3fe4..20dedc4 100644
>> --- a/android/hidhost.c
>> +++ b/android/hidhost.c
>> @@ -1226,6 +1226,14 @@ bool bt_hid_register(int sk, const bdaddr_t *addr)
>> return true;
>> }
>>
>> +static void free_hid_devices(gpointer data, gpointer user_data)
>> +{
>> + struct hid_device *dev = data;
>> +
>> + bt_hid_notify_state(dev, HAL_HIDHOST_STATE_DISCONNECTED);
>> + hid_device_free(dev);
>> +}
>> +
>> void bt_hid_unregister(void)
>> {
>> DBG("");
>> @@ -1233,6 +1241,8 @@ void bt_hid_unregister(void)
>> if (notification_sk < 0)
>> return;
>>
>> + g_slist_foreach(devices, free_hid_devices, NULL);
> I suppose you also need to set devices = NULL; after this call?
Sure.
Thanks,
Ravi.
Hi Johan,
On 11/19/2013 03:31 PM, Johan Hedberg wrote:
> Hi Ravi,
>
> On Tue, Nov 19, 2013, Ravi kumar Veeramally wrote:
>> ---
>> android/hidhost.c | 9 ++++++---
>> 1 file changed, 6 insertions(+), 3 deletions(-)
> Could you add a bit more descriptive commit message to this one which
> explains what was broken (and how) and how your patch fixes it. When I
> get a patch with an empty commit message I expect it to be very trivial
> and instantly understandable, but that's not the case here.
I will write more details.
Regards,
Ravi.
Hi,
On 11/19/2013 03:30 PM, Johan Hedberg wrote:
> Hi Ravi,
>
> On Tue, Nov 19, 2013, Ravi kumar Veeramally wrote:
>> @@ -2275,6 +2275,9 @@ bool bt_bluetooth_register(int sk)
>> {
>> DBG("");
>>
>> + if (notification_sk > 0)
> 0 is a valid file descriptor value so the check should be >= 0
>
>> @@ -1190,6 +1190,9 @@ bool bt_hid_register(int sk, const bdaddr_t *addr)
>>
>> DBG("");
>>
>> + if (notification_sk > 0)
>> + return false;
> Same here.
>
>> @@ -95,6 +95,9 @@ bool bt_pan_register(int sk, const bdaddr_t *addr)
>> {
>> DBG("");
>>
>> + if (notification_sk > 0)
>> + return false;
> And here.
>
> Johan
>
Ok.
Thanks,
Ravi.
On 11/19/2013 03:27 PM, Johan Hedberg wrote:
> Hi Ravi,
>
> On Tue, Nov 19, 2013, Ravi kumar Veeramally wrote:
>> ---
>> android/hal-pan.c | 11 +++++++++++
>> 1 file changed, 11 insertions(+)
>>
>> diff --git a/android/hal-pan.c b/android/hal-pan.c
>> index 2bc560e..30facd4 100644
>> --- a/android/hal-pan.c
>> +++ b/android/hal-pan.c
>> @@ -77,6 +77,9 @@ static bt_status_t pan_enable(int local_role)
>> if (!interface_ready())
>> return BT_STATUS_NOT_READY;
>>
>> + if (!(local_role == BTPAN_ROLE_PANU || local_role == BTPAN_ROLE_PANNAP))
>> + return BT_STATUS_UNSUPPORTED;
>> +
>> cmd.local_role = local_role;
>>
>> return hal_ipc_cmd(HAL_SERVICE_ID_PAN, HAL_OP_PAN_ENABLE,
>> @@ -112,6 +115,14 @@ static bt_status_t pan_connect(const bt_bdaddr_t *bd_addr, int local_role,
>> if (!interface_ready())
>> return BT_STATUS_NOT_READY;
>>
>> + if (!((local_role == BTPAN_ROLE_PANNAP &&
>> + remote_role == BTPAN_ROLE_PANU) ||
>> + (local_role == BTPAN_ROLE_PANU &&
>> + remote_role == BTPAN_ROLE_PANNAP) ||
>> + (local_role == BTPAN_ROLE_PANU &&
>> + remote_role == BTPAN_ROLE_PANU)))
>> + return BT_STATUS_UNSUPPORTED;
> First of all you've got incorrect indentation here which make this hard
> to read (the return statement being on the same column as parts of the
> if-statement. When you break lines indent at least by two tabs to make
> a clear separation from the actual branch code.
Ok.
>
> Secondly, shouldn't we be checking that the given local role has been
> enabled (through pan_enable)? Or does the daemon do that checking? In
> fact is there a clear reason not to let the daemon do all the
> verification checks and return an error status over IPC?
I thought it would be easier to do basic checks on supported combinations
at HAL level to reduce the ipc traffic.
I don't know exactly whether UI can send these combinations, but we
can at least
try with haltest tool.
>
> Thirdly, this if-statement takes a while to parse, so I'm wondering
> whether it'd be clearer to format it in a bit more readable way, e.g.:
>
> switch (local_role) {
> case BTPAN_ROLE_PANNAP:
> if (remote_role != BTPAN_ROLE_PANU)
> return BT_STATUS_UNSUPPORTED;
> break;
> case BTPAN_ROLE_PANU:
> if (remote_role != BTPAN_ROLE_PANNAP &&
> remote_role != BTPAN_ROLE_PANU)
> return BT_STATUS_UNSUPPORTED;
> break;
> default:
> return BT_STATUS_UNSUPPORTED;
> }
>
> Thoughts?
Yes, make sense to use 'switch' for more readability.
Is it ok to send _v2 with switch or better to handle in daemon?
Thanks,
Ravi.
Hi Ravi,
On Tue, Nov 19, 2013, Ravi kumar Veeramally wrote:
> ---
> android/hidhost.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/android/hidhost.c b/android/hidhost.c
> index 05a3fe4..20dedc4 100644
> --- a/android/hidhost.c
> +++ b/android/hidhost.c
> @@ -1226,6 +1226,14 @@ bool bt_hid_register(int sk, const bdaddr_t *addr)
> return true;
> }
>
> +static void free_hid_devices(gpointer data, gpointer user_data)
> +{
> + struct hid_device *dev = data;
> +
> + bt_hid_notify_state(dev, HAL_HIDHOST_STATE_DISCONNECTED);
> + hid_device_free(dev);
> +}
> +
> void bt_hid_unregister(void)
> {
> DBG("");
> @@ -1233,6 +1241,8 @@ void bt_hid_unregister(void)
> if (notification_sk < 0)
> return;
>
> + g_slist_foreach(devices, free_hid_devices, NULL);
I suppose you also need to set devices = NULL; after this call?
Johan
Hi Ravi,
On Tue, Nov 19, 2013, Ravi kumar Veeramally wrote:
> ---
> android/hidhost.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
Could you add a bit more descriptive commit message to this one which
explains what was broken (and how) and how your patch fixes it. When I
get a patch with an empty commit message I expect it to be very trivial
and instantly understandable, but that's not the case here.
Johan
Hi Ravi,
On Tue, Nov 19, 2013, Ravi kumar Veeramally wrote:
> @@ -2275,6 +2275,9 @@ bool bt_bluetooth_register(int sk)
> {
> DBG("");
>
> + if (notification_sk > 0)
0 is a valid file descriptor value so the check should be >= 0
> @@ -1190,6 +1190,9 @@ bool bt_hid_register(int sk, const bdaddr_t *addr)
>
> DBG("");
>
> + if (notification_sk > 0)
> + return false;
Same here.
> @@ -95,6 +95,9 @@ bool bt_pan_register(int sk, const bdaddr_t *addr)
> {
> DBG("");
>
> + if (notification_sk > 0)
> + return false;
And here.
Johan
Hi Ravi,
On Tue, Nov 19, 2013, Ravi kumar Veeramally wrote:
> ---
> android/hal-pan.c | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/android/hal-pan.c b/android/hal-pan.c
> index 2bc560e..30facd4 100644
> --- a/android/hal-pan.c
> +++ b/android/hal-pan.c
> @@ -77,6 +77,9 @@ static bt_status_t pan_enable(int local_role)
> if (!interface_ready())
> return BT_STATUS_NOT_READY;
>
> + if (!(local_role == BTPAN_ROLE_PANU || local_role == BTPAN_ROLE_PANNAP))
> + return BT_STATUS_UNSUPPORTED;
> +
> cmd.local_role = local_role;
>
> return hal_ipc_cmd(HAL_SERVICE_ID_PAN, HAL_OP_PAN_ENABLE,
> @@ -112,6 +115,14 @@ static bt_status_t pan_connect(const bt_bdaddr_t *bd_addr, int local_role,
> if (!interface_ready())
> return BT_STATUS_NOT_READY;
>
> + if (!((local_role == BTPAN_ROLE_PANNAP &&
> + remote_role == BTPAN_ROLE_PANU) ||
> + (local_role == BTPAN_ROLE_PANU &&
> + remote_role == BTPAN_ROLE_PANNAP) ||
> + (local_role == BTPAN_ROLE_PANU &&
> + remote_role == BTPAN_ROLE_PANU)))
> + return BT_STATUS_UNSUPPORTED;
First of all you've got incorrect indentation here which make this hard
to read (the return statement being on the same column as parts of the
if-statement. When you break lines indent at least by two tabs to make
a clear separation from the actual branch code.
Secondly, shouldn't we be checking that the given local role has been
enabled (through pan_enable)? Or does the daemon do that checking? In
fact is there a clear reason not to let the daemon do all the
verification checks and return an error status over IPC?
Thirdly, this if-statement takes a while to parse, so I'm wondering
whether it'd be clearer to format it in a bit more readable way, e.g.:
switch (local_role) {
case BTPAN_ROLE_PANNAP:
if (remote_role != BTPAN_ROLE_PANU)
return BT_STATUS_UNSUPPORTED;
break;
case BTPAN_ROLE_PANU:
if (remote_role != BTPAN_ROLE_PANNAP &&
remote_role != BTPAN_ROLE_PANU)
return BT_STATUS_UNSUPPORTED;
break;
default:
return BT_STATUS_UNSUPPORTED;
}
Thoughts?
Johan
Hi Ravi,
On Tue, Nov 19, 2013, Ravi kumar Veeramally wrote:
> ---
> android/pan.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
This patch has been applied. Thanks.
Johan
---
android/hidhost.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/android/hidhost.c b/android/hidhost.c
index 05a3fe4..20dedc4 100644
--- a/android/hidhost.c
+++ b/android/hidhost.c
@@ -1226,6 +1226,14 @@ bool bt_hid_register(int sk, const bdaddr_t *addr)
return true;
}
+static void free_hid_devices(gpointer data, gpointer user_data)
+{
+ struct hid_device *dev = data;
+
+ bt_hid_notify_state(dev, HAL_HIDHOST_STATE_DISCONNECTED);
+ hid_device_free(dev);
+}
+
void bt_hid_unregister(void)
{
DBG("");
@@ -1233,6 +1241,8 @@ void bt_hid_unregister(void)
if (notification_sk < 0)
return;
+ g_slist_foreach(devices, free_hid_devices, NULL);
+
notification_sk = -1;
if (ctrl_io) {
--
1.8.3.2
---
android/hidhost.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/android/hidhost.c b/android/hidhost.c
index a929a94..05a3fe4 100644
--- a/android/hidhost.c
+++ b/android/hidhost.c
@@ -522,7 +522,6 @@ static int uhid_create(struct hid_device *dev)
dev->uhid_fd = open(UHID_DEVICE_FILE, O_RDWR | O_CLOEXEC);
if (dev->uhid_fd < 0) {
error("Failed to open uHID device: %s", strerror(errno));
- bt_hid_notify_state(dev, HAL_HIDHOST_STATE_NO_HID);
return -errno;
}
@@ -541,7 +540,6 @@ static int uhid_create(struct hid_device *dev)
error("Failed to create uHID device: %s", strerror(errno));
close(dev->uhid_fd);
dev->uhid_fd = -1;
- bt_hid_notify_state(dev, HAL_HIDHOST_STATE_NO_HID);
return -errno;
}
@@ -559,16 +557,20 @@ static void interrupt_connect_cb(GIOChannel *chan, GError *conn_err,
gpointer user_data)
{
struct hid_device *dev = user_data;
+ uint8_t state;
DBG("");
if (conn_err) {
error("%s", conn_err->message);
+ state = HAL_HIDHOST_STATE_FAILED;
goto failed;
}
- if (uhid_create(dev) < 0)
+ if (uhid_create(dev) < 0) {
+ state = HAL_HIDHOST_STATE_NO_HID;
goto failed;
+ }
dev->intr_watch = g_io_add_watch(dev->intr_io,
G_IO_IN | G_IO_HUP | G_IO_ERR | G_IO_NVAL,
@@ -579,6 +581,7 @@ static void interrupt_connect_cb(GIOChannel *chan, GError *conn_err,
return;
failed:
+ bt_hid_notify_state(dev, state);
hid_device_free(dev);
}
--
1.8.3.2
This can be tested with haltest.
---
android/a2dp.c | 6 ++++++
android/bluetooth.c | 6 ++++++
android/hidhost.c | 6 ++++++
android/pan.c | 6 ++++++
4 files changed, 24 insertions(+)
diff --git a/android/a2dp.c b/android/a2dp.c
index 74d0082..28a1250 100644
--- a/android/a2dp.c
+++ b/android/a2dp.c
@@ -332,6 +332,9 @@ bool bt_a2dp_register(int sk, const bdaddr_t *addr)
DBG("");
+ if (notification_sk > 0)
+ return false;
+
bacpy(&adapter_addr, addr);
server = bt_io_listen(connect_cb, NULL, NULL, NULL, &err,
@@ -365,6 +368,9 @@ void bt_a2dp_unregister(void)
{
DBG("");
+ if (notification_sk < 0)
+ return;
+
notification_sk = -1;
bt_adapter_remove_record(record_id);
diff --git a/android/bluetooth.c b/android/bluetooth.c
index 7dc2ec3..7805d42 100644
--- a/android/bluetooth.c
+++ b/android/bluetooth.c
@@ -2275,6 +2275,9 @@ bool bt_bluetooth_register(int sk)
{
DBG("");
+ if (notification_sk > 0)
+ return false;
+
notification_sk = sk;
return true;
@@ -2284,5 +2287,8 @@ void bt_bluetooth_unregister(void)
{
DBG("");
+ if (notification_sk < 0)
+ return;
+
notification_sk = -1;
}
diff --git a/android/hidhost.c b/android/hidhost.c
index 842b8ad..a929a94 100644
--- a/android/hidhost.c
+++ b/android/hidhost.c
@@ -1190,6 +1190,9 @@ bool bt_hid_register(int sk, const bdaddr_t *addr)
DBG("");
+ if (notification_sk > 0)
+ return false;
+
bacpy(&adapter_addr, addr);
ctrl_io = bt_io_listen(connect_cb, NULL, NULL, NULL, &err,
@@ -1224,6 +1227,9 @@ void bt_hid_unregister(void)
{
DBG("");
+ if (notification_sk < 0)
+ return;
+
notification_sk = -1;
if (ctrl_io) {
diff --git a/android/pan.c b/android/pan.c
index fba86b8..970a8a7 100644
--- a/android/pan.c
+++ b/android/pan.c
@@ -95,6 +95,9 @@ bool bt_pan_register(int sk, const bdaddr_t *addr)
{
DBG("");
+ if (notification_sk > 0)
+ return false;
+
notification_sk = sk;
return true;
@@ -104,5 +107,8 @@ void bt_pan_unregister(void)
{
DBG("");
+ if (notification_sk < 0)
+ return;
+
notification_sk = -1;
}
--
1.8.3.2
---
android/hal-pan.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/android/hal-pan.c b/android/hal-pan.c
index 2bc560e..30facd4 100644
--- a/android/hal-pan.c
+++ b/android/hal-pan.c
@@ -77,6 +77,9 @@ static bt_status_t pan_enable(int local_role)
if (!interface_ready())
return BT_STATUS_NOT_READY;
+ if (!(local_role == BTPAN_ROLE_PANU || local_role == BTPAN_ROLE_PANNAP))
+ return BT_STATUS_UNSUPPORTED;
+
cmd.local_role = local_role;
return hal_ipc_cmd(HAL_SERVICE_ID_PAN, HAL_OP_PAN_ENABLE,
@@ -112,6 +115,14 @@ static bt_status_t pan_connect(const bt_bdaddr_t *bd_addr, int local_role,
if (!interface_ready())
return BT_STATUS_NOT_READY;
+ if (!((local_role == BTPAN_ROLE_PANNAP &&
+ remote_role == BTPAN_ROLE_PANU) ||
+ (local_role == BTPAN_ROLE_PANU &&
+ remote_role == BTPAN_ROLE_PANNAP) ||
+ (local_role == BTPAN_ROLE_PANU &&
+ remote_role == BTPAN_ROLE_PANU)))
+ return BT_STATUS_UNSUPPORTED;
+
memcpy(cmd.bdaddr, bd_addr, sizeof(cmd.bdaddr));
cmd.local_role = local_role;
cmd.remote_role = remote_role;
--
1.8.3.2