Skip to content

Conversation

@andrzej-kaczmarek
Copy link
Contributor

This adds initial support for chaining in periodic advertising, i.e. we can send AUX_SYNC_IND followed by sequence of AUX_CHAIN_IND.

New set of APIs to manage chains instead of single PDUs in advertising set is described in relevant commit. AUX_CHAIN_IND packets are not scheduled as they should be, instead they are just sent one after another at constant offset (T_MAFS) on best effort basis since there's no reservation done for complete train. There's no code that can do fragmentation of data coming from host so I added a temporary commit which will add single chain whenever periodic advertising data is changed on a set.

@andrzej-kaczmarek
Copy link
Contributor Author

@ppryga

Copy link
Contributor

@ppryga-nordic ppryga-nordic left a comment

Choose a reason for hiding this comment

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

I'd like to see a use of the AUX_CHAIN_IND in a set advertising data procedure. I'm especially interested about handling of data update procedure.

It would required to create complete chain from scratch. In such situation we have to take extended advertising header from last used chain PDUs and move them to new chain. During that process we have to incorporate new periodic advertising data. Am I correct?

Is it possible to see sample application that uses the functionality?

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.

Please setup a code walk-through meeting to sync-up on the design.

Comment on lines 115 to 145
Copy link
Contributor

Choose a reason for hiding this comment

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

Do not need this.

Comment on lines 150 to 167
Copy link
Contributor

Choose a reason for hiding this comment

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

Use singly linked list. Do not need these implementation.

@cvinayak cvinayak requested a review from wopu-ot February 19, 2021 06:43
@cvinayak
Copy link
Contributor

cvinayak commented Feb 19, 2021

@wopu-ot Please provide your design thoughts. I had decide during the introduction of 255 byte Advertising PDU to maintain the double buffering design for the advertising PDUs.

For chaining, my proposal is to use singly linked list, wherein:

  1. Increase the PDU_MEM_SIZE by sizeof(void *) to store next PDU pointer,

    #define PDU_MEM_SIZE MROUND(PDU_AC_LL_HEADER_SIZE + \

  2. Allocation of first PDU in chaining is using lll_adv_pdu_alloc ,

    struct pdu_adv *lll_adv_pdu_alloc(struct lll_adv_pdu *pdu, uint8_t *idx)

  3. Allocation of subsequent PDUs in chaining is using adv_pdu_allocate,

    static struct pdu_adv *adv_pdu_allocate(struct lll_adv_pdu *pdu, uint8_t last)

