canbus: isotp: allow frames with padding

ISO 15765-2 specifies that the CAN frame length for single frames and
the last consecutive frame may or may not be optimized to the actual
length of the PDU payload. The ISO-TP implementation should only
consider the information from the N_PCI section.

The previous implementation did not allow padding. With this commit,
padding is allowed by default and can even be required to be more
compliant to AUTOSAR standards.

Signed-off-by: Martin Jäger <[email protected]>

Author: martinjaeger

subsys/canbus/isotp/Kconfig

@@ -47,6 +47,22 @@ config ISOTP_CR_TIMEOUT
 	  Cr (receiver consecutive frame) timeout.
 	  ISO 15765-2: 1000ms
 
+config ISOTP_REQUIRE_RX_PADDING
+	bool "Require padding for received messages"
+	help
+	  If enabled, SFs and the last CF must always have a DLC of 8 bytes
+	  (for classic CAN) and unused bytes must be padded by the sending
+	  device. This setting allows to be compliant to AUTOSAR Specification
+	  of CAN Transport Layer.
+
+	  By default, received CAN frames with or without padding are accepted.
+
+config ISOTP_ENABLE_TX_PADDING
+	bool "Enable padding for transmitted messages"
+	help
+	  Add padding bytes 0xCC (as recommended by Bosch) if the PDU payload
+	  does not fit exactly into the CAN frame.
+
 config ISOTP_WORKQUEUE_PRIO
 	int "Priority level of the RX and TX work queue"
 	default 2

subsys/canbus/isotp/isotp.c

@@ -136,6 +136,7 @@ static void receive_send_fc(struct isotp_recv_ctx *ctx, uint8_t fs)
 		.id = ctx->tx_addr.ext_id
 	};
 	uint8_t *data = frame.data;
+	uint8_t payload_len;
 	int ret;
 
 	__ASSERT_NO_MSG(!(fs & ISOTP_PCI_TYPE_MASK));
@@ -147,7 +148,16 @@ static void receive_send_fc(struct isotp_recv_ctx *ctx, uint8_t fs)
 	*data++ = ISOTP_PCI_TYPE_FC | fs;
 	*data++ = ctx->opts.bs;
 	*data++ = ctx->opts.stmin;
-	frame.dlc = data - frame.data;
+	payload_len = data - frame.data;
+
+#if defined(CONFIG_ISOTP_REQUIRE_RX_PADDING) || \
+				defined(CONFIG_ISOTP_ENABLE_TX_PADDING)
+	/* AUTOSAR requirement SWS_CanTp_00347 */
+	memset(&frame.data[payload_len], 0xCC, ISOTP_CAN_DL - payload_len);
+	frame.dlc = ISOTP_CAN_DL;
+#else
+	frame.dlc = payload_len;
+#endif
 
 	ret = can_send(ctx->can_dev, &frame, K_MSEC(ISOTP_A),
 		       receive_can_tx_isr, ctx);
