Skip to content

Add sending multiple CTE with periodic advertising chains #36585

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 14 commits into from
Aug 8, 2021
Merged

Add sending multiple CTE with periodic advertising chains #36585

merged 14 commits into from
Aug 8, 2021

Conversation

@ppryga-nordic
Copy link
Contributor

The PR enhances support for connectionless direction finding with possibility to transmit more than single PDU including CTE.
The multiple CTEs are transmitted with periodic advertising PDUs chain.

If the number of available PDUs in a periodic advertising chain is enough then CTE is just added to requested number of PDUs.
If the number of available PDUs in a chain is lower than number of requested CTEs, then the chain is extended with empty AUX_CHAIN_IND PDUs.

The PR is based on PR: #31875
It may be merged after the base PR is merged to main.

@ppryga-nordic ppryga-nordic added the DNM This PR should not be merged (Do Not Merge) label Jun 28, 2021
@github-actions github-actions bot added area: Bluetooth area: Bluetooth Controller area: Tests Issues related to a particular existing or missing test labels Jun 28, 2021
@ppryga-nordic ppryga-nordic changed the title Add sending of multiple CTE with periodic advertising chains Add sending multiple CTE with periodic advertising chains Jun 28, 2021
@ppryga-nordic ppryga-nordic force-pushed the github-ble-df-aux-chain-tx-cte branch 3 times, most recently from 7c9e001 to b94bb38 Compare June 29, 2021 10:41
@zephyrbot zephyrbot requested review from Thalley and asbjornsabo June 29, 2021 13:20
Thalley
Thalley requested changes Jun 30, 2021
View reviewed changes
ppryga-nordic
ppryga-nordic commented Jul 2, 2021
View reviewed changes
@ppryga-nordic ppryga-nordic force-pushed the github-ble-df-aux-chain-tx-cte branch from b94bb38 to a513436 Compare July 2, 2021 15:38
@ppryga-nordic
Copy link
Contributor Author

@Thalley all comments were addressed.

I've to add a commit to clean up ull_adv_sync_pdu_alloc and ull_adv_sync_pdu_set_clear functions. I'll do that being back.

Thalley
Thalley requested changes Jul 5, 2021
View reviewed changes
Copy link
Contributor

@Thalley Thalley left a comment

Choose a reason for hiding this comment

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

A few minor comments, otherwise LGTM

@ppryga-nordic ppryga-nordic force-pushed the github-ble-df-aux-chain-tx-cte branch from a513436 to 2157b01 Compare July 12, 2021 06:43
@ppryga-nordic
Copy link
Contributor Author

Added changes requested by @Thalley.
Rebased on current main to clean number of commits introduced by this PR.

Thalley
Thalley requested changes Jul 12, 2021
View reviewed changes
@ppryga-nordic ppryga-nordic force-pushed the github-ble-df-aux-chain-tx-cte branch from 2157b01 to 644f043 Compare July 13, 2021 13:16
@ppryga-nordic
Copy link
Contributor Author

@Thalley Two comments were not addresses. The one with static inline and the one related with 2 us jitter. The latter requires separate commit and general change in controller code to remove jitter related magic numbers.

Thalley
Thalley approved these changes Jul 13, 2021
View reviewed changes
@ppryga-nordic ppryga-nordic force-pushed the github-ble-df-aux-chain-tx-cte branch from 644f043 to 015c9be Compare July 14, 2021 07:32
@ppryga-nordic ppryga-nordic removed the DNM This PR should not be merged (Do Not Merge) label Jul 21, 2021
@ppryga-nordic ppryga-nordic force-pushed the github-ble-df-aux-chain-tx-cte branch from 015c9be to 4e86bb1 Compare July 22, 2021 21:57
@ppryga-nordic
Copy link
Contributor Author

Rebased on laters main.
Fixed two issues:

  • PDU memory was released twice to pdu_free fifo and mem_pdu. Caused by function: rem_cte_info_from_per_adv_chain when CTE only PDU is found and end of chain is released.
  • assignment of wrong address to ter_dptr_prev in ull_adv_sync_pdu_set_clear. Caused evaluation of wrong size of ACAD and increase of PDU size. Eventually PDU size reached limit and BT_HCI_ERR_PACKET_TOO_LONG was returned.

@ppryga-nordic
Copy link
Contributor Author

@cvinayak @andrzej-kaczmarek @Thalley could you take a look?

Thalley
Thalley requested changes Jul 23, 2021
View reviewed changes
@ppryga-nordic
Bluetooth: controller: Add conf max num of PDUs with CTE in per adv c…
06266bf 
…hain

Add possiblity to configure maximum number of PDUs with Constant Tone
Extension in a single periodic advertising chain.

Signed-off-by: Piotr Pryga <[email protected]>
@ppryga-nordic
Bluetooth: controller: Make per. adv. PDUs count related to max CTE num
998345f 
Amount of memory allocated for advertising PDUs (including
periodic advertising) depends on two factors:
- maximum advertising data length
- maximum number of CTE in a periodic advertising chain.

