Skip to content

CAN ISO-TP updates to allow padding and fixed addressing #31078

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Apr 6, 2021
Merged

CAN ISO-TP updates to allow padding and fixed addressing #31078

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c0f4d14
canbus: isotp: fix asserts to check rx and tx addr
martinjaeger Jan 2, 2021
4ff2a9d
canbus: isotp: allow frames with padding
martinjaeger Jan 2, 2021
ca9d271
canbus: isotp: add fixed addressing feature
martinjaeger Dec 30, 2020
6f89259
canbus: isotp: prevent race-condition during debugging
martinjaeger Jan 3, 2021
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
51 changes: 46 additions & 5 deletions include/canbus/isotp.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,9 @@
* FC Flow Control
* FF First Frame
* FS Flow Status
* AE Adders Extension
* AE Address Extension
* SA Source Address
* TA Target Address
*/

/*
Expand Down Expand Up @@ -90,6 +92,38 @@
/** Timeout for recv */
#define ISOTP_RECV_TIMEOUT -14

/*
* CAN ID filtering for ISO-TP fixed addressing according to SAE J1939
*
* Format of 29-bit CAN identifier:
* ------------------------------------------------------
* | 28 .. 26 | 25 | 24 | 23 .. 16 | 15 .. 8 | 7 .. 0 |
* ------------------------------------------------------
* | Priority | EDP | DP | N_TAtype | N_TA | N_SA |
* ------------------------------------------------------
*/

/** Position of fixed source address (SA) */
#define ISOTP_FIXED_ADDR_SA_POS (0U)

/** Mask to obtain fixed source address (SA) */
#define ISOTP_FIXED_ADDR_SA_MASK (0xFF << ISOTP_FIXED_ADDR_SA_POS)

/** Position of fixed target address (TA) */
#define ISOTP_FIXED_ADDR_TA_POS (8U)

/** Mask to obtain fixed target address (TA) */
#define ISOTP_FIXED_ADDR_TA_MASK (0xFF << ISOTP_FIXED_ADDR_TA_POS)

/** Position of priority in fixed addressing mode */
#define ISOTP_FIXED_ADDR_PRIO_POS (26U)

/** Mask for priority in fixed addressing mode */
#define ISOTP_FIXED_ADDR_PRIO_MASK (0x7 << ISOTP_FIXED_ADDR_PRIO_POS)

/* CAN filter RX mask to match any priority and source address (SA) */
#define ISOTP_FIXED_ADDR_RX_MASK (0x03FFFF00)

