[PATCH] Bluetooth: Use continuous scanning when creating LE connections

[PATCH] Bluetooth: Use continuous scanning when creating LE connections

[Johan Hedberg]

From: Johan Hedberg <johan.hedberg@intel.com>

All LE connections are now triggered through a preceding passive scan
and waiting for a connectable advertising report. This means we've got
the best possible guarantee that the device is within range and should
be able to request the controller to perform continuous scanning. This
way we minimize the risk that we miss out on any advertising packets.

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
 net/bluetooth/hci_conn.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index e2600213cd50..f28741d78e0b 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -726,8 +726,12 @@ static void hci_req_add_le_create_conn(struct hci_request *req,
 	if (hci_update_random_address(req, false, &own_addr_type))
 		return;
 
+	/* Set interval and window to same value to enable continuous
+	 * scanning.
+	 */
 	cp.scan_interval = cpu_to_le16(hdev->le_scan_interval);
-	cp.scan_window = cpu_to_le16(hdev->le_scan_window);
+	cp.scan_window = cp.scan_interval;
+
 	bacpy(&cp.peer_addr, &conn->dst);
 	cp.peer_addr_type = conn->dst_type;
 	cp.own_address_type = own_addr_type;
-- 
2.5.0

Re: [PATCH] Bluetooth: Use continuous scanning when creating LE connections

[Jakub Pawlowski]

Hi Johan,

On Mon, Nov 30, 2015 at 2:48 AM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> From: Johan Hedberg <johan.hedberg@intel.com>
>
> All LE connections are now triggered through a preceding passive scan
> and waiting for a connectable advertising report. This means we've got
> the best possible guarantee that the device is within range and should
> be able to request the controller to perform continuous scanning. This
> way we minimize the risk that we miss out on any advertising packets.
>
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
>  net/bluetooth/hci_conn.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> index e2600213cd50..f28741d78e0b 100644
> --- a/net/bluetooth/hci_conn.c
> +++ b/net/bluetooth/hci_conn.c
> @@ -726,8 +726,12 @@ static void hci_req_add_le_create_conn(struct hci_request *req,
>         if (hci_update_random_address(req, false, &own_addr_type))
>                 return;
>
> +       /* Set interval and window to same value to enable continuous
> +        * scanning.
> +        */
>         cp.scan_interval = cpu_to_le16(hdev->le_scan_interval);
> -       cp.scan_window = cpu_to_le16(hdev->le_scan_window);
> +       cp.scan_window = cp.scan_interval;
> +
So I'm not 100% sure, but I think this code will make classic, or
interleaved inquiry not report during LE connection establishment.
When doing interleaved inquiry, depending on HCI_QUIRK_SIMULTANEOUS_DISCOVERY
we might be doing classic inquiry at same time LE scan is done.
Filling whole scan window with LE scan
will cause no time on radio left for classic on some controllers.

>         bacpy(&cp.peer_addr, &conn->dst);
>         cp.peer_addr_type = conn->dst_type;
>         cp.own_address_type = own_addr_type;
> --
> 2.5.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Bluetooth: Use continuous scanning when creating LE connections

[Marcel Holtmann]

Hi Jakub,

>> All LE connections are now triggered through a preceding passive scan
>> and waiting for a connectable advertising report. This means we've got
>> the best possible guarantee that the device is within range and should
>> be able to request the controller to perform continuous scanning. This
>> way we minimize the risk that we miss out on any advertising packets.
>> 
>> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
>> ---
>> net/bluetooth/hci_conn.c | 6 +++++-
>> 1 file changed, 5 insertions(+), 1 deletion(-)
>> 
>> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
>> index e2600213cd50..f28741d78e0b 100644
>> --- a/net/bluetooth/hci_conn.c
>> +++ b/net/bluetooth/hci_conn.c
>> @@ -726,8 +726,12 @@ static void hci_req_add_le_create_conn(struct hci_request *req,
>>        if (hci_update_random_address(req, false, &own_addr_type))
>>                return;
>> 
>> +       /* Set interval and window to same value to enable continuous
>> +        * scanning.
>> +        */
>>        cp.scan_interval = cpu_to_le16(hdev->le_scan_interval);
>> -       cp.scan_window = cpu_to_le16(hdev->le_scan_window);
>> +       cp.scan_window = cp.scan_interval;
>> +
> So I'm not 100% sure, but I think this code will make classic, or
> interleaved inquiry not report during LE connection establishment.
> When doing interleaved inquiry, depending on HCI_QUIRK_SIMULTANEOUS_DISCOVERY
> we might be doing classic inquiry at same time LE scan is done.
> Filling whole scan window with LE scan
> will cause no time on radio left for classic on some controllers.

that depends on how the controller schedules it. The values are just recommendations and it can always decide to interrupt LE scanning. I assume most controllers will do exactly that since they treat LE scanning as lowest priority.

Also keep in mind that after 2 seconds, the LE create connection attempt will be aborted and it will fall back to normal LE scanning that is not full duty cycle.

Reality is that if you want to connect to a certain device, then you want to connect to it. Period. So everything else is secondary at that point. The only thing a controller should care about is SCO/eSCO connections and ACL links for A2DP and everything else can be handled after the LE connection has been established. Missing an inquiry event is certainly irrelevant.

Regards

Marcel

Re: [PATCH] Bluetooth: Use continuous scanning when creating LE connections

[Marcel Holtmann]

Hi Johan,

> All LE connections are now triggered through a preceding passive scan
> and waiting for a connectable advertising report. This means we've got
> the best possible guarantee that the device is within range and should
> be able to request the controller to perform continuous scanning. This
> way we minimize the risk that we miss out on any advertising packets.
> 
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
> net/bluetooth/hci_conn.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> index e2600213cd50..f28741d78e0b 100644
> --- a/net/bluetooth/hci_conn.c
> +++ b/net/bluetooth/hci_conn.c
> @@ -726,8 +726,12 @@ static void hci_req_add_le_create_conn(struct hci_request *req,
> 	if (hci_update_random_address(req, false, &own_addr_type))
> 		return;
> 
> +	/* Set interval and window to same value to enable continuous
> +	 * scanning.
> +	 */

I think we want to phrase this as using a scan window as large as the scan interval to trigger a full duty / continuous scan. Just to be a bit more clearer.

> 	cp.scan_interval = cpu_to_le16(hdev->le_scan_interval);
> -	cp.scan_window = cpu_to_le16(hdev->le_scan_window);
> +	cp.scan_window = cp.scan_interval;
> +

And I know you try to optimize the cpu_to_le16 out here, but honestly I would prefer this:

	cp.scan_window = cpu_to_le16(hdev->le_scan_interval);

Regards

Marcel

Re: [PATCH] Bluetooth: Use continuous scanning when creating LE connections

[Johan Hedberg]

Hi Marcel,

On Tue, Dec 01, 2015, Marcel Holtmann wrote:
> > All LE connections are now triggered through a preceding passive scan
> > and waiting for a connectable advertising report. This means we've got
> > the best possible guarantee that the device is within range and should
> > be able to request the controller to perform continuous scanning. This
> > way we minimize the risk that we miss out on any advertising packets.
> > 
> > Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> > ---
> > net/bluetooth/hci_conn.c | 6 +++++-
> > 1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> > index e2600213cd50..f28741d78e0b 100644
> > --- a/net/bluetooth/hci_conn.c
> > +++ b/net/bluetooth/hci_conn.c
> > @@ -726,8 +726,12 @@ static void hci_req_add_le_create_conn(struct hci_request *req,
> > 	if (hci_update_random_address(req, false, &own_addr_type))
> > 		return;
> > 
> > +	/* Set interval and window to same value to enable continuous
> > +	 * scanning.
> > +	 */
> 
> I think we want to phrase this as using a scan window as large as the
> scan interval to trigger a full duty / continuous scan. Just to be a
> bit more clearer.

I was trying to follow the same phrasing as the specification uses:

"The LE_Scan_Window parameter shall always be set to a value smaller or
equal to the value set for the LE_Scan_Interval parameter. If they are
set to the same value scanning should be run continuously."

However I don't care too strongly about this, so if you still prefer "as
large as" I can change it.

> > 	cp.scan_interval = cpu_to_le16(hdev->le_scan_interval);
> > -	cp.scan_window = cpu_to_le16(hdev->le_scan_window);
> > +	cp.scan_window = cp.scan_interval;
> > +
> 
> And I know you try to optimize the cpu_to_le16 out here, but honestly
> I would prefer this:
> 
> 	cp.scan_window = cpu_to_le16(hdev->le_scan_interval);

My initial reaction to this is "looks like a copy-paste bug where we
forgot to change to use the right variable" whereas the variant I went
with gives more of an impression of explicit intent. However here too I
wont insist - either way is fine with me.

Johan

Re: [PATCH] Bluetooth: Use continuous scanning when creating LE connections

[Marcel Holtmann]

Hi Johan,

>>> All LE connections are now triggered through a preceding passive scan
>>> and waiting for a connectable advertising report. This means we've got
>>> the best possible guarantee that the device is within range and should
>>> be able to request the controller to perform continuous scanning. This
>>> way we minimize the risk that we miss out on any advertising packets.
>>> 
>>> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
>>> ---
>>> net/bluetooth/hci_conn.c | 6 +++++-
>>> 1 file changed, 5 insertions(+), 1 deletion(-)
>>> 
>>> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
>>> index e2600213cd50..f28741d78e0b 100644
>>> --- a/net/bluetooth/hci_conn.c
>>> +++ b/net/bluetooth/hci_conn.c
>>> @@ -726,8 +726,12 @@ static void hci_req_add_le_create_conn(struct hci_request *req,
>>> 	if (hci_update_random_address(req, false, &own_addr_type))
>>> 		return;
>>> 
>>> +	/* Set interval and window to same value to enable continuous
>>> +	 * scanning.
>>> +	 */
>> 
>> I think we want to phrase this as using a scan window as large as the
>> scan interval to trigger a full duty / continuous scan. Just to be a
>> bit more clearer.
> 
> I was trying to follow the same phrasing as the specification uses:
> 
> "The LE_Scan_Window parameter shall always be set to a value smaller or
> equal to the value set for the LE_Scan_Interval parameter. If they are
> set to the same value scanning should be run continuously."
> 
> However I don't care too strongly about this, so if you still prefer "as
> large as" I can change it.

I was actually not proposing "as large as" exact phrase. My suggestions was to phrase it in a more clear way, but maybe I am just overthinking it.

> 
>>> 	cp.scan_interval = cpu_to_le16(hdev->le_scan_interval);
>>> -	cp.scan_window = cpu_to_le16(hdev->le_scan_window);
>>> +	cp.scan_window = cp.scan_interval;
>>> +
>> 
>> And I know you try to optimize the cpu_to_le16 out here, but honestly
>> I would prefer this:
>> 
>> 	cp.scan_window = cpu_to_le16(hdev->le_scan_interval);
> 
> My initial reaction to this is "looks like a copy-paste bug where we
> forgot to change to use the right variable" whereas the variant I went
> with gives more of an impression of explicit intent. However here too I
> wont insist - either way is fine with me.

Fair point. Maybe better keep your version.

Regards

Marcel