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.
--
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
