diff mbox series

[v2] Added BREDR not supported bit in AD Flag when discoverable is off

Message ID 20240703070220.15246-1-quic_prathm@quicinc.com (mailing list archive)
State New
Headers show
Series [v2] Added BREDR not supported bit in AD Flag when discoverable is off | expand

Checks

Context Check Description
tedd_an/pre-ci_am success Success
tedd_an/CheckPatch success CheckPatch PASS
tedd_an/GitLint success Gitlint PASS
tedd_an/BuildEll success Build ELL PASS
tedd_an/BluezMake success Bluez Make PASS
tedd_an/MakeCheck success Bluez Make Check PASS
tedd_an/MakeDistcheck success Make Distcheck PASS
tedd_an/CheckValgrind success Check Valgrind PASS
tedd_an/CheckSmatch success CheckSparse PASS
tedd_an/bluezmakeextell success Make External ELL PASS
tedd_an/IncrementalBuild success Incremental Build PASS
tedd_an/ScanBuild success Scan Build PASS

Commit Message

Prathibha Madugonde July 3, 2024, 7:02 a.m. UTC
From: Prathibha Madugonde <quic_prathm@quicinc.com>

Fix for GAP/DISC/NONM/BV-02-C
As per GAP.TS.p44 test spec
IUT does not contain General Discoverable mode and Limited Discoverable
mode in the AD Type Flag. IUT shall send AD Type Flag to PASS the test
case, thus added BR/EDR not supported bit in the AD Type Flag when
discoverable is off.

Signed-off-by: Prathibha Madugonde <quic_prathm@quicinc.com>
---
 src/advertising.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

Comments

Paul Menzel July 3, 2024, 7:13 a.m. UTC | #1
Dear Prathibha,


Thank you for your patch.

Am 03.07.24 um 09:02 schrieb quic_prathm@quicinc.com:
> From: Prathibha Madugonde <quic_prathm@quicinc.com>

I’d use imperative mood in the commit message summary/title:

s/Added/Add/

> Fix for GAP/DISC/NONM/BV-02-C
> As per GAP.TS.p44 test spec
> IUT does not contain General Discoverable mode and Limited Discoverable
> mode in the AD Type Flag. IUT shall send AD Type Flag to PASS the test
> case, thus added BR/EDR not supported bit in the AD Type Flag when

s/added/add/

> discoverable is off.

Please document your test setup.

> Signed-off-by: Prathibha Madugonde <quic_prathm@quicinc.com>

Please following the rules in `HACKING` [1].

> ---
>   src/advertising.c | 14 ++++++++++++--
>   1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/src/advertising.c b/src/advertising.c
> index 5d373e088..cfb239585 100644
> --- a/src/advertising.c
> +++ b/src/advertising.c
> @@ -734,8 +734,7 @@ static bool set_flags(struct btd_adv_client *client, uint8_t flags)
>   	/* Set BR/EDR Not Supported if adapter is not discoverable but the
>   	 * instance is.
>   	 */
> -	if ((flags & (BT_AD_FLAG_GENERAL | BT_AD_FLAG_LIMITED)) &&
> -			!btd_adapter_get_discoverable(client->manager->adapter))
> +	if (!btd_adapter_get_discoverable(client->manager->adapter))
>   		flags |= BT_AD_FLAG_NO_BREDR;
>   
>   	if (!bt_ad_add_flags(client->data, &flags, 1))
> @@ -1499,6 +1498,17 @@ static DBusMessage *parse_advertisement(struct btd_adv_client *client)
>   		goto fail;
>   	}
>   
> +	/* GAP.TS.p44 Test Spec GAP/DISC/NONM/BV-02-C
> +	 * page 158:
> +	 * IUT does not contain
> +	 * ‘LE General Discoverable Mode’ flag or the
> +	 * ‘LE Limited Discoverable Mode’ flag in the Flags AD Type
> +	 * But AD Flag Type should be there for the test case to
> +	 * PASS. Thus BR/EDR Not Supported BIT shall be included
> +	 * in the AD Type flag.

You could re-flow for longer lines.

> +	 */
> +	set_flags(client, bt_ad_get_flags(client->data));
> +
>   	err = refresh_advertisement(client, add_adv_callback);
>   
>   	if (!err)