#ifdef __cplusplus
extern "C" {
#endif
Expand All @@ -100,17 +134,24 @@ extern "C" {
* Used to pass addresses to the bind and send functions.
*/
struct isotp_msg_id {
/** Message identifier*/
/**
* CAN identifier
*
* If ISO-TP fixed addressing is used, isotp_bind ignores SA and
* priority sections and modifies TA section in flow control frames.
*/
union {
uint32_t std_id : 11;
uint32_t ext_id : 29;
};
/** extended address */
/** ISO-TP extended address (if used) */
uint8_t ext_addr;
/** Indicates the identifier type (standard or extended) */
/** Indicates the CAN identifier type (standard or extended) */
uint8_t id_type : 1;
/** Indicates if extended addressing is used */
/** Indicates if ISO-TP extended addressing is used */
uint8_t use_ext_addr : 1;
/** Indicates if ISO-TP fixed addressing (acc. to SAE J1939) is used */
uint8_t use_fixed_addr : 1;
};

/*
Expand Down
16 changes: 16 additions & 0 deletions subsys/canbus/isotp/Kconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
115 changes: 94 additions & 21 deletions subsys/canbus/isotp/isotp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Expand All @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -380,30 +390,56 @@ 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;
uint32_t rx_sa; /* ISO-TP fixed source address (if used) */

if (ctx->rx_addr.use_ext_addr) {
if (frame->data[index++] != ctx->rx_addr.ext_addr) {
return;
}
}

if (ctx->rx_addr.use_fixed_addr) {
/* store actual CAN ID used by the sender */
ctx->rx_addr.ext_id = frame->id;
/* replace TX target address with RX source address */
rx_sa = (frame->id & ISOTP_FIXED_ADDR_SA_MASK) >>
ISOTP_FIXED_ADDR_SA_POS;
ctx->tx_addr.ext_id &= ~(ISOTP_FIXED_ADDR_TA_MASK);
ctx->tx_addr.ext_id |= rx_sa << ISOTP_FIXED_ADDR_TA_POS;
/* use same priority for TX as in received message */
ctx->tx_addr.ext_id &= ~(ISOTP_FIXED_ADDR_PRIO_MASK);
ctx->tx_addr.ext_id |= frame->id & ISOTP_FIXED_ADDR_PRIO_MASK;
}

switch (frame->data[index] & ISOTP_PCI_TYPE_MASK) {
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) +
Copy link
Member

Choose a reason for hiding this comment

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

Why do you remove this check? According to the standard, the length in the frame must match if there is no padding, IIRC

Copy link
Member

Choose a reason for hiding this comment

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

@martinjaeger any thoughts here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I can't see any statement that it must match. Some quotes from ISO 15765-2:2004:

6.5.2.2 (SF_DL error handling)

If the network layer receives an SF with an SF_DL greater than 7 when using normal addressing, or greater than 6 for extended or mixed addressing, then the network layer shall ignore the received SF N_PDU.

7.4.3 CAN frame data optimization

[...] The DLC parameter of the CAN frame is set by the sender and read by the receiver to determine the number of data bytes per CAN frame to be processed by the network layer. The DLC parameter cannot be used to determine the message length; this information shall be extracted from the N_PCI information in the beginning of a message.

7.4.4 Data Length Code error handling

[...] The reception of a CAN frame with a DLC value smaller than expected [...] shall be ignored by the network layer without any further action.

So in my understanding we should ignore any bytes beyond ISOTP_PCI_SF_DL, i.e. allow padded and non-padded messages. It is not clearly stated what to do with a message that is padded to less than 8 bytes.

However, we might still want to check if the DLC is too small to fit the message with the length calculated based on ISOTP_PCI_SF_DL. I can add that check. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

That makes sense!

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. And rebased.

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;
}

Expand All @@ -415,7 +451,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,
Expand All @@ -432,7 +468,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;
}
Expand All @@ -444,6 +480,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) {
Expand All @@ -470,14 +507,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) {
Expand Down Expand Up @@ -529,12 +572,20 @@ static void receive_can_rx_isr(struct zcan_frame *frame, void *arg)

static inline int attach_ff_filter(struct isotp_recv_ctx *ctx)
{
uint32_t mask;

if (ctx->rx_addr.use_fixed_addr) {
mask = ISOTP_FIXED_ADDR_RX_MASK;
} else {
mask = CAN_EXT_ID_MASK;
}

struct zcan_filter filter = {
.id_type = ctx->rx_addr.id_type,
.rtr = CAN_DATAFRAME,
.id = ctx->rx_addr.ext_id,
.rtr_mask = 1,
.id_mask = CAN_EXT_ID_MASK
.id_mask = mask
};

ctx->filter_id = can_attach_isr(ctx->can_dev, receive_can_rx_isr, ctx,
Expand All @@ -557,7 +608,7 @@ int isotp_bind(struct isotp_recv_ctx *ctx, const struct device *can_dev,

__ASSERT(ctx, "ctx is NULL");
__ASSERT(can_dev, "CAN device is NULL");
__ASSERT(rx_addr && rx_addr, "RX or TX addr is NULL");
__ASSERT(rx_addr && tx_addr, "RX or TX addr is NULL");
__ASSERT(opts, "OPTS is NULL");

ctx->can_dev = can_dev;
Expand Down Expand Up @@ -755,6 +806,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;
Expand Down Expand Up @@ -853,7 +913,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),
Expand Down Expand Up @@ -926,10 +992,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) {
Expand Down Expand Up @@ -995,11 +1068,11 @@ static void send_state_machine(struct isotp_send_ctx *ctx)
switch (ctx->state) {

case ISOTP_TX_SEND_FF:
LOG_DBG("SM send FF");
send_ff(ctx);
z_add_timeout(&ctx->timeout, send_timeout_handler,
K_MSEC(ISOTP_BS));
ctx->state = ISOTP_TX_WAIT_FC;
LOG_DBG("SM send FF");
break;

case ISOTP_TX_SEND_CF:
Expand All @@ -1021,11 +1094,11 @@ static void send_state_machine(struct isotp_send_ctx *ctx)
}

if (ctx->opts.bs && !ctx->bs) {
LOG_DBG("BS reached. Wait for FC again");
ctx->state = ISOTP_TX_WAIT_FC;
z_add_timeout(&ctx->timeout,
send_timeout_handler,
K_MSEC(ISOTP_BS));
ctx->state = ISOTP_TX_WAIT_FC;
LOG_DBG("BS reached. Wait for FC again");
break;
} else if (ctx->opts.stmin) {
ctx->state = ISOTP_TX_WAIT_ST;
Expand All @@ -1036,10 +1109,10 @@ static void send_state_machine(struct isotp_send_ctx *ctx)
break;

case ISOTP_TX_WAIT_ST:
LOG_DBG("SM wait ST");
z_add_timeout(&ctx->timeout, send_timeout_handler,
stmin_to_ticks(ctx->opts.stmin));
ctx->state = ISOTP_TX_SEND_CF;
LOG_DBG("SM wait ST");
break;

case ISOTP_TX_ERR:
Expand Down Expand Up @@ -1106,7 +1179,7 @@ static int send(struct isotp_send_ctx *ctx, const struct device *can_dev,

__ASSERT_NO_MSG(ctx);
__ASSERT_NO_MSG(can_dev);
__ASSERT_NO_MSG(rx_addr && rx_addr);
__ASSERT_NO_MSG(rx_addr && tx_addr);

if (complete_cb) {
ctx->fin_cb.cb = complete_cb;
Expand Down
Loading