Skip to content

Conversation

@ppryga-nordic
Copy link
Contributor

@ppryga-nordic ppryga-nordic commented Apr 29, 2022

There was missing handling of LL_UNKNOWN_RSP in CTE request control
procedure. In case there is a pending CTE request and peer responses
with LL_UNKNOWN_RSP then Host should be notified with HCI_LE_CTE_-
Request_Failed event. The pending CTE request procedure should be
completed.

The CTE request procedure should be enabled only if a peer supports
CTE response feature. That information can be obtained by feature
exchange procedure. If there were no feature exchange then CTE
request feature may be disabled if a peer responses with LL_UNKNOWN_RSP
for a CTE request.

The implementation of ll_df_set_conn_cte_req_enable was checking if
CTE response feature is supported only when there was feature exchange.
There was missing possibility to stop CTE request if a peer responded
with LL_UNKNOWN_RSP for an earlier CTE request.

The commit changes the implementation of ll_df_set_conn_cte_req_enable.
The CTE response feature check is moved to ull_cp_cte_req function,
because it belongs more to control procedure than to function that
handles Host request to start the procedure.
Second change is related with use of conn->llcp.fex.features_used.
It stores information about features supported by peer. It does
not depend on execution of the feature exchange control procedure.

By the way, there were removed else statement in ll_df_set_conn_cte_-
req_enable because it was not needed.

Add test that verify correct behavior of the CTE request procedure
in case of reception of LL_UNKNOWN_RSP PDU from peer device.

Fixes not working CTE request control procedure unit tests.
Adds small refactoring related changes to CTE request control procedure unit tests.

There was missing code responsible for handling of unexpected response
for CTE request. The commit adds code that will terminate connection
in case a peer device responses with unexpected control PDU that is not
a remote procedure request.

Fixes: #45708.

@github-actions github-actions bot added area: Bluetooth area: Bluetooth Controller area: Tests Issues related to a particular existing or missing test labels Apr 29, 2022
@ppryga-nordic ppryga-nordic force-pushed the github-ble-df-llcp-unknown-rsp-in-cte-req-proc branch from db5489a to c71b661 Compare April 29, 2022 20:58
@ppryga-nordic
Copy link
Contributor Author

Added commit that removes a test that was no longer valid one. More information in commit message.

@ppryga-nordic
Copy link
Contributor Author

@thoh-ot @cvinayak @kruithofa @erbr-ot could you take a look on the PR?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There seems to be a inconsistency here with that fact that your are validating if an notification based on the LL_UNKNOWN_RSP is received but you are giving the validator an instance of a struct pdu_data_llctrl_reject_ext_ind as the reference, by chance the notification of type struct pdu_data_llctrl_unknown_rsp has a type field which actually matches the reject_opcode field of given remote_reject_ext_ind. I guess this was not the intended behavior.

Copy link
Contributor Author

@ppryga-nordic ppryga-nordic May 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @thoh-ot! Good catch. It should be an address to unknown_rsp instance.
It is fixed now.

@ppryga-nordic ppryga-nordic force-pushed the github-ble-df-llcp-unknown-rsp-in-cte-req-proc branch 2 times, most recently from 4b95ab2 to f278b4b Compare May 13, 2022 07:08
kruithofa
kruithofa previously approved these changes May 16, 2022
thoh-ot
thoh-ot previously approved these changes May 16, 2022
@ppryga-nordic ppryga-nordic dismissed stale reviews from thoh-ot and kruithofa via 6f702d9 May 16, 2022 10:06
@ppryga-nordic ppryga-nordic force-pushed the github-ble-df-llcp-unknown-rsp-in-cte-req-proc branch from f278b4b to 6f702d9 Compare May 16, 2022 10:06
@ppryga-nordic
Copy link
Contributor Author

I have to rebase and fix merge conflicts.
Also I've spot that there were issues with unit tests, hence I've added 3 new commits that address:

  • wrong value assigned to CONFIG_BT_CTLR_DF_MAX_ANT_SW_PATTERN_LEN
  • missing CTE response feature in features supported by peer device
  • missing connection handle assignment to global connection handle variable

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dont you want to check if feature exchange is complete? Legacy had a flag to indicate the features bitmap was valid or not.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch; llcp.fex.features_used is initialized to the features of the local controller (LL_FEAT) and without checking llcp.fex.valid you cannot assume anything about the peer controller.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe also look into ull_llcp_features.h and add any missing feature helper functions for CTE

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that is wrong. There are two ways to get information that peer does not support CTE response feature. By use of feature exchange procedure or reception of LL_UNKNOWN_RSP. I've did it wrong here and also in places where local feature is disabled. The if here should check whether feat. exchange is done and if peer supports CTE RSP, or if the local CTE REQ is still enabled.

If a device receives LL_UNKNOWN_RSP as a response for previous CTE REQ then it should disable local CTE REQ feature for the connection to do not allow for new requests.

@ppryga-nordic ppryga-nordic force-pushed the github-ble-df-llcp-unknown-rsp-in-cte-req-proc branch from 6f702d9 to ab9bb65 Compare May 16, 2022 14:13
@ppryga-nordic
Copy link
Contributor Author

Fixed issue found by @cvinayak related with support for CTE RSP by peer device.

