<div dir="ltr"><div>Hi Christian,</div><div><br></div><div><span style="color:rgb(153,0,255)">> If you didn't have to agree to some really odd license, you can post it <br>
> as a patch on the list. Make sure to make it _very_ clear that you are <br>
> not sure about the license and for what reason. Things like licensing <br>
> should be discussed with the community in public so that the discussion <br>
> is archived together with the list.<span class="gmail-im"><br></span></span><div style="margin-left:40px">Ok, Need to make a few more changes and send them to review.<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></span>
> In that case the static variable will make trouble. You have a static <br>
> counter. If I register the bus, deregister it and register it again, the <br>
> counter will have odd values.</span><span style="color:rgb(153,0,255)">
Better: Use some macro to automate that and enable or disable all debug <br>> messages at once. Also it's not the nicest solution, it's not uncommon <br>
> to have something like that. Examples are:<br></span>
<span style="color:rgb(153,0,255)">><br>> cpukit/libdrvmgr/drvmgr.c:44:#define DBG(x...) printk(x)<br></span>
<span style="color:rgb(153,0,255)">><br>
> cpukit/include/rtems/posix/aio_misc.h:116:#define AIO_printf(_x) printf(_x)<br></span>
<span style="color:rgb(153,0,255)">> <br>
> cpukit/libfs/src/ftpfs/ftpfs.c:61: #define DEBUG_PRINTF(...) <br>
> printf(__VA_ARGS__)<br></span>
<span style="color:rgb(153,0,255)">><br>
> bsps/mips/malta/pci/pci.c:49: #define JPRINTK(fmt, ...) printk("%s: " <br>
> fmt, __FUNCTION__, ##__VA_ARGS__)</span><span style="color:rgb(153,0,255)">
You put a pointer to the tx_fifo into msg:<span class="gmail-im"><br>
></span></span></div><div><div style="margin-left:40px"><span style="color:rgb(153,0,255)"><span class="gmail-im"><font color="#000000">Ok<br></font></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">
> msg = &bus->tx_fifo.pbuf[bus->tx_fifo.tail];<br>
><br></span>
> and then return that pointer to the application:<br></span>
<span style="color:rgb(153,0,255)">><br>> return msg;<br></span>
<span style="color:rgb(153,0,255)"><br>
> The function is not static so it's clearly not internal use only. Even <br>
> if it would be internal only, I would suggest to split it into a get <br>
> buffer and mark buffer empty after it has been copied. Otherwise you <br>
> have to be _really_ carefull that the framework can't overwrite the <br>
> buffer while it is still used.<span class="gmail-im"><br></span></span><div style="margin-left:40px"><font color="#000000">Ok, I added as a single API, as we can change the type of <br></font></div><div style="margin-left:40px"><font color="#000000">data structure used (now ring buffer is used, maybe later a priority queue or red black tree).</font><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></span>
> Is the part in can-queue intendet to be used by an application? <br>
> Otherwise you should not install the headers so that a user can't use them.<span class="gmail-im"></span></span></div><div><div style="margin-left:40px"><font color="#000000">functions in can-queue.c are only for driver purpose, Moving them to can-queue.h</font><br></div><span style="color:rgb(153,0,255)"><span class="gmail-im">
<br></span>
> You should allways disable as few as possible. So if you can only <br>
> disable can interrupts instead of all interrupts, you should do that. <br>
> Otherwise you make assumptions about a user system. In that case you <br>
> assume that CAN is the most important thing in the system and therefore <br>
> it disables all other interrupts so that it can do it's work.<span class="gmail-im"></span></span></div><div><div style="margin-left:40px"><font color="#000000">Ok, I will disable interrupts locally and make changes accordingly.</font><br></div><span style="color:rgb(153,0,255)"><span class="gmail-im">
<br></span>
> Is it really necessary to copy the whole buffer in the interrupt lock or <br>
> could you prepare all data and then only copy one controll structure?</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">Yes, Moving, copying buffers out of interrupt lock (As we already got the buffer from can_tx_get_empty_buf).</font></span></div><div style="margin-left:40px"><span style="color:rgb(153,0,255)"><font color="#000000"><br></font></span></div><div><span style="color:rgb(153,0,255)">> Same as above: Don't make assumptions about what is critical in the <br>
> system. Disable only the CAN interrupt if that is possible.<span class="gmail-im"><br></span></span><div style="margin-left:40px">Ok<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>
>> Again: Printf will not work during an interrupt lock.<br>
> I will modify that.<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></span>
>> have to be really convincing that I agree that it's OK.If the
"isempty" function needs an interupt_lock: Shouldn't it be part<span class="gmail-im"><br>
>> of the function?<br>
>><br>
>> And again: Wouldn't a mutex work to protect the data instead of <br></span>
>> interrupt locks?Why is the label named "...release_lock"? It isn't releasing any <br>
> lock.The RTEMS_INTERRUPT_LOCK_INITIALIZER is for a static initialization of<span class="gmail-im"><br>
>> an interrupt lock. It won't work to initialize it dynamically here.<br>
> Yes, We need to disable interrupts as the tx and rx buffers are used in <br>
> the tx and rx interrupts handlers.<br>
> <br></span>
> Are the buffers used or only a small controll structure with a pointer <br>
> to the buffers?<span class="gmail-im"><br></span></span><div style="margin-left:40px">Yes the buffers are used to identify the type of message based on the flags.</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">
> Or could disable only CAN interrupts, as CAN messages being a higher <br>
> priority I added a Global interrupt disable.<br>
> Shall I disable it locally or globally?<br>
> <br>
> Changing RTEMS_INTERRUPT_LOCK_INITIALIZER to <br>
> rtems_interrupt_lock_initialize.<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>
> Ok, I will add the doxygen style comments.<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>
> Based on the suggestions, need to decide a suitable count.<br>
> <br>
><br></span>
> Can it be some parameter that is passed for example during <br>
> initialization so that a user can decide how much he needs?</span></div><div style="margin-left:40px"><font color="#000000">Yes, I think that can be done as a #define to be configured by the application, As the driver is initialized before the application starts.</font></div><div style="margin-left:40px"><font color="#000000">And can be changed dynamically also by using an ioctl.<br></font></div><div><br></div><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 19:05, Christian Mauderer <<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>
Am 19.07.22 um 15:09 schrieb Prashanth S:<br>
> Hi Christian,<br>
> <br>
> This is to reply to review comments.<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>
> Yes, Waiting for License confirmation.<br>
<br>
If you didn't have to agree to some really odd license, you can post it <br>
as a patch on the list. Make sure to make it _very_ clear that you are <br>
not sure about the license and for what reason. Things like licensing <br>
should be discussed with the community in public so that the discussion <br>
is archived together with the list.<br>
<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>
> Ok<br>
> <br>
>> Please make sure to stic to the RTEMS style and use a 80 character line <br>
>> length.<br>
> OK<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>
> Assuming we have less than 10 CAN controllers, I will update this to <br>
> limit to 10 queues.<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>
> Yes, the can_bus structure holds data related only to a specific bus.<br>
> <br>
<br>
In that case the static variable will make trouble. You have a static <br>
counter. If I register the bus, deregister it and register it again, the <br>
counter will have odd values.<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>
> I will make changes to limit line length to 80.<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>
> Ok, I will update the prints with the function name in it.<br>
> <br>
<br>
Better: Use some macro to automate that and enable or disable all debug <br>
messages at once. Also it's not the nicest solution, it's not uncommon <br>
to have something like that. Examples are:<br>
<br>
cpukit/libdrvmgr/drvmgr.c:44:#define DBG(x...) printk(x)<br>
<br>
cpukit/include/rtems/posix/aio_misc.h:116:#define AIO_printf(_x) printf(_x)<br>
<br>
cpukit/libfs/src/ftpfs/ftpfs.c:61: #define DEBUG_PRINTF(...) <br>
printf(__VA_ARGS__)<br>
<br>
bsps/mips/malta/pci/pci.c:49: #define JPRINTK(fmt, ...) printk("%s: " <br>
fmt, __FUNCTION__, ##__VA_ARGS__)<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>
> Ok, memset is done in the can_bus_init or calloc is used in <br>
> can_bus_alloc_and_init.<br>
> Removing bus->tx_fifo.head = bus->tx_fifo.tail = 0;<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>
> Ok, removing it.<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>
> Here the application does not use this (driver buffer) buffer, we copy <br>
> the message from this buffer (driver buffer)<br>
> to the application passed buffer.<br>
<br>
You put a pointer to the tx_fifo into msg:<br>
<br>
msg = &bus->tx_fifo.pbuf[bus->tx_fifo.tail];<br>
<br>
and then return that pointer to the application:<br>
<br>
return msg;<br>
<br>
The function is not static so it's clearly not internal use only. Even <br>
if it would be internal only, I would suggest to split it into a get <br>
buffer and mark buffer empty after it has been copied. Otherwise you <br>
have to be _really_ carefull that the framework can't overwrite the <br>
buffer while it is still used.<br>
<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>
> Functions in can-queue.c are used in can.c, Moving them to can-queue.h <br>
> or can.h header file.<br>
> I will add static to take_sem and give_sem.<br>
<br>
Is the part in can-queue intendet to be used by an application? <br>
Otherwise you should not install the headers so that a user can't use them.<br>
<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>
> Yes, We need to disable interrupts as the tx and rx buffers are used in <br>
> the tx and rx interrupts handlers.<br>
> Or could disable only CAN interrupts, as CAN messages being a higher <br>
> priority so I added a Global interrupt disable.<br>
> Shall I disable it locally or globally?<br>
> <br>
<br>
You should allways disable as few as possible. So if you can only <br>
disable can interrupts instead of all interrupts, you should do that. <br>
Otherwise you make assumptions about a user system. In that case you <br>
assume that CAN is the most important thing in the system and therefore <br>
it disables all other interrupts so that it can do it's work.<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>
> This function copies drivers buffers to the controller's hardware <br>
> buffer. Time taken is based on the<br>
> message size.<br>
<br>
Is it really necessary to copy the whole buffer in the interrupt lock or <br>
could you prepare all data and then only copy one controll structure?<br>
<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>
> I will print after releasing the interrupt lock.<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>
> Yes, can_print_message is for debug purposes only.<br>
> <br>
>> Same as above: Is an interrupt lock really necessary and the only solution?<br>
> Yes, We need to disable interrupts as the tx and rx buffers are used in <br>
> the tx and rx interrupts handlers.<br>
> Or could disable only CAN interrupts, as CAN messages being a higher <br>
> priority so I added a Global interrupt disable.<br>
> Shall I disable it locally or globally?<br>
> <br>
<br>
Same as above: Don't make assumptions about what is critical in the <br>
system. Disable only the CAN interrupt if that is possible.<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>
> I will modify that.<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.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?Why is the label named "...release_lock"? It isn't releasing any <br>
> lock.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>
> Yes, We need to disable interrupts as the tx and rx buffers are used in <br>
> the tx and rx interrupts handlers.<br>
<br>
Are the buffers used or only a small controll structure with a pointer <br>
to the buffers?<br>
<br>
> Or could disable only CAN interrupts, as CAN messages being a higher <br>
> priority I added a Global interrupt disable.<br>
> Shall I disable it locally or globally?<br>
> <br>
> Changing RTEMS_INTERRUPT_LOCK_INITIALIZER to <br>
> rtems_interrupt_lock_initialize.<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>
> Ok, I will add the doxygen style comments.<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>
> Based on the suggestions, need to decide a suitable count.<br>
> <br>
<br>
Can it be some parameter that is passed for example during <br>
initialization so that a user can decide how much he needs?<br>
<br>
Best regards<br>
<br>
Christian<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>
> That I used for debug purposes, as at first rtems_semaphore_release was <br>
> not working. So to test whether POSIX works I used that.<br>
> Removing POSIX API.<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>
> <<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>
> Ok, I will look into the link.<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>
> Moving them to can-queue.h or can.h header file.<br>
> <br>
> Regards<br>
> Prashanth S<br>
> <br>
> On Tue, 19 Jul 2022 at 14:47, <<a href="mailto:oss@c-mauderer.de" target="_blank">oss@c-mauderer.de</a> <br>
> <mailto:<a href="mailto:oss@c-mauderer.de" target="_blank">oss@c-mauderer.de</a>>> wrote:<br>
> <br>
> 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<br>
> 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.<br>
> 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>
> ++++++++++++++++++++++++++++++<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<br>
> without<br>
> > + * modification, are permitted provided that the following<br>
> 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<br>
> copyright<br>
> > + * notice, this list of conditions and the following disclaimer<br>
> in the<br>
> > + * documentation and/or other materials provided with the<br>
> distribution.<br>
> > + *<br>
> > + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND<br>
> CONTRIBUTORS "AS IS"<br>
> > + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT<br>
> LIMITED TO, THE<br>
> > + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A<br>
> PARTICULAR PURPOSE<br>
> > + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR<br>
> CONTRIBUTORS BE<br>
> > + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL,<br>
> EXEMPLARY, OR<br>
> > + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,<br>
> PROCUREMENT OF<br>
> > + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS;<br>
> OR BUSINESS<br>
> > + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY,<br>
> WHETHER IN<br>
> > + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR<br>
> OTHERWISE)<br>
> > + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF<br>
> 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',<br>
> 'n', '0' + init++), CAN_RX_BUF_COUNT, sizeof(struct can_msg),<br>
> > + RTEMS_FIFO | RTEMS_LOCAL,<br>
> &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:",<br>
> "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<br>
> *)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<br>
> 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<br>
> you<br>
> can enable / disable with a macro at the start of the file. You can<br>
> also<br>
> use that to add a prefix or function name to all prints. If you<br>
> 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<br>
> 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<br>
> 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<br>
> without<br>
> > + * modification, are permitted provided that the following<br>
> 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<br>
> copyright<br>
> > + * notice, this list of conditions and the following disclaimer<br>
> in the<br>
> > + * documentation and/or other materials provided with the<br>
> distribution.<br>
> > + *<br>
> > + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND<br>
> CONTRIBUTORS "AS IS"<br>
> > + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT<br>
> LIMITED TO, THE<br>
> > + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A<br>
> PARTICULAR PURPOSE<br>
> > + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR<br>
> CONTRIBUTORS BE<br>
> > + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL,<br>
> EXEMPLARY, OR<br>
> > + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,<br>
> PROCUREMENT OF<br>
> > + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS;<br>
> OR BUSINESS<br>
> > + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY,<br>
> WHETHER IN<br>
> > + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR<br>
> OTHERWISE)<br>
> > + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF<br>
> 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<br>
> *path, int oflag, mode_t mode);<br>
> > +static ssize_t can_bus_read(rtems_libio_t *iop, void *buffer,<br>
> size_t count);<br>
> > +static ssize_t can_bus_write(rtems_libio_t *iop, const void<br>
> *buffer, size_t count);<br>
> > +static ssize_t can_bus_ioctl(rtems_libio_t *iop, ioctl_command_t<br>
> 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<br>
> *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,<br>
> msg, sizeof(struct can_msg) - (CAN_MSG_MAX_SIZE - msg->len),<br>
> &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<br>
> member */<br>
> > +static ssize_t can_bus_read(rtems_libio_t *iop, void *buffer,<br>
> 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<br>
> *)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<br>
> make<br>
> it static and remove it from the header. Same is true for "take_sem"<br>
> 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',<br>
> '0' + init++), 1,<br>
> > + RTEMS_FIFO | RTEMS_SIMPLE_BINARY_SEMAPHORE | RTEMS_LOCAL,<br>
> 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,<br>
> 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,<br>
> 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<br>
> can<br>
> be delayed by the time that you need in your interrupt lock. You<br>
> 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>
> > + <br>
> printf("\n----------------------------------------------------------------------------------------------------------------------\n");<br>
> > + printf("id = %d len = %d flags = 0x%08X\n", msg->id, msg->len,<br>
> msg->flags);<br>
> > +<br>
> > + for (int i = 0; i < msg->len; i++) {<br>
> > + printf("%02x ", msg->data[i]);<br>
> > + }<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) !=<br>
> 0)) {<br>
> > + ret = give_sem(bus);<br>
> > + if (ret != 0) {<br>
> > + <br>
> printf(">>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>><br>
> 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<br>
> *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<br>
> 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<br>
> *)msg;<br>
> > +<br>
> > + if (msg_size > sizeof(struct can_msg)) {<br>
> > + printf("can message len error msg_size = %u struct<br>
> 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)<br>
> - (CAN_MSG_MAX_SIZE - msg->len));<br>
> <br>
> memcpy() in an interrupt lock is definitively takeing too much time.<br>
> 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.<br>
> 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,<br>
> &lock_context);<br>
> > + ret = can_tx_buf_isempty(bus);<br>
> > + rtems_interrupt_lock_release(&bus->can_bus_lock,<br>
> &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,<br>
> &lock_context);<br>
> > + bus->can_tx_buf_waiters++;<br>
> > + rtems_interrupt_lock_release(&bus->can_bus_lock,<br>
> &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,<br>
> &lock_context);<br>
> > + bus->can_tx_buf_waiters--;<br>
> > + rtems_interrupt_lock_release(&bus->can_bus_lock,<br>
> &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<br>
> 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 =<br>
> IMFS_GENERIC_INITIALIZER(&can_bus_handler,<br>
> > + <br>
> IMFS_node_initialize_generic, can_bus_node_destroy);<br>
> > +<br>
> > +rtems_status_code can_bus_register(can_bus *bus, const char<br>
> *bus_path)<br>
> > +{<br>
> > + int ret = RTEMS_SUCCESSFUL;<br>
> > +<br>
> > + ret = IMFS_make_generic_node(bus_path, S_IFCHR | S_IRWXU |<br>
> 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<br>
> *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<br>
> 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<br>
> without<br>
> > + * modification, are permitted provided that the following<br>
> 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<br>
> copyright<br>
> > + * notice, this list of conditions and the following disclaimer<br>
> in the<br>
> > + * documentation and/or other materials provided with the<br>
> distribution.<br>
> > + *<br>
> > + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND<br>
> CONTRIBUTORS "AS IS"<br>
> > + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT<br>
> LIMITED TO, THE<br>
> > + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A<br>
> PARTICULAR PURPOSE<br>
> > + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR<br>
> CONTRIBUTORS BE<br>
> > + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL,<br>
> EXEMPLARY, OR<br>
> > + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,<br>
> PROCUREMENT OF<br>
> > + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS;<br>
> OR BUSINESS<br>
> > + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY,<br>
> WHETHER IN<br>
> > + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR<br>
> OTHERWISE)<br>
> > + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF<br>
> 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,<br>
> 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<br>
> specific struct.<br>
> > + struct can_dev_ops *can_dev_ops; // CAN controller<br>
> operations.<br>
> > + struct ring_buf tx_fifo; // TX fifo<br>
> > +#ifndef _POSIX_SEM_<br>
> > + rtems_id tx_fifo_sem_id; // TX fifo counting<br>
> 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<br>
> 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<br>
> 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>
> <<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<br>
> driver concurrency.<br>
> > + RTEMS_INTERRUPT_LOCK_MEMBER(can_bus_lock);<br>
> > + void (*destroy)(struct can_bus *bus); // Clean the CAN<br>
> controller specific resources.<br>
> > +} can_bus;<br>
> > +<br>
> > +struct can_msg {<br>
> > + uint32_t id; // 32 bits to<br>
> 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<br>
> 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<br>
> 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<br>
> 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>
> <br>
</blockquote></div>