Maximum advertising data length is divided by maximum size
of a single fragment (number of payload bytes that single
advertising PDU may hold) to get required number of fragments.

Actual number of PDUs allocated for advertising is maximum
of acutal number of advertising payload fragments and maximum
number of CTEs.

Signed-off-by: Piotr Pryga <[email protected]>
@ppryga-nordic
Bluetooth: controller: radio: Add back2back TX switch after PHYEND event
0639afa 
Transmission of Constant Tone Extension is done after END event
triggered by radio. To correctly switch from TX to TX (back 2 back
TX) after transmission of CTE PHYEND event must be used instead of
END event.

The commit provides required function that allows LLL to correctly
setup radio to do back to back switch of TX when CTE transmission
is enabled.

The radio_switch_complete_and_phy_end_b2b_tx cannot be implemented
in radio.c source file. It will fail build of unit tests targeted
for NRFBSIM simulator board. There is no RADIO_SHORTS_PHYEND_DISABLE_Msk
macro defined in bsim_hw_models module.

Signed-off-by: Piotr Pryga <[email protected]>
@ppryga-nordic
Bluetooth: controller: df: Add const to antenna patterns pointer
58dee79 
Add const qualifier in radio API related with setting antenna
switch pattern.

Signed-off-by: Piotr Pryga <[email protected]>
@ppryga-nordic
Bluetooth: controller: LLL: Add peek extra_data related with lll_adv_…
a25994b 
…sync

Configuration of CTE for transmission is stored in extra_date member
of a lll_adv_sync object. PDUs in periodic advertising chain share the
same CTE configuration, so there is single instance of the extra_data.
To configure CTE transmission for every PDU in periodic advertising
chain a new function was introduced to peek extra_data instance without
peeking new PDU.

Signed-off-by: Piotr Pryga <[email protected]>
@ppryga-nordic ppryga-nordic force-pushed the github-ble-df-aux-chain-tx-cte branch from 4e86bb1 to 31f4803 Compare July 27, 2021 16:43
@ppryga-nordic
Bluetooth: controller: LLL: Add CTE confing for per. adv. chain TX
81f4ed0 
Add configuration of CTE for periodic advertising chain transmission.
The commit provides configuration of radio in prepare stage and
in handle of Tx ISR. CTE is configured only for PDUs that
have cte_info field in extended advertising header.

During prepare of periodic advertising event there are updated
aux_ptr fields in extended advertising header in other PDUs from
a periodic advertising chain. aux_ptr offset value also depends
on CTE length. CTE configuration is always the same for every
PDU in periodic advertising chain.
CTE may be added to requested number of PDUs in periodic
advertising chain. Although it is possible that there are PDUs
that don't have CTE. PDUs that have CTE are alsways at the beginning
of a periodic advertising chain.

Signed-off-by: Piotr Pryga <[email protected]>
@ppryga-nordic
Bluetooth: controller: ULL: Make API for per adv sync PDU change global
e7efb85 
Some functions, provided to handle changing content of periodic
advertising PDUs, were defined as static.
Code responsible for handling direction finding updates periodic
advertising PDUs also. For that purposes, those functions were mede
globally accessible.

Signed-off-by: Piotr Pryga <[email protected]>
@ppryga-nordic
Bluetooth: controller: ULL: ull_adv_sync_hdr_set_clear update PDU in …
5aee904 
…place

Enabling or disabling transmission of Constant Tone Extension with
periodic advertising PDUs requires update of a PDU content.
CTE_INFO field of periodic advertising PDU must be filled with
appropriate data. That operation may be done for periodic
advertising PDU (or chain of PDUs) filled with other payload that
wasn't enqueued towards LLL. In that situation PDUs are updated
in place, to avoid increase of memory consumption.

The commit changes ull_adv_sync_pdu_set_clear function to
make possible update of the advertising PDU in place.

Signed-off-by: Piotr Pryga <[email protected]>
@ppryga-nordic ppryga-nordic force-pushed the github-ble-df-aux-chain-tx-cte branch from 31f4803 to 9978a98 Compare July 27, 2021 21:13
@carlescufi carlescufi requested a review from Thalley July 29, 2021 11:25
Thalley
Thalley approved these changes Aug 2, 2021
View reviewed changes
Copy link
Contributor

@Thalley Thalley left a comment

Choose a reason for hiding this comment

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

Only a minor suggestion, otherwise LGTM

cvinayak
cvinayak requested changes Aug 4, 2021
View reviewed changes
Copy link
Contributor

@cvinayak cvinayak left a comment

Choose a reason for hiding this comment

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

Very minor change requests, otherwise looks good to me. (Have not tested on target).

subsys/bluetooth/controller/ll_sw/ull_adv_sync.c Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove initialization and place is close to its first reference.

