Bluetooth: controller: Add/Remove ACAD on create/terminate BIG

[cvinayak]

Added implementation to add/remove ACAD field in the common
extended header format of the periodic advertising PDU on
create/terminate BIG.

Signed-off-by: Vinayak Kariappa Chettimada [email protected]

[wopu-ot]

This assumes that "sizeof(void *)" is 4 bytes, which is not guaranteed.

[cvinayak]

sizeof(value) ?

[wopu-ot]

Incrementing by sizeof(uint32) would match what is stored above and would work if sizeof(void *) <= sizeof(uint32). If sizeof(void *) > sizeof(uint32), storing the pointer as uint32_t is broken in the first place.

[wopu-ot]

This logic to pass a length and return a pointer via field_data to then fill in the actual ACAD data is pretty convoluted. Wouldn't it be easier to just pass down the data to be filled into the ACAD?

[cvinayak]

This is where it is going in terms of implementation: 02e62ba#diff-46edd4480ca72c05f7bb026bdac384a2d48b03317361f2ea6ccdf70d15dc6769R197-R241

Passing by parameters or filling into a structure or passing the whole LLL context to fill inside set_clear, all have their disadvantage.

[wopu-ot]

I think having dedicated parameters or fields for passing the length and returning the address instead of reusing field_data would clear up a lot. Maybe the general pattern should be that field_data does not hold the data to be filled in on the way down, but the address where to write data on the way up.

So instead of writing (just as an example):

                cte_info.type = df_cfg->cte_type;
                cte_info.time = df_cfg->cte_length;
                pdu_data.field_data = (uint8_t *)&cte_info;
                ...
                err = ull_adv_sync_pdu_set_clear(adv, ULL_ADV_PDU_HDR_FIELD_CTE_INFO, 0, &pdu_data, &ter_idx);

the pattern could be

                err = ull_adv_sync_pdu_set_clear(adv, ULL_ADV_PDU_HDR_FIELD_CTE_INFO, 0, &pdu_data, &ter_idx);
                cte_info = pdu_data.field_data;                
                cte_info->type = df_cfg->cte_type;
                cte_info->time = df_cfg->cte_length;

This would then also fit the use case for the somewhat bigger big_info struct.

[cvinayak]

The intention is to use the ull_adv_aux_hdr_set_clear and ull_adv_sync_pdu_set_clear (in the future combine them into one function) to take-in variable number of data for numerous header fields (bitmap for set/clear, say adding adv and init addresses, may be other parameters together). Hence the field_data is length encoded (like the AD data format, length-data). This is done to save on execution time updating the PDU.

Let me review so far, if array of lengths can be passed and then return array of pointers that can be used to fill in. (But this will not happen until I get to the point I have an initial samples of ISO broadcaster and ISO receiver)

The current implementation of passing length+value for simple values (AdvA, TargetA, ADI, AuxPtr, TxPower and Host AD Data) and returning pointers for struct values (CTEInfo, SyncInfo, and ACAD).

This means, we keep as-is for AdvA, TargetA, ADI, AuxPtr, TxPower and Host AD Data; and CTEInfo and SyncInfo be ported to populating the return address, do you agree?

[wopu-ot]

I would prefer to use the same pattern for everything and I think it would eventually lead to simpler, more efficient code. But I can live with distinguishing between "simple" and "complex" cases.

[cvinayak]

@wopu-ot as communicated to you few weeks back in our status meeting, I tend to agree with passing down field bitmap and passing up address as stated in your comment here #32622 (comment)

@andrzej-kaczmarek FYI.

[cvinayak]

Problems making consistent for the below mentioned fields (these are all the fields in the current code base):

1. ULL_ADV_PDU_HDR_FIELD_ADVA
   -- Address type and address are in different locations, will need two pointers and bit offset, i.e. pointer return for the byte and offset to the bit, a pointer to the 6 byte address in the PDU; to be returned to caller so as to set the advertising address.
2. ULL_ADV_PDU_HDR_FIELD_AD_DATA
   -- LE Set Extended Advertising Data command can be used to set legacy advertising PDU data, moving the code that set the legacy advertising outside ull_adv_aux_hdr_set_clear adds (a little bit repeated) overhead code to peek into the current PDU.
3. ULL_ADV_PDU_HDR_FIELD_AUX_PTR
   -- Currently only the PHY is assigned, and is not directed called anywhere with this flag; PHY which is easily available in the passed advertising set structure, making this be assigned by the caller is added code and CPU cycles.
4. ULL_ADV_PDU_HDR_FIELD_CTE_INFO
   -- it is just one byte value, for which returning a pointer and being assigned by caller seems a bit of overhead.

The only field besides BIGInfo can be made to be caller assigned is:

1. ULL_ADV_PDU_HDR_FIELD_SYNC_INFO

I will work this week on extracting the sync_info outside to caller.

[cvinayak]

@wopu-ot Updated with the sync_info population by caller. Please review.

[Thalley]

There are some possible readability additions that could be applied, but since there's not really any precedence set for the controller for the specified lines I don't think it's strictly necessary to change, but something to keep in mind.

[cvinayak]

@wopu-ot Could you take another look.

[wopu-ot]

The change still passes pointers through a 4-byte/uint32_t value, which may work right now but is a bug that will manifest sooner or later. Please update to use appropriately typed fields and variables.

[cvinayak]

The change still passes pointers through a 4-byte/uint32_t value, which may work right now but is a bug that will manifest sooner or later. Please update to use appropriately typed fields and variables.

I have replaced to use memcpy, please review.

[cvinayak]

Also converted to draft as I want #31875 to be merged before this, to ensure I can rebase this PR thereafter

[cvinayak]

@wopu-ot This should cover pointer size, right?

[wopu-ot]

Hard-coded assumption that sizeof(void *) == sizeof(uint32_t)

[cvinayak]

will fix this and send updates

[wopu-ot]

Hard-coded assumption that sizeof(void *) == sizeof(uint32_t)

[cvinayak]

will fix this and send updates