<div dir="ltr"><div dir="ltr"><div>Hi Christian,</div><div><br></div><div>This is to reply to review comments.</div><div><br></div><div><span style="color:rgb(153,0,255)">> first question: You also worked on a driver for beagle DCAN. Did you <br>
> already adapt that driver to this API? If yes, it would be usefull to <br>> post that as a patch too so that the direction and the method how it <br>
> will be used is more clear.<br></span><div style="margin-left:40px"><span style="color:rgb(153,0,255)">
</span><font color="#000000">Yes, Waiting for License confirmation.</font></div><div style="margin-left:40px"><br></div>
<span style="color:rgb(153,0,255)">> Note that some of my comments might are a bit critical because you add a <br>
> general API. I would have a lot less problems if it would be only a <br>
> driver specific API. If you add a general API, it has to fit the needs <br>
> of not only your current driver but most future drivers too. Changing an <br>
> API later is allways tricky.</span></div><div style="margin-left:40px"><span style="color:rgb(153,0,255)"><font color="#000000">Ok</font></span></div><div style="margin-left:40px"><span style="color:rgb(153,0,255)"><br></span></div><span style="color:rgb(153,0,255)">> Please make sure to stic to the RTEMS style and use a 80 character line <br></span><div><span style="color:rgb(153,0,255)">
> length.</span></div><div style="margin-left:40px"><span style="color:rgb(153,0,255)"><font color="#000000">OK</font></span></div><div style="margin-left:40px"><span style="color:rgb(153,0,255)"><br></span></div><span style="color:rgb(153,0,255)">> You create a name with a "'0' + init++": Are you sure that a maximum of <br>
> 10 buffers are created? Otherwise you will get names like "can:", "can;" <br></span><div><span style="color:rgb(153,0,255)">
> and - sooner or later: "can\xFF", "can\x00", ...</span></div><div style="margin-left:40px"><span style="color:rgb(153,0,255)"><font color="#000000">Assuming we have less than 10 CAN controllers, I will update this</font> <font color="#000000">to limit to 10 queues.</font></span></div><div style="margin-left:40px"><span style="color:rgb(153,0,255)"><br></span></div><span style="color:rgb(153,0,255)">> In the header you somewhere had a "destroy" function. Do you need some <br>> way to de-initialize buffers only used by that bus?<span class="gmail-im"><br></span></span><div style="margin-left:40px"><font color="#000000">Yes, the can_bus structure holds data related only to a specific bus.</font></div><div style="margin-left:40px"><br></div><div style="margin-left:40px"><span style="color:rgb(153,0,255)"><span class="gmail-im"></span></span></div><span style="color:rgb(153,0,255)"><span class="gmail-im">
>> +}<br>
>> +<br>
>> +rtems_status_code can_create_tx_buffers(struct can_bus *bus)<br>
>> +{<br>
>> + if ((bus->tx_fifo.pbuf = (struct can_msg *)malloc(CAN_TX_BUF_COUNT * sizeof(struct can_msg))) == NULL) {<br>
<br></span>
> You seem to like writing a lot of code in one line. From my point of <br>
> view that makes code harder to read. If you write it into multiple lines <br>
> instead, someone else doesn't have to think that much about which <br>
> bracket closes where. For example in this case the following would be <br></span><div><span style="color:rgb(153,0,255)">
> simpler to read:<br></span></div><div style="margin-left:40px">I will make changes to limit line length to 80.<br></div><div style="margin-left:40px"><span style="color:rgb(153,0,255)"></span></div><div><span style="color:rgb(153,0,255)"></span></div><span style="color:rgb(153,0,255)">
<br>
> ====<span class="gmail-im"><br>
> bus->tx_fifo.pbuf = (struct can_msg *)malloc(CAN_TX_BUF_COUNT * <br></span>
> sizeof(struct can_msg));<br>
> if (bus->tx_fifo.pbuf == NULL) {<br>> ....<br>
> }<br>
> ====<span class="gmail-im"><br>
><br>
> + printf("malloc failed\n");<br>
><br></span>
> Prints are nice for debugging but can be really annoying in <br>
> applications. Think about some kind of "debug_print" or similar that you <br>
> can enable / disable with a macro at the start of the file. You can also <br>
> use that to add a prefix or function name to all prints. If you search a <br>
> problem, a line like "mallof failed" is meaningless if you are not <br>
> currently working on specially this code. A line like <br>
> "can_create_tx_buffers: malloc failed" is a lot more usefull. There are <br>
> preprocessor macros like __FUNCTION__ that you can use for something <br>
> like that.<span class="gmail-im"><br></span></span><div style="margin-left:40px"><font color="#000000">Ok, I will update the prints with the function name in it.</font></div><div style="margin-left:40px"><br></div><div style="margin-left:40px"><span style="color:rgb(153,0,255)"><span class="gmail-im"></span></span></div><span style="color:rgb(153,0,255)"><span class="gmail-im">
> + return RTEMS_NO_MEMORY;<br>
> + }<br>
> +<br>
> + bus->tx_fifo.head = bus->tx_fifo.tail = 0;<br>
> <br></span>> Same here: This is hard to read. I would suggest to split that into two <br>
> lines. Alternatively you might want to think about just using a memset <br>
> to have a clean start for a structure that you initialize.<span class="gmail-im"><br></span></span><div style="margin-left:40px">Ok, memset is done in the can_bus_init or calloc is used in can_bus_alloc_and_init.</div><div style="margin-left:40px">Removing <span style="color:rgb(0,0,0)"><span class="gmail-im">bus->tx_fifo.head = bus->tx_fifo.tail = 0;</span></span></div><div style="margin-left:40px"><br></div><div style="margin-left:40px"><span style="color:rgb(153,0,255)"><span class="gmail-im"></span></span></div><span style="color:rgb(153,0,255)"><span class="gmail-im">
> + bus->tx_fifo.empty_count = CAN_TX_BUF_COUNT;<br>
> +<br>
> + return 0;<br>
> +}<br>
> +<br>
> +bool can_tx_buf_isempty(struct can_bus *bus)<br>
> +{<br>
> + if (bus->tx_fifo.empty_count <= 0) {<br>
> + /* tx_fifo.empty_count does not go below zero, incase if it goes update to zero */<br>
> + bus->tx_fifo.empty_count = 0;<br>
<br></span>
empty_count is an unsigned type and therefore can never be smaller than <br>
0. This line is not necessary at all.<span class="gmail-im"><br></span></span><div style="margin-left:40px">Ok, removing it.<br></div><div style="margin-left:40px"><span style="color:rgb(153,0,255)"><span class="gmail-im"></span></span></div><span style="color:rgb(153,0,255)"><span class="gmail-im">
<br>
> +<br>
> + return false;<br>
> + }<br>
> +<br>
> + return true;<br>
> +}<br>
> +<br>
> +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>
> + printf("can_tx_get_next_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></span>
> You want to return the pointer to the message (msg) to the caller, <br>
> right? In that case you must not marke it as empty here. Otherwise <br>
> someone else could already use that memory again while the application <br></span><div><span style="color:rgb(153,0,255)">
> is still processing it.</span></div><div style="margin-left:40px"><span style="color:rgb(153,0,255)"><font color="#000000">Here the application does not use this (driver buffer) buffer, we copy the message from this buffer (driver buffer)</font></span></div><div style="margin-left:40px"><span style="color:rgb(153,0,255)"><font color="#000000">to the application passed buffer.</font><br></span></div><div style="margin-left:40px"><span style="color:rgb(153,0,255)"><br></span></div><div><div class="gmail-adm"><div id="gmail-q_30" class="gmail-ajR gmail-h4"><div class="gmail-ajT"></div></div></div></div>
<span style="color:rgb(153,0,255)">> Looks like a helper function that is only used in this file? Please make <br>
> it static and remove it from the header. Same is true for "take_sem" and <br></span><div><span style="color:rgb(153,0,255)">
> similar below.</span><span style="color:rgb(153,0,255)"> <br></span></div><div style="margin-left:40px"><span style="color:rgb(153,0,255)"><font color="#000000">Functions in can-queue.c are used in can.c, Moving them to can-queue.h or can.h header file.<br></font></span></div><div style="margin-left:40px"><span style="color:rgb(153,0,255)"><font color="#000000">I will add static to take_sem and give_sem.</font></span></div><div style="margin-left:40px"><span style="color:rgb(153,0,255)"><br></span></div><div><span style="color:rgb(153,0,255)">> Is an interrupt lock really necessary? Wouldn't a mutex or similar work </span></div><div><span style="color:rgb(153,0,255)"></span></div><span style="color:rgb(153,0,255)">
> too to protect the relevant data? An interrupt lock is something that <br>
> should be used really carefull. You tell the system: Stop everything <br>
> else. This here is the most important task.<br>
> <br>
> That means that for example a time critical data acquisition process can <br>
> be delayed by the time that you need in your interrupt lock. You have to <br>
> be really sure that the interrupt is the only solution to solve the <br></span><div><span style="color:rgb(153,0,255)">> problem before you use one.<span class="gmail-im"><br></span></span></div><div style="margin-left:40px">Yes, We need to disable interrupts as the tx and rx buffers are used in the tx and rx interrupts handlers.</div><div style="margin-left:40px">Or could disable only CAN interrupts, as CAN messages being a higher priority so I added a Global interrupt disable.</div><div style="margin-left:40px">Shall I disable it locally or globally?<br></div><div style="margin-left:40px"><span style="color:rgb(153,0,255)"><span class="gmail-im"></span></span></div><div><span style="color:rgb(153,0,255)"><span class="gmail-im"></span></span></div><span style="color:rgb(153,0,255)"><span class="gmail-im">
<br>
>> +<br>
>> + ret = bus->can_dev_ops->dev_tx_ready(bus->priv);<br>
>> +<br>
>> + if (ret != true) {<br>
>> + goto return_with_lock_release;<br>
>> + }<br>
>> +<br>
>> + msg = can_tx_get_data_buf(bus);<br>
>> +<br>
>> + if (msg == NULL) {<br></span></span><div><span style="color:rgb(153,0,255)"><span class="gmail-im">>> + goto return_with_lock_release;</span></span></div><div><span style="color:rgb(153,0,255)"><span class="gmail-im"></span></span></div><span style="color:rgb(153,0,255)"><span class="gmail-im">
>> + }<br>
>> +<br>
>> + ret = bus->can_dev_ops->dev_tx(bus->priv, msg);<br>
><br></span>
> You have an interrupt lock here. Is "dev_tx" a function that is <br>
> guaranteed to be fast enough not to make problems with that?<span class="gmail-im"><br></span></span><div style="margin-left:40px"><font color="#000000">This function copies drivers buffers to the controller's hardware buffer. Time taken is based on the</font></div><div style="margin-left:40px"><font color="#000000">message size.</font></div><div style="margin-left:40px"><br></div><div style="margin-left:40px"><span style="color:rgb(153,0,255)"><span class="gmail-im"></span></span></div><span style="color:rgb(153,0,255)"><span class="gmail-im">
>> +<br>
>> + if (ret != RTEMS_SUCCESSFUL) {<br>
>> + printf("dev_send failed\n");<br>
><br></span>
> You have an interrupt lock here. You can't use a printf because that <br>
> uses interrupts.<span class="gmail-im"><br></span></span><div style="margin-left:40px">I will print after releasing the interrupt lock.</div><div style="margin-left:40px"><br></div><div style="margin-left:40px"><span style="color:rgb(153,0,255)"><span class="gmail-im"></span></span></div><span style="color:rgb(153,0,255)"><span class="gmail-im">
>> + }<br>
>> +<br>
>> + rtems_interrupt_lock_release(&bus->can_bus_lock, &lock_context);<br>
>> +<br>
>> + //can_tx_done(bus);<br>
>> + }<br>
>> +<br>
>> + return RTEMS_SUCCESSFUL;<br>
>> +<br>
>> +return_with_lock_release:<br>
>> +<br>
>> + rtems_interrupt_lock_release(&bus->can_bus_lock, &lock_context);<br>
>> + return RTEMS_SUCCESSFUL;<br>
>> +}<br>
>> +<br>
>> +void can_print_canmsg(struct can_msg const *msg)<br>
><br></span>
> I assume that's a debug only function. Otherwise I wouldn't be happy <br></span><div><span style="color:rgb(153,0,255)">
> with the output format.</span><span style="color:rgb(153,0,255)"> <br></span></div><div><div style="margin-left:40px">Yes, can_print_message is for debug purposes only.</div><span style="color:rgb(153,0,255)"></span></div><div><span style="color:rgb(153,0,255)"><br></span></div><div><span style="color:rgb(153,0,255)">> Same as above: Is an interrupt lock really necessary and the only solution?</span></div><div style="margin-left:40px">Yes, We need to disable interrupts as the tx and rx buffers are used in the tx and rx interrupts handlers.</div><div style="margin-left:40px">Or could disable only CAN interrupts, as CAN messages being a higher priority so I added a Global interrupt disable.</div><div style="margin-left:40px">Shall I disable it locally or globally?</div><div><div style="margin-left:40px"><br></div><div style="margin-left:40px"><span style="color:rgb(153,0,255)"><span class="gmail-im"></span></span></div><span style="color:rgb(153,0,255)"><span class="gmail-im">
>> +<br>
>> + fifo_buf = can_tx_get_empty_buf(bus);<br>
>> +<br>
>> + if (fifo_buf != NULL) {<br>
>> + uint32_t msg_size = (char *)&msg->data[msg->len] - (char *)msg;<br>
>> +<br>
>> + if (msg_size > sizeof(struct can_msg)) {<br>
>> + printf("can message len error msg_size = %u struct can_msg = %u\n", msg_size, sizeof(struct can_msg));<br>
><br></span>
> Again: Printf will not work during an interrupt lock.<span class="gmail-im"><br></span></span><div style="margin-left:40px">I will modify that.</div><div style="margin-left:40px"><br></div><div style="margin-left:40px"><span style="color:rgb(153,0,255)"><span class="gmail-im"></span></span></div><span style="color:rgb(153,0,255)"><span class="gmail-im">
>> + return -RTEMS_INVALID_SIZE;<br>
>> + }<br>
>> +<br>
>> + memcpy(fifo_buf, msg, msg_size); //sizeof(struct can_msg) - (CAN_MSG_MAX_SIZE - msg->len));<br>
><br></span>
> memcpy() in an interrupt lock is definitively takeing too much time. You <br>> have to be really convincing that I agree that it's OK.</span><span style="color:rgb(153,0,255)">
If the "isempty" function needs an interupt_lock: Shouldn't it be part <br>
> of the function?<br>
><br>
> And again: Wouldn't a mutex work to protect the data instead of <br>
> interrupt locks?</span><span style="color:rgb(153,0,255)">
Why is the label named "...release_lock"? It isn't releasing any lock.</span><span style="color:rgb(153,0,255)">
The RTEMS_INTERRUPT_LOCK_INITIALIZER is for a static initialization of <br></span><div><span style="color:rgb(153,0,255)">
> an interrupt lock. It won't work to initialize it dynamically here.</span><span style="color:rgb(153,0,255)"> <br></span></div><span style="color:rgb(153,0,255)"></span><div style="margin-left:40px">Yes, We need to disable interrupts as the tx and rx buffers are used in the tx and rx interrupts handlers.</div><div style="margin-left:40px">Or could disable only CAN interrupts, as CAN messages being a higher priority I added a Global interrupt disable.</div><div style="margin-left:40px">Shall I disable it locally or globally?</div><div style="margin-left:40px"><br></div><div style="margin-left:40px">Changing <span style="color:rgb(0,0,0)">RTEMS_INTERRUPT_LOCK_INITIALIZER </span>to rtems_interrupt_lock_initialize.</div><div style="margin-left:40px"><br></div><div><div><span style="color:rgb(153,0,255)">> A more or less general note for the header file: You want to introduce </span></div><div><span style="color:rgb(153,0,255)"></span></div><span style="color:rgb(153,0,255)">
> it as a new general API. Please make sure to document that API enough. <br>
> Use doxygen-comments in the header file for that. For an example, take a <br>
> look at cpukit/include/linux/i2c.h or some other headers. Documentation <br>
> is often really annoying to write as a developer but at the same time <br>
> it's really convenient to have it for all other developers.<br></span><div style="margin-left:40px"><font color="#000000">Ok, I will add the doxygen style comments.</font></div><div style="margin-left:40px"><br></div><div style="margin-left:40px"><span style="color:rgb(153,0,255)"></span></div><span style="color:rgb(153,0,255)">
> For me it's also not really clear whether you want to create a user <br>
> facing API with this file or a driver facing one.<span class="gmail-im"><br>
><br>
>> +#define CAN_MSG_MAX_SIZE (8u)<br>
>> +<br>
>> +#define CAN_TX_BUF_COUNT (10u)<br>
>> +#define CAN_RX_BUF_COUNT (10u)<br>
><br></span>
> Is that really a value that does match the needs of all drivers / <br>
> applications?</span><span style="color:rgb(153,0,255)"> <br></span></div><div style="margin-left:40px"><span style="color:rgb(153,0,255)"><font color="#000000">Based on the suggestions, need to decide a suitable count.</font></span></div><div style="margin-left:40px"><span style="color:rgb(153,0,255)"><br></span></div><div><span style="color:rgb(153,0,255)">> What's the advantage of implementing two types of semaphores depending <br>
> on the _POSIX_SEM_ macro instead of just using only one RTEMS-native type?<br></span></div><div><div style="margin-left:40px"><font color="#000000">That I used for debug purposes, as at first rtems_semaphore_release was not working. So to test whether POSIX works I used that.</font></div><div style="margin-left:40px"><font color="#000000">Removing POSIX API.</font></div><div style="margin-left:40px"><br></div><div style="margin-left:40px"><span style="color:rgb(153,0,255)"></span></div><span style="color:rgb(153,0,255)">
> I haven't had a detailed look at the useage of the semaphores yet. <br>
> Personally I prefer the self-contained objects because they have the <br>
> advantage that the application-developers don't have to think about them<br>> when configuring their application:<br>
><br>
><br>
<a href="https://docs.rtems.org/branches/master/c-user/self_contained_objects.html#" rel="noreferrer" target="_blank"><span style="color:rgb(153,0,255)">> </span>https://docs.rtems.org/branches/master/c-user/self_contained_objects.html#</a><br>
></span></div><div><span style="color:rgb(153,0,255)">
> Not sure whether that would be an option here.</span><span style="color:rgb(153,0,255)"> <br></span></div><div style="margin-left:40px"><span style="color:rgb(153,0,255)"><font color="#000000">Ok, I will look into the link.</font></span></div><div style="margin-left:40px"><span style="color:rgb(153,0,255)"><br></span></div><div><span style="color:rgb(153,0,255)">> Some of the functions seem to be internal functions that are only used <br>
> in the framework (especially the ones with *_sem() but also the whole <br>
> create*buffers() stuff). You shouldn't expose them to the user. Make <br>
> them static or put them into some internal header.</span></div><div style="margin-left:40px">Moving them to can-queue.h or can.h header file.</div><div style="margin-left:40px"><br></div>Regards</div><div>Prashanth S<br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, 19 Jul 2022 at 14:47, <<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 Prashanth,<br>
<br>
first question: You also worked on a driver for beagle DCAN. Did you <br>
already adapt that driver to this API? If yes, it would be usefull to <br>
post that as a patch too so that the direction and the method how it <br>
will be used is more clear.<br>
<br>
Note that some of my comments might are a bit critical because you add a <br>
general API. I would have a lot less problems if it would be only a <br>
driver specific API. If you add a general API, it has to fit the needs <br>
of not only your current driver but most future drivers too. Changing an <br>
API later is allways tricky.<br>
<br>
Am 15.07.22 um 20:11 schrieb Prashanth S:<br>
> ---<br>
> cpukit/dev/can/can-queue.c | 112 +++++++<br>
> cpukit/dev/can/can.c | 480 ++++++++++++++++++++++++++++++<br>
> cpukit/include/dev/can/can.h | 115 +++++++<br>
> spec/build/cpukit/librtemscpu.yml | 5 +<br>
> 4 files changed, 712 insertions(+)<br>
> create mode 100644 cpukit/dev/can/can-queue.c<br>
> create mode 100644 cpukit/dev/can/can.c<br>
> create mode 100644 cpukit/include/dev/can/can.h<br>
> <br>
> diff --git a/cpukit/dev/can/can-queue.c b/cpukit/dev/can/can-queue.c<br>
> new file mode 100644<br>
> index 0000000000..1cebed2ca4<br>
> --- /dev/null<br>
> +++ b/cpukit/dev/can/can-queue.c<br>
> @@ -0,0 +1,112 @@<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<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.h><br>
> +<br>
> +#include <string.h><br>
> +#include <stdlib.h><br>
> +#include <stdio.h><br>
> +<br>
> +rtems_status_code can_create_rx_buffers(struct can_bus *bus)<br>
> +{<br>
> + static int init = 0;<br>
> +<br>
> + return rtems_message_queue_create(rtems_build_name('c', 'a', 'n', '0' + init++), CAN_RX_BUF_COUNT, sizeof(struct can_msg),<br>
> + RTEMS_FIFO | RTEMS_LOCAL, &bus->rx_queue_id);<br>
<br>
Please make sure to stic to the RTEMS style and use a 80 character line <br>
length.<br>
<br>
You create a name with a "'0' + init++": Are you sure that a maximum of <br>
10 buffers are created? Otherwise you will get names like "can:", "can;" <br>
and - sooner or later: "can\xFF", "can\x00", ...<br>
<br>
In the header you somewhere had a "destroy" function. Do you need some <br>
way to de-initialize buffers only used by that bus?<br>
<br>
> +}<br>
> +<br>
> +rtems_status_code can_create_tx_buffers(struct can_bus *bus)<br>
> +{<br>
> + if ((bus->tx_fifo.pbuf = (struct can_msg *)malloc(CAN_TX_BUF_COUNT * sizeof(struct can_msg))) == NULL) {<br>
<br>
You seem to like writing a lot of code in one line. From my point of <br>
view that makes code harder to read. If you write it into multiple lines <br>
instead, someone else doesn't have to think that much about which <br>
bracket closes where. For example in this case the following would be <br>
simpler to read:<br>
<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>
....<br>
}<br>
====<br>
<br>
> + printf("malloc failed\n");<br>
<br>
Prints are nice for debugging but can be really annoying in <br>
applications. Think about some kind of "debug_print" or similar that you <br>
can enable / disable with a macro at the start of the file. You can also <br>
use that to add a prefix or function name to all prints. If you search a <br>
problem, a line like "mallof failed" is meaningless if you are not <br>
currently working on specially this code. A line like <br>
"can_create_tx_buffers: malloc failed" is a lot more usefull. There are <br>
preprocessor macros like __FUNCTION__ that you can use for something <br>
like that.<br>
<br>
> + return RTEMS_NO_MEMORY;<br>
> + }<br>
> +<br>
> + bus->tx_fifo.head = bus->tx_fifo.tail = 0;<br>
<br>
Same here: This is hard to read. I would suggest to split that into two <br>
lines. Alternatively you might want to think about just using a memset <br>
to have a clean start for a structure that you initialize.<br>
<br>
> + bus->tx_fifo.empty_count = CAN_TX_BUF_COUNT;<br>
> +<br>
> + return 0;<br>
> +}<br>
> +<br>
> +bool can_tx_buf_isempty(struct can_bus *bus)<br>
> +{<br>
> + if (bus->tx_fifo.empty_count <= 0) {<br>
> + /* tx_fifo.empty_count does not go below zero, incase if it goes update to zero */<br>
> + bus->tx_fifo.empty_count = 0;<br>
<br>
empty_count is an unsigned type and therefore can never be smaller than <br>
0. This line is not necessary at all.<br>
<br>
<br>
> +<br>
> + return false;<br>
> + }<br>
> +<br>
> + return true;<br>
> +}<br>
> +<br>
> +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>
> + printf("can_tx_get_next_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>
You want to return the pointer to the message (msg) to the caller, <br>
right? In that case you must not marke it as empty here. Otherwise <br>
someone else could already use that memory again while the application <br>
is still processing it.<br>
<br>
> +<br>
> + return msg;<br>
> +}<br>
> +<br>
> +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>
> + printf("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>
> diff --git a/cpukit/dev/can/can.c b/cpukit/dev/can/can.c<br>
> new file mode 100644<br>
> index 0000000000..e8aa9d20ca<br>
> --- /dev/null<br>
> +++ b/cpukit/dev/can/can.c<br>
> @@ -0,0 +1,480 @@<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<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.h><br>
> +<br>
> +#include <string.h><br>
> +#include <stdlib.h><br>
> +<br>
> +#include <fcntl.h><br>
> +<br>
> +static ssize_t can_bus_open(rtems_libio_t *iop, const char *path, int oflag, mode_t mode);<br>
> +static ssize_t can_bus_read(rtems_libio_t *iop, void *buffer, size_t count);<br>
> +static ssize_t can_bus_write(rtems_libio_t *iop, const void *buffer, size_t count);<br>
> +static ssize_t can_bus_ioctl(rtems_libio_t *iop, ioctl_command_t request, void *buffer);<br>
> +static int can_xmit(struct can_bus *bus);<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>
> +static ssize_t can_bus_open(rtems_libio_t *iop, const char *path, int oflag, mode_t mode)<br>
> +{<br>
> +/*<br>
> + static int init = 0;<br>
> +<br>
> + if (init == 1) {<br>
> + return 0;<br>
> + }<br>
> +<br>
> + init = 1;<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>
> + can_create_sem(bus);<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>
> + if ((ret = rtems_message_queue_broadcast(bus->rx_queue_id, msg, sizeof(struct can_msg) - (CAN_MSG_MAX_SIZE - msg->len), &count)) != RTEMS_SUCCESSFUL) {<br>
> + printf("rtems_message_queue_send failed\n");<br>
> + }<br>
> +<br>
> + printf("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>
> + if ((ret = rtems_message_queue_receive(bus->rx_queue_id, (void *)msg, &count, flags, 0)) != RTEMS_SUCCESSFUL) {<br>
> + return ret;<br>
> + }<br>
> +<br>
> + return count;<br>
> +}<br>
> +<br>
> +int can_create_sem(struct can_bus *bus)<br>
<br>
Looks like a helper function that is only used in this file? Please make <br>
it static and remove it from the header. Same is true for "take_sem" and <br>
similar below.<br>
<br>
> +{<br>
> + int ret = 0;<br>
> +<br>
> +#ifndef _POSIX_SEM_<br>
> + //TODO: for test purpose<br>
> + static int init = 0;<br>
> +<br>
> + ret = rtems_semaphore_create(rtems_build_name('c', 'a', 'n', '0' + init++), 1,<br>
> + RTEMS_FIFO | RTEMS_SIMPLE_BINARY_SEMAPHORE | RTEMS_LOCAL, 0, &bus->tx_fifo_sem_id);<br>
> +<br>
> + if (ret != 0) {<br>
> + printf("rtems_semaphore_create failed %d\n", ret);<br>
> + return ret;<br>
> + }<br>
> +<br>
> + ret = rtems_semaphore_obtain(bus->tx_fifo_sem_id, RTEMS_WAIT, RTEMS_NO_TIMEOUT);<br>
> +<br>
> + if (ret != 0) {<br>
> + printf("rtems_semaphore_obtain failed %d\n", ret);<br>
> + return ret;<br>
> + }<br>
> +#else<br>
> + ret = sem_init(&bus->tx_sem_id, 0, 1);<br>
> +<br>
> + if (ret != 0) {<br>
> + printf("sem_init failed %d\n", ret);<br>
> + return ret;<br>
> + }<br>
> +#endif /* _POSIX_SEM_ */<br>
> + return ret;<br>
> +}<br>
> +<br>
> +int take_sem(struct can_bus *bus)<br>
> +{<br>
> + int ret = 0;<br>
> +<br>
> +#ifndef _POSIX_SEM_<br>
> + ret = rtems_semaphore_obtain(bus->tx_fifo_sem_id, RTEMS_WAIT, RTEMS_NO_TIMEOUT);<br>
> +#else<br>
> + ret = sem_wait(&bus->tx_sem_id);<br>
> +#endif /* _POSIX_SEM_ */<br>
> +<br>
> + return ret;<br>
> +}<br>
> +<br>
> +int give_sem(struct can_bus *bus)<br>
> +{<br>
> + int ret = 0;<br>
> +<br>
> +#ifndef _POSIX_SEM_<br>
> + ret = rtems_semaphore_release(bus->tx_fifo_sem_id);<br>
> +#else<br>
> + ret = sem_post(&bus->tx_sem_id);<br>
> +#endif /* _POSIX_SEM_ */<br>
> +<br>
> + return ret;<br>
> +}<br>
> +<br>
> +static int can_xmit(struct can_bus *bus)<br>
> +{<br>
> + int ret = 0;<br>
> +<br>
> + struct can_msg *msg = NULL;<br>
> +<br>
> + rtems_interrupt_lock_context lock_context;<br>
> +<br>
> + while (1) {<br>
> + rtems_interrupt_lock_acquire(&bus->can_bus_lock, &lock_context);<br>
<br>
Is an interrupt lock really necessary? Wouldn't a mutex or similar work <br>
too to protect the relevant data? An interrupt lock is something that <br>
should be used really carefull. You tell the system: Stop everything <br>
else. This here is the most important task.<br>
<br>
That means that for example a time critical data acquisition process can <br>
be delayed by the time that you need in your interrupt lock. You have to <br>
be really sure that the interrupt is the only solution to solve the <br>
problem before you use one.<br>
<br>
> +<br>
> + ret = bus->can_dev_ops->dev_tx_ready(bus->priv);<br>
> +<br>
> + if (ret != true) {<br>
> + goto return_with_lock_release;<br>
> + }<br>
> +<br>
> + msg = can_tx_get_data_buf(bus);<br>
> +<br>
> + if (msg == NULL) {<br>
> + goto return_with_lock_release;<br>
> + }<br>
> +<br>
> + ret = bus->can_dev_ops->dev_tx(bus->priv, msg);<br>
<br>
You have an interrupt lock here. Is "dev_tx" a function that is <br>
guaranteed to be fast enough not to make problems with that?<br>
<br>
> +<br>
> + if (ret != RTEMS_SUCCESSFUL) {<br>
> + printf("dev_send failed\n");<br>
<br>
You have an interrupt lock here. You can't use a printf because that <br>
uses interrupts.<br>
<br>
> + }<br>
> +<br>
> + rtems_interrupt_lock_release(&bus->can_bus_lock, &lock_context);<br>
> +<br>
> + //can_tx_done(bus);<br>
> + }<br>
> +<br>
> + return RTEMS_SUCCESSFUL;<br>
> +<br>
> +return_with_lock_release:<br>
> +<br>
> + rtems_interrupt_lock_release(&bus->can_bus_lock, &lock_context);<br>
> + return RTEMS_SUCCESSFUL;<br>
> +}<br>
> +<br>
> +void can_print_canmsg(struct can_msg const *msg)<br>
<br>
I assume that's a debug only function. Otherwise I wouldn't be happy <br>
with the output format.<br>
<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>
> +int can_tx_done(struct can_bus *bus)<br>
> +{<br>
> + int ret = 0;<br>
> +<br>
> + if (bus->can_dev_ops->dev_tx_ready(bus) == true) {<br>
> + can_xmit(bus);<br>
> + }<br>
> +<br>
> + if (bus->can_tx_buf_waiters > 0 && (can_tx_buf_isempty(bus) != 0)) {<br>
> + ret = give_sem(bus);<br>
> + if (ret != 0) {<br>
> + printf(">>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> rtems_semaphore_release failed = %d\n", ret);<br>
> + } else {<br>
> + }<br>
> + }<br>
> +<br>
> + return ret;<br>
> +}<br>
> +<br>
> +static ssize_t 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>
> + rtems_interrupt_lock_context lock_context;<br>
> +<br>
> + while (1) {<br>
> + /* sleep is for debug purpose to test concurrency issues */<br>
> + sleep(1);<br>
> +<br>
> + rtems_interrupt_lock_acquire(&bus->can_bus_lock, &lock_context);<br>
<br>
Same as above: Is an interrupt lock really necessary and the only solution?<br>
<br>
> +<br>
> + fifo_buf = can_tx_get_empty_buf(bus);<br>
> +<br>
> + if (fifo_buf != NULL) {<br>
> + uint32_t msg_size = (char *)&msg->data[msg->len] - (char *)msg;<br>
> +<br>
> + if (msg_size > sizeof(struct can_msg)) {<br>
> + printf("can message len error msg_size = %u struct can_msg = %u\n", msg_size, sizeof(struct can_msg));<br>
<br>
Again: Printf will not work during an interrupt lock.<br>
<br>
> + return -RTEMS_INVALID_SIZE;<br>
> + }<br>
> +<br>
> + memcpy(fifo_buf, msg, msg_size); //sizeof(struct can_msg) - (CAN_MSG_MAX_SIZE - msg->len));<br>
<br>
memcpy() in an interrupt lock is definitively takeing too much time. You <br>
have to be really convincing that I agree that it's OK.<br>
<br>
> + ret = msg_size;<br>
> + //can_print_canmsg(msg);<br>
> + }<br>
> +<br>
> + rtems_interrupt_lock_release(&bus->can_bus_lock, &lock_context);<br>
> +<br>
> + /* sleep is for debug purpose to test concurrency issues */<br>
> + sleep(1);<br>
> +<br>
> + if (fifo_buf != NULL) {<br>
> + break;<br>
> + }<br>
> +<br>
> + /* Enters if clause, if there are no empty tx fifo buffers. Based on the flags, sleep until buffer<br>
> + * is empty or return error status<br>
> + */<br>
> +<br>
> + if (fifo_buf == NULL) {<br>
> + if ((iop->flags & O_NONBLOCK) != 0) {<br>
> + return -RTEMS_RESOURCE_IN_USE;<br>
> + }<br>
> +<br>
> + if (bus->can_dev_ops->dev_tx_ready(bus) == true) {<br>
> + can_xmit(bus);<br>
> + }<br>
> +<br>
> + rtems_interrupt_lock_acquire(&bus->can_bus_lock, &lock_context);<br>
> + ret = can_tx_buf_isempty(bus);<br>
> + rtems_interrupt_lock_release(&bus->can_bus_lock, &lock_context);<br>
<br>
If the "isempty" function needs an interupt_lock: Shouldn't it be part <br>
of the function?<br>
<br>
And again: Wouldn't a mutex work to protect the data instead of <br>
interrupt locks?<br>
<br>
> +<br>
> + if (ret == true) {<br>
> + continue;<br>
> + }<br>
> +<br>
> + rtems_interrupt_lock_acquire(&bus->can_bus_lock, &lock_context);<br>
> + bus->can_tx_buf_waiters++;<br>
> + rtems_interrupt_lock_release(&bus->can_bus_lock, &lock_context);<br>
> +<br>
> + printf("empty_count = %u\n", bus->tx_fifo.empty_count);<br>
> +<br>
> + ret = take_sem(bus);<br>
> +<br>
> + rtems_interrupt_lock_acquire(&bus->can_bus_lock, &lock_context);<br>
> + bus->can_tx_buf_waiters--;<br>
> + rtems_interrupt_lock_release(&bus->can_bus_lock, &lock_context);<br>
> +<br>
> + if (ret != RTEMS_SUCCESSFUL) {<br>
> + printf("cannot take semaphore\n");<br>
> + ret = -RTEMS_INTERNAL_ERROR;<br>
> + goto return_release_lock;<br>
> + }<br>
> + }<br>
> + }<br>
> +<br>
> + if (bus->can_dev_ops->dev_tx_ready(bus) == true) {<br>
> + can_xmit(bus);<br>
> + }<br>
> +<br>
> +return_release_lock:<br>
<br>
Why is the label named "...release_lock"? It isn't releasing any lock.<br>
<br>
> +<br>
> + return ret;<br>
> +}<br>
> +<br>
> +static ssize_t 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 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>
> + ret = IMFS_make_generic_node(bus_path, S_IFCHR | S_IRWXU | S_IRWXG | S_IRWXO, &can_bus_node_control, bus);<br>
> +<br>
> + if (ret != RTEMS_SUCCESSFUL) {<br>
> + printf("Creating node failed: %d\n", ret);<br>
> + goto fail;<br>
> + }<br>
> +<br>
> + RTEMS_INTERRUPT_LOCK_INITIALIZER(bus->can_bus_lock);<br>
<br>
The RTEMS_INTERRUPT_LOCK_INITIALIZER is for a static initialization of <br>
an interrupt lock. It won't work to initialize it dynamically here.<br>
<br>
> +<br>
> + if ((ret = can_create_sem(bus)) != RTEMS_SUCCESSFUL) {<br>
> + printf("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_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_create_rx_buffers failed = %d\n", ret);<br>
> + goto fail;<br>
> + }<br>
> +<br>
> + return ret;<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>
> + int rv;<br>
> +<br>
> + rv = can_bus_do_init(bus, can_bus_destroy_and_free);<br>
> + if (rv != 0) {<br>
> + return NULL;<br>
> + }<br>
> + }<br>
> + }<br>
> +<br>
> + return bus;<br>
> +}<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..2210820504<br>
> --- /dev/null<br>
> +++ b/cpukit/include/dev/can/can.h<br>
> @@ -0,0 +1,115 @@<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<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>
<br>
A more or less general note for the header file: You want to introduce <br>
it as a new general API. Please make sure to document that API enough. <br>
Use doxygen-comments in the header file for that. For an example, take a <br>
look at cpukit/include/linux/i2c.h or some other headers. Documentation <br>
is often really annoying to write as a developer but at the same time <br>
it's really convenient to have it for all other developers.<br>
<br>
For me it's also not really clear whether you want to create a user <br>
facing API with this file or a driver facing one.<br>
<br>
> +#define CAN_MSG_MAX_SIZE (8u)<br>
> +<br>
> +#define CAN_TX_BUF_COUNT (10u)<br>
> +#define CAN_RX_BUF_COUNT (10u)<br>
<br>
Is that really a value that does match the needs of all drivers / <br>
applications?<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_tx_int)(void *priv, bool);<br>
> + int32_t (*dev_ioctl)(void *priv, void *buffer, size_t cmd);<br>
> +} can_dev_ops;<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_bus {<br>
> + void *priv; // CAN controller specific struct.<br>
> + struct can_dev_ops *can_dev_ops; // CAN controller operations.<br>
> + struct ring_buf tx_fifo; // TX fifo<br>
> +#ifndef _POSIX_SEM_<br>
> + rtems_id tx_fifo_sem_id; // TX fifo counting semaphore id<br>
> +#else<br>
> + sem_t tx_sem_id;<br>
> + uint32_t tx_fifo_sem_name; // TX fifo sem name<br>
> +#endif /* _POSIX_SEM_ */<br>
<br>
What's the advantage of implementing two types of semaphores depending <br>
on the _POSIX_SEM_ macro instead of just using only one RTEMS-native type?<br>
<br>
I haven't had a detailed look at the useage of the semaphores yet. <br>
Personally I prefer the self-contained objects because they have the <br>
advantage that the application-developers don't have to think about them <br>
when configuring their application:<br>
<br>
<br>
<a href="https://docs.rtems.org/branches/master/c-user/self_contained_objects.html#" rel="noreferrer" target="_blank">https://docs.rtems.org/branches/master/c-user/self_contained_objects.html#</a><br>
<br>
Not sure whether that would be an option here.<br>
<br>
> + uint32_t can_tx_buf_waiters;<br>
> + rtems_id rx_queue_id;<br>
> + uint32_t rx_queue_name;<br>
> + rtems_recursive_mutex mutex; // For handling driver concurrency.<br>
> + RTEMS_INTERRUPT_LOCK_MEMBER(can_bus_lock);<br>
> + void (*destroy)(struct can_bus *bus); // Clean the CAN controller specific resources.<br>
> +} can_bus;<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>
> +extern can_bus *can_bus_alloc_and_init(size_t size);<br>
> +extern int can_bus_init(can_bus *bus);<br>
> +extern rtems_status_code can_bus_register(can_bus *bus, const char *bus_path);<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 rtems_status_code can_create_tx_buffers(struct can_bus *bus);<br>
> +extern rtems_status_code can_create_rx_buffers(struct can_bus *bus);<br>
> +extern bool can_tx_buf_isempty(struct can_bus *bus);<br>
> +extern struct can_msg *can_tx_get_data_buf(struct can_bus *bus);<br>
> +extern struct can_msg *can_tx_get_empty_buf(struct can_bus *bus);<br>
> +<br>
> +extern int take_sem(struct can_bus *);<br>
> +extern int give_sem(struct can_bus *);<br>
> +extern int can_create_sem(struct can_bus *bus);<br>
<br>
Some of the functions seem to be internal functions that are only used <br>
in the framework (especially the ones with *_sem() but also the whole <br>
create*buffers() stuff). You shouldn't expose them to the user. Make <br>
them static or put them into some internal header.<br>
<br>
Best regards<br>
<br>
Christian<br>
<br>
> +<br>
> +extern void can_print_canmsg(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..b700935f82 100644<br>
> --- a/spec/build/cpukit/librtemscpu.yml<br>
> +++ b/spec/build/cpukit/librtemscpu.yml<br>
> @@ -50,6 +50,9 @@ 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>
> - destination: ${BSP_INCLUDEDIR}/linux<br>
> source:<br>
> - cpukit/include/linux/i2c-dev.h<br>
> @@ -530,6 +533,8 @@ 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/dev/can/can-queue.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>