Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions subsys/bluetooth/controller/Kconfig.ll_sw_split
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,33 @@ config BT_CTLR_ADV_SYNC_SET
help
Maximum supported periodic advertising sets.

config BT_CTLR_ADV_PDU_LINK
bool "Enable linking of advertising PDU trains"
help
Enables extra space in each advertising PDU to allow linking PDUs. This
is required to enable advertising data trains (i.e. transmission of
AUX_CHAIN_IND).

config BT_CTLR_ADV_SYNC_PDU_BACK2BACK
bool "Enable back-to-back transmission of periodic advertising trains"
depends on BT_CTLR_ADV_PERIODIC
select BT_CTLR_ADV_PDU_LINK
help
Enables transmission of AUX_CHAIN_IND in periodic advertising train by
sending each AUX_CHAIN_IND one after another back-to-back.
Note, consecutive AUX_CHAIN_IND packets are not scheduled but sent at
a constant offset on a best effort basis. This means advertising train can
be preempted by other event at any time.

config BT_CTLR_ADV_SYNC_PDU_BACK2BACK_AFS
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add information about time unit.

int "AUX Frame Space for back-to-back transmission of periodic advertising trains"
depends on BT_CTLR_ADV_SYNC_PDU_BACK2BACK
default 300
range 300 1000
help
Specific AUX Frame Space to be used for back-to-back transmission of
periodic advertising trains. Time specified in microseconds.

config BT_CTLR_ADV_DATA_BUF_MAX
int "Advertising Data Maximum Buffers"
depends on BT_BROADCASTER
Expand Down
1 change: 1 addition & 0 deletions subsys/bluetooth/controller/hci/hci.c
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@

#include "util/util.h"
#include "util/memq.h"
#include "util/mem.h"

#include "hal/ecb.h"
#include "hal/ccm.h"
Expand Down
1 change: 1 addition & 0 deletions subsys/bluetooth/controller/ll_sw/ll_addr.c
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

#include "util/util.h"
#include "util/memq.h"
#include "util/mem.h"

#include "pdu.h"

Expand Down
1 change: 1 addition & 0 deletions subsys/bluetooth/controller/ll_sw/ll_tx_pwr.c
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

#include "util/util.h"
#include "util/memq.h"
#include "util/mem.h"

#include "pdu.h"

