<div dir="ltr"><div dir="ltr"><div>Hi Christian,</div><div><br></div><div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span style="color:rgb(153,0,255)">general note: You add an API to cpukit. Most APIs there should have a <br>
test case in the testsuite. In this case the test case would have to <br>
create some dummy CAN driver and check whether everything works like <br>
expected. As an example take a look at the SPI test:<br><br>
<a href="https://git.rtems.org/rtems/tree/testsuites/libtests/spi01/init.c" rel="noreferrer" target="_blank">https://git.rtems.org/rtems/tree/testsuites/libtests/spi01/init.c</a><br><br>
It creates a dummy device and checks whether the SPI layer does <br>
everything as expected. Would be great if you could add something like <br>
this. Target of such a test should be more or less that every branch in <br>
your code is triggered at least once (code coverage). It doesn't has to <br>
be every combination but it shows that the code is at least reachable <br>
and works as expected.<br></span></blockquote>
</div><div style="margin-left:40px">Adding a test application in testsuites/libtests/can01/init.c<br></div><div><br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<span style="color:rgb(153,0,255)">Do you really mean "LOCK" or should it be a "LOG"? Both is reasonable <br>
here so I'm not entirely sure. But you use the LOCK macro also in <br>
can_xmit and similar so I'm a bit unsure.</span><br></blockquote>
</div><div style="margin-left:40px">It is a log for the lock. Now moved the logs and call to the lock acquire/release to #defines.<br></div><div>
<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<span style="color:rgb(153,0,255)">> + if (sem_count > CAN_TX_BUF_COUNT) {<br><br>
If you increment sem_count only in a debug macro, you must not use it <br>
for any other code. The macro is processed by the preprocessor. So if <br>
you define the CAN_DEBUG_LOCK to something empty, the ++sem_count from <br>
above will never happen. If CAN_DEBUG_LOCK interpretes the argument <br>
twice, you will get the wrong value.<br><br>
Please note: It's in general a good idea not to use anything that <br>
changes a variable or has any side effect in a macro. For example if I <br>
define the following<br><br>
#define SQUARE(x) ((x) * (x))<br><br>
And use it like this:<br><br>
int a = 2;<br>
int b;<br>
b = SQUARE(++a);<br><br>
The result that I would expect from reading the code would be that b is <br>
9 and a is 3 afterwards. But instead I will get b = 16 and a = 4. The <br>
reason for this is that the preprocessor will replace the code with the <br>
following one:<br><br>
int a = 2;<br>
int b;<br>
b = ((++a) * (++a));<br><br>
Therefore: Try to avoid statements with a side effect as a parameter to <br>
something if you are not 100% sure that the something is a function or <br>
is allways executed. You don't have to save lines. We don't have a limit <br>
on the maximum lines in a file. Readability is much more important than <br>
compact code.<br><br>
PS: Also true for "if". I wouldn't be sure whether the printf in the <br>
following code will be executed:<br><br>
if (0 || printf("Will someone call me?")) {<br>
}<br><br>
In general: Try to avoid anything where you have to think about the <br>
order of execution except if you can't avoid it or if it is really obvious.</span></blockquote><span style="color:rgb(153,0,255)">
</span><div><div class="gmail-im"><div style="margin-left:40px">Moved the increment out of the #define.<br></div>
<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<span style="color:rgb(153,0,255)">> +static int can_xmit(struct can_bus *bus)<br>
> +{<br>
> + int ret = RTEMS_SUCCESSFUL;<br>
> +<br>
> + struct can_msg *msg = NULL;<br>
> +<br>
> + while (1) {<br>
> + CAN_DEBUG_LOCK("can_xmit: acquiring lock\n");<br>
> + can_interrupt_lock_acquire(bus</span><span style="color:rgb(153,0,255)">);<br>
> +<br>
> + ret = bus->can_dev_ops->dev_tx_ready</span><span style="color:rgb(153,0,255)">(bus->priv);<br>
> + if (ret != true) {<br>
> + goto return_with_lock_release;<br>
> + }<br>
> +<br>
> + msg = can_tx_get_data_buf(bus);<br>
> + if (msg == NULL) {<br>
> + goto return_with_lock_release;<br>
> + }<br>
> +<br>
> + ret = bus->can_dev_ops->dev_tx(bus-></span><span style="color:rgb(153,0,255)">priv, msg);<br>
> + if (ret != RTEMS_SUCCESSFUL) {<br>
> + printf("can_xmit: dev_send failed\n");<br>
> + }<br>
> +<br>
> + ret = give_sem(bus);<br>
> + if (ret != RTEMS_SUCCESSFUL) {<br>
> + printf("can_tx_done: rtems_semaphore_release failed = %d\n", ret);<br>
> + }<br>
> +<br>
> + CAN_DEBUG_LOCK("can_xmit: releasing lock\n");<br>
> + can_interrupt_lock_release(bus</span><span style="color:rgb(153,0,255)">);<br>
> +<br>
> + //can_tx_done(bus);<br>
> + }<br>
> +<br>
> + return ret;<br><br></span></blockquote><span style="color:rgb(153,0,255)">
</span></div></div><blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span style="color:rgb(153,0,255)">
If I see it correctly, you can never reach this return. You have a <br>
while(1) without any break condition. By the way: Code with goto, loops <br>
and break tends to get sphaghetti code. You should only use these <br>
constructions if it improves readability compared to a solution without <br>
these.</span></blockquote><div><br></div><div>Updated to release the lock and break. <br></div></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<span style="color:rgb(153,0,255)">You use the macros a lot. You are aware that you could do something like <br>
that:<br><br>
- Declare the original can_interrupt_lock_acquire() as something with a <br>
prefix or postifix for example as real_can_interrupt_lock_acquire.<br><br>
- Then add a define that wrapps it with:<br><br>
#define can_interrupt_lock_acquire(...) do { \<br>
CAN_DEBUG_LOCK("%s:%d\n", __FILE__, __LINE__); \<br>
real_can_interrupt_lock_acquire(__VA_ARGS__);<br>
} while(0)</span><br></blockquote>
<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<span style="color:rgb(153,0,255)">Then you can just write it as normal can_interrupt_lock_acquire but get <br>
a debug print with file and line on every call.<span class="gmail-im"></span></span><br><span class="gmail-im"></span></blockquote><div style="margin-left:40px"><span class="gmail-im">
now moved the logs and call to the lock acquire/release to #defines.</span></div></div><div><span class="gmail-im"><br></span><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span class="gmail-im">
<span style="color:rgb(153,0,255)">> + can_interrupt_lock_acquire(bus</span><span style="color:rgb(153,0,255)">);</span></span><span style="color:rgb(153,0,255)"><br><span class="gmail-im">
> + bus->can_tx_buf_waiters++;</span><br><span class="gmail-im">
> + CAN_DEBUG_LOCK("can_bus_write: release lock can_tx_buf_waiters++\n");</span><br><span class="gmail-im">
> + can_interrupt_lock_release(bus);</span><br><span class="gmail-im">
> +</span><br><span class="gmail-im">
> + ret = take_sem(bus);</span><br><span class="gmail-im">
> +</span><br><span class="gmail-im">
> + CAN_DEBUG_LOCK("can_bus_write: acquiring lock can_tx_buf_waiters--\n");</span><br><span class="gmail-im">
> + can_interrupt_lock_acquire(bus);</span><br><span class="gmail-im">
> + bus->can_tx_buf_waiters--;</span><br><span class="gmail-im">
> + CAN_DEBUG_LOCK("can_bus_write: release lock can_tx_buf_waiters--\n");</span><br><span class="gmail-im">
> + can_interrupt_lock_release(bus);</span><br><span class="gmail-im">
> +</span></span><br><span class="gmail-im"></span></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span class="gmail-im">
<span style="color:rgb(153,0,255)">> + if (ret != RTEMS_SUCCESSFUL) {</span></span><span style="color:rgb(153,0,255)"><br><span class="gmail-im">
> + printf("can_bus_write: cannot take semaphore\n");</span><br><span class="gmail-im">
> + ret = -RTEMS_INTERNAL_ERROR;</span><br></span><span class="gmail-im"><span style="color:rgb(153,0,255)">
> + goto return_</span>from_func;</span><br><span class="gmail-im"></span></blockquote><span class="gmail-im">
<br></span><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<span style="color:rgb(153,0,255)">Why not just use a return here instead of the goto? The label isn't used <br>
anywhere else.<span class="gmail-im"></span></span><br><span class="gmail-im"></span></blockquote><div style="margin-left:40px">changed to return.<br></div><div style="margin-left:40px"><span class="gmail-im"></span></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span class="gmail-im">
<span style="color:rgb(153,0,255)">> + }</span></span><span style="color:rgb(153,0,255)"><br><span class="gmail-im">
> + }</span><br><span class="gmail-im">
> +</span><br><span class="gmail-im">
> + /* sleep is for debug purpose to test concurrency issues */</span><br><span class="gmail-im">
> + printf("can_bus_write: calling sleep\n");</span><br><span class="gmail-im">
> + sleep(1);</span><br><span class="gmail-im">
> + printf("can_bus_write: coming out sleep\n");</span><br><span class="gmail-im">
</span><br><span class="gmail-im"></span>
Obviously this shouldn't be there in the later merge-ready patch.
</span></blockquote><div style="margin-left:40px">Yes, these sleeps will be removed.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span style="color:rgb(153,0,255)">Do these really have to be in a header that you (most likely) will later <br>
install? With that they are public visible API.</span></blockquote><div style="margin-left:40px"><br></div><div style="margin-left:40px"> </div></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, 2 Aug 2022 at 23:19, <<a href="mailto:oss@c-mauderer.de">oss@c-mauderer.de</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hello Duc,<br>
<br>
general note: You add an API to cpukit. Most APIs there should have a <br>
test case in the testsuite. In this case the test case would have to <br>
create some dummy CAN driver and check whether everything works like <br>
expected. As an example take a look at the SPI test:<br>
<br>
<a href="https://git.rtems.org/rtems/tree/testsuites/libtests/spi01/init.c" rel="noreferrer" target="_blank">https://git.rtems.org/rtems/tree/testsuites/libtests/spi01/init.c</a><br>
<br>
It creates a dummy device and checks whether the SPI layer does <br>
everything as expected. Would be great if you could add something like <br>
this. Target of such a test should be more or less that every branch in <br>
your code is triggered at least once (code coverage). It doesn't has to <br>
be every combination but it shows that the code is at least reachable <br>
and works as expected.<br>
<br>
Am 31.07.22 um 13:53 schrieb Prashanth S:<br>
> ---<br>
> cpukit/dev/can/can.c | 514 +++++++++++++++++++++++++++++<br>
> cpukit/include/dev/can/can-msg.h | 55 +++<br>
> cpukit/include/dev/can/can-queue.h | 127 +++++++<br>
> cpukit/include/dev/can/can.h | 102 ++++++<br>
> spec/build/cpukit/librtemscpu.yml | 6 +<br>
> 5 files changed, 804 insertions(+)<br>
> create mode 100644 cpukit/dev/can/can.c<br>
> create mode 100644 cpukit/include/dev/can/can-msg.h<br>
> create mode 100644 cpukit/include/dev/can/can-queue.h<br>
> create mode 100644 cpukit/include/dev/can/can.h<br>
> <br>
> diff --git a/cpukit/dev/can/can.c b/cpukit/dev/can/can.c<br>
> new file mode 100644<br>
> index 0000000000..8feec8800b<br>
> --- /dev/null<br>
> +++ b/cpukit/dev/can/can.c<br>
> @@ -0,0 +1,514 @@<br>
> +/* SPDX-License-Identifier: BSD-2-Clause */<br>
> +<br>
> +/**<br>
> + * @file<br>
> + *<br>
> + * @ingroup CANBus<br>
> + *<br>
> + * @brief Controller Area Network (CAN) Bus Implementation<br>
> + *<br>
> + */<br>
> +<br>
> +/*<br>
> + * Copyright (C) 2022 Prashanth S (<a href="mailto:fishesprashanth@gmail.com" target="_blank">fishesprashanth@gmail.com</a>)<br>
> + *<br>
> + * Redistribution and use in source and binary forms, with or without<br>
> + * modification, are permitted provided that the following conditions<br>
> + * are met:<br>
> + * 1. Redistributions of source code must retain the above copyright<br>
> + * notice, this list of conditions and the following disclaimer.<br>
> + * 2. Redistributions in binary form must reproduce the above copyright<br>
> + * notice, this list of conditions and the following disclaimer in the<br>
> + * documentation and/or other materials provided with the distribution.<br>
> + *<br>
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"<br>
> + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE<br>
> + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE<br>
> + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE<br>
> + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR<br>
> + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF<br>
> + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS<br>
> + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN<br>
> + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)<br>
> + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE<br>
> + * POSSIBILITY OF SUCH DAMAGE.<br>
> + */<br>
> +<br>
> +#include <rtems/imfs.h><br>
> +#include <rtems/thread.h><br>
> +<br>
> +#include <dev/can/can-queue.h><br>
> +#include <dev/can/can.h><br>
> +<br>
> +#include <string.h><br>
> +#include <stdlib.h><br>
> +<br>
> +#include <fcntl.h><br>
> +<br>
> +static ssize_t<br>
> +can_bus_open(rtems_libio_t *iop, const char *path, int oflag, mode_t mode);<br>
> +static ssize_t<br>
> +can_bus_read(rtems_libio_t *iop, void *buffer, size_t count);<br>
> +static ssize_t<br>
> +can_bus_write(rtems_libio_t *iop, const void *buffer, size_t count);<br>
> +static ssize_t<br>
> +can_bus_ioctl(rtems_libio_t *iop, ioctl_command_t request, void *buffer);<br>
> +<br>
> +static int can_xmit(struct can_bus *bus);<br>
> +<br>
> +static int can_create_sem(struct can_bus *);<br>
> +static int try_sem(struct can_bus *);<br>
> +static int take_sem(struct can_bus *);<br>
> +static int give_sem(struct can_bus *);<br>
> +<br>
> +/* sem_count this is for debug purpose, for debugging<br>
> +the take_sem and give_sem<br>
> +*/<br>
> +static int sem_count = 0;<br>
> +<br>
> +static void can_bus_obtain(can_bus *bus)<br>
> +{<br>
> + rtems_recursive_mutex_lock(&bus->mutex);<br>
> +}<br>
> +<br>
> +static void can_bus_release(can_bus *bus)<br>
> +{<br>
> + rtems_recursive_mutex_unlock(&bus->mutex);<br>
> +}<br>
> +<br>
> +int can_create_sem(struct can_bus *bus)<br>
> +{<br>
> + int ret = 0;<br>
> +<br>
> + ret = rtems_semaphore_create(rtems_build_name('c', 'a', 'n', bus->index),<br>
> + CAN_TX_BUF_COUNT, RTEMS_FIFO | RTEMS_COUNTING_SEMAPHORE | RTEMS_LOCAL,<br>
> + 0, &bus->tx_fifo_sem_id);<br>
> +<br>
> + if (ret != 0) {<br>
> + printf("can_create_sem: rtems_semaphore_create failed %d\n", ret);<br>
> + }<br>
> +<br>
> + return ret;<br>
> +}<br>
> +<br>
> +static void can_interrupt_lock_acquire(struct can_bus *bus)<br>
> +{<br>
> + can_bus_obtain(bus);<br>
> + bus->can_dev_ops->dev_int(bus->priv, false);<br>
> +}<br>
> +<br>
> +static void can_interrupt_lock_release(struct can_bus *bus)<br>
> +{<br>
> + bus->can_dev_ops->dev_int(bus->priv, true);<br>
> + can_bus_release(bus);<br>
> +}<br>
> +<br>
> +static int take_sem(struct can_bus *bus)<br>
> +{<br>
> + int ret = rtems_semaphore_obtain(bus->tx_fifo_sem_id, RTEMS_WAIT,<br>
> + RTEMS_NO_TIMEOUT);<br>
> + if (ret == RTEMS_SUCCESSFUL) {<br>
> + CAN_DEBUG_LOCK("take_sem: Counting semaphore count = %d\n", ++sem_count);<br>
<br>
<br>
Do you really mean "LOCK" or should it be a "LOG"? Both is reasonable <br>
here so I'm not entirely sure. But you use the LOCK macro also in <br>
can_xmit and similar so I'm a bit unsure.<br>
<br>
<br>
> + if (sem_count > CAN_TX_BUF_COUNT) {<br>
<br>
If you increment sem_count only in a debug macro, you must not use it <br>
for any other code. The macro is processed by the preprocessor. So if <br>
you define the CAN_DEBUG_LOCK to something empty, the ++sem_count from <br>
above will never happen. If CAN_DEBUG_LOCK interpretes the argument <br>
twice, you will get the wrong value.<br>
<br>
Please note: It's in general a good idea not to use anything that <br>
changes a variable or has any side effect in a macro. For example if I <br>
define the following<br>
<br>
#define SQUARE(x) ((x) * (x))<br>
<br>
And use it like this:<br>
<br>
int a = 2;<br>
int b;<br>
b = SQUARE(++a);<br>
<br>
The result that I would expect from reading the code would be that b is <br>
9 and a is 3 afterwards. But instead I will get b = 16 and a = 4. The <br>
reason for this is that the preprocessor will replace the code with the <br>
following one:<br>
<br>
int a = 2;<br>
int b;<br>
b = ((++a) * (++a));<br>
<br>
Therefore: Try to avoid statements with a side effect as a parameter to <br>
something if you are not 100% sure that the something is a function or <br>
is allways executed. You don't have to save lines. We don't have a limit <br>
on the maximum lines in a file. Readability is much more important than <br>
compact code.<br>
<br>
PS: Also true for "if". I wouldn't be sure whether the printf in the <br>
following code will be executed:<br>
<br>
if (0 || printf("Will someone call me?")) {<br>
}<br>
<br>
In general: Try to avoid anything where you have to think about the <br>
order of execution except if you can't avoid it or if it is really obvious.<br>
<br>
> + printf("take_sem error: sem_count is misleading\n");<br>
> + while (1);<br>
> + }<br>
> + }<br>
> +<br>
> + return ret;<br>
> +}<br>
> +<br>
> +static int give_sem(struct can_bus *bus)<br>
> +{<br>
> + int ret = rtems_semaphore_release(bus->tx_fifo_sem_id);<br>
> + if (ret == RTEMS_SUCCESSFUL) {<br>
> + CAN_DEBUG_LOCK("give_sem: Counting semaphore count = %d\n", --sem_count);<br>
> + if (sem_count < 0) {<br>
> + printf("give_sem error: sem_count is misleading\n");<br>
> + while (1);<br>
> + }<br>
> + }<br>
> +<br>
> + return ret;<br>
> +}<br>
> +<br>
> +static int try_sem(struct can_bus *bus)<br>
> +{<br>
> + int ret = rtems_semaphore_obtain(bus->tx_fifo_sem_id, RTEMS_NO_WAIT,<br>
> + RTEMS_NO_TIMEOUT);<br>
> + if (ret == RTEMS_SUCCESSFUL) {<br>
> + CAN_DEBUG_LOCK("take_sem: Counting semaphore count = %d\n", ++sem_count);<br>
> + if (sem_count > CAN_TX_BUF_COUNT) {<br>
> + printf("take_sem error: sem_count is misleading\n");<br>
> + while (1);<br>
> + }<br>
> + }<br>
> +<br>
> + return ret;<br>
> +}<br>
> +<br>
> +static ssize_t<br>
> +can_bus_open(rtems_libio_t *iop, const char *path, int oflag, mode_t mode)<br>
> +{<br>
> +/*<br>
> + can_bus *bus = IMFS_generic_get_context_by_iop(iop);<br>
> +<br>
> + if (bus == NULL) {<br>
> + return -RTEMS_NOT_DEFINED;<br>
> + }<br>
> +*/<br>
> + return 0;<br>
> +}<br>
> +<br>
> +int can_receive(struct can_bus *bus, struct can_msg *msg)<br>
> +{<br>
> + int32_t ret = 0;<br>
> +<br>
> + uint32_t count = 0;<br>
> +<br>
> + ret = rtems_message_queue_broadcast(bus->rx_queue_id, msg, CAN_MSGLEN(msg),<br>
> + &count);<br>
> + if (ret != RTEMS_SUCCESSFUL) {<br>
> + CAN_DEBUG_RX("rtems_message_queue_send failed\n");<br>
> + }<br>
> +<br>
> + CAN_DEBUG_RX("rtems_message_queue_broadcast = %u\n", count);<br>
> +<br>
> + return ret;<br>
> +}<br>
> +<br>
> +/* count argument is not used here as struct can_msg has the dlc member */<br>
> +static ssize_t can_bus_read(rtems_libio_t *iop, void *buffer, size_t count)<br>
> +{<br>
> + int32_t ret = 0;<br>
> + uint32_t flags = 0;<br>
> +<br>
> + can_bus *bus = IMFS_generic_get_context_by_iop(iop);<br>
> +<br>
> + if (bus == NULL || bus->can_dev_ops->dev_rx == NULL) {<br>
> + return -RTEMS_NOT_DEFINED;<br>
> + }<br>
> +<br>
> + struct can_msg *msg = (struct can_msg *)buffer;<br>
> +<br>
> + if ((iop->flags & O_NONBLOCK) != 0) {<br>
> + flags = RTEMS_NO_WAIT;<br>
> + }<br>
> +<br>
> + ret = rtems_message_queue_receive(bus->rx_queue_id, (void *)msg, &count,<br>
> + flags, 0);<br>
> + if (ret != RTEMS_SUCCESSFUL) {<br>
> + return ret;<br>
> + }<br>
> +<br>
> + return count;<br>
> +}<br>
> +<br>
> +static int can_xmit(struct can_bus *bus)<br>
> +{<br>
> + int ret = RTEMS_SUCCESSFUL;<br>
> +<br>
> + struct can_msg *msg = NULL;<br>
> +<br>
> + while (1) {<br>
> + CAN_DEBUG_LOCK("can_xmit: acquiring lock\n");<br>
> + can_interrupt_lock_acquire(bus);<br>
> +<br>
> + ret = bus->can_dev_ops->dev_tx_ready(bus->priv);<br>
> + if (ret != true) {<br>
> + goto return_with_lock_release;<br>
> + }<br>
> +<br>
> + msg = can_tx_get_data_buf(bus);<br>
> + if (msg == NULL) {<br>
> + goto return_with_lock_release;<br>
> + }<br>
> +<br>
> + ret = bus->can_dev_ops->dev_tx(bus->priv, msg);<br>
> + if (ret != RTEMS_SUCCESSFUL) {<br>
> + printf("can_xmit: dev_send failed\n");<br>
> + }<br>
> +<br>
> + ret = give_sem(bus);<br>
> + if (ret != RTEMS_SUCCESSFUL) {<br>
> + printf("can_tx_done: rtems_semaphore_release failed = %d\n", ret);<br>
> + }<br>
> +<br>
> + CAN_DEBUG_LOCK("can_xmit: releasing lock\n");<br>
> + can_interrupt_lock_release(bus);<br>
> +<br>
> + //can_tx_done(bus);<br>
> + }<br>
> +<br>
> + return ret;<br>
<br>
If I see it correctly, you can never reach this return. You have a <br>
while(1) without any break condition. By the way: Code with goto, loops <br>
and break tends to get sphaghetti code. You should only use these <br>
constructions if it improves readability compared to a solution without <br>
these.<br>
<br>
> +<br>
> +return_with_lock_release:<br>
> +<br>
> + CAN_DEBUG_LOCK("can_xmit: releasing lock\n");<br>
> + can_interrupt_lock_release(bus);<br>
> +<br>
> + return ret;<br>
> +}<br>
> +<br>
> +void can_print_msg(struct can_msg const *msg)<br>
> +{<br>
> + printf("\n----------------------------------------------------------------\n");<br>
> + printf("id = %d len = %d flags = 0x%08X\n", msg->id, msg->len, msg->flags);<br>
> +<br>
> + for (int i = 0; i < msg->len; i++) {<br>
> + printf("%02x ", msg->data[i]);<br>
> + }<br>
> +<br>
> + printf("\n-----------------------------------------------------------------\n");<br>
> +}<br>
> +<br>
> +/* can_tx_done should be called only with interrupts disabled */<br>
> +int can_tx_done(struct can_bus *bus)<br>
> +{<br>
> + int ret = 0;<br>
> +<br>
> + if (bus->can_dev_ops->dev_tx_ready(bus->priv) == true) {<br>
> + can_xmit(bus);<br>
> + }<br>
> +<br>
> + return ret;<br>
> +}<br>
> +<br>
> +static ssize_t<br>
> +can_bus_write(rtems_libio_t *iop, const void *buffer, size_t count)<br>
> +{<br>
> + can_bus *bus = IMFS_generic_get_context_by_iop(iop);<br>
> +<br>
> + if (bus == NULL || bus->can_dev_ops->dev_tx == NULL) {<br>
> + return -RTEMS_NOT_DEFINED;<br>
> + }<br>
> +<br>
> + int32_t ret = 0;<br>
> +<br>
> + struct can_msg const *msg = buffer;<br>
> + struct can_msg *fifo_buf = NULL;<br>
> +<br>
> + CAN_DEBUG_LOCK("can_bus_write: acquiring lock tx_fifo.empty_count\n");<br>
> + can_interrupt_lock_acquire(bus);<br>
> + CAN_DEBUG_TX("can_bus_write: empty_count = %u\n", bus->tx_fifo.empty_count);<br>
> + CAN_DEBUG_LOCK("can_bus_write: release lock tx_fifo.empty_count\n");<br>
> + can_interrupt_lock_release(bus);<br>
> +<br>
> + if ((iop->flags & O_NONBLOCK) != 0) {<br>
> + ret = try_sem(bus);<br>
> + if (ret != RTEMS_SUCCESSFUL) {<br>
> + return -RTEMS_NO_MEMORY;<br>
> + }<br>
> + } else {<br>
> + CAN_DEBUG_LOCK("can_bus_write: acquiring lock can_tx_buf_waiters++\n");<br>
<br>
You use the macros a lot. You are aware that you could do something like <br>
that:<br>
<br>
- Declare the original can_interrupt_lock_acquire() as something with a <br>
prefix or postifix for example as real_can_interrupt_lock_acquire.<br>
<br>
- Then add a define that wrapps it with:<br>
<br>
#define can_interrupt_lock_acquire(...) do { \<br>
CAN_DEBUG_LOCK("%s:%d\n", __FILE__, __LINE__); \<br>
real_can_interrupt_lock_acquire(__VA_ARGS__);<br>
} while(0)<br>
<br>
Then you can just write it as normal can_interrupt_lock_acquire but get <br>
a debug print with file and line on every call.<br>
<br>
> + can_interrupt_lock_acquire(bus);<br>
> + bus->can_tx_buf_waiters++;<br>
> + CAN_DEBUG_LOCK("can_bus_write: release lock can_tx_buf_waiters++\n");<br>
> + can_interrupt_lock_release(bus);<br>
> +<br>
> + ret = take_sem(bus);<br>
> +<br>
> + CAN_DEBUG_LOCK("can_bus_write: acquiring lock can_tx_buf_waiters--\n");<br>
> + can_interrupt_lock_acquire(bus);<br>
> + bus->can_tx_buf_waiters--;<br>
> + CAN_DEBUG_LOCK("can_bus_write: release lock can_tx_buf_waiters--\n");<br>
> + can_interrupt_lock_release(bus);<br>
> +<br>
> + if (ret != RTEMS_SUCCESSFUL) {<br>
> + printf("can_bus_write: cannot take semaphore\n");<br>
> + ret = -RTEMS_INTERNAL_ERROR;<br>
> + goto return_from_func;<br>
<br>
Why not just use a return here instead of the goto? The label isn't used <br>
anywhere else.<br>
<br>
> + }<br>
> + }<br>
> +<br>
> + /* sleep is for debug purpose to test concurrency issues */<br>
> + printf("can_bus_write: calling sleep\n");<br>
> + sleep(1);<br>
> + printf("can_bus_write: coming out sleep\n");<br>
<br>
Obviously this shouldn't be there in the later merge-ready patch.<br>
<br>
> +<br>
> + CAN_DEBUG_LOCK("can_bus_write: acquiring lock can_tx_get_empty_buf\n");<br>
> + can_interrupt_lock_acquire(bus);<br>
> + fifo_buf = can_tx_get_empty_buf(bus);<br>
> + CAN_DEBUG_LOCK("can_bus_write: releasing lock can_tx_get_empty_buf\n");<br>
> + can_interrupt_lock_release(bus);<br>
> +<br>
> + if (fifo_buf == NULL) {<br>
> + printf("can_bus_write: Buffer counts are not synchronized with semaphore count\n");<br>
> + return -RTEMS_INTERNAL_ERROR;<br>
> + }<br>
> +<br>
> + /* sleep is for debug purpose to test concurrency issues */<br>
> + printf("can_bus_write: calling sleep\n");<br>
> + sleep(1);<br>
> + printf("can_bus_write: coming out sleep\n");<br>
> +<br>
> + uint32_t msg_size = (char *)&msg->data[msg->len] - (char *)msg;<br>
> +<br>
> + CAN_DEBUG_TX("can_bus_write: can_msg_size = %u\n", msg_size);<br>
> + if (msg_size > sizeof(struct can_msg)) {<br>
> + printf("can_bus_write:"<br>
> + "can message len error msg_size = %u struct can_msg = %u\n",<br>
> + msg_size, sizeof(struct can_msg));<br>
> + return -RTEMS_INVALID_SIZE;<br>
> + }<br>
> +<br>
> + CAN_DEBUG_TX("can_bus_write: copying msg from application to driver buffer\n");<br>
> + memcpy(fifo_buf, msg, msg_size);<br>
> + ret = msg_size;<br>
> +<br>
> + /* sleep is for debug purpose to test concurrency issues */<br>
> + printf("can_bus_write: calling sleep\n");<br>
> + sleep(1);<br>
> + printf("can_bus_write: coming out sleep\n");<br>
> +<br>
> + if (bus->can_dev_ops->dev_tx_ready(bus->priv) == true) {<br>
> + can_xmit(bus);<br>
> + }<br>
> +<br>
> +return_from_func:<br>
> + return ret;<br>
> +}<br>
> +<br>
> +static ssize_t<br>
> +can_bus_ioctl(rtems_libio_t *iop, ioctl_command_t request, void *buffer)<br>
> +{<br>
> + can_bus *bus = IMFS_generic_get_context_by_iop(iop);<br>
> +<br>
> + if (bus == NULL || bus->can_dev_ops->dev_ioctl == NULL) {<br>
> + return -RTEMS_NOT_DEFINED;<br>
> + }<br>
> +<br>
> + can_bus_obtain(bus);<br>
> +<br>
> + bus->can_dev_ops->dev_ioctl(bus->priv, NULL, 0);<br>
> +<br>
> + can_bus_release(bus);<br>
> +<br>
> + return RTEMS_SUCCESSFUL;<br>
> +}<br>
> +<br>
> +static const rtems_filesystem_file_handlers_r can_bus_handler = {<br>
> + .open_h = can_bus_open,<br>
> + .close_h = rtems_filesystem_default_close,<br>
> + .read_h = can_bus_read,<br>
> + .write_h = can_bus_write,<br>
> + .ioctl_h = can_bus_ioctl,<br>
> + .lseek_h = rtems_filesystem_default_lseek,<br>
> + .fstat_h = IMFS_stat,<br>
> + .ftruncate_h = rtems_filesystem_default_ftruncate,<br>
> + .fsync_h = rtems_filesystem_default_fsync_or_fdatasync,<br>
> + .fdatasync_h = rtems_filesystem_default_fsync_or_fdatasync,<br>
> + .fcntl_h = rtems_filesystem_default_fcntl,<br>
> + .kqfilter_h = rtems_filesystem_default_kqfilter,<br>
> + .mmap_h = rtems_filesystem_default_mmap,<br>
> + .poll_h = rtems_filesystem_default_poll,<br>
> + .readv_h = rtems_filesystem_default_readv,<br>
> + .writev_h = rtems_filesystem_default_writev<br>
> +};<br>
> +<br>
> +static void can_bus_node_destroy(IMFS_jnode_t *node)<br>
> +{<br>
> + can_bus *bus;<br>
> +<br>
> + bus = IMFS_generic_get_context_by_node(node);<br>
> + (*bus->destroy)(bus);<br>
> +<br>
> + IMFS_node_destroy_default(node);<br>
> +}<br>
> +<br>
> +static const<br>
> +IMFS_node_control can_bus_node_control = IMFS_GENERIC_INITIALIZER(&can_bus_handler,<br>
> + IMFS_node_initialize_generic, can_bus_node_destroy);<br>
> +<br>
> +rtems_status_code can_bus_register(can_bus *bus, const char *bus_path)<br>
> +{<br>
> + int ret = RTEMS_SUCCESSFUL;<br>
> +<br>
> + static uint8_t index = 0;<br>
> +<br>
> + if (index == 255) {<br>
> + printf("can_bus_register: can bus registration limit reached\n");<br>
> + return RTEMS_TOO_MANY;<br>
> + }<br>
> +<br>
> + bus->index = index++;<br>
> + CAN_DEBUG("Registering CAN bus index = %u\n", bus->index);<br>
> +<br>
> + ret = IMFS_make_generic_node(bus_path, S_IFCHR | S_IRWXU | S_IRWXG | S_IRWXO,<br>
> + &can_bus_node_control, bus);<br>
> + if (ret != RTEMS_SUCCESSFUL) {<br>
> + printf("can_bus_register: Creating node failed = %d\n", ret);<br>
> + goto fail;<br>
> + }<br>
> +<br>
> + if ((ret = can_create_sem(bus)) != RTEMS_SUCCESSFUL) {<br>
> + printf("can_bus_register: can_create_sem failed = %d\n", ret);<br>
> + goto fail;<br>
> + }<br>
> +<br>
> + if ((ret = can_create_tx_buffers(bus)) != RTEMS_SUCCESSFUL) {<br>
> + printf("can_bus_register: can_create_tx_buffers failed = %d\n", ret);<br>
> + goto fail;<br>
> + }<br>
> +<br>
> + if ((ret = can_create_rx_buffers(bus)) != RTEMS_SUCCESSFUL) {<br>
> + printf("can_bus_register: can_create_rx_buffers failed = %d\n", ret);<br>
> + goto free_tx_buffers_return;<br>
> + }<br>
> +<br>
> + return ret;<br>
> +<br>
> +free_tx_buffers_return:<br>
> + free(bus->tx_fifo.pbuf);<br>
> +<br>
> +fail:<br>
> + (*bus->destroy)(bus);<br>
> + return ret;<br>
> +<br>
> +}<br>
> +<br>
> +static void can_bus_destroy(can_bus *bus)<br>
> +{<br>
> + rtems_recursive_mutex_destroy(&bus->mutex);<br>
> +}<br>
> +<br>
> +static int can_bus_do_init(can_bus *bus, void (*destroy)(can_bus *bus))<br>
> +{<br>
> + rtems_recursive_mutex_init(&bus->mutex, "CAN Bus");<br>
> + bus->destroy = can_bus_destroy;<br>
> +<br>
> + return RTEMS_SUCCESSFUL;<br>
> +}<br>
> +<br>
> +static void can_bus_destroy_and_free(can_bus *bus)<br>
> +{<br>
> + can_bus_destroy(bus);<br>
> + free(bus);<br>
> +}<br>
> +<br>
> +int can_bus_init(can_bus *bus)<br>
> +{<br>
> + memset(bus, 0, sizeof(*bus));<br>
> +<br>
> + return can_bus_do_init(bus, can_bus_destroy);<br>
> +}<br>
> +<br>
> +can_bus *can_bus_alloc_and_init(size_t size)<br>
> +{<br>
> + can_bus *bus = NULL;<br>
> +<br>
> + if (size >= sizeof(*bus)) {<br>
> + bus = calloc(1, size);<br>
> + if (bus == NULL) {<br>
> + printf("can_bus_alloc_and_init: calloc failed\b");<br>
> + return NULL;<br>
> + }<br>
> +<br>
> + int rv = can_bus_do_init(bus, can_bus_destroy_and_free);<br>
> + if (rv != 0) {<br>
> + printf("can_bus_alloc_and_init: can_bus_do_init failed\n");<br>
> + return NULL;<br>
> + }<br>
> + }<br>
> + return bus;<br>
> +}<br>
> diff --git a/cpukit/include/dev/can/can-msg.h b/cpukit/include/dev/can/can-msg.h<br>
> new file mode 100644<br>
> index 0000000000..5c79b91600<br>
> --- /dev/null<br>
> +++ b/cpukit/include/dev/can/can-msg.h<br>
> @@ -0,0 +1,55 @@<br>
> +/* SPDX-License-Identifier: BSD-2-Clause */<br>
> +<br>
> +/**<br>
> + * @file<br>
> + *<br>
> + * @ingroup CANBus<br>
> + *<br>
> + * @brief Controller Area Network (CAN) Bus Implementation<br>
> + *<br>
> + */<br>
> +<br>
> +/*<br>
> + * Copyright (C) 2022 Prashanth S <<a href="mailto:fishesprashanth@gmail.com" target="_blank">fishesprashanth@gmail.com</a>><br>
> + *<br>
> + * Redistribution and use in source and binary forms, with or without<br>
> + * modification, are permitted provided that the following conditions<br>
> + * are met:<br>
> + * 1. Redistributions of source code must retain the above copyright<br>
> + * notice, this list of conditions and the following disclaimer.<br>
> + * 2. Redistributions in binary form must reproduce the above copyright<br>
> + * notice, this list of conditions and the following disclaimer in the<br>
> + * documentation and/or other materials provided with the distribution.<br>
> + *<br>
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"<br>
> + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE<br>
> + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE<br>
> + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE<br>
> + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR<br>
> + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF<br>
> + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS<br>
> + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN<br>
> + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)<br>
> + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE<br>
> + * POSSIBILITY OF SUCH DAMAGE.<br>
> + */<br>
> +<br>
> +#ifndef _DEV_CAN_CAN_MSG_H<br>
> +#define _DEV_CAN_CAN_MSG_H<br>
> +<br>
> +#define CAN_MSG_MAX_SIZE (8u)<br>
> +<br>
> +#define CAN_TX_BUF_COUNT (2u)<br>
> +#define CAN_RX_BUF_COUNT (2u)<br>
> +<br>
> +#define CAN_MSGLEN(msg) (sizeof(struct can_msg) - (CAN_MSG_MAX_SIZE - msg->len))<br>
> +<br>
> +struct can_msg {<br>
> + uint32_t id; // 32 bits to support extended id (29 bits).<br>
> + uint32_t timestamp;<br>
> + uint16_t flags; // RTR | BRS | EXT ...<br>
> + uint16_t len;<br>
> + uint8_t data[CAN_MSG_MAX_SIZE]; // For supporting data transfer up to 64 bytes.<br>
> +};<br>
> +<br>
> +#endif /* _DEV_CAN_CAN_MSG_H */<br>
> diff --git a/cpukit/include/dev/can/can-queue.h b/cpukit/include/dev/can/can-queue.h<br>
> new file mode 100644<br>
> index 0000000000..c018441320<br>
> --- /dev/null<br>
> +++ b/cpukit/include/dev/can/can-queue.h<br>
> @@ -0,0 +1,127 @@<br>
> +/* SPDX-License-Identifier: BSD-2-Clause */<br>
> +<br>
> +/**<br>
> + * @file<br>
> + *<br>
> + * @ingroup CANBus<br>
> + *<br>
> + * @brief Controller Area Network (CAN) Bus Implementation<br>
> + *<br>
> + */<br>
> +<br>
> +/*<br>
> + * Copyright (C) 2022 Prashanth S <<a href="mailto:fishesprashanth@gmail.com" target="_blank">fishesprashanth@gmail.com</a>><br>
> + *<br>
> + * Redistribution and use in source and binary forms, with or without<br>
> + * modification, are permitted provided that the following conditions<br>
> + * are met:<br>
> + * 1. Redistributions of source code must retain the above copyright<br>
> + * notice, this list of conditions and the following disclaimer.<br>
> + * 2. Redistributions in binary form must reproduce the above copyright<br>
> + * notice, this list of conditions and the following disclaimer in the<br>
> + * documentation and/or other materials provided with the distribution.<br>
> + *<br>
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"<br>
> + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE<br>
> + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE<br>
> + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE<br>
> + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR<br>
> + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF<br>
> + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS<br>
> + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN<br>
> + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)<br>
> + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE<br>
> + * POSSIBILITY OF SUCH DAMAGE.<br>
> + */<br>
> +<br>
> +#ifndef _DEV_CAN_CAN_QUEUE_H<br>
> +#define _DEV_CAN_CAN_QUEUE_H<br>
> +<br>
> +#include <rtems/imfs.h><br>
> +#include <rtems/thread.h><br>
> +<br>
> +#include <string.h><br>
> +#include <stdlib.h><br>
> +#include <stdio.h><br>
> +<br>
> +#include <dev/can/can-msg.h><br>
> +#include <dev/can/can.h><br>
> +<br>
> +static rtems_status_code can_create_tx_buffers(struct can_bus *bus);<br>
> +static rtems_status_code can_create_rx_buffers(struct can_bus *bus);<br>
> +static bool can_tx_buf_isempty(struct can_bus *bus);<br>
> +static struct can_msg *can_tx_get_data_buf(struct can_bus *bus);<br>
> +static struct can_msg *can_tx_get_empty_buf(struct can_bus *bus);<br>
> +<br>
> +static rtems_status_code can_create_rx_buffers(struct can_bus *bus)<br>
> +{<br>
> + return rtems_message_queue_create(rtems_build_name('c', 'a', 'n', bus->index),<br>
> + CAN_RX_BUF_COUNT, sizeof(struct can_msg),<br>
> + RTEMS_FIFO | RTEMS_LOCAL, &bus->rx_queue_id);<br>
> +}<br>
> +<br>
> +static rtems_status_code can_create_tx_buffers(struct can_bus *bus)<br>
> +{<br>
> + bus->tx_fifo.pbuf = (struct can_msg *)malloc(CAN_TX_BUF_COUNT *<br>
> + sizeof(struct can_msg));<br>
> + if (bus->tx_fifo.pbuf == NULL) {<br>
> + printf("can_create_tx_buffers: malloc failed\n");<br>
> + return RTEMS_NO_MEMORY;<br>
> + }<br>
> +<br>
> + bus->tx_fifo.empty_count = CAN_TX_BUF_COUNT;<br>
> +<br>
> + return RTEMS_SUCCESSFUL;<br>
> +}<br>
> +<br>
> +static bool can_tx_buf_isempty(struct can_bus *bus)<br>
> +{<br>
> + if (bus->tx_fifo.empty_count == 0) {<br>
> + return false;<br>
> + }<br>
> +<br>
> + return true;<br>
> +}<br>
> +<br>
> +// TODO: free the given data buf is done in the same function<br>
> +// SO the returned buffer should be consumed before releasing the<br>
> +// lock acquired while calling this function.<br>
> +static struct can_msg *can_tx_get_data_buf(struct can_bus *bus)<br>
> +{<br>
> + struct can_msg *msg = NULL;<br>
> +<br>
> + if (bus->tx_fifo.empty_count == CAN_TX_BUF_COUNT) {<br>
> + CAN_DEBUG_BUF("can_tx_get_data_buf: All buffers are empty\n");<br>
> + return NULL;<br>
> + }<br>
> +<br>
> + msg = &bus->tx_fifo.pbuf[bus->tx_fifo.tail];<br>
> + bus->tx_fifo.empty_count++;<br>
> + bus->tx_fifo.tail = (bus->tx_fifo.tail + 1) % CAN_TX_BUF_COUNT;<br>
> +<br>
> + return msg;<br>
> +}<br>
> +<br>
> +// TODO: marking the given buf as produced is done in the same function<br>
> +// So the returned buffer should be produced before releasing the<br>
> +// lock acquired while calling this function.<br>
> +static struct can_msg *can_tx_get_empty_buf(struct can_bus *bus)<br>
> +{<br>
> + struct can_msg *msg = NULL;<br>
> +<br>
> + /* Check whether there is a empty CAN msg buffer */<br>
> + if (can_tx_buf_isempty(bus) == false) {<br>
> + CAN_DEBUG_BUF("can_tx_get_empty_buf: No empty buffer\n");<br>
> + return NULL;<br>
> + }<br>
> +<br>
> + bus->tx_fifo.empty_count--;<br>
> +<br>
> + /* tx_fifo.head always points to a empty buffer if there is atleast one */<br>
> + msg = &bus->tx_fifo.pbuf[bus->tx_fifo.head];<br>
> + bus->tx_fifo.head = (bus->tx_fifo.head + 1) % CAN_TX_BUF_COUNT;<br>
> +<br>
> + return msg;<br>
> +}<br>
> +<br>
> +#endif /*_DEV_CAN_CAN_QUEUE_H */<br>
> diff --git a/cpukit/include/dev/can/can.h b/cpukit/include/dev/can/can.h<br>
> new file mode 100644<br>
> index 0000000000..eeb2eb2c0b<br>
> --- /dev/null<br>
> +++ b/cpukit/include/dev/can/can.h<br>
> @@ -0,0 +1,102 @@<br>
> +/* SPDX-License-Identifier: BSD-2-Clause */<br>
> +<br>
> +/**<br>
> + * @file<br>
> + *<br>
> + * @ingroup CANBus<br>
> + *<br>
> + * @brief Controller Area Network (CAN) Bus Implementation<br>
> + *<br>
> + */<br>
> +<br>
> +/*<br>
> + * Copyright (C) 2022 Prashanth S (<a href="mailto:fishesprashanth@gmail.com" target="_blank">fishesprashanth@gmail.com</a>)<br>
> + *<br>
> + * Redistribution and use in source and binary forms, with or without<br>
> + * modification, are permitted provided that the following conditions<br>
> + * are met:<br>
> + * 1. Redistributions of source code must retain the above copyright<br>
> + * notice, this list of conditions and the following disclaimer.<br>
> + * 2. Redistributions in binary form must reproduce the above copyright<br>
> + * notice, this list of conditions and the following disclaimer in the<br>
> + * documentation and/or other materials provided with the distribution.<br>
> + *<br>
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"<br>
> + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE<br>
> + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE<br>
> + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE<br>
> + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR<br>
> + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF<br>
> + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS<br>
> + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN<br>
> + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)<br>
> + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE<br>
> + * POSSIBILITY OF SUCH DAMAGE.<br>
> + */<br>
> +<br>
> +#ifndef _DEV_CAN_CAN_H<br>
> +#define _DEV_CAN_CAN_H<br>
> +<br>
> +#include <rtems/imfs.h><br>
> +#include <rtems/thread.h><br>
> +#include <semaphore.h><br>
> +<br>
> +#include <string.h><br>
> +#include <stdlib.h><br>
> +#include <stdio.h><br>
> +<br>
> +#include <dev/can/can-msg.h><br>
> +<br>
> +#define CAN_DEBUG(str, ...) printf(str, ##__VA_ARGS__)<br>
> +#define CAN_DEBUG_BUF(str, ...) printf(str, ##__VA_ARGS__)<br>
> +#define CAN_DEBUG_ISR(str, ...) printf(str, ##__VA_ARGS__)<br>
> +#define CAN_DEBUG_LOCK(str, ...) printf(str, ##__VA_ARGS__)<br>
> +#define CAN_DEBUG_RX(str, ...) printf(str, ##__VA_ARGS__)<br>
> +#define CAN_DEBUG_TX(str, ...) printf(str, ##__VA_ARGS__)<br>
<br>
Do these really have to be in a header that you (most likely) will later <br>
install? With that they are public visible API.<br>
<br>
Best regards<br>
<br>
Christian<br>
<br>
> +<br>
> +struct ring_buf {<br>
> + struct can_msg *pbuf;<br>
> + uint32_t head;<br>
> + uint32_t tail;<br>
> + uint32_t empty_count;<br>
> +};<br>
> +<br>
> +typedef struct can_dev_ops {<br>
> + int32_t (*dev_rx)(void *priv, void *buffer);<br>
> + int32_t (*dev_tx)(void *priv, void *buffer);<br>
> + bool (*dev_tx_ready)(void *priv);<br>
> + void (*dev_int)(void *priv, bool);<br>
> + int32_t (*dev_ioctl)(void *priv, void *buffer, size_t cmd);<br>
> +} can_dev_ops;<br>
> +<br>
> +typedef struct can_bus {<br>
> + void *priv; // CAN controller specific struct.<br>
> + uint8_t index;<br>
> + struct can_dev_ops *can_dev_ops; // CAN controller operations.<br>
> +<br>
> + struct ring_buf tx_fifo; // TX fifo<br>
> + rtems_id tx_fifo_sem_id; // TX fifo counting semaphore id<br>
> + uint32_t can_tx_buf_waiters;<br>
> +<br>
> + rtems_id rx_queue_id;<br>
> + uint32_t rx_queue_name;<br>
> +<br>
> + rtems_recursive_mutex mutex; // For handling driver concurrency.<br>
> + RTEMS_INTERRUPT_LOCK_MEMBER(can_bus_lock);<br>
> +<br>
> + void (*destroy)(struct can_bus *bus); // Clean the CAN controller specific resources.<br>
> +} can_bus;<br>
> +<br>
> +extern rtems_status_code can_bus_register(can_bus *bus, const char *bus_path);<br>
> +<br>
> +extern can_bus *can_bus_alloc_and_init(size_t size);<br>
> +extern int can_bus_init(can_bus *bus);<br>
> +extern int can_tx_done(struct can_bus *);<br>
> +extern int can_receive(struct can_bus *, struct can_msg *);<br>
> +<br>
> +extern uint16_t can_len_to_dlc(uint16_t len);<br>
> +extern uint16_t can_dlc_to_len(uint16_t dlc);<br>
> +<br>
> +extern void can_print_msg(struct can_msg const *);<br>
> +<br>
> +#endif /* _DEV_CAN_CAN_H */<br>
> diff --git a/spec/build/cpukit/librtemscpu.yml b/spec/build/cpukit/librtemscpu.yml<br>
> index 31a68cf85a..b7f6c0457c 100644<br>
> --- a/spec/build/cpukit/librtemscpu.yml<br>
> +++ b/spec/build/cpukit/librtemscpu.yml<br>
> @@ -50,6 +50,11 @@ install:<br>
> - destination: ${BSP_INCLUDEDIR}/dev/spi<br>
> source:<br>
> - cpukit/include/dev/spi/spi.h<br>
> +- destination: ${BSP_INCLUDEDIR}/dev/can<br>
> + source:<br>
> + - cpukit/include/dev/can/can.h<br>
> + - cpukit/include/dev/can/can-msg.h<br>
> + - cpukit/include/dev/can/can-queue.h<br>
> - destination: ${BSP_INCLUDEDIR}/linux<br>
> source:<br>
> - cpukit/include/linux/i2c-dev.h<br>
> @@ -530,6 +535,7 @@ source:<br>
> - cpukit/dev/serial/sc16is752-spi.c<br>
> - cpukit/dev/serial/sc16is752.c<br>
> - cpukit/dev/spi/spi-bus.c<br>
> +- cpukit/dev/can/can.c<br>
> - cpukit/dtc/libfdt/fdt.c<br>
> - cpukit/dtc/libfdt/fdt_addresses.c<br>
> - cpukit/dtc/libfdt/fdt_empty_tree.c<br>
</blockquote></div></div>