[PATCH] cpukit/dev/can: Added CAN support
Prashanth S
fishesprashanth at gmail.com
Tue Jul 19 14:59:02 UTC 2022
Hi Christian,
> If you didn't have to agree to some really odd license, you can post it
> as a patch on the list. Make sure to make it _very_ clear that you are
> not sure about the license and for what reason. Things like licensing
> should be discussed with the community in public so that the discussion
> is archived together with the list.
Ok, Need to make a few more changes and send them to review.
> In that case the static variable will make trouble. You have a static
> counter. If I register the bus, deregister it and register it again, the
> counter will have odd values. Better: Use some macro to automate that and
enable or disable all debug
> messages at once. Also it's not the nicest solution, it's not uncommon
> to have something like that. Examples are:
>
> cpukit/libdrvmgr/drvmgr.c:44:#define DBG(x...) printk(x)
>
> cpukit/include/rtems/posix/aio_misc.h:116:#define AIO_printf(_x)
printf(_x)
>
> cpukit/libfs/src/ftpfs/ftpfs.c:61: #define DEBUG_PRINTF(...)
> printf(__VA_ARGS__)
>
> bsps/mips/malta/pci/pci.c:49: #define JPRINTK(fmt, ...) printk("%s: "
> fmt, __FUNCTION__, ##__VA_ARGS__) You put a pointer to the tx_fifo into
msg:
>
Ok
> msg = &bus->tx_fifo.pbuf[bus->tx_fifo.tail];
>
> and then return that pointer to the application:
>
> return msg;
> The function is not static so it's clearly not internal use only. Even
> if it would be internal only, I would suggest to split it into a get
> buffer and mark buffer empty after it has been copied. Otherwise you
> have to be _really_ carefull that the framework can't overwrite the
> buffer while it is still used.
Ok, I added as a single API, as we can change the type of
data structure used (now ring buffer is used, maybe later a priority queue
or red black tree).
> Is the part in can-queue intendet to be used by an application?
> Otherwise you should not install the headers so that a user can't use
them.
functions in can-queue.c are only for driver purpose, Moving them to
can-queue.h
> You should allways disable as few as possible. So if you can only
> disable can interrupts instead of all interrupts, you should do that.
> Otherwise you make assumptions about a user system. In that case you
> assume that CAN is the most important thing in the system and therefore
> it disables all other interrupts so that it can do it's work.
Ok, I will disable interrupts locally and make changes accordingly.
> Is it really necessary to copy the whole buffer in the interrupt lock or
> could you prepare all data and then only copy one controll structure?
Yes, Moving, copying buffers out of interrupt lock (As we already got the
buffer from can_tx_get_empty_buf).
> Same as above: Don't make assumptions about what is critical in the
> system. Disable only the CAN interrupt if that is possible.
Ok
>>> +
>>> + fifo_buf = can_tx_get_empty_buf(bus);
>>> +
>>> + if (fifo_buf != NULL) {
>>> + uint32_t msg_size = (char *)&msg->data[msg->len] - (char *)msg;
>>> +
>>> + if (msg_size > sizeof(struct can_msg)) {
>>> + printf("can message len error msg_size = %u struct can_msg =
%u\n", msg_size, sizeof(struct can_msg));
>>
>> Again: Printf will not work during an interrupt lock.
> I will modify that.
>
>>> + return -RTEMS_INVALID_SIZE;
>>> + }
>>> +
>>> + memcpy(fifo_buf, msg, msg_size); //sizeof(struct can_msg) -
(CAN_MSG_MAX_SIZE - msg->len));
>>
>> memcpy() in an interrupt lock is definitively takeing too much time. You
>> 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
>> of the function?
>>
>> And again: Wouldn't a mutex work to protect the data instead of
>> interrupt locks?Why is the label named "...release_lock"? It isn't
releasing any
> lock.The RTEMS_INTERRUPT_LOCK_INITIALIZER is for a static initialization
of
>> an interrupt lock. It won't work to initialize it dynamically here.
> Yes, We need to disable interrupts as the tx and rx buffers are used in
> the tx and rx interrupts handlers.
>
> Are the buffers used or only a small controll structure with a pointer
> to the buffers?
Yes the buffers are used to identify the type of message based on the flags.
> Or could disable only CAN interrupts, as CAN messages being a higher
> priority I added a Global interrupt disable.
> Shall I disable it locally or globally?
>
> Changing RTEMS_INTERRUPT_LOCK_INITIALIZER to
> rtems_interrupt_lock_initialize.
>
>> A more or less general note for the header file: You want to introduce
>> it as a new general API. Please make sure to document that API enough.
>> Use doxygen-comments in the header file for that. For an example, take a
>> look at cpukit/include/linux/i2c.h or some other headers. Documentation
>> is often really annoying to write as a developer but at the same time
>> it's really convenient to have it for all other developers.
> Ok, I will add the doxygen style comments.
>
>> For me it's also not really clear whether you want to create a user
>> facing API with this file or a driver facing one.
>>
>>> +#define CAN_MSG_MAX_SIZE (8u)
>>> +
>>> +#define CAN_TX_BUF_COUNT (10u)
>>> +#define CAN_RX_BUF_COUNT (10u)
>>
>> Is that really a value that does match the needs of all drivers /
>> applications?
> Based on the suggestions, need to decide a suitable count.
>
>
> Can it be some parameter that is passed for example during
> initialization so that a user can decide how much he needs?
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.
And can be changed dynamically also by using an ioctl.
Regards
Prashanth S
On Tue, 19 Jul 2022 at 19:05, Christian Mauderer <oss at c-mauderer.de> wrote:
> Hello Prashanth,
>
> Am 19.07.22 um 15:09 schrieb Prashanth S:
> > Hi Christian,
> >
> > This is to reply to review comments.
> >
> >> first question: You also worked on a driver for beagle DCAN. Did you
> >> already adapt that driver to this API? If yes, it would be usefull to
> >> post that as a patch too so that the direction and the method how it
> >> will be used is more clear.
> > Yes, Waiting for License confirmation.
>
> If you didn't have to agree to some really odd license, you can post it
> as a patch on the list. Make sure to make it _very_ clear that you are
> not sure about the license and for what reason. Things like licensing
> should be discussed with the community in public so that the discussion
> is archived together with the list.
>
> >
> >> Note that some of my comments might are a bit critical because you add
> a
> >> general API. I would have a lot less problems if it would be only a
> >> driver specific API. If you add a general API, it has to fit the needs
> >> of not only your current driver but most future drivers too. Changing
> an
> >> API later is allways tricky.
> > Ok
> >
> >> Please make sure to stic to the RTEMS style and use a 80 character line
> >> length.
> > OK
> >
> >> You create a name with a "'0' + init++": Are you sure that a maximum of
> >> 10 buffers are created? Otherwise you will get names like "can:",
> "can;"
> >> and - sooner or later: "can\xFF", "can\x00", ...
> > Assuming we have less than 10 CAN controllers, I will update this to
> > limit to 10 queues.
> >
> >> In the header you somewhere had a "destroy" function. Do you need some
> >> way to de-initialize buffers only used by that bus?
> > Yes, the can_bus structure holds data related only to a specific bus.
> >
>
> In that case the static variable will make trouble. You have a static
> counter. If I register the bus, deregister it and register it again, the
> counter will have odd values.
>
> >>> +}
> >>> +
> >>> +rtems_status_code can_create_tx_buffers(struct can_bus *bus)
> >>> +{
> >>> + if ((bus->tx_fifo.pbuf = (struct can_msg *)malloc(CAN_TX_BUF_COUNT
> * sizeof(struct can_msg))) == NULL) {
> >
> >> You seem to like writing a lot of code in one line. From my point of
> >> view that makes code harder to read. If you write it into multiple
> lines
> >> instead, someone else doesn't have to think that much about which
> >> bracket closes where. For example in this case the following would be
> >> simpler to read:
> > I will make changes to limit line length to 80.
> >
> >> ====
> >> bus->tx_fifo.pbuf = (struct can_msg *)malloc(CAN_TX_BUF_COUNT *
> >> sizeof(struct can_msg));
> >> if (bus->tx_fifo.pbuf == NULL) {
> >> ....
> >> }
> >> ====
> >>
> >> + printf("malloc failed\n");
> >>
> >> Prints are nice for debugging but can be really annoying in
> >> applications. Think about some kind of "debug_print" or similar that
> you
> >> can enable / disable with a macro at the start of the file. You can
> also
> >> use that to add a prefix or function name to all prints. If you search
> a
> >> problem, a line like "mallof failed" is meaningless if you are not
> >> currently working on specially this code. A line like
> >> "can_create_tx_buffers: malloc failed" is a lot more usefull. There are
> >> preprocessor macros like __FUNCTION__ that you can use for something
> >> like that.
> > Ok, I will update the prints with the function name in it.
> >
>
> Better: Use some macro to automate that and enable or disable all debug
> messages at once. Also it's not the nicest solution, it's not uncommon
> to have something like that. Examples are:
>
> cpukit/libdrvmgr/drvmgr.c:44:#define DBG(x...) printk(x)
>
> cpukit/include/rtems/posix/aio_misc.h:116:#define AIO_printf(_x) printf(_x)
>
> cpukit/libfs/src/ftpfs/ftpfs.c:61: #define DEBUG_PRINTF(...)
> printf(__VA_ARGS__)
>
> bsps/mips/malta/pci/pci.c:49: #define JPRINTK(fmt, ...) printk("%s: "
> fmt, __FUNCTION__, ##__VA_ARGS__)
>
> >> + return RTEMS_NO_MEMORY;
> >> + }
> >> +
> >> + bus->tx_fifo.head = bus->tx_fifo.tail = 0;
> >>
> >> Same here: This is hard to read. I would suggest to split that into two
> >> lines. Alternatively you might want to think about just using a memset
> >> to have a clean start for a structure that you initialize.
> > Ok, memset is done in the can_bus_init or calloc is used in
> > can_bus_alloc_and_init.
> > Removing bus->tx_fifo.head = bus->tx_fifo.tail = 0;
> >
> >> + bus->tx_fifo.empty_count = CAN_TX_BUF_COUNT;
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +bool can_tx_buf_isempty(struct can_bus *bus)
> >> +{
> >> + if (bus->tx_fifo.empty_count <= 0) {
> >> + /* tx_fifo.empty_count does not go below zero, incase if it goes
> update to zero */
> >> + bus->tx_fifo.empty_count = 0;
> >
> > empty_count is an unsigned type and therefore can never be smaller than
> > 0. This line is not necessary at all.
> > Ok, removing it.
> >
> >> +
> >> + return false;
> >> + }
> >> +
> >> + return true;
> >> +}
> >> +
> >> +struct can_msg *can_tx_get_data_buf(struct can_bus *bus)
> >> +{
> >> + struct can_msg *msg = NULL;
> >> +
> >> + if (bus->tx_fifo.empty_count == CAN_TX_BUF_COUNT) {
> >> + printf("can_tx_get_next_data_buf All buffers are empty\n");
> >> + return NULL;
> >> + }
> >> +
> >> + msg = &bus->tx_fifo.pbuf[bus->tx_fifo.tail];
> >> + bus->tx_fifo.empty_count++;
> >> + bus->tx_fifo.tail = (bus->tx_fifo.tail + 1) % CAN_TX_BUF_COUNT;
> >
> >> You want to return the pointer to the message (msg) to the caller,
> >> right? In that case you must not marke it as empty here. Otherwise
> >> someone else could already use that memory again while the application
> >> is still processing it.
> > Here the application does not use this (driver buffer) buffer, we copy
> > the message from this buffer (driver buffer)
> > to the application passed buffer.
>
> You put a pointer to the tx_fifo into msg:
>
> msg = &bus->tx_fifo.pbuf[bus->tx_fifo.tail];
>
> and then return that pointer to the application:
>
> return msg;
>
> The function is not static so it's clearly not internal use only. Even
> if it would be internal only, I would suggest to split it into a get
> buffer and mark buffer empty after it has been copied. Otherwise you
> have to be _really_ carefull that the framework can't overwrite the
> buffer while it is still used.
>
> >
> >> Looks like a helper function that is only used in this file? Please
> make
> >> it static and remove it from the header. Same is true for "take_sem"
> and
> >> similar below.
> > Functions in can-queue.c are used in can.c, Moving them to can-queue.h
> > or can.h header file.
> > I will add static to take_sem and give_sem.
>
> Is the part in can-queue intendet to be used by an application?
> Otherwise you should not install the headers so that a user can't use them.
>
> >
> >> Is an interrupt lock really necessary? Wouldn't a mutex or similar work
> >> too to protect the relevant data? An interrupt lock is something that
> >> should be used really carefull. You tell the system: Stop everything
> >> else. This here is the most important task.
> >>
> >> That means that for example a time critical data acquisition process
> can
> >> be delayed by the time that you need in your interrupt lock. You have
> to
> >> be really sure that the interrupt is the only solution to solve the
> >> problem before you use one.
> > Yes, We need to disable interrupts as the tx and rx buffers are used in
> > the tx and rx interrupts handlers.
> > Or could disable only CAN interrupts, as CAN messages being a higher
> > priority so I added a Global interrupt disable.
> > Shall I disable it locally or globally?
> >
>
> You should allways disable as few as possible. So if you can only
> disable can interrupts instead of all interrupts, you should do that.
> Otherwise you make assumptions about a user system. In that case you
> assume that CAN is the most important thing in the system and therefore
> it disables all other interrupts so that it can do it's work.
>
> >>> +
> >>> + ret = bus->can_dev_ops->dev_tx_ready(bus->priv);
> >>> +
> >>> + if (ret != true) {
> >>> + goto return_with_lock_release;
> >>> + }
> >>> +
> >>> + msg = can_tx_get_data_buf(bus);
> >>> +
> >>> + if (msg == NULL) {
> >>> + goto return_with_lock_release;
> >>> + }
> >>> +
> >>> + ret = bus->can_dev_ops->dev_tx(bus->priv, msg);
> >>
> >> You have an interrupt lock here. Is "dev_tx" a function that is
> >> guaranteed to be fast enough not to make problems with that?
> > This function copies drivers buffers to the controller's hardware
> > buffer. Time taken is based on the
> > message size.
>
> Is it really necessary to copy the whole buffer in the interrupt lock or
> could you prepare all data and then only copy one controll structure?
>
> >
> >>> +
> >>> + if (ret != RTEMS_SUCCESSFUL) {
> >>> + printf("dev_send failed\n");
> >>
> >> You have an interrupt lock here. You can't use a printf because that
> >> uses interrupts.
> > I will print after releasing the interrupt lock.
> >
> >>> + }
> >>> +
> >>> + rtems_interrupt_lock_release(&bus->can_bus_lock, &lock_context);
> >>> +
> >>> + //can_tx_done(bus);
> >>> + }
> >>> +
> >>> + return RTEMS_SUCCESSFUL;
> >>> +
> >>> +return_with_lock_release:
> >>> +
> >>> + rtems_interrupt_lock_release(&bus->can_bus_lock, &lock_context);
> >>> + return RTEMS_SUCCESSFUL;
> >>> +}
> >>> +
> >>> +void can_print_canmsg(struct can_msg const *msg)
> >>
> >> I assume that's a debug only function. Otherwise I wouldn't be happy
> >> with the output format.
> > Yes, can_print_message is for debug purposes only.
> >
> >> Same as above: Is an interrupt lock really necessary and the only
> solution?
> > Yes, We need to disable interrupts as the tx and rx buffers are used in
> > the tx and rx interrupts handlers.
> > Or could disable only CAN interrupts, as CAN messages being a higher
> > priority so I added a Global interrupt disable.
> > Shall I disable it locally or globally?
> >
>
> Same as above: Don't make assumptions about what is critical in the
> system. Disable only the CAN interrupt if that is possible.
>
> >>> +
> >>> + fifo_buf = can_tx_get_empty_buf(bus);
> >>> +
> >>> + if (fifo_buf != NULL) {
> >>> + uint32_t msg_size = (char *)&msg->data[msg->len] - (char *)msg;
> >>> +
> >>> + if (msg_size > sizeof(struct can_msg)) {
> >>> + printf("can message len error msg_size = %u struct can_msg =
> %u\n", msg_size, sizeof(struct can_msg));
> >>
> >> Again: Printf will not work during an interrupt lock.
> > I will modify that.
> >
> >>> + return -RTEMS_INVALID_SIZE;
> >>> + }
> >>> +
> >>> + memcpy(fifo_buf, msg, msg_size); //sizeof(struct can_msg) -
> (CAN_MSG_MAX_SIZE - msg->len));
> >>
> >> memcpy() in an interrupt lock is definitively takeing too much time.
> You
> >> 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
> >> of the function?
> >>
> >> And again: Wouldn't a mutex work to protect the data instead of
> >> interrupt locks?Why is the label named "...release_lock"? It isn't
> releasing any
> > lock.The RTEMS_INTERRUPT_LOCK_INITIALIZER is for a static initialization
> of
> >> an interrupt lock. It won't work to initialize it dynamically here.
> > Yes, We need to disable interrupts as the tx and rx buffers are used in
> > the tx and rx interrupts handlers.
>
> Are the buffers used or only a small controll structure with a pointer
> to the buffers?
>
> > Or could disable only CAN interrupts, as CAN messages being a higher
> > priority I added a Global interrupt disable.
> > Shall I disable it locally or globally?
> >
> > Changing RTEMS_INTERRUPT_LOCK_INITIALIZER to
> > rtems_interrupt_lock_initialize.
> >
> >> A more or less general note for the header file: You want to introduce
> >> it as a new general API. Please make sure to document that API enough.
> >> Use doxygen-comments in the header file for that. For an example, take
> a
> >> look at cpukit/include/linux/i2c.h or some other headers. Documentation
> >> is often really annoying to write as a developer but at the same time
> >> it's really convenient to have it for all other developers.
> > Ok, I will add the doxygen style comments.
> >
> >> For me it's also not really clear whether you want to create a user
> >> facing API with this file or a driver facing one.
> >>
> >>> +#define CAN_MSG_MAX_SIZE (8u)
> >>> +
> >>> +#define CAN_TX_BUF_COUNT (10u)
> >>> +#define CAN_RX_BUF_COUNT (10u)
> >>
> >> Is that really a value that does match the needs of all drivers /
> >> applications?
> > Based on the suggestions, need to decide a suitable count.
> >
>
> Can it be some parameter that is passed for example during
> initialization so that a user can decide how much he needs?
>
> Best regards
>
> Christian
>
> >> What's the advantage of implementing two types of semaphores depending
> >> on the _POSIX_SEM_ macro instead of just using only one RTEMS-native
> type?
> > That I used for debug purposes, as at first rtems_semaphore_release was
> > not working. So to test whether POSIX works I used that.
> > Removing POSIX API.
> >
> >> I haven't had a detailed look at the useage of the semaphores yet.
> >> Personally I prefer the self-contained objects because they have the
> >> advantage that the application-developers don't have to think about them
> >> when configuring their application:
> >>
> >>
> >>
> https://docs.rtems.org/branches/master/c-user/self_contained_objects.html#
> > <
> https://docs.rtems.org/branches/master/c-user/self_contained_objects.html#
> >
> >>
> >> Not sure whether that would be an option here.
> > Ok, I will look into the link.
> >
> >> Some of the functions seem to be internal functions that are only used
> >> in the framework (especially the ones with *_sem() but also the whole
> >> create*buffers() stuff). You shouldn't expose them to the user. Make
> >> them static or put them into some internal header.
> > Moving them to can-queue.h or can.h header file.
> >
> > Regards
> > Prashanth S
> >
> > On Tue, 19 Jul 2022 at 14:47, <oss at c-mauderer.de
> > <mailto:oss at c-mauderer.de>> wrote:
> >
> > Hello Prashanth,
> >
> > first question: You also worked on a driver for beagle DCAN. Did you
> > already adapt that driver to this API? If yes, it would be usefull to
> > post that as a patch too so that the direction and the method how it
> > will be used is more clear.
> >
> > Note that some of my comments might are a bit critical because you
> > add a
> > general API. I would have a lot less problems if it would be only a
> > driver specific API. If you add a general API, it has to fit the
> needs
> > of not only your current driver but most future drivers too.
> > Changing an
> > API later is allways tricky.
> >
> > Am 15.07.22 um 20:11 schrieb Prashanth S:
> > > ---
> > > cpukit/dev/can/can-queue.c | 112 +++++++
> > > cpukit/dev/can/can.c | 480
> > ++++++++++++++++++++++++++++++
> > > cpukit/include/dev/can/can.h | 115 +++++++
> > > spec/build/cpukit/librtemscpu.yml | 5 +
> > > 4 files changed, 712 insertions(+)
> > > create mode 100644 cpukit/dev/can/can-queue.c
> > > create mode 100644 cpukit/dev/can/can.c
> > > create mode 100644 cpukit/include/dev/can/can.h
> > >
> > > diff --git a/cpukit/dev/can/can-queue.c
> b/cpukit/dev/can/can-queue.c
> > > new file mode 100644
> > > index 0000000000..1cebed2ca4
> > > --- /dev/null
> > > +++ b/cpukit/dev/can/can-queue.c
> > > @@ -0,0 +1,112 @@
> > > +/* SPDX-License-Identifier: BSD-2-Clause */
> > > +
> > > +/**
> > > + * @file
> > > + *
> > > + * @ingroup CANBus
> > > + *
> > > + * @brief Controller Area Network (CAN) Bus Implementation
> > > + *
> > > + */
> > > +
> > > +/*
> > > + * Copyright (C) 2022
> > > + *
> > > + * Redistribution and use in source and binary forms, with or
> > without
> > > + * modification, are permitted provided that the following
> > conditions
> > > + * are met:
> > > + * 1. Redistributions of source code must retain the above
> copyright
> > > + * notice, this list of conditions and the following disclaimer.
> > > + * 2. Redistributions in binary form must reproduce the above
> > copyright
> > > + * notice, this list of conditions and the following disclaimer
> > in the
> > > + * documentation and/or other materials provided with the
> > distribution.
> > > + *
> > > + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND
> > CONTRIBUTORS "AS IS"
> > > + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> > LIMITED TO, THE
> > > + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
> > PARTICULAR PURPOSE
> > > + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR
> > CONTRIBUTORS BE
> > > + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL,
> > EXEMPLARY, OR
> > > + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,
> > PROCUREMENT OF
> > > + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS;
> > OR BUSINESS
> > > + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY,
> > WHETHER IN
> > > + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR
> > OTHERWISE)
> > > + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF
> > ADVISED OF THE
> > > + * POSSIBILITY OF SUCH DAMAGE.
> > > + */
> > > +
> > > +#include <rtems/imfs.h>
> > > +#include <rtems/thread.h>
> > > +
> > > +#include <dev/can/can.h>
> > > +
> > > +#include <string.h>
> > > +#include <stdlib.h>
> > > +#include <stdio.h>
> > > +
> > > +rtems_status_code can_create_rx_buffers(struct can_bus *bus)
> > > +{
> > > + static int init = 0;
> > > +
> > > + return rtems_message_queue_create(rtems_build_name('c', 'a',
> > 'n', '0' + init++), CAN_RX_BUF_COUNT, sizeof(struct can_msg),
> > > + RTEMS_FIFO | RTEMS_LOCAL,
> > &bus->rx_queue_id);
> >
> > Please make sure to stic to the RTEMS style and use a 80 character
> line
> > length.
> >
> > You create a name with a "'0' + init++": Are you sure that a maximum
> of
> > 10 buffers are created? Otherwise you will get names like "can:",
> > "can;"
> > and - sooner or later: "can\xFF", "can\x00", ...
> >
> > In the header you somewhere had a "destroy" function. Do you need
> some
> > way to de-initialize buffers only used by that bus?
> >
> > > +}
> > > +
> > > +rtems_status_code can_create_tx_buffers(struct can_bus *bus)
> > > +{
> > > + if ((bus->tx_fifo.pbuf = (struct can_msg
> > *)malloc(CAN_TX_BUF_COUNT * sizeof(struct can_msg))) == NULL) {
> >
> > You seem to like writing a lot of code in one line. From my point of
> > view that makes code harder to read. If you write it into multiple
> > lines
> > instead, someone else doesn't have to think that much about which
> > bracket closes where. For example in this case the following would be
> > simpler to read:
> >
> > ====
> > bus->tx_fifo.pbuf = (struct can_msg *)malloc(CAN_TX_BUF_COUNT *
> > sizeof(struct can_msg));
> > if (bus->tx_fifo.pbuf == NULL) {
> > ....
> > }
> > ====
> >
> > > + printf("malloc failed\n");
> >
> > Prints are nice for debugging but can be really annoying in
> > applications. Think about some kind of "debug_print" or similar that
> > you
> > can enable / disable with a macro at the start of the file. You can
> > also
> > use that to add a prefix or function name to all prints. If you
> > search a
> > problem, a line like "mallof failed" is meaningless if you are not
> > currently working on specially this code. A line like
> > "can_create_tx_buffers: malloc failed" is a lot more usefull. There
> are
> > preprocessor macros like __FUNCTION__ that you can use for something
> > like that.
> >
> > > + return RTEMS_NO_MEMORY;
> > > + }
> > > +
> > > + bus->tx_fifo.head = bus->tx_fifo.tail = 0;
> >
> > Same here: This is hard to read. I would suggest to split that into
> two
> > lines. Alternatively you might want to think about just using a
> memset
> > to have a clean start for a structure that you initialize.
> >
> > > + bus->tx_fifo.empty_count = CAN_TX_BUF_COUNT;
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +bool can_tx_buf_isempty(struct can_bus *bus)
> > > +{
> > > + if (bus->tx_fifo.empty_count <= 0) {
> > > + /* tx_fifo.empty_count does not go below zero, incase if it
> > goes update to zero */
> > > + bus->tx_fifo.empty_count = 0;
> >
> > empty_count is an unsigned type and therefore can never be smaller
> than
> > 0. This line is not necessary at all.
> >
> >
> > > +
> > > + return false;
> > > + }
> > > +
> > > + return true;
> > > +}
> > > +
> > > +struct can_msg *can_tx_get_data_buf(struct can_bus *bus)
> > > +{
> > > + struct can_msg *msg = NULL;
> > > +
> > > + if (bus->tx_fifo.empty_count == CAN_TX_BUF_COUNT) {
> > > + printf("can_tx_get_next_data_buf All buffers are empty\n");
> > > + return NULL;
> > > + }
> > > +
> > > + msg = &bus->tx_fifo.pbuf[bus->tx_fifo.tail];
> > > + bus->tx_fifo.empty_count++;
> > > + bus->tx_fifo.tail = (bus->tx_fifo.tail + 1) % CAN_TX_BUF_COUNT;
> >
> > You want to return the pointer to the message (msg) to the caller,
> > right? In that case you must not marke it as empty here. Otherwise
> > someone else could already use that memory again while the
> application
> > is still processing it.
> >
> > > +
> > > + return msg;
> > > +}
> > > +
> > > +struct can_msg *can_tx_get_empty_buf(struct can_bus *bus)
> > > +{
> > > + struct can_msg *msg = NULL;
> > > +
> > > + /* Check whether there is a empty CAN msg buffer */
> > > + if (can_tx_buf_isempty(bus) == false) {
> > > + printf("can_tx_get_empty_buf No empty buffer\n");
> > > + return NULL;
> > > + }
> > > +
> > > + bus->tx_fifo.empty_count--;
> > > +
> > > + /* tx_fifo.head always points to a empty buffer if there is
> > atleast one */
> > > + msg = &bus->tx_fifo.pbuf[bus->tx_fifo.head];
> > > + bus->tx_fifo.head = (bus->tx_fifo.head + 1) % CAN_TX_BUF_COUNT;
> > > +
> > > + return msg;
> > > +}
> > > diff --git a/cpukit/dev/can/can.c b/cpukit/dev/can/can.c
> > > new file mode 100644
> > > index 0000000000..e8aa9d20ca
> > > --- /dev/null
> > > +++ b/cpukit/dev/can/can.c
> > > @@ -0,0 +1,480 @@
> > > +/* SPDX-License-Identifier: BSD-2-Clause */
> > > +
> > > +/**
> > > + * @file
> > > + *
> > > + * @ingroup CANBus
> > > + *
> > > + * @brief Controller Area Network (CAN) Bus Implementation
> > > + *
> > > + */
> > > +
> > > +/*
> > > + * Copyright (C) 2022
> > > + *
> > > + * Redistribution and use in source and binary forms, with or
> > without
> > > + * modification, are permitted provided that the following
> > conditions
> > > + * are met:
> > > + * 1. Redistributions of source code must retain the above
> copyright
> > > + * notice, this list of conditions and the following disclaimer.
> > > + * 2. Redistributions in binary form must reproduce the above
> > copyright
> > > + * notice, this list of conditions and the following disclaimer
> > in the
> > > + * documentation and/or other materials provided with the
> > distribution.
> > > + *
> > > + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND
> > CONTRIBUTORS "AS IS"
> > > + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> > LIMITED TO, THE
> > > + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
> > PARTICULAR PURPOSE
> > > + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR
> > CONTRIBUTORS BE
> > > + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL,
> > EXEMPLARY, OR
> > > + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,
> > PROCUREMENT OF
> > > + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS;
> > OR BUSINESS
> > > + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY,
> > WHETHER IN
> > > + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR
> > OTHERWISE)
> > > + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF
> > ADVISED OF THE
> > > + * POSSIBILITY OF SUCH DAMAGE.
> > > + */
> > > +
> > > +#include <rtems/imfs.h>
> > > +#include <rtems/thread.h>
> > > +
> > > +#include <dev/can/can.h>
> > > +
> > > +#include <string.h>
> > > +#include <stdlib.h>
> > > +
> > > +#include <fcntl.h>
> > > +
> > > +static ssize_t can_bus_open(rtems_libio_t *iop, const char
> > *path, int oflag, mode_t mode);
> > > +static ssize_t can_bus_read(rtems_libio_t *iop, void *buffer,
> > size_t count);
> > > +static ssize_t can_bus_write(rtems_libio_t *iop, const void
> > *buffer, size_t count);
> > > +static ssize_t can_bus_ioctl(rtems_libio_t *iop, ioctl_command_t
> > request, void *buffer);
> > > +static int can_xmit(struct can_bus *bus);
> > > +
> > > +static void can_bus_obtain(can_bus *bus)
> > > +{
> > > + rtems_recursive_mutex_lock(&bus->mutex);
> > > +}
> > > +
> > > +static void can_bus_release(can_bus *bus)
> > > +{
> > > + rtems_recursive_mutex_unlock(&bus->mutex);
> > > +}
> > > +
> > > +static ssize_t can_bus_open(rtems_libio_t *iop, const char
> > *path, int oflag, mode_t mode)
> > > +{
> > > +/*
> > > + static int init = 0;
> > > +
> > > + if (init == 1) {
> > > + return 0;
> > > + }
> > > +
> > > + init = 1;
> > > +
> > > + can_bus *bus = IMFS_generic_get_context_by_iop(iop);
> > > +
> > > + if (bus == NULL) {
> > > + return -RTEMS_NOT_DEFINED;
> > > + }
> > > +
> > > + can_create_sem(bus);
> > > +*/
> > > + return 0;
> > > +}
> > > +
> > > +int can_receive(struct can_bus *bus, struct can_msg *msg)
> > > +{
> > > + int32_t ret = 0;
> > > +
> > > + uint32_t count = 0;
> > > +
> > > + if ((ret = rtems_message_queue_broadcast(bus->rx_queue_id,
> > msg, sizeof(struct can_msg) - (CAN_MSG_MAX_SIZE - msg->len),
> > &count)) != RTEMS_SUCCESSFUL) {
> > > + printf("rtems_message_queue_send failed\n");
> > > + }
> > > +
> > > + printf("rtems_message_queue_broadcast = %u\n", count);
> > > +
> > > + return ret;
> > > +}
> > > +
> > > +/* count argument is not used here as struct can_msg has the dlc
> > member */
> > > +static ssize_t can_bus_read(rtems_libio_t *iop, void *buffer,
> > size_t count)
> > > +{
> > > + int32_t ret = 0;
> > > + uint32_t flags = 0;
> > > +
> > > + can_bus *bus = IMFS_generic_get_context_by_iop(iop);
> > > +
> > > + if (bus == NULL || bus->can_dev_ops->dev_rx == NULL) {
> > > + return -RTEMS_NOT_DEFINED;
> > > + }
> > > +
> > > + struct can_msg *msg = (struct can_msg *)buffer;
> > > +
> > > + if ((iop->flags & O_NONBLOCK) != 0) {
> > > + flags = RTEMS_NO_WAIT;
> > > + }
> > > +
> > > + if ((ret = rtems_message_queue_receive(bus->rx_queue_id, (void
> > *)msg, &count, flags, 0)) != RTEMS_SUCCESSFUL) {
> > > + return ret;
> > > + }
> > > +
> > > + return count;
> > > +}
> > > +
> > > +int can_create_sem(struct can_bus *bus)
> >
> > Looks like a helper function that is only used in this file? Please
> > make
> > it static and remove it from the header. Same is true for "take_sem"
> > and
> > similar below.
> >
> > > +{
> > > + int ret = 0;
> > > +
> > > +#ifndef _POSIX_SEM_
> > > + //TODO: for test purpose
> > > + static int init = 0;
> > > +
> > > + ret = rtems_semaphore_create(rtems_build_name('c', 'a', 'n',
> > '0' + init++), 1,
> > > + RTEMS_FIFO | RTEMS_SIMPLE_BINARY_SEMAPHORE | RTEMS_LOCAL,
> > 0, &bus->tx_fifo_sem_id);
> > > +
> > > + if (ret != 0) {
> > > + printf("rtems_semaphore_create failed %d\n", ret);
> > > + return ret;
> > > + }
> > > +
> > > + ret = rtems_semaphore_obtain(bus->tx_fifo_sem_id, RTEMS_WAIT,
> > RTEMS_NO_TIMEOUT);
> > > +
> > > + if (ret != 0) {
> > > + printf("rtems_semaphore_obtain failed %d\n", ret);
> > > + return ret;
> > > + }
> > > +#else
> > > + ret = sem_init(&bus->tx_sem_id, 0, 1);
> > > +
> > > + if (ret != 0) {
> > > + printf("sem_init failed %d\n", ret);
> > > + return ret;
> > > + }
> > > +#endif /* _POSIX_SEM_ */
> > > + return ret;
> > > +}
> > > +
> > > +int take_sem(struct can_bus *bus)
> > > +{
> > > + int ret = 0;
> > > +
> > > +#ifndef _POSIX_SEM_
> > > + ret = rtems_semaphore_obtain(bus->tx_fifo_sem_id, RTEMS_WAIT,
> > RTEMS_NO_TIMEOUT);
> > > +#else
> > > + ret = sem_wait(&bus->tx_sem_id);
> > > +#endif /* _POSIX_SEM_ */
> > > +
> > > + return ret;
> > > +}
> > > +
> > > +int give_sem(struct can_bus *bus)
> > > +{
> > > + int ret = 0;
> > > +
> > > +#ifndef _POSIX_SEM_
> > > + ret = rtems_semaphore_release(bus->tx_fifo_sem_id);
> > > +#else
> > > + ret = sem_post(&bus->tx_sem_id);
> > > +#endif /* _POSIX_SEM_ */
> > > +
> > > + return ret;
> > > +}
> > > +
> > > +static int can_xmit(struct can_bus *bus)
> > > +{
> > > + int ret = 0;
> > > +
> > > + struct can_msg *msg = NULL;
> > > +
> > > + rtems_interrupt_lock_context lock_context;
> > > +
> > > + while (1) {
> > > + rtems_interrupt_lock_acquire(&bus->can_bus_lock,
> &lock_context);
> >
> > Is an interrupt lock really necessary? Wouldn't a mutex or similar
> work
> > too to protect the relevant data? An interrupt lock is something that
> > should be used really carefull. You tell the system: Stop everything
> > else. This here is the most important task.
> >
> > That means that for example a time critical data acquisition process
> > can
> > be delayed by the time that you need in your interrupt lock. You
> > have to
> > be really sure that the interrupt is the only solution to solve the
> > problem before you use one.
> >
> > > +
> > > + ret = bus->can_dev_ops->dev_tx_ready(bus->priv);
> > > +
> > > + if (ret != true) {
> > > + goto return_with_lock_release;
> > > + }
> > > +
> > > + msg = can_tx_get_data_buf(bus);
> > > +
> > > + if (msg == NULL) {
> > > + goto return_with_lock_release;
> > > + }
> > > +
> > > + ret = bus->can_dev_ops->dev_tx(bus->priv, msg);
> >
> > You have an interrupt lock here. Is "dev_tx" a function that is
> > guaranteed to be fast enough not to make problems with that?
> >
> > > +
> > > + if (ret != RTEMS_SUCCESSFUL) {
> > > + printf("dev_send failed\n");
> >
> > You have an interrupt lock here. You can't use a printf because that
> > uses interrupts.
> >
> > > + }
> > > +
> > > + rtems_interrupt_lock_release(&bus->can_bus_lock,
> &lock_context);
> > > +
> > > + //can_tx_done(bus);
> > > + }
> > > +
> > > + return RTEMS_SUCCESSFUL;
> > > +
> > > +return_with_lock_release:
> > > +
> > > + rtems_interrupt_lock_release(&bus->can_bus_lock,
> &lock_context);
> > > + return RTEMS_SUCCESSFUL;
> > > +}
> > > +
> > > +void can_print_canmsg(struct can_msg const *msg)
> >
> > I assume that's a debug only function. Otherwise I wouldn't be happy
> > with the output format.
> >
> > > +{
> > > +
> >
> printf("\n----------------------------------------------------------------------------------------------------------------------\n");
> > > + printf("id = %d len = %d flags = 0x%08X\n", msg->id, msg->len,
> > msg->flags);
> > > +
> > > + for (int i = 0; i < msg->len; i++) {
> > > + printf("%02x ", msg->data[i]);
> > > + }
> > > +
> > > +
> >
> printf("\n----------------------------------------------------------------------------------------------------------------------\n");
> > > +}
> > > +
> > > +int can_tx_done(struct can_bus *bus)
> > > +{
> > > + int ret = 0;
> > > +
> > > + if (bus->can_dev_ops->dev_tx_ready(bus) == true) {
> > > + can_xmit(bus);
> > > + }
> > > +
> > > + if (bus->can_tx_buf_waiters > 0 && (can_tx_buf_isempty(bus) !=
> > 0)) {
> > > + ret = give_sem(bus);
> > > + if (ret != 0) {
> > > +
> >
> printf(">>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
> > rtems_semaphore_release failed = %d\n", ret);
> > > + } else {
> > > + }
> > > + }
> > > +
> > > + return ret;
> > > +}
> > > +
> > > +static ssize_t can_bus_write(rtems_libio_t *iop, const void
> > *buffer, size_t count)
> > > +{
> > > + can_bus *bus = IMFS_generic_get_context_by_iop(iop);
> > > +
> > > + if (bus == NULL || bus->can_dev_ops->dev_tx == NULL) {
> > > + return -RTEMS_NOT_DEFINED;
> > > + }
> > > +
> > > + int32_t ret = 0;
> > > +
> > > + struct can_msg const *msg = buffer;
> > > + struct can_msg *fifo_buf = NULL;
> > > +
> > > + rtems_interrupt_lock_context lock_context;
> > > +
> > > + while (1) {
> > > + /* sleep is for debug purpose to test concurrency issues */
> > > + sleep(1);
> > > +
> > > + rtems_interrupt_lock_acquire(&bus->can_bus_lock,
> &lock_context);
> >
> > Same as above: Is an interrupt lock really necessary and the only
> > solution?
> >
> > > +
> > > + fifo_buf = can_tx_get_empty_buf(bus);
> > > +
> > > + if (fifo_buf != NULL) {
> > > + uint32_t msg_size = (char *)&msg->data[msg->len] - (char
> > *)msg;
> > > +
> > > + if (msg_size > sizeof(struct can_msg)) {
> > > + printf("can message len error msg_size = %u struct
> > can_msg = %u\n", msg_size, sizeof(struct can_msg));
> >
> > Again: Printf will not work during an interrupt lock.
> >
> > > + return -RTEMS_INVALID_SIZE;
> > > + }
> > > +
> > > + memcpy(fifo_buf, msg, msg_size); //sizeof(struct can_msg)
> > - (CAN_MSG_MAX_SIZE - msg->len));
> >
> > memcpy() in an interrupt lock is definitively takeing too much time.
> > You
> > have to be really convincing that I agree that it's OK.
> >
> > > + ret = msg_size;
> > > + //can_print_canmsg(msg);
> > > + }
> > > +
> > > + rtems_interrupt_lock_release(&bus->can_bus_lock,
> &lock_context);
> > > +
> > > + /* sleep is for debug purpose to test concurrency issues */
> > > + sleep(1);
> > > +
> > > + if (fifo_buf != NULL) {
> > > + break;
> > > + }
> > > +
> > > + /* Enters if clause, if there are no empty tx fifo buffers.
> > Based on the flags, sleep until buffer
> > > + * is empty or return error status
> > > + */
> > > +
> > > + if (fifo_buf == NULL) {
> > > + if ((iop->flags & O_NONBLOCK) != 0) {
> > > + return -RTEMS_RESOURCE_IN_USE;
> > > + }
> > > +
> > > + if (bus->can_dev_ops->dev_tx_ready(bus) == true) {
> > > + can_xmit(bus);
> > > + }
> > > +
> > > + rtems_interrupt_lock_acquire(&bus->can_bus_lock,
> > &lock_context);
> > > + ret = can_tx_buf_isempty(bus);
> > > + rtems_interrupt_lock_release(&bus->can_bus_lock,
> > &lock_context);
> >
> > If the "isempty" function needs an interupt_lock: Shouldn't it be
> part
> > of the function?
> >
> > And again: Wouldn't a mutex work to protect the data instead of
> > interrupt locks?
> >
> > > +
> > > + if (ret == true) {
> > > + continue;
> > > + }
> > > +
> > > + rtems_interrupt_lock_acquire(&bus->can_bus_lock,
> > &lock_context);
> > > + bus->can_tx_buf_waiters++;
> > > + rtems_interrupt_lock_release(&bus->can_bus_lock,
> > &lock_context);
> > > +
> > > + printf("empty_count = %u\n", bus->tx_fifo.empty_count);
> > > +
> > > + ret = take_sem(bus);
> > > +
> > > + rtems_interrupt_lock_acquire(&bus->can_bus_lock,
> > &lock_context);
> > > + bus->can_tx_buf_waiters--;
> > > + rtems_interrupt_lock_release(&bus->can_bus_lock,
> > &lock_context);
> > > +
> > > + if (ret != RTEMS_SUCCESSFUL) {
> > > + printf("cannot take semaphore\n");
> > > + ret = -RTEMS_INTERNAL_ERROR;
> > > + goto return_release_lock;
> > > + }
> > > + }
> > > + }
> > > +
> > > + if (bus->can_dev_ops->dev_tx_ready(bus) == true) {
> > > + can_xmit(bus);
> > > + }
> > > +
> > > +return_release_lock:
> >
> > Why is the label named "...release_lock"? It isn't releasing any
> lock.
> >
> > > +
> > > + return ret;
> > > +}
> > > +
> > > +static ssize_t can_bus_ioctl(rtems_libio_t *iop, ioctl_command_t
> > request, void *buffer)
> > > +{
> > > + can_bus *bus = IMFS_generic_get_context_by_iop(iop);
> > > +
> > > + if (bus == NULL || bus->can_dev_ops->dev_ioctl == NULL) {
> > > + return -RTEMS_NOT_DEFINED;
> > > + }
> > > +
> > > + can_bus_obtain(bus);
> > > +
> > > + bus->can_dev_ops->dev_ioctl(bus->priv, NULL, 0);
> > > +
> > > + can_bus_release(bus);
> > > +
> > > + return RTEMS_SUCCESSFUL;
> > > +}
> > > +
> > > +static const rtems_filesystem_file_handlers_r can_bus_handler = {
> > > + .open_h = can_bus_open,
> > > + .close_h = rtems_filesystem_default_close,
> > > + .read_h = can_bus_read,
> > > + .write_h = can_bus_write,
> > > + .ioctl_h = can_bus_ioctl,
> > > + .lseek_h = rtems_filesystem_default_lseek,
> > > + .fstat_h = IMFS_stat,
> > > + .ftruncate_h = rtems_filesystem_default_ftruncate,
> > > + .fsync_h = rtems_filesystem_default_fsync_or_fdatasync,
> > > + .fdatasync_h = rtems_filesystem_default_fsync_or_fdatasync,
> > > + .fcntl_h = rtems_filesystem_default_fcntl,
> > > + .kqfilter_h = rtems_filesystem_default_kqfilter,
> > > + .mmap_h = rtems_filesystem_default_mmap,
> > > + .poll_h = rtems_filesystem_default_poll,
> > > + .readv_h = rtems_filesystem_default_readv,
> > > + .writev_h = rtems_filesystem_default_writev
> > > +};
> > > +
> > > +static void can_bus_node_destroy(IMFS_jnode_t *node)
> > > +{
> > > + can_bus *bus;
> > > +
> > > + bus = IMFS_generic_get_context_by_node(node);
> > > + (*bus->destroy)(bus);
> > > +
> > > + IMFS_node_destroy_default(node);
> > > +}
> > > +
> > > +static const IMFS_node_control can_bus_node_control =
> > IMFS_GENERIC_INITIALIZER(&can_bus_handler,
> > > +
> > IMFS_node_initialize_generic, can_bus_node_destroy);
> > > +
> > > +rtems_status_code can_bus_register(can_bus *bus, const char
> > *bus_path)
> > > +{
> > > + int ret = RTEMS_SUCCESSFUL;
> > > +
> > > + ret = IMFS_make_generic_node(bus_path, S_IFCHR | S_IRWXU |
> > S_IRWXG | S_IRWXO, &can_bus_node_control, bus);
> > > +
> > > + if (ret != RTEMS_SUCCESSFUL) {
> > > + printf("Creating node failed: %d\n", ret);
> > > + goto fail;
> > > + }
> > > +
> > > + RTEMS_INTERRUPT_LOCK_INITIALIZER(bus->can_bus_lock);
> >
> > The RTEMS_INTERRUPT_LOCK_INITIALIZER is for a static initialization
> of
> > an interrupt lock. It won't work to initialize it dynamically here.
> >
> > > +
> > > + if ((ret = can_create_sem(bus)) != RTEMS_SUCCESSFUL) {
> > > + printf("can_create_sem failed = %d\n", ret);
> > > + goto fail;
> > > + }
> > > +
> > > + if ((ret = can_create_tx_buffers(bus)) != RTEMS_SUCCESSFUL) {
> > > + printf("can_create_tx_buffers failed = %d\n", ret);
> > > + goto fail;
> > > + }
> > > +
> > > + if ((ret = can_create_rx_buffers(bus)) != RTEMS_SUCCESSFUL) {
> > > + printf("can_create_rx_buffers failed = %d\n", ret);
> > > + goto fail;
> > > + }
> > > +
> > > + return ret;
> > > +
> > > +fail:
> > > + (*bus->destroy)(bus);
> > > + return ret;
> > > +
> > > +}
> > > +
> > > +static void can_bus_destroy(can_bus *bus)
> > > +{
> > > + rtems_recursive_mutex_destroy(&bus->mutex);
> > > +}
> > > +
> > > +static int can_bus_do_init(can_bus *bus, void (*destroy)(can_bus
> > *bus))
> > > +{
> > > + rtems_recursive_mutex_init(&bus->mutex, "CAN Bus");
> > > + bus->destroy = can_bus_destroy;
> > > +
> > > + return RTEMS_SUCCESSFUL;
> > > +}
> > > +
> > > +static void can_bus_destroy_and_free(can_bus *bus)
> > > +{
> > > + can_bus_destroy(bus);
> > > + free(bus);
> > > +}
> > > +
> > > +int can_bus_init(can_bus *bus)
> > > +{
> > > + memset(bus, 0, sizeof(*bus));
> > > +
> > > + return can_bus_do_init(bus, can_bus_destroy);
> > > +}
> > > +
> > > +can_bus *can_bus_alloc_and_init(size_t size)
> > > +{
> > > + can_bus *bus = NULL;
> > > +
> > > + if (size >= sizeof(*bus)) {
> > > + bus = calloc(1, size);
> > > + if (bus != NULL) {
> > > + int rv;
> > > +
> > > + rv = can_bus_do_init(bus, can_bus_destroy_and_free);
> > > + if (rv != 0) {
> > > + return NULL;
> > > + }
> > > + }
> > > + }
> > > +
> > > + return bus;
> > > +}
> > > diff --git a/cpukit/include/dev/can/can.h
> > b/cpukit/include/dev/can/can.h
> > > new file mode 100644
> > > index 0000000000..2210820504
> > > --- /dev/null
> > > +++ b/cpukit/include/dev/can/can.h
> > > @@ -0,0 +1,115 @@
> > > +/* SPDX-License-Identifier: BSD-2-Clause */
> > > +
> > > +/**
> > > + * @file
> > > + *
> > > + * @ingroup CANBus
> > > + *
> > > + * @brief Controller Area Network (CAN) Bus Implementation
> > > + *
> > > + */
> > > +
> > > +/*
> > > + * Copyright (C) 2022
> > > + *
> > > + * Redistribution and use in source and binary forms, with or
> > without
> > > + * modification, are permitted provided that the following
> > conditions
> > > + * are met:
> > > + * 1. Redistributions of source code must retain the above
> copyright
> > > + * notice, this list of conditions and the following disclaimer.
> > > + * 2. Redistributions in binary form must reproduce the above
> > copyright
> > > + * notice, this list of conditions and the following disclaimer
> > in the
> > > + * documentation and/or other materials provided with the
> > distribution.
> > > + *
> > > + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND
> > CONTRIBUTORS "AS IS"
> > > + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> > LIMITED TO, THE
> > > + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
> > PARTICULAR PURPOSE
> > > + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR
> > CONTRIBUTORS BE
> > > + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL,
> > EXEMPLARY, OR
> > > + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,
> > PROCUREMENT OF
> > > + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS;
> > OR BUSINESS
> > > + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY,
> > WHETHER IN
> > > + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR
> > OTHERWISE)
> > > + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF
> > ADVISED OF THE
> > > + * POSSIBILITY OF SUCH DAMAGE.
> > > + */
> > > +
> > > +#ifndef _DEV_CAN_CAN_H
> > > +#define _DEV_CAN_CAN_H
> > > +
> > > +#include <rtems/imfs.h>
> > > +#include <rtems/thread.h>
> > > +#include <semaphore.h>
> > > +
> > > +#include <string.h>
> > > +#include <stdlib.h>
> > > +#include <stdio.h>
> > > +
> >
> > A more or less general note for the header file: You want to
> introduce
> > it as a new general API. Please make sure to document that API
> enough.
> > Use doxygen-comments in the header file for that. For an example,
> > take a
> > look at cpukit/include/linux/i2c.h or some other headers.
> Documentation
> > is often really annoying to write as a developer but at the same time
> > it's really convenient to have it for all other developers.
> >
> > For me it's also not really clear whether you want to create a user
> > facing API with this file or a driver facing one.
> >
> > > +#define CAN_MSG_MAX_SIZE (8u)
> > > +
> > > +#define CAN_TX_BUF_COUNT (10u)
> > > +#define CAN_RX_BUF_COUNT (10u)
> >
> > Is that really a value that does match the needs of all drivers /
> > applications?
> >
> > > +
> > > +typedef struct can_dev_ops {
> > > + int32_t (*dev_rx)(void *priv, void *buffer);
> > > + int32_t (*dev_tx)(void *priv, void *buffer);
> > > + bool (*dev_tx_ready)(void *priv);
> > > + void (*dev_tx_int)(void *priv, bool);
> > > + int32_t (*dev_ioctl)(void *priv, void *buffer, size_t cmd);
> > > +} can_dev_ops;
> > > +
> > > +struct ring_buf {
> > > + struct can_msg *pbuf;
> > > + uint32_t head;
> > > + uint32_t tail;
> > > + uint32_t empty_count;
> > > +};
> > > +
> > > +typedef struct can_bus {
> > > + void *priv; // CAN controller
> > specific struct.
> > > + struct can_dev_ops *can_dev_ops; // CAN controller
> > operations.
> > > + struct ring_buf tx_fifo; // TX fifo
> > > +#ifndef _POSIX_SEM_
> > > + rtems_id tx_fifo_sem_id; // TX fifo counting
> > semaphore id
> > > +#else
> > > + sem_t tx_sem_id;
> > > + uint32_t tx_fifo_sem_name; // TX fifo sem name
> > > +#endif /* _POSIX_SEM_ */
> >
> > What's the advantage of implementing two types of semaphores
> depending
> > on the _POSIX_SEM_ macro instead of just using only one RTEMS-native
> > type?
> >
> > I haven't had a detailed look at the useage of the semaphores yet.
> > Personally I prefer the self-contained objects because they have the
> > advantage that the application-developers don't have to think about
> > them
> > when configuring their application:
> >
> >
> >
> https://docs.rtems.org/branches/master/c-user/self_contained_objects.html#
> > <
> https://docs.rtems.org/branches/master/c-user/self_contained_objects.html#
> >
> >
> > Not sure whether that would be an option here.
> >
> > > + uint32_t can_tx_buf_waiters;
> > > + rtems_id rx_queue_id;
> > > + uint32_t rx_queue_name;
> > > + rtems_recursive_mutex mutex; // For handling
> > driver concurrency.
> > > + RTEMS_INTERRUPT_LOCK_MEMBER(can_bus_lock);
> > > + void (*destroy)(struct can_bus *bus); // Clean the CAN
> > controller specific resources.
> > > +} can_bus;
> > > +
> > > +struct can_msg {
> > > + uint32_t id; // 32 bits to
> > support extended id (29 bits).
> > > + uint32_t timestamp;
> > > + uint16_t flags; // RTR | BRS | EXT
> ...
> > > + uint16_t len;
> > > + uint8_t data[CAN_MSG_MAX_SIZE]; // For supporting
> > data transfer up to 64 bytes.
> > > +};
> > > +
> > > +extern can_bus *can_bus_alloc_and_init(size_t size);
> > > +extern int can_bus_init(can_bus *bus);
> > > +extern rtems_status_code can_bus_register(can_bus *bus, const
> > char *bus_path);
> > > +extern int can_tx_done(struct can_bus *);
> > > +extern int can_receive(struct can_bus *, struct can_msg *);
> > > +
> > > +extern uint16_t can_len_to_dlc(uint16_t len);
> > > +extern uint16_t can_dlc_to_len(uint16_t dlc);
> > > +
> > > +extern rtems_status_code can_create_tx_buffers(struct can_bus
> *bus);
> > > +extern rtems_status_code can_create_rx_buffers(struct can_bus
> *bus);
> > > +extern bool can_tx_buf_isempty(struct can_bus *bus);
> > > +extern struct can_msg *can_tx_get_data_buf(struct can_bus *bus);
> > > +extern struct can_msg *can_tx_get_empty_buf(struct can_bus *bus);
> > > +
> > > +extern int take_sem(struct can_bus *);
> > > +extern int give_sem(struct can_bus *);
> > > +extern int can_create_sem(struct can_bus *bus);
> >
> > Some of the functions seem to be internal functions that are only
> used
> > in the framework (especially the ones with *_sem() but also the whole
> > create*buffers() stuff). You shouldn't expose them to the user. Make
> > them static or put them into some internal header.
> >
> > Best regards
> >
> > Christian
> >
> > > +
> > > +extern void can_print_canmsg(struct can_msg const *);
> > > +
> > > +#endif /* _DEV_CAN_CAN_H */
> > > diff --git a/spec/build/cpukit/librtemscpu.yml
> > b/spec/build/cpukit/librtemscpu.yml
> > > index 31a68cf85a..b700935f82 100644
> > > --- a/spec/build/cpukit/librtemscpu.yml
> > > +++ b/spec/build/cpukit/librtemscpu.yml
> > > @@ -50,6 +50,9 @@ install:
> > > - destination: ${BSP_INCLUDEDIR}/dev/spi
> > > source:
> > > - cpukit/include/dev/spi/spi.h
> > > +- destination: ${BSP_INCLUDEDIR}/dev/can
> > > + source:
> > > + - cpukit/include/dev/can/can.h
> > > - destination: ${BSP_INCLUDEDIR}/linux
> > > source:
> > > - cpukit/include/linux/i2c-dev.h
> > > @@ -530,6 +533,8 @@ source:
> > > - cpukit/dev/serial/sc16is752-spi.c
> > > - cpukit/dev/serial/sc16is752.c
> > > - cpukit/dev/spi/spi-bus.c
> > > +- cpukit/dev/can/can.c
> > > +- cpukit/dev/can/can-queue.c
> > > - cpukit/dtc/libfdt/fdt.c
> > > - cpukit/dtc/libfdt/fdt_addresses.c
> > > - cpukit/dtc/libfdt/fdt_empty_tree.c
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.rtems.org/pipermail/devel/attachments/20220719/2c993223/attachment-0001.htm>
More information about the devel
mailing list