@ppryga-nordic ppryga-nordic added the bug The issue is a bug, or the PR is fixing a bug label May 17, 2022
@ppryga-nordic ppryga-nordic added this to the v3.1.0 milestone May 17, 2022
@ppryga-nordic ppryga-nordic added the DNM This PR should not be merged (Do Not Merge) label May 17, 2022
There was missing handling of LL_UNKNOWN_RSP in CTE request control
procedure.In case there is a pending CTE request and peer responses
with LL_UNKNOWN_RSP then Host should be notified with HCI_LE_CTE_-
Request_Failed event. The pending CTE request procedure should be
completed.
Signed-off-by: Piotr Pryga <[email protected]>
The CTE request procedure should be enabled only if a peer supports
CTE response feature. That information can be obtained by feature
exchange procedure. If there were no feature exchange then CTE
request feature may be disabled if a peer responses with LL_UNKNOWN_RSP
for a CTE request.

The implementation of ll_df_set_conn_cte_req_enable was checking if
CTE response feature is supported only when there was feature exchange.
There was missing possibility to stop CTE request if a peer responded
with LL_UNKNOWN_RSP for an earlier CTE request.

The commit changes the implementation of ll_df_set_conn_cte_req_enable.
The CTE response feature check is moved to ull_cp_cte_req function,
because it belongs more to control procedure than to function that
handles Host request to start the procedure.
Second change is related with use of conn->llcp.fex.features_used.
It stores information about features supported by peer. It does
not depend on execution of the feature exchange control procedure.

By the way, there were removed else statement in ll_df_set_conn_cte_-
req_enable because it was not needed.

Signed-off-by: Piotr Pryga <[email protected]>
Add test that verify correct behavior of the CTE request procedure
in case of reception of LL_UNKNOWN_RSP PDU from peer device.

Signed-off-by: Piotr Pryga <[email protected]>
There was a test case that was never executed. It was not
added to a test suite in test_main. The commit adds the
test case.

Signed-off-by: Piotr Pryga <[email protected]>
There were couple of test cases where Host notification object
was not released. The commit adds missing release calls.

Also commnets related with check if RX queue is empty were
changed to describe what is verified then.

Signed-off-by: Piotr Pryga <[email protected]>
There was missing code responsible for handling of unexpected response
for CTE request. The commit adds code that will terminate connection
in case a peer device reposnes with unexpected control PDU that is not
a remote procedure reques.

Signed-off-by: Piotr Pryga <[email protected]>
There is a test that verifies a behavior of CTE request control
procedure if receives LL_UNKNOWN_RSP. The expected behavior
is to terminate connection if that is response handling is
not implemented by a command. Other unexpected responses are
treated as new remote control procedures.

The CTE request has implemented handling of LL_UNKNOWN_RSP,
hence this particular test is no longer valid.

There is still open question how to handle other unexpected
reposne PDUs while waiting for LL_CTE_RSP. That is not defined
by BT 5.3. Core specification and not decided by community.

The test is removed to because it fails and blocks CI.

Signed-off-by: Piotr Pryga <[email protected]>
Due to change in Kconfig CONFIG_BT_CTLR_DF_MAX_ANT_SW_PATTERN_LEN
the Kconfigs in tests were wrong and tests were not building.
The commit sets the max antenna switch pattern length to actual
maximum value.

Signed-off-by: Piotr Pryga <[email protected]>
There was missing information about supported CTE response feature
by peer device. That caused a test_set_conn_cte_req_enable to fail.
The test expected that peer device supprots CTE response.

The missing information is added in a common code responsible for
connection object preparation.

Signed-off-by: Piotr Pryga <[email protected]>
There was missing assignment to a global variable that stores
connection handle. The value was always the same, hence it test
were not failing. The value was passed over from setup state
done for other tests. This change makes sure there is always
correct handle stored in global variable.

Signed-off-by: Piotr Pryga <[email protected]>
@ppryga-nordic ppryga-nordic force-pushed the github-ble-df-llcp-unknown-rsp-in-cte-req-proc branch from ab9bb65 to b33b05d Compare May 17, 2022 11:55
@ppryga-nordic
Copy link
Contributor Author

I've found an issue with use of features_used. After successful feature exchange procedure the feature set could be limited.
In case of CTE REQ/RSP the feature is asymmetric. A device may support both CTE REQ and CTE RSP, one of these or none.
E.g. local device supports both CTE REQ and CTE RSP. Remote device supports CTE RSP only. After completion of feature exchange features_used would have held CTE REQ disabled. The feature_used should not be used to check if CTE REQ is enabled after completion of feature exchange.

BTW. I've added comments to features_used and features_peer to provide more context to purpose of the members.

@ppryga-nordic ppryga-nordic removed the DNM This PR should not be merged (Do Not Merge) label May 17, 2022
@kruithofa kruithofa self-requested a review May 19, 2022 09:00
@carlescufi carlescufi merged commit b9dcc3d into zephyrproject-rtos:main May 19, 2022
@ppryga-nordic ppryga-nordic deleted the github-ble-df-llcp-unknown-rsp-in-cte-req-proc branch September 5, 2022 13:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: Bluetooth Controller area: Bluetooth LLCP area: Bluetooth area: Tests Issues related to a particular existing or missing test bug The issue is a bug, or the PR is fixing a bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bluetooth: Controller: llcp: CTE request control procedure has missing support for LL_UNKNOWN_RSP

5 participants