bluetooth: controller: removing 'no-brainer' TODOs from refactored LLCP

Getting rid of TODOs that are fairly straight forward to resolve
* introduce role checks in ENC API
* Remove ASSERT on re-rx of LL_VERSION_IND, ignore instead
* in tests/.../ctrl_chmu: rename variable and initialize initial ch map
* in tests/.../mock_ctrl/util.c: Changing TODO into FYI
* in tests/.../helper_features.h update mask and remove TODO comment
* in ull_llcp_remote.c: re-order cases in proc_with_instant switch
* in ull_conn_upd.c: PARAM REQ only uses REJECT_EXT_IND
* in ull_llcp_common.c: in CENTRAL on rx of LL_MIN_USED_CHANNELS_IND
     chose to do nothing re. channel map. Update unit test accordingly

Signed-off-by: Erik Brockhoff <erbr@oticon.com>

subsys/bluetooth/controller/ll_sw/ull_llcp.c

@@ -622,7 +622,9 @@ uint8_t ull_cp_encryption_start(struct ll_conn *conn, const uint8_t rand[8], con
 {
 	struct proc_ctx *ctx;
 
-	/* TODO(thoh): Proper checks for role, parameters etc. */
+	if (conn->lll.role != BT_HCI_ROLE_CENTRAL) {
+		return BT_HCI_ERR_CMD_DISALLOWED;
+	}
 
 	ctx = llcp_create_local_procedure(PROC_ENCRYPTION_START);
 	if (!ctx) {
@@ -646,7 +648,9 @@ uint8_t ull_cp_encryption_pause(struct ll_conn *conn, const uint8_t rand[8], con
 {
 	struct proc_ctx *ctx;
 
-	/* TODO(thoh): Proper checks for role, parameters etc. */
+	if (conn->lll.role != BT_HCI_ROLE_CENTRAL) {
+		return BT_HCI_ERR_CMD_DISALLOWED;
+	}
 
 	ctx = llcp_create_local_procedure(PROC_ENCRYPTION_PAUSE);
 	if (!ctx) {

subsys/bluetooth/controller/ll_sw/ull_llcp_common.c

@@ -933,6 +933,9 @@ static void rp_comm_send_rsp(struct ll_conn *conn, struct proc_ctx *ctx, uint8_t
 	case PROC_VERSION_EXCHANGE:
 		/* The Link Layer shall only queue for transmission a maximum of one
 		 * LL_VERSION_IND PDU during a connection.
+		 * If the Link Layer receives an LL_VERSION_IND PDU and has already sent an
+		 * LL_VERSION_IND PDU then the Link Layer shall not send another
+		 * LL_VERSION_IND PDU to the peer device.
 		 */
 		if (!conn->llcp.vex.sent) {
 			if (ctx->pause || !llcp_tx_alloc_peek(conn, ctx)) {
@@ -944,13 +947,12 @@ static void rp_comm_send_rsp(struct ll_conn *conn, struct proc_ctx *ctx, uint8_t
 				ctx->state = RP_COMMON_STATE_IDLE;
 			}
 		} else {
-			/* Protocol Error.
+			/* Invalid behaviour
-			 *
 			 * A procedure already sent a LL_VERSION_IND and received a LL_VERSION_IND.
+			 * For now we chose to ignore the 'out of order' PDU
 			 */
-			/* TODO */
-			LL_ASSERT(0);
 		}
+
 		break;
 #if defined(CONFIG_BT_CTLR_MIN_USED_CHAN) && defined(CONFIG_BT_CENTRAL)
 	case PROC_MIN_USED_CHANS:
@@ -959,17 +961,18 @@ static void rp_comm_send_rsp(struct ll_conn *conn, struct proc_ctx *ctx, uint8_t
 		 *     The procedure has completed when the Link Layer acknowledgment of the
 		 *     LL_MIN_USED_CHANNELS_IND PDU is sent or received.
 		 * In effect, for this procedure, this is equivalent to RX of PDU
+		 *
+		 * Also:
+		 *     If the Link Layer receives an LL_MIN_USED_CHANNELS_IND PDU, it should ensure
+		 *     that, whenever the Peripheral-to-Central PHY is one of those specified,
+		 *     the connection uses at least the number of channels given in the
+		 *     MinUsedChannels field of the PDU.
+		 *
+		 * The 'should' is here interpreted as 'permission' to do nothing
+		 *
+		 * Future improvement could implement logic to support this
 		 */
-		/* Inititate a chmap update, but only if acting as central, just in case ... */
-		if (conn->lll.role == BT_HCI_ROLE_CENTRAL &&
-		    ull_conn_lll_phy_active(conn, conn->llcp.muc.phys)) {
-			uint8_t chmap[5];
 
-			ull_chan_map_get((uint8_t *const)chmap);
-			ull_cp_chan_map_update(conn, chmap);
-			/* TODO - what to do on failure of ull_cp_chan_map_update() */
-		}
-		/* No response */
 		llcp_rr_complete(conn);
 		ctx->state = RP_COMMON_STATE_IDLE;
 		break;

subsys/bluetooth/controller/ll_sw/ull_llcp_conn_upd.c

@@ -336,7 +336,6 @@ static void lp_cu_st_wait_rx_conn_param_rsp(struct ll_conn *conn, struct proc_ct
 		lp_cu_send_conn_update_ind(conn, ctx, evt, param);
 		break;
 	case LP_CU_EVT_REJECT:
-		/* TODO(tosk): Select between LL_REJECT_IND and LL_REJECT_EXT_IND */
 		if (pdu->llctrl.reject_ext_ind.error_code == BT_HCI_ERR_UNSUPP_REMOTE_FEATURE) {
 			/* Remote legacy Host */
 			llcp_rr_set_incompat(conn, INCOMPAT_RESERVED);
@@ -547,7 +546,6 @@ static void rp_cu_tx(struct ll_conn *conn, struct proc_ctx *ctx, uint8_t opcode)
 		break;
 #if defined(CONFIG_BT_CTLR_CONN_PARAM_REQ)
 	case PDU_DATA_LLCTRL_TYPE_REJECT_EXT_IND:
-		/* TODO(thoh): Select between LL_REJECT_IND and LL_REJECT_EXT_IND */
 		llcp_pdu_encode_reject_ext_ind(pdu, PDU_DATA_LLCTRL_TYPE_CONN_PARAM_REQ,
 					       ctx->data.cu.error);
 		break;

subsys/bluetooth/controller/ll_sw/ull_llcp_remote.c

@@ -73,37 +73,23 @@ enum {
 
 static bool proc_with_instant(struct proc_ctx *ctx)
 {
-	/*
-	 * TODO: should we combine all the cases that return 0
-	 * and all the cases that return 1?
-	 */
 	switch (ctx->proc) {
 	case PROC_UNKNOWN:
-		return 0U;
 	case PROC_FEATURE_EXCHANGE:
-		return 0U;
 	case PROC_MIN_USED_CHANS:
-		return 0U;
 	case PROC_LE_PING:
-		return 0U;
 	case PROC_VERSION_EXCHANGE:
-		return 0U;
 	case PROC_ENCRYPTION_START:
 	case PROC_ENCRYPTION_PAUSE:
-		return 0U;
-	case PROC_PHY_UPDATE:
-		return 1U;
-	case PROC_CONN_UPDATE:
-	case PROC_CONN_PARAM_REQ:
-		return 1U;
 	case PROC_TERMINATE:
-		return 0U;
-	case PROC_CHAN_MAP_UPDATE:
-		return 1U;
 	case PROC_DATA_LENGTH_UPDATE:
-		return 0U;
 	case PROC_CTE_REQ:
 		return 0U;
+	case PROC_PHY_UPDATE:
+	case PROC_CONN_UPDATE:
+	case PROC_CONN_PARAM_REQ:
+	case PROC_CHAN_MAP_UPDATE:
+		return 1U;
 	default:
 		/* Unknown procedure */
 		LL_ASSERT(0);

tests/bluetooth/controller/common/include/helper_features.h

@@ -237,13 +237,8 @@
 
 /*
  * The following two are defined as per
- * Core Spec V5.2 Volume 6, Part B, chapter 4.6
+ * Core Spec V5.3 Volume 6, Part B, chapter 4.6
- * LL_FEAT_BIT_MASK_VALID does not account for the bits
- * for the new features in V5.2
- * TODO: EXPECTED_FEAT_EXCH_VALID is not used at the moment
- *       but probably LL_FEAT_BIT_MASK_VALID should get this
- *       value in ll_feat.h
  */
-#define EXPECTED_FEAT_EXCH_VALID 0x0000000FF787CF2F
+#define EXPECTED_FEAT_EXCH_VALID 0x000000EFF787CF2F
 #define FEAT_FILTER_OCTET0 0xFFFFFFFFFFFFFF00
 #define COMMON_FEAT_OCTET0(x) (FEAT_FILTER_OCTET0 | ((x) & ~FEAT_FILTER_OCTET0))

tests/bluetooth/controller/ctrl_chmu/src/main.c

@@ -53,8 +53,7 @@ static bool is_instant_reached(struct ll_conn *conn, uint16_t instant)
 void test_channel_map_update_central_loc(void)
 {
 	uint8_t chm[5] = { 0x00, 0x04, 0x05, 0x06, 0x00 };
-	/* TODO should test setup set this to valid value? */
+	uint8_t initial_chm[5];
-	uint8_t defchm[5] = {};
 	uint8_t err;
 	struct node_tx *tx;
 	struct pdu_data *pdu;
@@ -64,6 +63,9 @@ void test_channel_map_update_central_loc(void)
 		.chm = { 0x00, 0x04, 0x05, 0x06, 0x00 },
 	};
 
+	/* Store initial channel map */
+	memcpy(initial_chm, conn.lll.data_chan_map, sizeof(conn.lll.data_chan_map));
+
 	/* Role */
 	test_set_role(&conn, BT_HCI_ROLE_CENTRAL);
 
@@ -104,8 +106,9 @@ void test_channel_map_update_central_loc(void)
 		/* There should NOT be a host notification */
 		ut_rx_q_is_empty();
 
-		/* check if using old channel map */
+		/* check if still using initial channel map */
-		zassert_mem_equal(conn.lll.data_chan_map, defchm, sizeof(conn.lll.data_chan_map),
+		zassert_mem_equal(conn.lll.data_chan_map, initial_chm,
+				  sizeof(conn.lll.data_chan_map),
 				  "Channel map invalid");
 	}
 
@@ -132,14 +135,16 @@ void test_channel_map_update_central_loc(void)
 void test_channel_map_update_periph_rem(void)
 {
 	uint8_t chm[5] = { 0x00, 0x04, 0x05, 0x06, 0x00 };
-	/* TODO should test setup set this to valid value? */
+	uint8_t initial_chm[5];
-	uint8_t defchm[5] = {};
 	struct pdu_data_llctrl_chan_map_ind chmu_ind = {
 		.instant = 6,
 		.chm = { 0x00, 0x04, 0x05, 0x06, 0x00 },
 	};
 	uint16_t instant = 6;
 
+	/* Store initial channel map */
+	memcpy(initial_chm, conn.lll.data_chan_map, sizeof(conn.lll.data_chan_map));
+
 	/* Role */
 	test_set_role(&conn, BT_HCI_ROLE_PERIPHERAL);
 
@@ -173,7 +178,8 @@ void test_channel_map_update_periph_rem(void)
 		ut_rx_q_is_empty();
 
 		/* check if using old channel map */
-		zassert_mem_equal(conn.lll.data_chan_map, defchm, sizeof(conn.lll.data_chan_map),
+		zassert_mem_equal(conn.lll.data_chan_map, initial_chm,
+				  sizeof(conn.lll.data_chan_map),
 				  "Channel map invalid");
 	}
 

tests/bluetooth/controller/ctrl_min_used_chans/src/main.c

@@ -126,10 +126,6 @@ void test_min_used_chans_central_rem(void)
 {
 	struct pdu_data_llctrl_min_used_chans_ind remote_muc_ind = { .phys = 1,
 		.min_used_chans = 2 };
-	struct pdu_data_llctrl_chan_map_ind ch_map_ind = { .chm = { 0xff, 0xff, 0xff, 0xff, 0x1f },
-		.instant = 7 };
-
-	struct node_tx *tx;
 
 	/* Role */
 	test_set_role(&conn, BT_HCI_ROLE_CENTRAL);
@@ -143,17 +139,13 @@ void test_min_used_chans_central_rem(void)
 	/* Rx */
 	lt_tx(LL_MIN_USED_CHANS_IND, &conn,  &remote_muc_ind);
 
-	/* Emulate a phy to trigger channel map update */
-	conn.lll.phy_tx = 0x7;
-
 	/* Done */
 	event_done(&conn);
 
 	/* Prepare */
 	event_prepare(&conn);
 
-	/* Tx Queue should have one LL Control PDU */
+	/* Tx Queue should have no LL Control PDU */
-	lt_rx(LL_CHAN_MAP_UPDATE_IND, &conn,  &tx, &ch_map_ind);
 	lt_rx_q_is_empty(&conn);
 
 	/* Done */
@@ -162,7 +154,7 @@ void test_min_used_chans_central_rem(void)
 	/* There should not be a host notifications */
 	ut_rx_q_is_empty();
 
-	zassert_equal(ctx_buffers_free(), test_ctx_buffers_cnt() - 1,
+	zassert_equal(ctx_buffers_free(), test_ctx_buffers_cnt(),
 		      "Free CTX buffers %d", ctx_buffers_free());
 }
 

tests/bluetooth/controller/mock_ctrl/src/util.c

@@ -17,9 +17,6 @@
 
 /**
  * @brief Population count: Count the number of bits set to 1
- * @details
- * TODO: Faster methods available at [1].
- * [1] http://graphics.stanford.edu/~seander/bithacks.html#CountBitsSetParallel
  *
  * @param octets     Data to count over
  * @param octets_len Must not be bigger than 255/8 = 31 bytes