Like in the case of ter_hdr_prev place it in else clause of the if at line 902 (this will avoid double assignment, i.e. the current initializer and then inside if clause)

Copy link
Contributor

Choose a reason for hiding this comment

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

iirc, structs can only be initialized when declared, so if we remove the initialization here, we'd need to a memset later or set the individual fields to 0.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Change to memset increases FLASH size by 16 bytes in comparison to current version. Unfortunately I haven't got J-Trace to measure it during execution. I'm leaving it as it is implemented to avoid increase of FLASH size.

Copy link
Contributor

@cvinayak cvinayak Aug 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

The header is 1 byte, use memset and compiler will optimize it to *((uint8_t *)&ter_hdr_prev) = 0U; which is what was done in the original code, with memset and use of initializer we are at the mercy of the toolchain to assign the one byte.

@ppryga-nordic ppryga-nordic force-pushed the github-ble-df-aux-chain-tx-cte branch from 9978a98 to 04f2a2e Compare August 5, 2021 10:58
@ppryga-nordic
Bluetooth: controller: ULL: Add sending of number of CTEs in per adv …
a58854d 
…chain

Direction finding functionality allows to send a number of periodic
advertising PDUs in a chain that have CTE.
ll_df_set_cl_cte_tx_enable function was changed to update periodic
advertising chain to include cte_info field. If the chain is too short,
there is less PDUs in the chain than requested number of CTEs,
the function will add new empty PDUs to the chain end.

Signed-off-by: Piotr Pryga <[email protected]>
@ppryga-nordic
Bluetooth: controller: Add macro with minimum allowed adv. PDU size
e4c3909 
To check if advertising PDU is empty we can compare its length to 1.
To avoid use of magic number, the commit provides a macro for that
purpose.

Signed-off-by: Piotr Pryga <[email protected]>
@ppryga-nordic
Bluetooth: controller: Add release of chains when get latest adv pdu
81a4474 
Add release of chained PDUs by lll_adv_pdu_and_extra_data_-
latest_get function. It is requier to release unused
PDUs from a chain to avoid PDUs leakage.
It maight happen when chained PDUs were used by Direction
Finding, then lll_adv_pdu_and_extra_data_lates_get is used
instead of lll_adv_pdu_lates_get.

Signed-off-by: Piotr Pryga <[email protected]>
@ppryga-nordic
Bluetooth: controller: ULL: Add handling of chains in disable CTE
f3203c4 
Direction finding functionality allows to send a number of periodic
advertising PDUs in a chain that have CTE.
Disable sending CTE requiers additional steps while removing
cte_info from periodic advertising chains.
Removal of cte_info fields may be just delete of that filed
from extended advertising header. In case the PDUs are empty
PDUs created just to transport CTE. Those PDUs should be removed
from a periodic advertising chain.

Signed-off-by: Piotr Pryga <[email protected]>
@ppryga-nordic
tests: Bluetooth: df: Add tests for use of CTE in per adv chains
7401991 
Bluetooth host may request to send more than one CTE in
connectionless mode. That is relized by periodic advertising
chained PDUs.
To check correctness of implementation of such functionality
new unit tests were added.

Signed-off-by: Piotr Pryga <[email protected]>
@ppryga-nordic
Bluetooth: controller: remove warning about defined bvt unused function
2010fd1 
Remove compilation warning about adv_sync_pdu_ad_data_set function
that is defined but not used.

Signed-off-by: Piotr Pryga <[email protected]>
@ppryga-nordic ppryga-nordic force-pushed the github-ble-df-aux-chain-tx-cte branch from 04f2a2e to 2010fd1 Compare August 5, 2021 11:13
@ppryga-nordic
Copy link
Contributor Author

Applied changes requested by @cvinayak.
Added commit that removes compilation warning about unused function: adv_sync_pdu_ad_data_set.

Code was tested by direction_finding_connectionless_tx sample and Bluetooth sniffer.

@cfriedt cfriedt merged commit 2675b8d into zephyrproject-rtos:main Aug 8, 2021
@ppryga-nordic ppryga-nordic deleted the github-ble-df-aux-chain-tx-cte branch September 5, 2022 13:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Thalley Thalley Thalley approved these changes

@cvinayak cvinayak cvinayak approved these changes

@carlescufi carlescufi Awaiting requested review from carlescufi

@jhedberg jhedberg Awaiting requested review from jhedberg

@joerchan joerchan Awaiting requested review from joerchan

@kruithofa kruithofa Awaiting requested review from kruithofa

@thoh-ot thoh-ot Awaiting requested review from thoh-ot

@Vudentz Vudentz Awaiting requested review from Vudentz

@asbjornsabo asbjornsabo Awaiting requested review from asbjornsabo

Assignees

@cvinayak cvinayak

Labels

area: Bluetooth Controller area: Bluetooth area: Tests Issues related to a particular existing or missing test

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@ppryga-nordic @Thalley @cvinayak @cfriedt