Kind regards,

Paul


[1]: 
https://github.com/bluez/bluez/blob/be0b08770e9214592e89589b2dc069426c0d9b91/HACKING#L98
bluez.test.bot@gmail.com July 3, 2024, 8:42 a.m. UTC | #2
This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=867858

---Test result---

Test Summary:
CheckPatch                    PASS      0.38 seconds
GitLint                       PASS      0.27 seconds
BuildEll                      PASS      25.00 seconds
BluezMake                     PASS      1764.21 seconds
MakeCheck                     PASS      13.46 seconds
MakeDistcheck                 PASS      181.73 seconds
CheckValgrind                 PASS      258.05 seconds
CheckSmatch                   PASS      361.46 seconds
bluezmakeextell               PASS      121.69 seconds
IncrementalBuild              PASS      1504.55 seconds
ScanBuild                     PASS      1052.02 seconds



---
Regards,
Linux Bluetooth
Luiz Augusto von Dentz July 3, 2024, 1:57 p.m. UTC | #3
Hi,

On Wed, Jul 3, 2024 at 3:02 AM <quic_prathm@quicinc.com> wrote:
>
> From: Prathibha Madugonde <quic_prathm@quicinc.com>
>
> Fix for GAP/DISC/NONM/BV-02-C
> As per GAP.TS.p44 test spec
> IUT does not contain General Discoverable mode and Limited Discoverable
> mode in the AD Type Flag. IUT shall send AD Type Flag to PASS the test
> case, thus added BR/EDR not supported bit in the AD Type Flag when
> discoverable is off.
>
> Signed-off-by: Prathibha Madugonde <quic_prathm@quicinc.com>
> ---
>  src/advertising.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/src/advertising.c b/src/advertising.c
> index 5d373e088..cfb239585 100644
> --- a/src/advertising.c
> +++ b/src/advertising.c
> @@ -734,8 +734,7 @@ static bool set_flags(struct btd_adv_client *client, uint8_t flags)
>         /* Set BR/EDR Not Supported if adapter is not discoverable but the
>          * instance is.
>          */
> -       if ((flags & (BT_AD_FLAG_GENERAL | BT_AD_FLAG_LIMITED)) &&
> -                       !btd_adapter_get_discoverable(client->manager->adapter))
> +       if (!btd_adapter_get_discoverable(client->manager->adapter))
>                 flags |= BT_AD_FLAG_NO_BREDR;
>
>         if (!bt_ad_add_flags(client->data, &flags, 1))
> @@ -1499,6 +1498,17 @@ static DBusMessage *parse_advertisement(struct btd_adv_client *client)
>                 goto fail;
>         }
>
> +       /* GAP.TS.p44 Test Spec GAP/DISC/NONM/BV-02-C
> +        * page 158:
> +        * IUT does not contain
> +        * ‘LE General Discoverable Mode’ flag or the
> +        * ‘LE Limited Discoverable Mode’ flag in the Flags AD Type
> +        * But AD Flag Type should be there for the test case to
> +        * PASS. Thus BR/EDR Not Supported BIT shall be included
> +        * in the AD Type flag.
> +        */
> +       set_flags(client, bt_ad_get_flags(client->data));

Was there a bug or something that you are having to add this code here?

