Skip to content

drivers: CAN: Abstract blocking send call #20890

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

Closed
wants to merge 1 commit into from
Closed

drivers: CAN: Abstract blocking send call #20890

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 8 additions & 0 deletions drivers/can/can_common.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,14 @@ LOG_MODULE_REGISTER(can_driver);

#define WORK_BUF_FULL 0xFFFF

void can_common_isr_callback_handler(u32_t err, void *cb_arg)
{
struct can_send_wait *send_wait = (struct can_send_wait *) cb_arg;

send_wait->err = err;
k_sem_give(&send_wait->sem);
}

static void can_msgq_put(struct zcan_frame *frame, void *arg)
{
struct k_msgq *msgq = (struct k_msgq *)arg;
Expand Down
15 changes: 3 additions & 12 deletions drivers/can/can_mcp2515.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -389,6 +389,8 @@ static int mcp2515_send(struct device *dev, const struct zcan_frame *msg,
u8_t len;
u8_t tx_frame[MCP2515_FRAME_LEN];

__ASSERT(callback != NULL, "callback is null");

if (msg->dlc > CAN_MAX_DLC) {
LOG_ERR("DLC of %d exceeds maximum (%d)", msg->dlc, CAN_MAX_DLC);
return CAN_TX_EINVAL;
Expand Down Expand Up @@ -432,10 +434,6 @@ static int mcp2515_send(struct device *dev, const struct zcan_frame *msg,
nnn = BIT(tx_idx);
mcp2515_cmd_rts(dev, nnn);

if (callback == NULL) {
k_sem_take(&dev_data->tx_cb[tx_idx].sem, K_FOREVER);
}

return 0;
}

Expand Down Expand Up @@ -564,11 +562,7 @@ static void mcp2515_tx_done(struct device *dev, u8_t tx_idx)
{
struct mcp2515_data *dev_data = DEV_DATA(dev);

if (dev_data->tx_cb[tx_idx].cb == NULL) {
k_sem_give(&dev_data->tx_cb[tx_idx].sem);
} else {
dev_data->tx_cb[tx_idx].cb(0, dev_data->tx_cb[tx_idx].cb_arg);
}
dev_data->tx_cb[tx_idx].cb(CAN_TX_OK, dev_data->tx_cb[tx_idx].cb_arg);

k_mutex_lock(&dev_data->mutex, K_FOREVER);
dev_data->tx_busy_map &= ~BIT(tx_idx);
Expand Down Expand Up @@ -745,9 +739,6 @@ static int mcp2515_init(struct device *dev)
k_sem_init(&dev_data->int_sem, 0, 1);
k_mutex_init(&dev_data->mutex);
k_sem_init(&dev_data->tx_sem, MCP2515_TX_CNT, MCP2515_TX_CNT);
k_sem_init(&dev_data->tx_cb[0].sem, 0, 1);
k_sem_init(&dev_data->tx_cb[1].sem, 0, 1);
k_sem_init(&dev_data->tx_cb[2].sem, 0, 1);

/* SPI config */
dev_data->spi_cfg.operation = SPI_WORD_SET(8);
Expand Down
1 change: 0 additions & 1 deletion drivers/can/can_mcp2515.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
#define DEV_DATA(dev) ((struct mcp2515_data *const)(dev)->driver_data)

struct mcp2515_tx_cb {
struct k_sem sem;
can_tx_callback_t cb;
void *cb_arg;
};
Expand Down
28 changes: 4 additions & 24 deletions drivers/can/can_mcux_flexcan.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,8 +69,6 @@ struct mcux_flexcan_rx_callback {
};

struct mcux_flexcan_tx_callback {
struct k_sem done;
int status;
flexcan_frame_t frame;
can_tx_callback_t function;
void *arg;
Expand Down Expand Up @@ -296,6 +294,8 @@ static int mcux_flexcan_send(struct device *dev, const struct zcan_frame *msg,
status_t status;
int alloc;

__ASSERT(callback_isr != NULL, "callback is null");

if (msg->dlc > CAN_MAX_DLC) {
LOG_ERR("DLC of %d exceeds maximum (%d)", msg->dlc, CAN_MAX_DLC);
return CAN_TX_EINVAL;
Expand Down Expand Up @@ -328,11 +328,6 @@ static int mcux_flexcan_send(struct device *dev, const struct zcan_frame *msg,
return CAN_TX_ERR;
}

if (callback_isr == NULL) {
k_sem_take(&data->tx_cbs[alloc].done, K_FOREVER);
return data->tx_cbs[alloc].status;
}

return CAN_TX_OK;
}

Expand Down Expand Up @@ -546,13 +541,8 @@ static inline void mcux_flexcan_transfer_error_status(struct device *dev,
if (atomic_test_and_clear_bit(data->tx_allocs, alloc)) {
FLEXCAN_TransferAbortSend(config->base, &data->handle,
ALLOC_IDX_TO_TXMB_IDX(alloc));
if (function != NULL) {
function(status, arg);
} else {
data->tx_cbs[alloc].status = status;
k_sem_give(&data->tx_cbs[alloc].done);
}

function(status, arg);
k_sem_give(&data->tx_allocs_sem);
}
}
Expand All @@ -573,12 +563,7 @@ static inline void mcux_flexcan_transfer_tx_idle(struct device *dev,
arg = data->tx_cbs[alloc].arg;

if (atomic_test_and_clear_bit(data->tx_allocs, alloc)) {
if (function != NULL) {
function(CAN_TX_OK, arg);
} else {
data->tx_cbs[alloc].status = CAN_TX_OK;
k_sem_give(&data->tx_cbs[alloc].done);
}
function(CAN_TX_OK, arg);
k_sem_give(&data->tx_allocs_sem);
}
}
Expand Down Expand Up @@ -658,15 +643,10 @@ static int mcux_flexcan_init(struct device *dev)
const struct mcux_flexcan_config *config = dev->config->config_info;
struct mcux_flexcan_data *data = dev->driver_data;
int err;
int i;

k_mutex_init(&data->rx_mutex);
k_sem_init(&data->tx_allocs_sem, 0, 1);

for (i = 0; i < ARRAY_SIZE(data->tx_cbs); i++) {
k_sem_init(&data->tx_cbs[i].done, 0, 1);
}

err = mcux_flexcan_configure(dev, CAN_NORMAL_MODE, 0);
if (err) {
return err;
Expand Down
62 changes: 23 additions & 39 deletions drivers/can/can_stm32.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,15 +29,6 @@ LOG_MODULE_DECLARE(can_driver, CONFIG_CAN_LOG_LEVEL);
static const u8_t filter_in_bank[] = {2, 4, 1, 2};
static const u8_t reg_demand[] = {2, 1, 4, 2};

static void can_stm32_signal_tx_complete(struct can_mailbox *mb)
{
if (mb->tx_callback) {
mb->tx_callback(mb->error_flags, mb->callback_arg);
} else {
k_sem_give(&mb->tx_int_sem);
}
}

static void can_stm32_get_msg_fifo(CAN_FIFOMailBox_TypeDef *mbox,
struct zcan_frame *msg)
{
Expand Down Expand Up @@ -123,43 +114,44 @@ static inline
void can_stm32_tx_isr_handler(CAN_TypeDef *can, struct can_stm32_data *data)
{
u32_t bus_off;
int err;

bus_off = can->ESR & CAN_ESR_BOFF;

if ((can->TSR & CAN_TSR_RQCP0) | bus_off) {
data->mb0.error_flags =
can->TSR & CAN_TSR_TXOK0 ? CAN_TX_OK :
can->TSR & CAN_TSR_TERR0 ? CAN_TX_ERR :
can->TSR & CAN_TSR_ALST0 ? CAN_TX_ARB_LOST :
bus_off ? CAN_TX_BUS_OFF :
CAN_TX_UNKNOWN;
err = can->TSR & CAN_TSR_TXOK0 ? CAN_TX_OK :
can->TSR & CAN_TSR_TERR0 ? CAN_TX_ERR :
can->TSR & CAN_TSR_ALST0 ? CAN_TX_ARB_LOST :
bus_off ? CAN_TX_BUS_OFF :
CAN_TX_UNKNOWN;

data->mb0.tx_callback(err, data->mb0.callback_arg);
/* clear the request. */
can->TSR |= CAN_TSR_RQCP0;
can_stm32_signal_tx_complete(&data->mb0);
}

if ((can->TSR & CAN_TSR_RQCP1) | bus_off) {
data->mb1.error_flags =
can->TSR & CAN_TSR_TXOK1 ? CAN_TX_OK :
can->TSR & CAN_TSR_TERR1 ? CAN_TX_ERR :
can->TSR & CAN_TSR_ALST1 ? CAN_TX_ARB_LOST :
bus_off ? CAN_TX_BUS_OFF :
CAN_TX_UNKNOWN;
err = can->TSR & CAN_TSR_TXOK1 ? CAN_TX_OK :
can->TSR & CAN_TSR_TERR1 ? CAN_TX_ERR :
can->TSR & CAN_TSR_ALST1 ? CAN_TX_ARB_LOST :
bus_off ? CAN_TX_BUS_OFF :
CAN_TX_UNKNOWN;

data->mb1.tx_callback(err, data->mb1.callback_arg);
/* clear the request. */
can->TSR |= CAN_TSR_RQCP1;
can_stm32_signal_tx_complete(&data->mb1);
}

if ((can->TSR & CAN_TSR_RQCP2) | bus_off) {
data->mb2.error_flags =
can->TSR & CAN_TSR_TXOK2 ? CAN_TX_OK :
can->TSR & CAN_TSR_TERR2 ? CAN_TX_ERR :
can->TSR & CAN_TSR_ALST2 ? CAN_TX_ARB_LOST :
bus_off ? CAN_TX_BUS_OFF :
CAN_TX_UNKNOWN;
err = can->TSR & CAN_TSR_TXOK2 ? CAN_TX_OK :
can->TSR & CAN_TSR_TERR2 ? CAN_TX_ERR :
can->TSR & CAN_TSR_ALST2 ? CAN_TX_ARB_LOST :
bus_off ? CAN_TX_BUS_OFF :
CAN_TX_UNKNOWN;

data->mb2.tx_callback(err, data->mb2.callback_arg);
/* clear the request. */
can->TSR |= CAN_TSR_RQCP2;
can_stm32_signal_tx_complete(&data->mb2);
}

if (can->TSR & CAN_TSR_TME) {
Expand Down Expand Up @@ -386,9 +378,6 @@ static int can_stm32_init(struct device *dev)

k_mutex_init(&data->inst_mutex);
k_sem_init(&data->tx_int_sem, 0, 1);
k_sem_init(&data->mb0.tx_int_sem, 0, 1);
k_sem_init(&data->mb1.tx_int_sem, 0, 1);
k_sem_init(&data->mb2.tx_int_sem, 0, 1);
data->mb0.tx_callback = NULL;
data->mb1.tx_callback = NULL;
data->mb2.tx_callback = NULL;
Expand Down Expand Up @@ -548,6 +537,7 @@ int can_stm32_send(struct device *dev, const struct zcan_frame *msg,
, msg->rtr == CAN_DATAFRAME ? "no" : "yes");

__ASSERT(msg->dlc == 0U || msg->data != NULL, "Dataptr is null");
__ASSERT(callback != NULL, "callback is null");

if (msg->dlc > CAN_MAX_DLC) {
LOG_ERR("DLC of %d exceeds maximum (%d)", msg->dlc, CAN_MAX_DLC);
Expand Down Expand Up @@ -587,7 +577,6 @@ int can_stm32_send(struct device *dev, const struct zcan_frame *msg,

mb->tx_callback = callback;
mb->callback_arg = callback_arg;
k_sem_reset(&mb->tx_int_sem);

/* mailbix identifier register setup */
mailbox->TIR &= CAN_TI0R_TXRQ;
Expand All @@ -612,11 +601,6 @@ int can_stm32_send(struct device *dev, const struct zcan_frame *msg,
mailbox->TIR |= CAN_TI0R_TXRQ;
k_mutex_unlock(&data->inst_mutex);

if (callback == NULL) {
k_sem_take(&mb->tx_int_sem, K_FOREVER);
return mb->error_flags;
}

return 0;
}

Expand Down
2 changes: 0 additions & 2 deletions drivers/can/can_stm32.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,6 @@
struct can_mailbox {
can_tx_callback_t tx_callback;
void *callback_arg;
struct k_sem tx_int_sem;
u32_t error_flags;
};


Expand Down
25 changes: 24 additions & 1 deletion include/drivers/can.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -325,6 +325,14 @@ struct can_driver_api {

};

/** @cond INTERNAL_HIDDEN */
void can_common_isr_callback_handler(u32_t err, void *cb_arg);
struct can_send_wait {
u32_t err;
struct k_sem sem;
};
/** @endcond */

/**
* @brief Perform data transfer to CAN bus.
*
Expand Down Expand Up @@ -355,7 +363,22 @@ static inline int z_impl_can_send(struct device *dev,
const struct can_driver_api *api =
(const struct can_driver_api *)dev->driver_api;

return api->send(dev, msg, timeout, callback_isr, callback_arg);
if (callback_isr) {
return api->send(dev, msg, timeout, callback_isr, callback_arg);
} else {
struct can_send_wait send_wait;
int ret;

k_sem_init(&send_wait.sem, 0, 1);
Copy link
Member

@henrikbrixandersen henrikbrixandersen Nov 21, 2019

Choose a reason for hiding this comment

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

This change will introduce an overhead of initialising a semaphore on each can_send() call without any callback defined. I am not sure it is worth it.

It also seem very unlike any other API implementation in Zephyr?

Copy link
Contributor

@karstenkoenig karstenkoenig Nov 21, 2019

Choose a reason for hiding this comment

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

Yea I am sort of ambivalent about it. Not sure how much of a trade-off the memory saved vs the calling overhead it is.
In terms of driver complexity I think it isn't a big improvement, we all had the same approach and it's not so hard to follow either.

Copy link
Member Author

@alexanderwachter alexanderwachter Nov 22, 2019

Choose a reason for hiding this comment

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

This change will introduce an overhead of initialising a semaphore on each can_send() call without any callback defined. I am not sure it is worth it.

The overhead of initializing is not that much. The blocking send call is used rarely, and in the current implementation, we need to maintain a sem for every mailbox just in case we will need it.

In terms of driver complexity I think it isn't a big improvement, we all had the same approach and it's not so hard to follow either.

The complexity raised with the timeout patch :-)
I really don't like #19707 in its current state.

Copy link
Contributor

@karstenkoenig karstenkoenig Nov 22, 2019

Choose a reason for hiding this comment

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

Ah gotcha. What would be your plan then, use that can_common_isr_callback_handler() for failing sends with timeouts?

Copy link
Member Author

@alexanderwachter alexanderwachter Nov 25, 2019

Choose a reason for hiding this comment

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

Updated #19707 on top of this PR. Looks better now!

ret = api->send(dev, msg, timeout,
can_common_isr_callback_handler, &send_wait);
if (ret != CAN_TX_OK) {
return ret;
}

k_sem_take(&send_wait.sem, K_FOREVER);
return send_wait.err;
}
}

/*
Expand Down