Bluetooth: Controller: Fix missing Extended Advertising Type Validations

[cvinayak]

Add Extended Advertising Type validation when associated
Periodic Advertising is enable, and Extended Advertising
set is re-configured to other advertising types.

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

[cvinayak]

Suggested change

	/* high duty cycle directed */
	if (evt_prop & BT_HCI_LE_ADV_PROP_HI_DC_CONN) {
	adv_type = 0x01; /* PDU_ADV_TYPE_DIRECT_IND */
	} else {
	adv_type = leg_adv_type[evt_prop & 0x03];
	}
	/* high duty cycle directed */
	if (evt_prop & BT_HCI_LE_ADV_PROP_HI_DC_CONN) {
	adv_type = 0x01; /* Index of PDU_ADV_TYPE_DIRECT_IND in pdu_adv_type[] */
	} else {
	/* We take advantage of the fact that 2 LS bits of evt_prop can map to legacy advertising PDU types by using a look up */
	adv_type = leg_adv_type[evt_prop & BIT_MASK(sizeof(leg_adv_type)];
	}

0x03 is BIT_MASK(sizeof(leg_adv_type)) which is to restrict the index to the size of the lookup table.

Is the about comments and code suggestion acceptable?

[Thalley]

Hmm, that is actually making it more confusing to me. So adv_type is not an adv_type, but rather an index of a value in a local array that is unordered:

			uint8_t const leg_adv_type[] = {
				0x03, /* PDU_ADV_TYPE_NONCONN_IND */
				0x04, /* PDU_ADV_TYPE_DIRECT_IND */
				0x02, /* PDU_ADV_TYPE_SCAN_IND */
				0x00  /* PDU_ADV_TYPE_ADV_IND */
};

but only when evt_prop & BT_HCI_LE_ADV_PROP_HI_DC_CONN else it is a value in the array?

I think using the same variable as an index and as a value is hard to follow in terms of readability.

[cvinayak]

adv_type is an index into pdu_adv_type[]. leg_adv_type[] is second level of lookup from evt_prop value that gives an index into pdu_adv_type[]. Do suggest changes that can make it clearer to understand the level of lookup.

[cvinayak]

That is:

			uint8_t const leg_adv_type[] = {
				0x03, /* index of PDU_ADV_TYPE_NONCONN_IND in pdu_adv_type[] */
				0x04, /* index of PDU_ADV_TYPE_DIRECT_IND in pdu_adv_type[] */
				0x02, /* index of PDU_ADV_TYPE_SCAN_IND in pdu_adv_type[] */
				0x00  /* index of PDU_ADV_TYPE_ADV_IND in pdu_adv_type[] */
			};

[Thalley]

I see. That explains it. Would possibly also suggest to change adv_type to pdu_adv_type_index to make it very explicit that the variable is a index to the pdu_adv_type array

[cvinayak]

adv_type is the Advertising Type parameter name in HCI LE Set Advertising Parameters command that is used in the lookup pdu_adv_type[], will keep the name as-is.

Updated the PR will additional comments in code.

[Thalley]

Minor request to make the array name explicit and a suggestion (can be postponed) to move the entire conversion to a separate function

[Thalley]

Suggested change

	0x03, /* index of PDU_ADV_TYPE_NONCONN_IND */
	0x04, /* index of PDU_ADV_TYPE_DIRECT_IND */
	0x02, /* index of PDU_ADV_TYPE_SCAN_IND */
	0x00 /* index of PDU_ADV_TYPE_ADV_IND */
	0x03, /* index of PDU_ADV_TYPE_NONCONN_IND in pdu_adv_type[] */
	0x04, /* index of PDU_ADV_TYPE_DIRECT_IND in pdu_adv_type[] */
	0x02, /* index of PDU_ADV_TYPE_SCAN_IND in pdu_adv_type[] */
	0x00 /* index of PDU_ADV_TYPE_ADV_IND in pdu_adv_type[] */

[Thalley]

This entire lookup/conversion from evt_prop to adv type could also be moved into a separate function which could possibly also improve readability and understandability.

I guess since leg_adv_type is only used for adv_type = leg_adv_type[evt_prop & 0x03]; the entire array could be moved into the else { clause.

[Thalley]

It is still unclear why we are using the index of the pdu_adv_type array instead of the adv_type from the array itself, but I guess this is as good as it gets with the current way :)

[cvinayak]

It is still unclear why we are using the index of the pdu_adv_type array instead of the adv_type from the array itself, but I guess this is as good as it gets with the current way :)

adv_type is the HCI parameter value and is not the advertising type value in PDUs hence the pdu_adv_type array lookup. This lookup is used in multiple places to check if advertising type of a set is being changed. Without the use of adv_type index, multiple places need to have conditional compile and runtime checks comparing extended vs legacy advertising.