Expand Down
3 changes: 3 additions & 0 deletions subsys/bluetooth/controller/ll_sw/lll_adv.h
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,9 @@ struct lll_adv_sync {
uint32_t ticks_offset;

struct lll_adv_pdu data;
#if defined(CONFIG_BT_CTLR_ADV_PDU_LINK)
struct pdu_adv *last_pdu;
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 */

#endif /* CONFIG_BT_CTLR_ADV_PDU_LINK */

#if defined(CONFIG_BT_CTLR_ADV_ISO)
struct lll_adv_iso *iso;
Expand Down
184 changes: 101 additions & 83 deletions subsys/bluetooth/controller/ll_sw/nordic/lll/lll_adv.c
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,6 @@

static int init_reset(void);

static struct pdu_adv *adv_pdu_allocate(struct lll_adv_pdu *pdu, uint8_t last);
#if defined(CONFIG_BT_CTLR_ADV_EXT_PDU_EXTRA_DATA_MEMORY)
static inline void adv_extra_data_release(struct lll_adv_pdu *pdu, int idx);
static void *adv_extra_data_allocate(struct lll_adv_pdu *pdu, uint8_t last);
Expand Down Expand Up @@ -98,8 +97,7 @@ static inline bool isr_rx_ci_adva_check(uint8_t tx_addr, uint8_t *addr,
#define BT_CTLR_ADV_SYNC_SET 0
#endif

#define PDU_MEM_SIZE MROUND(PDU_AC_LL_HEADER_SIZE + \
PDU_AC_PAYLOAD_SIZE_MAX)
#define PDU_MEM_SIZE PDU_ADV_MEM_SIZE
#define PDU_MEM_COUNT_MIN (BT_CTLR_ADV_SET + \
(BT_CTLR_ADV_SET * PAYLOAD_FRAG_COUNT) + \
(BT_CTLR_ADV_AUX_SET * PAYLOAD_FRAG_COUNT) + \
Expand Down Expand Up @@ -257,6 +255,7 @@ int lll_adv_data_release(struct lll_adv_pdu *pdu)
struct pdu_adv *lll_adv_pdu_alloc(struct lll_adv_pdu *pdu, uint8_t *idx)
{
uint8_t first, last;
void *p;

first = pdu->first;
last = pdu->last;
Expand All @@ -281,9 +280,77 @@ struct pdu_adv *lll_adv_pdu_alloc(struct lll_adv_pdu *pdu, uint8_t *idx)

*idx = last;

return adv_pdu_allocate(pdu, last);
p = (void *)pdu->pdu[last];
if (p) {
return p;
}

p = lll_adv_pdu_alloc_pdu_adv();

pdu->pdu[last] = (void *)p;

return p;
}

struct pdu_adv *lll_adv_pdu_alloc_pdu_adv(void)
Copy link
Contributor

@ppryga-nordic ppryga-nordic Apr 26, 2021

Choose a reason for hiding this comment

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

This is a more general comment related with PDU handling API naming.
When look on these functions: lll_adv_pdu_alloc , lll_adv_pdu_alloc_pdu_adv from new person perspective then I'm forced to go into implementation to figure out what they actually do. What more I think that after some break from this code, even one knew what those functions do, he will be forced to look into implementation again.

It would be nice to avoid that by use of more meaningful API names for advertising PDU handling. That may require to change naming for other functions as well.

This comment is related with #31875 (comment). I'm not sure it there was a meeting about that (@cvinayak do you know something about it?).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

while I agree that some existing API names are not quite self-explanatory I don't think this is the right moment to change it since there are a lot of people working on that code so there will be lots of effort needed to rebase after refactoring. so I think we just have to live with it for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, maybe create an issue in Github to enhance it in near future?

{
struct pdu_adv *p;
int err;

p = MFIFO_DEQUEUE_PEEK(pdu_free);
if (p) {
err = k_sem_take(&sem_pdu_free, K_NO_WAIT);
LL_ASSERT(!err);

MFIFO_DEQUEUE(pdu_free);

#if defined(CONFIG_BT_CTLR_ADV_PDU_LINK)
PDU_ADV_NEXT_PTR(p) = NULL;
#endif
return p;
}

p = mem_acquire(&mem_pdu.free);
if (p) {
#if defined(CONFIG_BT_CTLR_ADV_PDU_LINK)
PDU_ADV_NEXT_PTR(p) = NULL;
#endif
return p;
}

err = k_sem_take(&sem_pdu_free, K_FOREVER);
LL_ASSERT(!err);

p = MFIFO_DEQUEUE(pdu_free);
LL_ASSERT(p);

#if defined(CONFIG_BT_CTLR_ADV_PDU_LINK)
PDU_ADV_NEXT_PTR(p) = NULL;
#endif
return p;
}

#if defined(CONFIG_BT_CTLR_ADV_PDU_LINK)
void lll_adv_pdu_release(struct pdu_adv *pdu)
{
mem_release(pdu, &mem_pdu.free);
}

void lll_adv_pdu_linked_release_all(struct pdu_adv *pdu_first)
{
struct pdu_adv *pdu = pdu_first;

while (pdu) {
struct pdu_adv *pdu_next;

pdu_next = PDU_ADV_NEXT_PTR(pdu);
PDU_ADV_NEXT_PTR(pdu) = NULL;
lll_adv_pdu_release(pdu);
pdu = pdu_next;
}
}
#endif

struct pdu_adv *lll_adv_pdu_latest_get(struct lll_adv_pdu *pdu,
uint8_t *is_modified)
{
Expand All @@ -295,11 +362,34 @@ struct pdu_adv *lll_adv_pdu_latest_get(struct lll_adv_pdu *pdu,
uint8_t pdu_idx;
void *p;

if (!MFIFO_ENQUEUE_IDX_GET(pdu_free, &free_idx)) {
return NULL;
}

pdu_idx = first;
p = pdu->pdu[pdu_idx];

do {
void *next;

/* Store partial list in current data index if there is
* no free slot in mfifo. It can be released on next
* switch attempt (on next event).
*/
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.

if (!MFIFO_ENQUEUE_IDX_GET(pdu_free, &free_idx)) {
pdu->pdu[pdu_idx] = p;
return NULL;
}

#if defined(CONFIG_BT_CTLR_ADV_PDU_LINK)
next = lll_adv_pdu_linked_next_get(p);
#else
next = NULL;
#endif

MFIFO_BY_IDX_ENQUEUE(pdu_free, free_idx, p);
k_sem_give(&sem_pdu_free);

p = next;
} while (p);

pdu->pdu[pdu_idx] = NULL;

first += 1U;
if (first == DOUBLE_BUFFER_SIZE) {
Expand All @@ -308,11 +398,7 @@ struct pdu_adv *lll_adv_pdu_latest_get(struct lll_adv_pdu *pdu,
pdu->first = first;
*is_modified = 1U;

p = pdu->pdu[pdu_idx];
pdu->pdu[pdu_idx] = NULL;

MFIFO_BY_IDX_ENQUEUE(pdu_free, free_idx, p);
k_sem_give(&sem_pdu_free);
}

return (void *)pdu->pdu[first];
Expand Down Expand Up @@ -372,38 +458,13 @@ struct pdu_adv *lll_adv_pdu_and_extra_data_alloc(struct lll_adv_pdu *pdu,
void **extra_data,
uint8_t *idx)
{
uint8_t first, last;
struct pdu_adv *p;

first = pdu->first;
last = pdu->last;
if (first == last) {
last++;
if (last == DOUBLE_BUFFER_SIZE) {
last = 0U;
}
} else {
uint8_t first_latest;

pdu->last = first;
cpu_dmb();
first_latest = pdu->first;
if (first_latest != first) {
last++;
if (last == DOUBLE_BUFFER_SIZE) {
last = 0U;
}
}
}

*idx = last;

p = adv_pdu_allocate(pdu, last);
p = lll_adv_pdu_alloc(pdu, idx);

if (extra_data) {
*extra_data = adv_extra_data_allocate(pdu, last);
*extra_data = adv_extra_data_allocate(pdu, *idx);
} else {
if (adv_extra_data_free(pdu, last)) {
if (adv_extra_data_free(pdu, *idx)) {
/* There is no release of memory allocated by
* adv_pdu_allocate because there is no memory leak.
* If caller can recover from this error and subsequent
Expand Down Expand Up @@ -607,49 +668,6 @@ static int init_reset(void)
return 0;
}

static struct pdu_adv *adv_pdu_allocate(struct lll_adv_pdu *pdu, uint8_t last)
{
void *p;
int err;

p = (void *)pdu->pdu[last];
if (p) {
return p;
}

p = MFIFO_DEQUEUE_PEEK(pdu_free);
if (p) {
err = k_sem_take(&sem_pdu_free, K_NO_WAIT);
LL_ASSERT(!err);

MFIFO_DEQUEUE(pdu_free);
pdu->pdu[last] = (void *)p;

return p;
}

p = mem_acquire(&mem_pdu.free);
if (p) {
pdu->pdu[last] = (void *)p;

return p;
}

err = k_sem_take(&sem_pdu_free, K_FOREVER);
LL_ASSERT(!err);

p = MFIFO_DEQUEUE(pdu_free);
LL_ASSERT(p);
/* If !p then check initial value of sem_pdu_free. It must be the same
* as number of elements in pdu_free store. This may not happen in
* runtime.
*/

pdu->pdu[last] = (void *)p;

return p;
}

#if defined(CONFIG_BT_CTLR_ADV_EXT_PDU_EXTRA_DATA_MEMORY)
static void *adv_extra_data_allocate(struct lll_adv_pdu *pdu, uint8_t last)
{
Expand Down
1 change: 1 addition & 0 deletions subsys/bluetooth/controller/ll_sw/nordic/lll/lll_adv_aux.c
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
#include "hal/ticker.h"

#include "util/util.h"
#include "util/mem.h"
#include "util/memq.h"

#include "pdu.h"
Expand Down
Loading