RE: [PATCH] HCI Commands for LE White List

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 

Hi Claudio,

 
> Could you please split this patch into minor functional patches?
Ok. Would resubmit four patches now-one each for Add Device, Remove Device, Clear White List and Read White List size.

> 
> On Thu, Jan 6, 2011 at 12:52 AM, Sumit Kumar BAJPAI
> <sumitkumar.bajpai@xxxxxxxxxxxxxx> wrote:
> > Added HCI commands for LE White List support.
> > These LE White List Commands can be tested from
> > hcitool.
> >
> >
> > ---
> > Âlib/hci.c    | Â107 +++++++++++++++++++++++++++++++
> > Âlib/hci_lib.h  |  Â4 +
> > Âtools/hcitool.c | Â186
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > Â3 files changed, 297 insertions(+), 0 deletions(-)
> > Âmode change 100644 => 100755 lib/hci.c
> > Âmode change 100644 => 100755 lib/hci_lib.h
> >
> > diff --git a/lib/hci.c b/lib/hci.c
> > old mode 100644
> > new mode 100755
> > index 048fda4..5a0275c
> > --- a/lib/hci.c
> > +++ b/lib/hci.c
> > @@ -1291,6 +1291,113 @@ int hci_disconnect(int dd, uint16_t handle,
> uint8_t reason, int to)
> > Â Â Â Âreturn 0;
> > Â}
> >
> > +int hci_le_add_to_white_list(int dd, bdaddr_t addr, uint8_t type)
> Use pointer for bdaddr_t
> address types have defined values in the BT spec, declare an enum in
> hci.h
> 
Ok.
> > +{
> > + Â Â Â struct hci_request rq;
> > + Â Â Â le_add_device_to_white_list_cp param_cp;
> It seems that you copied this declaration from set scan parameters
> function. Use only "cp", it is short for command parameters.
> 
Ok.
> > + Â Â Â uint8_t status;
> > +
> > + Â Â Â memset(&param_cp, 0, sizeof(param_cp));
> > + Â Â Â param_cp.bdaddr_type = type;
> > + Â Â Â param_cp.bdaddr= addr;
> > +
> > + Â Â Â memset(&rq, 0, sizeof(rq));
> > + Â Â Â rq.ogf = OGF_LE_CTL;
> > + Â Â Â rq.ocf = OCF_LE_ADD_DEVICE_TO_WHITE_LIST;
> > + Â Â Â rq.cparam = &param_cp;
> > + Â Â Â rq.clen = LE_ADD_DEVICE_TO_WHITE_LIST_CP_SIZE;
> > + Â Â Â rq.rparam = &status;
> > + Â Â Â rq.rlen = 1;
> > +
> > + Â Â Â if (hci_send_req(dd, &rq, 100) < 0)
> > + Â Â Â Â Â Â Â return -1;
> Use 1000, we are noticing timeout on some LE hardwares.
Ok. but would recommend defining it as a macro somewhere in hci.h for easy manipulation later. Would re-submit the patch using 1000 for now.

> > +
> > + Â Â Â if (status) {
> > + Â Â Â Â Â Â Â errno = EIO;
> > + Â Â Â Â Â Â Â return -1;
> > + Â Â Â }
> > +
> > + Â Â Â return 0;
> > +}
> > +
> > +int hci_le_remove_from_white_list(int dd, bdaddr_t addr, uint8_t
> type)
> > +{
> Same comment about address type and bdaddr_t pointer
Ok.
> 
> > + Â Â Â struct hci_request rq;
> > + Â Â Â le_remove_device_from_white_list_cp param_cp;
> Same comment about param_cp: s/param_cp/cp
> 
Ok.

> > + Â Â Â uint8_t status;
> > +
> > + Â Â Â memset(&param_cp, 0, sizeof(param_cp));
> > + Â Â Â param_cp.bdaddr_type = type;
> > + Â Â Â param_cp.bdaddr= addr;
> > +
> > + Â Â Â memset(&rq, 0, sizeof(rq));
> > + Â Â Â rq.ogf = OGF_LE_CTL;
> > + Â Â Â rq.ocf = OCF_LE_REMOVE_DEVICE_FROM_WHITE_LIST;
> > + Â Â Â rq.cparam = &param_cp;
> > + Â Â Â rq.clen = LE_REMOVE_DEVICE_FROM_WHITE_LIST_CP_SIZE;
> > + Â Â Â rq.rparam = &status;
> > + Â Â Â rq.rlen = 1;
> > +
> > + Â Â Â if (hci_send_req(dd, &rq, 100) < 0)
> > + Â Â Â Â Â Â Â return -1;
> Use 1000
> 
Ok.

> > +
> > + Â Â Â if (status) {
> > + Â Â Â Â Â Â Â errno = EIO;
> > + Â Â Â Â Â Â Â return -1;
> > + Â Â Â }
> > +
> > + Â Â Â return 0;
> > +}
> > +
> > +int hci_le_read_white_list_size(int dd)
> > +{
> > + Â Â Â struct hci_request rq;
> > + Â Â Â le_read_white_list_size_rp param_cp;
> Rename param_cp to rp
> 
Ok.
> > +
> > + Â Â Â memset(&param_cp, 0, sizeof(param_cp));
> > + Â Â Â param_cp.size=0;
> > +
> > + Â Â Â memset(&rq, 0, sizeof(rq));
> > + Â Â Â rq.ogf = OGF_LE_CTL;
> > + Â Â Â rq.ocf = OCF_LE_READ_WHITE_LIST_SIZE;
> > + Â Â Â rq.rparam = &param_cp;
> > + Â Â Â rq.rlen = LE_READ_WHITE_LIST_SIZE_RP_SIZE;
> > +
> > + Â Â Â if (hci_send_req(dd, &rq, 100) < 0)
> > + Â Â Â Â Â Â Â return -1;
> > +
> > + Â Â Â if (param_cp.status) {
> > + Â Â Â Â Â Â Â errno = EIO;
> > + Â Â Â Â Â Â Â return -1;
> > + Â Â Â }
> > +
> > + Â Â Â printf("LE White list size= %d\n", param_cp.size);
> printf? the size should be returned to the caller.
> Suggestion: add a pointer in the function parameters
> if (size)
>     *size = param_cp.size;
>
Yup. Would take care while re-submitting the patches.
 
> > +
> > + Â Â Â return 0;
> > +}
> > +
> > +int hci_le_clear_white_list(int dd)
> > +{
> > + Â Â Â struct hci_request rq;
> > + Â Â Â uint8_t status;
> > +
> > + Â Â Â memset(&rq, 0, sizeof(rq));
> > + Â Â Â rq.ogf = OGF_LE_CTL;
> > + Â Â Â rq.ocf = OCF_LE_CLEAR_WHITE_LIST;
> > + Â Â Â rq.rparam = &status;
> > + Â Â Â rq.rlen = 1;
> > +
> > + Â Â Â if (hci_send_req(dd, &rq, 100) < 0)
> Use 1000
> 
Ok.

> > + Â Â Â Â Â Â Â return -1;
> > +
> > + Â Â Â if (status) {
> > + Â Â Â Â Â Â Â errno = EIO;
> > + Â Â Â Â Â Â Â return -1;
> > + Â Â Â }
> > +
> > + Â Â Â return 0;
> > +}
> > +
> > Âint hci_read_local_name(int dd, int len, char *name, int to)
> > Â{
> > Â Â Â Âread_local_name_rp rp;
> > diff --git a/lib/hci_lib.h b/lib/hci_lib.h
> > old mode 100644
> > new mode 100755
> > index b63a2a4..ed74dfc
> > --- a/lib/hci_lib.h
> > +++ b/lib/hci_lib.h
> > @@ -127,6 +127,10 @@ int hci_le_create_conn(int dd, uint16_t
> interval, uint16_t window,
> > Â Â Â Â Â Â Â Âuint16_t latency, uint16_t supervision_timeout,
> > Â Â Â Â Â Â Â Âuint16_t min_ce_length, uint16_t max_ce_length,
> > Â Â Â Â Â Â Â Âuint16_t *handle, int to);
> > +int hci_le_add_to_white_list(int dd, bdaddr_t addr, uint8_t type);
> > +int hci_le_remove_from_white_list(int dd, bdaddr_t addr, uint8_t
> type);
> > +int hci_le_read_white_list_size(int dd);
> > +int hci_le_clear_white_list(int dd);
> >
> > Âint hci_for_each_dev(int flag, int(*func)(int dd, int dev_id, long
> arg), long arg);
> > Âint hci_get_route(bdaddr_t *bdaddr);
> > diff --git a/tools/hcitool.c b/tools/hcitool.c
> > index d50adaf..387c47c 100644
> > --- a/tools/hcitool.c
> > +++ b/tools/hcitool.c
> > @@ -2471,6 +2471,188 @@ static void cmd_lecc(int dev_id, int argc,
> char **argv)
> > Â Â Â Âhci_close_dev(dd);
> > Â}
> >
> > +static struct option leaddwl_options[] = {
> > + Â Â Â { "help", Â Â Â 0, 0, 'h' },
> > + Â Â Â { 0, 0, 0, 0 }
> > +};
> > +
> > +static const char *leaddwl_help =
> > + Â Â Â "Usage:\n"
> > + Â Â Â "\tleaddwl <bdaddr>\n";
> > +
> > +static void cmd_leaddwl(int dev_id, int argc,char **argv)
> > +{
> > + Â Â Â int err, opt, dd;
> > + Â Â Â bdaddr_t bdaddr;
> > + Â Â Â uint8_t bdaddr_type;
> > +
> > + Â Â Â for_each_opt(opt, leaddwl_options, NULL) {
> > + Â Â Â Â Â Â Â switch (opt) {
> > + Â Â Â Â Â Â Â default:
> > + Â Â Â Â Â Â Â Â Â Â Â printf("%s", leaddwl_help);
> > + Â Â Â Â Â Â Â Â Â Â Â return;
> > + Â Â Â Â Â Â Â }
> > + Â Â Â }
> > + Â Â Â helper_arg(1, 1, &argc, &argv, leaddwl_help);
> > +
> > + Â Â Â if (dev_id < 0)
> > + Â Â Â Â Â Â Â dev_id = hci_get_route(NULL);
> > +
> > + Â Â Â dd = hci_open_dev(dev_id);
> > + Â Â Â if (dd < 0) {
> > + Â Â Â Â Â Â Â perror("Could not open device");
> > + Â Â Â Â Â Â Â exit(1);
> > + Â Â Â }
> > +
> > + Â Â Â str2ba(argv[1], &bdaddr);
> > + Â Â Â bdaddr_type = 0x00;
> Use enum
> 
Ok. Would define an enum le_device_addr_type in hci.h.

> > +
> > + Â Â Â err = hci_le_add_to_white_list(dd, bdaddr ,bdaddr_type);
> Missing space before bdaddr_type
> 
> > + Â Â Â if (err < 0) {
> > + Â Â Â Â Â Â Â perror("Cant add to white list");
> > + Â Â Â Â Â Â Â exit(1);
> > + Â Â Â }
> wrong coding style
> 
Is using a separate variable err for return type of a function problem here? 
I could see same coding style of defining a variable to carry the error status in hcitool.c function cmd_lecc.
Please confirm. 

> > + Â Â Â else {
> > + Â Â Â Â Â Â Â printf("Device added to white list");
> > + Â Â Â }
> "{" not necessary
> 

Ok.

> > +
> > + Â Â Â hci_close_dev(dd);
> Move this line after "hci_le_add_to_white_list()" call, otherwise dd
> will not be closed if the cmd fails
> 
> > +
> remove empty line

Ok.
> > +}
> > +
> > +static struct option lermwl_options[] = {
> > + Â Â Â { "help", Â Â Â 0, 0, 'h' },
> > + Â Â Â { 0, 0, 0, 0 }
> > +};
> > +
> > +static const char *lermwl_help =
> > + Â Â Â "Usage:\n"
> > + Â Â Â "\tlermwl <bdaddr>\n";
> > +
> > +static void cmd_lermwl(int dev_id, int argc,char **argv)
> > +{
> > + Â Â Â int err, opt, dd;
> > + Â Â Â bdaddr_t bdaddr;
> > + Â Â Â uint8_t bdaddr_type;
> > +
> > + Â Â Â for_each_opt(opt, lermwl_options, NULL) {
> > + Â Â Â Â Â Â Â switch (opt) {
> > + Â Â Â Â Â Â Â default:
> > + Â Â Â Â Â Â Â Â Â Â Â printf("%s", lermwl_help);
> > + Â Â Â Â Â Â Â Â Â Â Â return;
> > + Â Â Â Â Â Â Â }
> > + Â Â Â }
> > + Â Â Â helper_arg(1, 1, &argc, &argv, lermwl_help);
> > +
> > + Â Â Â if (dev_id < 0)
> > + Â Â Â Â Â Â Â dev_id = hci_get_route(NULL);
> > +
> > + Â Â Â dd = hci_open_dev(dev_id);
> > + Â Â Â if (dd < 0) {
> > + Â Â Â Â Â Â Â perror("Could not open device");
> > + Â Â Â Â Â Â Â exit(1);
> > + Â Â Â }
> > +
> > + Â Â Â str2ba(argv[1], &bdaddr);
> > + Â Â Â bdaddr_type = 0x00;
> > +
> > + Â Â Â err = hci_le_remove_from_white_list(dd, bdaddr ,bdaddr_type);
> > + Â Â Â if (err < 0) {
> > + Â Â Â Â Â Â Â perror("Cant remove from white list");
> > + Â Â Â Â Â Â Â exit(1);
> > + Â Â Â }
> > + Â Â Â Âelse {
> > + Â Â Â Â Â Â Â printf("Device removed from white list");
> > + Â Â Â }
> > +
> > + Â Â Â hci_close_dev(dd);
> Same coding style issue and close "dd"
> 


Ok. Good point. But I could still find premature returns on error conditions via exits at several places in hcitool.c. Can I submit a separate patch for rectifying all such error conditions too?

> > +
> > +}
> > +
> > +static struct option lerdwlsz_options[] = {
> > + Â Â Â { "help", Â Â Â 0, 0, 'h' },
> > + Â Â Â { 0, 0, 0, 0 }
> > +};
> > +
> > +static const char *lerdwlsz_help =
> > + Â Â Â "Usage:\n"
> > + Â Â Â "\tlerdwlsz\n";
> > +
> > +static void cmd_lerdwlsz(int dev_id, int argc,char **argv)
> > +{
> > + Â Â Â int err,dd, opt;
> > +
> > + Â Â Â for_each_opt(opt, lerdwlsz_options, NULL) {
> > + Â Â Â Â Â Â Â switch (opt) {
> > + Â Â Â Â Â Â Â default:
> > + Â Â Â Â Â Â Â Â Â Â Â printf("%s", lerdwlsz_help);
> > + Â Â Â Â Â Â Â Â Â Â Â return;
> > + Â Â Â Â Â Â Â }
> > + Â Â Â }
> Add empty line here

Ok.
> > + Â Â Â helper_arg(0, 0, &argc, &argv, lermwl_help);
> > +
> > + Â Â Â if (dev_id < 0)
> > + Â Â Â Â Â Â Â dev_id = hci_get_route(NULL);
> > +
> > + Â Â Â dd = hci_open_dev(dev_id);
> > + Â Â Â if (dd < 0) {
> > + Â Â Â Â Â Â Â perror("Could not open device");
> > + Â Â Â Â Â Â Â exit(1);
> > + Â Â Â }
> > +
> > + Â Â Â err = hci_le_read_white_list_size(dd);
> > + Â Â Â if (err < 0) {
> > + Â Â Â Â Â Â Â perror("Cant read white list size");
> > + Â Â Â Â Â Â Â exit(1);
> > + Â Â Â }
> > +
> > + Â Â Â hci_close_dev(dd);
> Missing close dd if the cmd fails
Ok.
> Remove empty line here
Ok.
> > +
> > +}
> > +
> > +
> Remove empty line here
Ok.
> > +static struct option leclrwl_options[] = {
> > + Â Â Â { "help", Â Â Â 0, 0, 'h' },
> > + Â Â Â { 0, 0, 0, 0 }
> > +};
> > +
> > +static const char *leclrwl_help =
> > + Â Â Â "Usage:\n"
> > + Â Â Â "\tleclrwl\n";
> > +
> > +static void cmd_leclrwl(int dev_id, int argc,char **argv)
> > +{
> > + Â Â Â int err,dd, opt;
> > +
> > + Â Â Â for_each_opt(opt, leclrwl_options, NULL) {
> > + Â Â Â Â Â Â Â switch (opt) {
> > + Â Â Â Â Â Â Â default:
> > + Â Â Â Â Â Â Â Â Â Â Â printf("%s", leclrwl_help);
> > + Â Â Â Â Â Â Â Â Â Â Â return;
> > + Â Â Â Â Â Â Â }
> > + Â Â Â }
> > + Â Â Â helper_arg(0, 0, &argc, &argv, leclrwl_help);
> > +
> > + Â Â Â if (dev_id < 0)
> > + Â Â Â Â Â Â Â dev_id = hci_get_route(NULL);
> > +
> > + Â Â Â dd = hci_open_dev(dev_id);
> > + Â Â Â if (dd < 0) {
> > + Â Â Â Â Â Â Â perror("Could not open device");
> > + Â Â Â Â Â Â Â exit(1);
> > + Â Â Â }
> > +
> > + Â Â Â err = hci_le_clear_white_list(dd);
> > + Â Â Â if (err < 0) {
> > + Â Â Â Â Â Â Â perror("Cant clear white list");
> > + Â Â Â Â Â Â Â exit(1);
> > + Â Â Â }
> > +
> > + Â Â Â hci_close_dev(dd);
> Missing close dd when the cmd fails
> Remove empty line here
> 
Ok.

> Claudio
> > +
> > +}
> > +
> > +
> Remove empty line here

Ok.
> > Âstatic struct option ledc_options[] = {
> > Â Â Â Â{ "help", Â Â Â 0, 0, 'h' },
> > Â Â Â Â{ 0, 0, 0, 0 }
> > @@ -2547,6 +2729,10 @@ static struct {
> > Â Â Â Â{ "clkoff", cmd_clkoff, "Read clock offset"
> Â},
> > Â Â Â Â{ "clock", Âcmd_clock, Â"Read local or remote clock"
> },
> > Â Â Â Â{ "lescan", cmd_lescan, "Start LE scan"
> Â},
> > + Â Â Â { "leaddwl", cmd_leaddwl, "Add this device to white list"
> Â Â Â},
> > + Â Â Â { "lermwl", cmd_lermwl, "Remove this device from white list"
> Â },
> > + Â Â Â { "lerdwlsz", Âcmd_lerdwlsz, Â"Read white list size"
> Â Â Â },
> > + Â Â Â { "leclrwl", Â cmd_leclrwl, Â "Clear white list"
> Â Â Â },
> > Â Â Â Â{ "lecc", Â cmd_lecc, Â "Create a LE Connection",
> Â},
> > Â Â Â Â{ "ledc", Â cmd_ledc, Â "Disconnect a LE Connection",
> Â},
> > Â Â Â Â{ NULL, NULL, 0 }
> > --
> > 1.6.5
> >
> >
> > --
> > 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
> >

Thanks, 
Sumit.
ÿô.nlj·Ÿ®‰­†+%ŠË±é¥Šwÿº{.nlj·¥Š{±ý¶â^n‡r¡öë¨è&£ûz¹Þúzf£¢·hšˆ§~†­†Ûÿÿïÿ‘ê_èæ+v‰¨þ)ßø
[Index of Archives]     [Bluez Devel]     [Linux Wireless Networking]     [Linux Wireless Personal Area Networking]     [Linux ATH6KL]     [Linux USB Devel]     [Linux Media Drivers]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Big List of Linux Books]

  Powered by Linux