>         err = refresh_advertisement(client, add_adv_callback);
>
>         if (!err)
> --
> 2.17.1
>
Prathibha Madugonde July 5, 2024, 5:38 a.m. UTC | #4
On 7/3/2024 7:27 PM, Luiz Augusto von Dentz wrote:
> Hi,
> 
> On Wed, Jul 3, 2024 at 3:02 AM <quic_prathm@quicinc.com> wrote:
>>
>> From: Prathibha Madugonde <quic_prathm@quicinc.com>
>>
>> Fix for GAP/DISC/NONM/BV-02-C
>> As per GAP.TS.p44 test spec
>> IUT does not contain General Discoverable mode and Limited Discoverable
>> mode in the AD Type Flag. IUT shall send AD Type Flag to PASS the test
>> case, thus added BR/EDR not supported bit in the AD Type Flag when
>> discoverable is off.
>>
>> Signed-off-by: Prathibha Madugonde <quic_prathm@quicinc.com>
>> ---
>>   src/advertising.c | 14 ++++++++++++--
>>   1 file changed, 12 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/advertising.c b/src/advertising.c
>> index 5d373e088..cfb239585 100644
>> --- a/src/advertising.c
>> +++ b/src/advertising.c
>> @@ -734,8 +734,7 @@ static bool set_flags(struct btd_adv_client *client, uint8_t flags)
>>          /* Set BR/EDR Not Supported if adapter is not discoverable but the
>>           * instance is.
>>           */
>> -       if ((flags & (BT_AD_FLAG_GENERAL | BT_AD_FLAG_LIMITED)) &&
>> -                       !btd_adapter_get_discoverable(client->manager->adapter))
>> +       if (!btd_adapter_get_discoverable(client->manager->adapter))
>>                  flags |= BT_AD_FLAG_NO_BREDR;
>>
>>          if (!bt_ad_add_flags(client->data, &flags, 1))
>> @@ -1499,6 +1498,17 @@ static DBusMessage *parse_advertisement(struct btd_adv_client *client)
>>                  goto fail;
>>          }
>>
>> +       /* GAP.TS.p44 Test Spec GAP/DISC/NONM/BV-02-C
>> +        * page 158:
>> +        * IUT does not contain
>> +        * ‘LE General Discoverable Mode’ flag or the
>> +        * ‘LE Limited Discoverable Mode’ flag in the Flags AD Type
>> +        * But AD Flag Type should be there for the test case to
>> +        * PASS. Thus BR/EDR Not Supported BIT shall be included
>> +        * in the AD Type flag.
>> +        */
>> +       set_flags(client, bt_ad_get_flags(client->data));
> 
> Was there a bug or something that you are having to add this code here?

Hi Luiz,

Yes there is an issue when discoverable is off the parse_discoverable()
is not triggering when we do advertise on. Fixed this issue and sending
in next patch- [PATCH BlueZ] Set BREDR not supported bit in AD Flag when
discoverable is off. Please review.

Thanks
Prathibha Madugonde
> 
>>          err = refresh_advertisement(client, add_adv_callback);
>>
>>          if (!err)
>> --
>> 2.17.1
>>
> 
>
diff mbox series

Patch

diff --git a/src/advertising.c b/src/advertising.c
index 5d373e088..cfb239585 100644
--- a/src/advertising.c
+++ b/src/advertising.c
@@ -734,8 +734,7 @@  static bool set_flags(struct btd_adv_client *client, uint8_t flags)
 	/* Set BR/EDR Not Supported if adapter is not discoverable but the
 	 * instance is.
 	 */
-	if ((flags & (BT_AD_FLAG_GENERAL | BT_AD_FLAG_LIMITED)) &&
-			!btd_adapter_get_discoverable(client->manager->adapter))
+	if (!btd_adapter_get_discoverable(client->manager->adapter))
 		flags |= BT_AD_FLAG_NO_BREDR;
 
 	if (!bt_ad_add_flags(client->data, &flags, 1))
@@ -1499,6 +1498,17 @@  static DBusMessage *parse_advertisement(struct btd_adv_client *client)
 		goto fail;
 	}
 
+	/* GAP.TS.p44 Test Spec GAP/DISC/NONM/BV-02-C
+	 * page 158:
+	 * IUT does not contain
+	 * ‘LE General Discoverable Mode’ flag or the
+	 * ‘LE Limited Discoverable Mode’ flag in the Flags AD Type
+	 * But AD Flag Type should be there for the test case to
+	 * PASS. Thus BR/EDR Not Supported BIT shall be included
+	 * in the AD Type flag.
+	 */
+	set_flags(client, bt_ad_get_flags(client->data));
+
 	err = refresh_advertisement(client, add_adv_callback);
 
 	if (!err)