Repeated call to lll_adv_pdu_alloc, always returns the first PDU in the chain under preparation. This way HCI commands to append fragments can append PDUs at the tail of the singly linked list.

  1. Release of all PDUs in a stale chain by using lll_adv_pdu_latest_get, which enqueue each PDU into the MFIFO,
    struct pdu_adv *lll_adv_pdu_latest_get(struct lll_adv_pdu *pdu,

@andrzej-kaczmarek
Copy link
Contributor Author

There are some new helpers to manage PDUs, e.g. duplicate them or create new one based on old one. Not sure if this is the best approach but we need something to conveniently create/duplicate PDUs and link them into chains - I don't think ull_adv_sync_pdu_set_clear alone can do everything internally unless it will become really complicated.

This also does not yet allow to accept fragmented data from host, AD is only allowed in 1st PDU as previously but chain can be preserved when updating AD after CTEInfo was added.

@andrzej-kaczmarek
Copy link
Contributor Author

I think I addressed comments that were still relevant after changing to new chaining design as proposed by Vinayak.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently there will be no next switch attempt because caller will just assert, as it does without linking. So we can assume this is ok as there always should be enough free slots in mfifo or we can add handling in caller later, i.e. it will skip an event.

Alternatively we can put only 1st PDU in mfifo with all linked PDUs and let ULL deal with it, i.e. it will dequeue this PDU and use it, then release linked PDUs directly to pool.

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.

Good work. Minor request to re-use sw_switch, otherwise lets meet for "one last time" to review change requested by @ppryga and settle for a list of pending changes to be reworked.

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
struct pdu_adv *last_pdu;
#if defined(CONFIG_BT_CTLR_ADV_PDU_LINK)
struct pdu_adv *last_pdu;
#endif /* CONFIG_BT_CTLR_ADV_PDU_LINK */

Copy link
Contributor

Choose a reason for hiding this comment

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

Why sw_switch can not be re-used? What is the difference?
is this because of sw_switch assuming its always tx-to-rx or rx-to-tx?

If that is the difference, then change sw_switch to:
static void sw_switch(uint8_t dir_curr, uint8_t dir_next, uint8_t phy_curr, uint8_t flags_curr, uint8_t phy_next, uint8_t flags_next)

based on dir_curr and dir_next use the right chain delay and ready delay. sw_switch can be reused to piggyback receptions too.

Pass constants to these dir_curr and dir_next parameters at the callers of this functions in LLL files, compiler will optimize call paths not exercised by constant value parameter passed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Something that I am trying to use for BIS subevents: #33118

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sw_switch is not inlined so it will not be optimized (unless I misunderstood what you meant) for new case, but regardless reusing sw_switch as in mentioned PR works for me since it's just a single extra branch so not much of a difference. I initially thought that there will be more to change there thus I created a copy.

Copy link
Contributor

Choose a reason for hiding this comment

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

What about BT_CTLR_ADV_SYNC_PDU_BACK2BACK?

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
void radio_switch_complete_and_pback(uint8_t phy, uint8_t flags)
void radio_switch_complete_and_b2b_tx(uint8_t phy, uint8_t flags)

i.e. if using the term back-to-back is ok? That is what is happening in reality. I am ok with piggyback/pback.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

back2back sounds better, will use it

Copy link
Contributor

Choose a reason for hiding this comment

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

BT_CTLR_ADV_SYNC_PDU_BACK2BACK for me sounds better. If this is changed to BACK2BACK then I suggest to change API names with suffixes _pback.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why check value provided by ext_hdr_flags by ext_hdr?
Probably it would be optimized by compiler but in my humble opinion it is easier to spot that it depends on function parameter than search for assignment to ext_hdr.

It seems to me that ext_hrd may be removed at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can change this but I guess this is just a matter of preference. The way I read this code is that I check flags inside PDU which is initialized earlier using ext_hdr_flags, thus I do not check that parameter explicitly since.

Copy link
Contributor

Choose a reason for hiding this comment

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

For me it makes the code more unclear.

@andrzej-kaczmarek
Copy link
Contributor Author

I included b2b radio interface from #33118 instead of previous implementation, works fine.

@ppryga-nordic
Copy link
Contributor

@andrzej-kaczmarek I've provided a PR to your fork repository that includes some commits that suggest fixes for issues I've found while testing the implementation provided by this PR. (for ref. to others here is link to mentioned PR: andrzej-kaczmarek#1).

I've got two additional things:

  • what do you think about refactorization of functions that do operations on periodic advertising PDUs? I mean former code that is already in upstream and your new code. It looks like it may be made more common.
  • your design assumes that every time PDUs are updated new chain is created. In my humble opinion new chain should be created only in situation that former one is already in use by LLL (it was enqueued to LLL and peeked by it). In case we update the chain (e.g. we have set new periodic advertising data and want to enable CTE), it seems to be a better option to re-use existing chain:
  1. update PDUs is possible
  2. add new PDUs to chain if required (e.g. number of CTE is greater than previous length of chain)
  3. remove (and release memory) if we find out that there are empty PDUs that do nothing.

Thanks to re-use of existing chain we save a lot of memory. To handle CTE (max number of PDUs is 16) we need ~4K of RAM.
If we want to have memory for: chain in use by LLL, chain that was updated and chain for future updates we need ~12K of RAM.
If we re-use existing chain we save 4K memory.

@andrzej-kaczmarek
Copy link
Contributor Author

@andrzej-kaczmarek I've provided a PR to your fork repository that includes some commits that suggest fixes for issues I've found while testing the implementation provided by this PR. (for ref. to others here is link to mentioned PR: andrzej-kaczmarek#1).

Would be better if we stick to traditional GitHub way and have comments here. It's not quite easy to figure out which parts of that PR are purely fixes and which are added for CTE.

I've got two additional things:

* what do you think about refactorization of functions that do operations on periodic advertising PDUs? I mean former code that is already in upstream and your new code. It looks like it may be made more common.

I think the main question here whether we want to have one big function like adv_sync_hdr_set_clear that will just do whatever it takes to update chain with new/removed fields or set of small helpers that can be combined in each case (e.g. set data, enable CTE) to do specific task. I prefer the latter, but don't know what others think about it.

* your design assumes that every time PDUs are updated new chain is created. In my humble opinion new chain should be created only in situation that former one is already in use by LLL (it was enqueued to LLL and peeked by it). In case we update the chain (e.g. we have set new periodic advertising data and want to enable CTE), it seems to be a better option to re-use existing chain:


1. update PDUs is possible

2. add new PDUs to chain if required (e.g. number of CTE is greater than previous length of chain)

3. remove (and release memory) if we find out that there are empty PDUs that do nothing.

I can update set data to reuse old PDUs.

Thanks to re-use of existing chain we save a lot of memory. To handle CTE (max number of PDUs is 16) we need ~4K of RAM.
If we want to have memory for: chain in use by LLL, chain that was updated and chain for future updates we need ~12K of RAM.
If we re-use existing chain we save 4K memory.

That could be improved if we have pools of different buffer sizes so small PDUs do not waste memory.

@cvinayak
Copy link
Contributor

I think the main question here whether we want to have one big function like adv_sync_hdr_set_clear that will just do whatever it takes to update chain with new/removed fields or set of small helpers that can be combined in each case (e.g. set data, enable CTE) to do specific task. I prefer the latter, but don't know what others think about it.

@andrzej-kaczmarek Please refer to this discussion, and lets align, so we dont have redundancy in our efforts. #32622 (comment)

@andrzej-kaczmarek
Copy link
Contributor Author

I think the main question here whether we want to have one big function like adv_sync_hdr_set_clear that will just do whatever it takes to update chain with new/removed fields or set of small helpers that can be combined in each case (e.g. set data, enable CTE) to do specific task. I prefer the latter, but don't know what others think about it.

@andrzej-kaczmarek Please refer to this discussion, and lets align, so we dont have redundancy in our efforts. #32622 (comment)

@cvinayak does it mean that adv_sync_hdr_set_clear should be do-it-all, i.e. should also handle chain internally?

@cvinayak
Copy link
Contributor

@andrzej-kaczmarek I am open to a discussion and settle on a solution based on pros and cons. This will have to be a group meeting after the Easter vacations.

@wopu-ot Could you please include this as a action item that we can settle ASAP (just after Easter vacations) ?

Copy link
Contributor

@ppryga-nordic ppryga-nordic Jun 23, 2021

Choose a reason for hiding this comment

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

Suggested change
} else if ((hdr_rem_fields & ULL_ADV_PDU_HDR_FIELD_CTE_INFO) ||
} else if (!(hdr_rem_fields & ULL_ADV_PDU_HDR_FIELD_CTE_INFO) ||

This is an error that is available from first implementation of the code.

Comment on lines 312 to 313
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
ULL_ADV_PDU_HDR_FIELD_CTE_INFO,
0, NULL);
0,
ULL_ADV_PDU_HDR_FIELD_CTE_INFO, NULL);

Copy link
Contributor

@ppryga-nordic ppryga-nordic Jun 24, 2021

Choose a reason for hiding this comment

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

Suggested change
static uint8_t adv_sync_pdu_init(struct pdu_adv *pdu, uint8_t ext_hdr_flags)
static void adv_sync_pdu_init(struct pdu_adv *pdu, uint8_t ext_hdr_flags)

It always returns zero.

@cvinayak cvinayak changed the title Bluetooth: controller; Add initial support for chaining in periodic advertising Bluetooth: controller: Add initial support for chaining in periodic advertising Jul 2, 2021
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.

Quick ack, do not have any strong objections, overall design was previously agreed. Any rework if any would be minor, can be taken up in subsequent PRs.

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#define ULL_ADV_PDU_HDR_FIELD_TX_POWER BIT(7)
#define ULL_ADV_PDU_HDR_FIELD_TX_POWER BIT(6)

Copy link
Contributor

Choose a reason for hiding this comment

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

I will fix this in one of my subsequent PRs.

@ppryga-nordic
Copy link
Contributor

Removed part of code responsible for setting address timeout, it should not be configured in isr_tx for back to back TX.

@ppryga-nordic ppryga-nordic marked this pull request as ready for review July 2, 2021 16:12
@cvinayak
Copy link
Contributor

cvinayak commented Jul 6, 2021

@wopu-ot This PR has changed the way extended advertising PDU is allocated, this is conflicting my BIS work, and wish to have the changes here upstreamed before I continue with rework on my PRs. Please do the needful to review this PR.

Advertising channel packets do not have MIC, there's no need to have
extra parameter which always has to be set to 0 anyway.

Signed-off-by: Andrzej Kaczmarek <[email protected]>
This adds helper to always allocate advertising PDU either from memory
pool or pdu_free queue and does not reuse existing PDU in adv_pdu.

Signed-off-by: Andrzej Kaczmarek <[email protected]>
This function is the same as lll_adv_pdu_alloc except it also allocates
extra data at the end - it can just use lll_adv_pdu_alloc call to avoid
extensive c&p.

Signed-off-by: Andrzej Kaczmarek <[email protected]>
This adds support to allow advertising PDUs to be linked which is
required to send advertising trains, i.e. AUX_CHAIN_IND.

PDUs are linked with a simple single-linked list, the pointer to next
PDU is stored at the very end of PDU buffer. This prevents it from
being overwritten if PDU is modified and allows for build time offset
calculation.

There are few helpers added to make handling easier, e.g.:
- get next linked PDU
- get last linked PDU
- link one PDU to another
- link PDU at the end
- release all linked PDUs (except for 1st)

Signed-off-by: Andrzej Kaczmarek <[email protected]>
This enables chaining ota for periodic advertising. AUX_CHAIN_IND PDUs
will be sent automatically if AuxPtr is detected in preceding PDU.
AuxPtr offset is always set to achieve minimal required frame spacing,
i.e. 300us (T_mafs). AuxPtr in all PDUs in advertising train are
updated on enqueue since PDU spacing is already known at that time so
we do not need to waste time in LLL.

Signed-off-by: Andrzej Kaczmarek <[email protected]>
This adds adv_sync_pdu_init helper which initializes pdu_adv buffer
with contents of AUX_SYNC_IND/AUX_CHAIN_IND PDU. Extended header flags
can be specified to reserve required space for corresponding fields if
necessary.

Signed-off-by: Andrzej Kaczmarek <[email protected]>
This adds some initial support to update AD in chain. We still only
support placing AD in 1st PDU, but this will properly copy any linked
PDUs that may be added due to e.g. CTEInfo present.

Signed-off-by: Andrzej Kaczmarek <[email protected]>
This adds simple helper to update CTEInfo. It assumes proper periodic
adv PDU as input to simplify code.

Signed-off-by: Andrzej Kaczmarek <[email protected]>
adv_sync_hdr_set_clear was just wrapped by ull_adv_sync_pdu_set_clear
so we can merge both into single function.

Signed-off-by: Andrzej Kaczmarek <[email protected]>
@cvinayak
Copy link
Contributor

cvinayak commented Jul 6, 2021

@ppryga I have rebased to upstream/main and pushed.

@ppryga-nordic
Copy link
Contributor

@nashif, @galak, @carlescufi could one of you merge the PR? CI is green.

@nashif nashif merged commit 9db0a18 into zephyrproject-rtos:main Jul 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: Bluetooth Controller area: Bluetooth Enhancement Changes/Updates/Additions to existing features

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants