Hi Marcel,
On Sep 20, 2011, at 10:00 AM, Marcel Holtmann wrote:
> Hi Andre,
>
>> This patch adds support for LE-Only discovery procedure through
>> management interface.
>>
>> Signed-off-by: Andre Guedes <andre.guedes@xxxxxxxxxxxxx>
>> ---
>> net/bluetooth/hci_event.c | 16 +++++++++++++---
>> net/bluetooth/mgmt.c | 13 ++++++++++++-
>> 2 files changed, 25 insertions(+), 4 deletions(-)
>>
>> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
>> index 5bbba54..0408c50 100644
>> --- a/net/bluetooth/hci_event.c
>> +++ b/net/bluetooth/hci_event.c
>> @@ -889,14 +889,16 @@ static void hci_cc_le_set_scan_enable(struct hci_dev *hdev,
>>
>> BT_DBG("%s status 0x%x", hdev->name, status);
>>
>> - if (status)
>> - return;
>> -
>> cp = hci_sent_cmd_data(hdev, HCI_OP_LE_SET_SCAN_ENABLE);
>> if (!cp)
>> return;
>>
>> if (cp->enable == 0x01) {
>> + if (status) {
>> + mgmt_discovery_complete(hdev->id, status);
>> + return;
>> + }
>> +
>> set_bit(HCI_LE_SCAN, &hdev->flags);
>>
>> mgmt_discovering(hdev->id, 1);
>> @@ -906,7 +908,15 @@ static void hci_cc_le_set_scan_enable(struct hci_dev *hdev,
>> hci_dev_lock(hdev);
>> hci_adv_entries_clear(hdev);
>> hci_dev_unlock(hdev);
>> +
>> + if (mgmt_has_pending_stop_discov(hdev->id))
>> + hci_cancel_le_scan(hdev);
>> } else if (cp->enable == 0x00) {
>> + mgmt_discovery_complete(hdev->id, status);
>> +
>> + if (status)
>> + return;
>> +
>> clear_bit(HCI_LE_SCAN, &hdev->flags);
>>
>> mgmt_discovering(hdev->id, 0);
>> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
>> index 1eee5cc..e869422 100644
>> --- a/net/bluetooth/mgmt.c
>> +++ b/net/bluetooth/mgmt.c
>> @@ -32,6 +32,14 @@
>> #define MGMT_VERSION 0
>> #define MGMT_REVISION 1
>>
>> +/*
>> + * These LE scan and inquiry parameters were chosen according to LE General
>> + * Discovery Procedure specification.
>> + */
>> +#define LE_SCAN_TYPE 0x01
>> +#define LE_SCAN_WIN 0x12
>> +#define LE_SCAN_INT 0x12
>> +#define LE_SCAN_TIMEOUT_LE_ONLY 10240 /* TGAP(gen_disc_scan_min) */
>
> And an extra empty line here to make clear these a independent values.
Ok, I'll do it.
>> #define INQUIRY_LEN_BREDR 0x08 /* TGAP(100) */
>>
>> struct pending_cmd {
>> @@ -1637,7 +1645,8 @@ static int start_discovery(struct sock *sk, u16 index)
>> if (lmp_host_le_capable(hdev) && lmp_bredr_capable(hdev))
>> err = -ENOSYS;
>> else if (lmp_host_le_capable(hdev))
>> - err = -ENOSYS;
>> + err = hci_do_le_scan(hdev, LE_SCAN_TYPE, LE_SCAN_INT,
>> + LE_SCAN_WIN, LE_SCAN_TIMEOUT_LE_ONLY);
>> else
>> err = hci_do_inquiry(hdev, INQUIRY_LEN_BREDR);
>>
>> @@ -1684,6 +1693,8 @@ static int stop_discovery(struct sock *sk, u16 index)
>>
>> if (test_bit(HCI_INQUIRY, &hdev->flags))
>> err = hci_cancel_inquiry(hdev);
>> + else if (test_bit(HCI_LE_SCAN, &hdev->flags))
>> + err = hci_cancel_le_scan(hdev);
>> else
>> err = 0;
>>
>
> I see where you wanna go with this now. I am a bit afraid of the code
> complexity with the discovery stop/start/complete etc. messages.
>
> This is all too much done differently for 3 different possible use
> cases. And I just think we need more general handling. For example:
>
> inquiry_started()
> inquiry_result()
> inquiry_completed()
> le_scan_started()
> le_scan_result()
> le_scan_completed()
>
> And based on the controller capabilities, the current states etc. a
> central place decides to send out which events at which time and to
> allow certain commands or not.
>
> If we do not centralize this, then I am afraid we duplicate this logic
> three times.
Not sure I get the idea right. Why would we repeat this logic three
times?
BTW, I don't see discovery logic too complex. We have, basically,
three main functions:
mgmt_start_discovery: based on device capabilities it initializes
the right discovery procedure.
mgmt_stop_discovery: based on the current device state it cancels
the ongoing discovery procedure.
mgmt_discovery_complete: once the discovery procedure is finished
(it doesn't matter if the discovery procedure was canceled by a
mgmt stop discovery command, a hci command failed or it finished
successfully) this function does the proper handling of mgmt
discovery pending commands and decides which event is sent to
userspace. So, we have the whole discovery procedure termination
logic centralized in this function.
BR,
Andre--
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