@@ -264,10 +274,10 @@ static void receive_state_machine(struct isotp_recv_ctx *ctx)
 
 	switch (ctx->state) {
 	case ISOTP_RX_STATE_PROCESS_SF:
-		ctx->buf->len = receive_get_sf_length(ctx->buf);
+		ctx->length = receive_get_sf_length(ctx->buf);
 		ud_rem_len = net_buf_user_data(ctx->buf);
 		*ud_rem_len = 0;
-		LOG_DBG("SM process SF of length %d", ctx->buf->len);
+		LOG_DBG("SM process SF of length %d", ctx->length);
 		net_buf_put(&ctx->fifo, ctx->buf);
 		ctx->state = ISOTP_RX_STATE_RECYCLE;
 		receive_state_machine(ctx);
@@ -380,6 +390,7 @@ static void receive_work_handler(struct k_work *item)
 static void process_ff_sf(struct isotp_recv_ctx *ctx, struct zcan_frame *frame)
 {
 	int index = 0;
+	uint8_t payload_len;
 
 	if (ctx->rx_addr.use_ext_addr) {
 		if (frame->data[index++] != ctx->rx_addr.ext_addr) {
@@ -391,19 +402,30 @@ static void process_ff_sf(struct isotp_recv_ctx *ctx, struct zcan_frame *frame)
 	case ISOTP_PCI_TYPE_FF:
 		LOG_DBG("Got FF IRQ");
 		if (frame->dlc != ISOTP_CAN_DL) {
-			LOG_INF("FF DL does not match. Ignore");
+			LOG_INF("FF DLC invalid. Ignore");
 			return;
 		}
 
+		payload_len = ISOTP_CAN_DL;
 		ctx->state = ISOTP_RX_STATE_PROCESS_FF;
 		ctx->sn_expected = 1;
 		break;
 
 	case ISOTP_PCI_TYPE_SF:
 		LOG_DBG("Got SF IRQ");
-		if ((frame->data[index] & ISOTP_PCI_FF_DL_UPPER_MASK) +
-		    index + 1 != frame->dlc) {
-			LOG_INF("SF DL does not match. Ignore");
+#ifdef CONFIG_ISOTP_REQUIRE_RX_PADDING
+		/* AUTOSAR requirement SWS_CanTp_00345 */
+		if (frame->dlc != ISOTP_CAN_DL) {
+			LOG_INF("SF DLC invalid. Ignore");
+			return;
+		}
+#endif
+
+		payload_len = index + 1 + (frame->data[index] &
+						ISOTP_PCI_SF_DL_MASK);
+
+		if (payload_len > frame->dlc) {
+			LOG_INF("SF DL does not fit. Ignore");
 			return;
 		}
 
@@ -415,7 +437,7 @@ static void process_ff_sf(struct isotp_recv_ctx *ctx, struct zcan_frame *frame)
 		return;
 	}
 
-	net_buf_add_mem(ctx->buf, &frame->data[index], frame->dlc - index);
+	net_buf_add_mem(ctx->buf, &frame->data[index], payload_len - index);
 }
 
 static inline void receive_add_mem(struct isotp_recv_ctx *ctx, uint8_t *data,
@@ -432,7 +454,7 @@ static inline void receive_add_mem(struct isotp_recv_ctx *ctx, uint8_t *data,
 	net_buf_add_mem(ctx->act_frag, data, tailroom);
 	ctx->act_frag = ctx->act_frag->frags;
 	if (!ctx->act_frag) {
-		LOG_ERR("No fragmet left to append data");
+		LOG_ERR("No fragment left to append data");
 		receive_report_error(ctx, ISOTP_N_BUFFER_OVERFLW);
 		return;
 	}
@@ -444,6 +466,7 @@ static void process_cf(struct isotp_recv_ctx *ctx, struct zcan_frame *frame)
 {
 	uint32_t *ud_rem_len = (uint32_t *)net_buf_user_data(ctx->buf);
 	int index = 0;
+	uint32_t data_len;
 
 	if (ctx->rx_addr.use_ext_addr) {
 		if (frame->data[index++] != ctx->rx_addr.ext_addr) {
@@ -470,14 +493,20 @@ static void process_cf(struct isotp_recv_ctx *ctx, struct zcan_frame *frame)
 		return;
 	}
 
-	if (frame->dlc - index > ctx->length) {
-		LOG_ERR("The frame contains more bytes than expected");
+#ifdef CONFIG_ISOTP_REQUIRE_RX_PADDING
+	/* AUTOSAR requirement SWS_CanTp_00346 */
+	if (frame->dlc != ISOTP_CAN_DL) {
+		LOG_ERR("CF DL invalid");
 		receive_report_error(ctx, ISOTP_N_ERROR);
+		return;
 	}
+#endif
 
 	LOG_DBG("Got CF irq. Appending data");
-	receive_add_mem(ctx, &frame->data[index], frame->dlc - index);
-	ctx->length -= frame->dlc - index;
+	data_len = (ctx->length > frame->dlc - index) ? frame->dlc - index :
+		ctx->length;
+	receive_add_mem(ctx, &frame->data[index], data_len);
+	ctx->length -= data_len;
 	LOG_DBG("%d bytes remaining", ctx->length);
 
 	if (ctx->length == 0) {
@@ -755,6 +784,15 @@ static void send_process_fc(struct isotp_send_ctx *ctx,
 		return;
 	}
 
+#ifdef CONFIG_ISOTP_ENABLE_TX_PADDING
+	/* AUTOSAR requirement SWS_CanTp_00349 */
+	if (frame->dlc != ISOTP_CAN_DL) {
+		LOG_ERR("FC DL invalid. Ignore");
+		send_report_error(ctx, ISOTP_N_ERROR);
+		return;
+	}
+#endif
+
 	switch (*data++ & ISOTP_PCI_FS_MASK) {
 	case ISOTP_PCI_FS_CTS:
 		ctx->state = ISOTP_TX_SEND_CF;
@@ -853,7 +891,13 @@ static inline int send_sf(struct isotp_send_ctx *ctx)
 	__ASSERT_NO_MSG(len <= ISOTP_CAN_DL - index);
 	memcpy(&frame.data[index], data, len);
 
+#ifdef CONFIG_ISOTP_ENABLE_TX_PADDING
+	/* AUTOSAR requirement SWS_CanTp_00348 */
+	memset(&frame.data[index + len], 0xCC, ISOTP_CAN_DL - len - index);
+	frame.dlc = ISOTP_CAN_DL;
+#else
 	frame.dlc = len + index;
+#endif
 
 	ctx->state = ISOTP_TX_SEND_SF;
 	ret = can_send(ctx->can_dev, &frame, K_MSEC(ISOTP_A),
@@ -926,10 +970,17 @@ static inline int send_cf(struct isotp_send_ctx *ctx)
 	rem_len = get_ctx_data_length(ctx);
 	len = MIN(rem_len, ISOTP_CAN_DL - index);
 	rem_len -= len;
-	frame.dlc = len + index;
 	data = get_data_ctx(ctx);
 	memcpy(&frame.data[index], data, len);
 
+#ifdef CONFIG_ISOTP_ENABLE_TX_PADDING
+	/* AUTOSAR requirement SWS_CanTp_00348 */
+	memset(&frame.data[index + len], 0xCC, ISOTP_CAN_DL - len - index);
+	frame.dlc = ISOTP_CAN_DL;
+#else
+	frame.dlc = len + index;
+#endif
+
 	ret = can_send(ctx->can_dev, &frame, K_MSEC(ISOTP_A),
 		       send_can_tx_isr, ctx);
 	if (ret == CAN_TX_OK) {

tests/subsys/canbus/isotp/conformance/src/main.c

@@ -31,13 +31,19 @@
 #define FC_PCI_BYTE_1(FS)  ((FC_PCI_TYPE << PCI_TYPE_POS) | (FS))
 #define FC_PCI_BYTE_2(BS)  (BS)
 #define FC_PCI_BYTE_3(ST_MIN) (ST_MIN)
-#define DATA_SIZE_FC       3
 #define CF_PCI_TYPE        2
 #define CF_PCI_BYTE_1      (CF_PCI_TYPE << PCI_TYPE_POS)
 #define STMIN_VAL_1        5
 #define STMIN_VAL_2        50
 #define STMIN_UPPER_TOLERANCE 5
 
+#if defined(CONFIG_ISOTP_ENABLE_TX_PADDING) || \
+				defined(CONFIG_ISOTP_ENABLE_TX_PADDING)
+#define DATA_SIZE_FC       CAN_DL
+#else
+#define DATA_SIZE_FC       3
+#endif
+
 #define CEIL(A, B) (((A) + (B) - 1) / (B))
 
 #define BS_TIMEOUT_UPPER_MS   1100
@@ -167,6 +173,8 @@ static void get_sf(struct isotp_recv_ctx *recv_ctx, size_t data_size)
 	zassert_equal(ret, 0, "Data differ");
 }
 
+#ifdef CONFIG_ISOTP_REQUIRE_RX_PADDING
+
 static void get_sf_ignore(struct isotp_recv_ctx *recv_ctx)
 {
 	int ret;
@@ -175,6 +183,8 @@ static void get_sf_ignore(struct isotp_recv_ctx *recv_ctx)
 	zassert_equal(ret, ISOTP_RECV_TIMEOUT, "recv returned %d", ret);
 }
 
+#endif
+
 static void send_test_data(const uint8_t *data, size_t len)
 {
 	int ret;
@@ -246,11 +256,18 @@ static void check_frame_series(struct frame_desired *frames, size_t length,
 		zassert_equal(ret, 0, "Timeout waiting for msg nr %d. ret: %d",
 			      i, ret);
 
+#if !defined(CONFIG_ISOTP_REQUIRE_RX_PADDING) && \
+					!defined(CONFIG_ISOTP_ENABLE_TX_PADDING)
 		zassert_equal(frame.dlc, desired->length,
 			      "DLC of frame nr %d differ. Desired: %d, Got: %d",
 			      i, desired->length, frame.dlc);
+#endif
+
+#if !defined(CONFIG_ISOTP_ENABLE_TX_PADDING)
 		ret = check_data(frame.data, desired->data, desired->length);
 		zassert_equal(ret, 0, "Data differ");
+#endif
+
 		desired++;
 	}
 	ret = k_msgq_get(msgq, &frame, K_MSEC(200));
@@ -289,7 +306,9 @@ static void prepare_cf_frames(struct frame_desired *frames, size_t frames_cnt,
 		memcpy(&des_frames[i].data[1], data_ptr, DATA_SIZE_CF);
 
 		if (remaining_length < DATA_SIZE_CF) {
+#ifndef CONFIG_ISOTP_ENABLE_TX_PADDING
 			frames[i].length = remaining_length + 1;
+#endif
 			remaining_length = 0;
 		}
 
@@ -338,11 +357,13 @@ static void test_receive_sf(void)
 	single_frame.data[0] = SF_PCI_BYTE_LEN_8;
 	send_frame_series(&single_frame, 1, rx_addr.std_id);
 
+#ifdef CONFIG_ISOTP_REQUIRE_RX_PADDING
 	single_frame.data[0] = SF_PCI_BYTE_1;
 	single_frame.length = 7;
 	send_frame_series(&single_frame, 1, rx_addr.std_id);
 
 	get_sf_ignore(&recv_ctx);
+#endif
 
 	isotp_unbind(&recv_ctx);
 }
@@ -392,11 +413,13 @@ static void test_receive_sf_ext(void)
 	single_frame.data[1] = SF_PCI_BYTE_1;
 	send_frame_series(&single_frame, 1, rx_addr.std_id);
 
+#ifdef CONFIG_ISOTP_REQUIRE_RX_PADDING
 	single_frame.data[1] = SF_PCI_BYTE_2_EXT;
 	single_frame.length = 7;
 	send_frame_series(&single_frame, 1, rx_addr.std_id);
 
 	get_sf_ignore(&recv_ctx);
+#endif
 
 	isotp_unbind(&recv_ctx);
 }
@@ -418,7 +441,7 @@ static void test_send_data(void)
 	fc_frame.data[0] = FC_PCI_BYTE_1(FC_PCI_CTS);
 	fc_frame.data[1] = FC_PCI_BYTE_2(0);
 	fc_frame.data[2] = FC_PCI_BYTE_3(0);
-	fc_frame.length = 3;
+	fc_frame.length = DATA_SIZE_FC;
 
 	prepare_cf_frames(des_frames, ARRAY_SIZE(des_frames), data_ptr,
 			  remaining_length);
@@ -457,7 +480,7 @@ static void test_send_data_blocks(void)
 	fc_frame.data[0] = FC_PCI_BYTE_1(FC_PCI_CTS);
 	fc_frame.data[1] = FC_PCI_BYTE_2(fc_opts.bs);
 	fc_frame.data[2] = FC_PCI_BYTE_3(0);
-	fc_frame.length = 3;
+	fc_frame.length = DATA_SIZE_FC;
 
 	prepare_cf_frames(des_frames, ARRAY_SIZE(des_frames), data_ptr,
 			  remaining_length);
@@ -520,7 +543,7 @@ static void test_receive_data(void)
 	fc_frame.data[0] = FC_PCI_BYTE_1(FC_PCI_CTS);
 	fc_frame.data[1] = FC_PCI_BYTE_2(fc_opts_single.bs);
 	fc_frame.data[2] = FC_PCI_BYTE_3(fc_opts_single.stmin);
-	fc_frame.length = 3;
+	fc_frame.length = DATA_SIZE_FC;
 
 	prepare_cf_frames(des_frames, ARRAY_SIZE(des_frames), data_ptr,
 			  remaining_length);
@@ -564,7 +587,7 @@ static void test_receive_data_blocks(void)
 	fc_frame.data[0] = FC_PCI_BYTE_1(FC_PCI_CTS);
 	fc_frame.data[1] = FC_PCI_BYTE_2(fc_opts.bs);
 	fc_frame.data[2] = FC_PCI_BYTE_3(fc_opts.stmin);
-	fc_frame.length = 3;
+	fc_frame.length = DATA_SIZE_FC;
 
 	prepare_cf_frames(des_frames, ARRAY_SIZE(des_frames), data_ptr,
 			  remaining_length);
@@ -615,7 +638,7 @@ static void test_send_timeouts(void)
 	fc_cts_frame.data[0] = FC_PCI_BYTE_1(FC_PCI_CTS);
 	fc_cts_frame.data[1] = FC_PCI_BYTE_2(fc_opts.bs);
 	fc_cts_frame.data[2] = FC_PCI_BYTE_3(0);
-	fc_cts_frame.length = 3;
+	fc_cts_frame.length = DATA_SIZE_FC;
 
 	/* Test timeout for first FC*/
 	start_time = k_uptime_get_32();
@@ -716,7 +739,7 @@ static void test_stmin(void)
 	fc_frame.data[0] = FC_PCI_BYTE_1(FC_PCI_CTS);
 	fc_frame.data[1] = FC_PCI_BYTE_2(2);
 	fc_frame.data[2] = FC_PCI_BYTE_3(STMIN_VAL_1);
-	fc_frame.length = 3;
+	fc_frame.length = DATA_SIZE_FC;
 
 	filter_id = attach_msgq(rx_addr.std_id);
 	zassert_true((filter_id >= 0), "Negative filter number [%d]",
@@ -771,7 +794,7 @@ void test_receiver_fc_errors(void)
 	fc_frame.data[0] = FC_PCI_BYTE_1(FC_PCI_CTS);
 	fc_frame.data[1] = FC_PCI_BYTE_2(fc_opts.bs);
 	fc_frame.data[2] = FC_PCI_BYTE_3(fc_opts.stmin);
-	fc_frame.length = 3;
+	fc_frame.length = DATA_SIZE_FC;
 
 	filter_id = attach_msgq(tx_addr.std_id);
 	zassert_true((filter_id >= 0), "Negative filter number [%d]",
@@ -819,7 +842,7 @@ void test_sender_fc_errors(void)
 	fc_frame.data[0] = FC_PCI_BYTE_1(3);
 	fc_frame.data[1] = FC_PCI_BYTE_2(fc_opts.bs);
 	fc_frame.data[2] = FC_PCI_BYTE_3(fc_opts.stmin);
-	fc_frame.length = 3;
+	fc_frame.length = DATA_SIZE_FC;
 
 	k_sem_reset(&send_compl_sem);
 	ret = isotp_send(&send_ctx, can_dev, random_data, DATA_SEND_LENGTH,
@@ -841,7 +864,7 @@ void test_sender_fc_errors(void)
 	ret = isotp_send(&send_ctx, can_dev, random_data, 5*1024,
 			 &tx_addr, &rx_addr, NULL, NULL);
 	zassert_equal(ret, ISOTP_N_BUFFER_OVERFLW,
-		      "Expected overflow bot got %d", ret);
+		      "Expected overflow but got %d", ret);
 	isotp_unbind(&recv_ctx);
 	filter_id = attach_msgq(tx_addr.std_id);