Re: [PATCH] cpukit/dev/can: Added CAN support

[Christian MAUDERER]

Hello Prashanth,

Am 08.08.22 um 15:49 schrieb Prashanth S:

Hi Christian,



We have Chris Johns and Christian Mauderer on the list. In this case 
Chris has sent the mail. To avoid confusion, I never use a short form of 
my name on the list. But it's easy to mix up so don't worry about it.


Oops, I apologize. I think I used gmail style quotes that might have 
added html.

I will not use it.


Your mail client added both formats: Text and HTML, regardless whether 
you use formatting or not. For example your response was mixed too. 
Interesting is that the mail client managed to use classic usenet style 
for the text format and some odd HTML with colors for HTML. That's an 
interesting behavior.


For mailing lists it's better to send plain-text-only mails (without the 
additional HTML stuff). If you use the gmail Web-Client, there seems to 
be an option hidden in some menu for that:



https://www.lifewire.com/how-to-send-a-message-in-plain-text-from-gmail-1171963

Best regards

Christian



Regards
Prashanth S

On Mon, 8 Aug 2022 at 05:31, Chris Johns > wrote:


Hi

Could you please configure your email client to not send HTML emails
to the list?

I am reluctant to enable filtering in mailman and have managed to
avoid it so far.

Thanks
Chris


___
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel


--

embedded brains GmbH
Herr Christian MAUDERER
Dornierstr. 4
82178 Puchheim
Germany
email:  christian.maude...@embedded-brains.de
phone:  +49-89-18 94 741 - 18
mobile: +49-176-152 206 08

Registergericht: Amtsgericht München
Registernummer: HRB 157899
Vertretungsberechtigte Geschäftsführer: Peter Rasmussen, Thomas Dörfler
Unsere Datenschutzerklärung finden Sie hier:
https://embedded-brains.de/datenschutzerklaerung/
___
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel

Re: [PATCH] cpukit/dev/can: Added CAN support

[Prashanth S]

Hi Christian,

Oops, I apologize. I think I used gmail style quotes that might have added
html.
I will not use it.

Regards
Prashanth S

On Mon, 8 Aug 2022 at 05:31, Chris Johns  wrote:

> Hi
>
> Could you please configure your email client to not send HTML emails to
> the list?
>
> I am reluctant to enable filtering in mailman and have managed to avoid it
> so far.
>
> Thanks
> Chris
>
___
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel

Re: [PATCH] cpukit/dev/can: Added CAN support

[Chris Johns]

Hi

Could you please configure your email client to not send HTML emails to the 
list?

I am reluctant to enable filtering in mailman and have managed to avoid it so 
far.

Thanks
Chris
___
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel

Re: [PATCH] cpukit/dev/can: Added CAN support

[Prashanth S]

Hi Christian,

general note: You add an API to cpukit. Most APIs there should have a
> test case in the testsuite. In this case the test case would have to
> create some dummy CAN driver and check whether everything works like
> expected. As an example take a look at the SPI test:
>
>https://git.rtems.org/rtems/tree/testsuites/libtests/spi01/init.c
>
> It creates a dummy device and checks whether the SPI layer does
> everything as expected. Would be great if you could add something like
> this. Target of such a test should be more or less that every branch in
> your code is triggered at least once (code coverage). It doesn't has to
> be every combination but it shows that the code is at least reachable
> and works as expected.
>
Adding a test application in testsuites/libtests/can01/init.c

Do you really mean "LOCK" or should it be a "LOG"? Both is reasonable
> here so I'm not entirely sure. But you use the LOCK macro also in
> can_xmit and similar so I'm a bit unsure.
>
It is a log for the lock. Now moved the logs and call to the lock
acquire/release to #defines.

> +if (sem_count > CAN_TX_BUF_COUNT) {
>
> If you increment sem_count only in a debug macro, you must not use it
> for any other code. The macro is processed by the preprocessor. So if
> you define the CAN_DEBUG_LOCK to something empty, the ++sem_count from
> above will never happen. If CAN_DEBUG_LOCK interpretes the argument
> twice, you will get the wrong value.
>
> Please note: It's in general a good idea not to use anything that
> changes a variable or has any side effect in a macro. For example if I
> define the following
>
>#define SQUARE(x) ((x) * (x))
>
> And use it like this:
>
>int a = 2;
>int b;
>b = SQUARE(++a);
>
> The result that I would expect from reading the code would be that b is
> 9 and a is 3 afterwards. But instead I will get b = 16 and a = 4. The
> reason for this is that the preprocessor will replace the code with the
> following one:
>
>int a = 2;
>int b;
>b = ((++a) * (++a));
>
> Therefore: Try to avoid statements with a side effect as a parameter to
> something if you are not 100% sure that the something is a function or
> is allways executed. You don't have to save lines. We don't have a limit
> on the maximum lines in a file. Readability is much more important than
> compact code.
>
> PS: Also true for "if". I wouldn't be sure whether the printf in the
> following code will be executed:
>
>if (0 || printf("Will someone call me?")) {
>}
>
> In general: Try to avoid anything where you have to think about the
> order of execution except if you can't avoid it or if it is really obvious.

Moved the increment out of the #define.

> +static int can_xmit(struct can_bus *bus)
> > +{
> > +  int ret = RTEMS_SUCCESSFUL;
> > +
> > +  struct can_msg *msg = NULL;
> > +
> > +  while (1) {
> > +CAN_DEBUG_LOCK("can_xmit: acquiring lock\n");
> > +can_interrupt_lock_acquire(bus);
> > +
> > +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);
> > +if (ret != RTEMS_SUCCESSFUL) {
> > +printf("can_xmit: dev_send failed\n");
> > +}
> > +
> > +ret = give_sem(bus);
> > +if (ret != RTEMS_SUCCESSFUL) {
> > +  printf("can_tx_done: rtems_semaphore_release failed = %d\n", ret);
> > +}
> > +
> > +CAN_DEBUG_LOCK("can_xmit: releasing lock\n");
> > +can_interrupt_lock_release(bus);
> > +
> > +//can_tx_done(bus);
> > +  }
> > +
> > +  return ret;
>
> If I see it correctly, you can never reach this return. You have a
> while(1) without any break condition. By the way: Code with goto, loops
> and break tends to get sphaghetti code. You should only use these
> constructions if it improves readability compared to a solution without
> these.


Updated to release the lock and break.

You use the macros a lot. You are aware that you could do something like
> that:
>
> - Declare the original can_interrupt_lock_acquire() as something with a
> prefix or postifix for example as real_can_interrupt_lock_acquire.
>
> - Then add a define that wrapps it with:
>
> #define can_interrupt_lock_acquire(...) do { \
>  CAN_DEBUG_LOCK("%s:%d\n", __FILE__, __LINE__); \
>  real_can_interrupt_lock_acquire(__VA_ARGS__);
>  } while(0)
>

Then you can just write it as normal can_interrupt_lock_acquire but get
> a debug print with file and line on every call.
>
now moved the logs and call to the lock acquire/release to #defines.

> +can_interrupt_lock_acquire(bus);
> > +bus->can_tx_buf_waiters++;
> > +CAN_DEBUG_LOCK("can_bus_write: release lock
> can_tx_buf_waiters++\n");
> > +can_interrupt_lock_release(bus);
> > +
> > +ret = take_sem(bus);
> > +
> > +CAN_DEBUG_LOCK("can_bus_write: 

Re: [PATCH] cpukit/dev/can: Added CAN support

[oss]

Hello Duc,

general note: You add an API to cpukit. Most APIs there should have a 
test case in the testsuite. In this case the test case would have to 
create some dummy CAN driver and check whether everything works like 
expected. As an example take a look at the SPI test:


  https://git.rtems.org/rtems/tree/testsuites/libtests/spi01/init.c

It creates a dummy device and checks whether the SPI layer does 
everything as expected. Would be great if you could add something like 
this. Target of such a test should be more or less that every branch in 
your code is triggered at least once (code coverage). It doesn't has to 
be every combination but it shows that the code is at least reachable 
and works as expected.


Am 31.07.22 um 13:53 schrieb Prashanth S:

---
  cpukit/dev/can/can.c   | 514 +
  cpukit/include/dev/can/can-msg.h   |  55 +++
  cpukit/include/dev/can/can-queue.h | 127 +++
  cpukit/include/dev/can/can.h   | 102 ++
  spec/build/cpukit/librtemscpu.yml  |   6 +
  5 files changed, 804 insertions(+)
  create mode 100644 cpukit/dev/can/can.c
  create mode 100644 cpukit/include/dev/can/can-msg.h
  create mode 100644 cpukit/include/dev/can/can-queue.h
  create mode 100644 cpukit/include/dev/can/can.h

diff --git a/cpukit/dev/can/can.c b/cpukit/dev/can/can.c
new file mode 100644
index 00..8feec8800b
--- /dev/null
+++ b/cpukit/dev/can/can.c
@@ -0,0 +1,514 @@
+/* SPDX-License-Identifier: BSD-2-Clause */
+
+/**
+ * @file
+ *
+ * @ingroup CANBus
+ *
+ * @brief Controller Area Network (CAN) Bus Implementation
+ *
+ */
+
+/*
+ * Copyright (C) 2022 Prashanth S (fishesprasha...@gmail.com)
+ *
+ * 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 
+#include 
+
+#include 
+#include 
+
+#include 
+#include 
+
+#include 
+
+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 int can_create_sem(struct can_bus *);
+static int try_sem(struct can_bus *);
+static int take_sem(struct can_bus *);
+static int give_sem(struct can_bus *);
+
+/* sem_count this is for debug purpose, for debugging
+the take_sem and give_sem
+*/
+static int sem_count = 0;
+
+static void can_bus_obtain(can_bus *bus)
+{
+  rtems_recursive_mutex_lock(>mutex);
+}
+
+static void can_bus_release(can_bus *bus)
+{
+  rtems_recursive_mutex_unlock(>mutex);
+}
+
+int can_create_sem(struct can_bus *bus)
+{
+  int ret = 0;
+
+  ret = rtems_semaphore_create(rtems_build_name('c', 'a', 'n', bus->index),
+  CAN_TX_BUF_COUNT, RTEMS_FIFO | RTEMS_COUNTING_SEMAPHORE | RTEMS_LOCAL,
+  0, >tx_fifo_sem_id);
+
+  if (ret != 0) {
+printf("can_create_sem: rtems_semaphore_create failed %d\n", ret);
+  }
+
+  return ret;
+}
+
+static void can_interrupt_lock_acquire(struct can_bus *bus)
+{
+  can_bus_obtain(bus);
+  bus->can_dev_ops->dev_int(bus->priv, false);
+}
+
+static void can_interrupt_lock_release(struct can_bus *bus)
+{
+  bus->can_dev_ops->dev_int(bus->priv, true);
+  can_bus_release(bus);
+}
+
+static int take_sem(struct can_bus *bus)
+{
+  int ret = rtems_semaphore_obtain(bus->tx_fifo_sem_id, RTEMS_WAIT,
+RTEMS_NO_TIMEOUT);
+  if (ret == RTEMS_SUCCESSFUL) {
+CAN_DEBUG_LOCK("take_sem: Counting semaphore count = %d\n", ++sem_count);



Do you really mean "LOCK" or should it be a "LOG"? Both is reasonable 
here so I'm not entirely sure. But you use the LOCK macro 

[PATCH] cpukit/dev/can: Added CAN support (Prashanth S)

[Prashanth S]

Hi All,

This is a review request for CAN support. I have addressed the review
comments.

The files are work in progress, I need to add few more comments
in the include (*.h) files.

Regards
Prashanth S

On Sun, 31 Jul 2022 at 17:24,  wrote:

> Send devel mailing list submissions to
> devel@rtems.org
>
> To subscribe or unsubscribe via the World Wide Web, visit
> http://lists.rtems.org/mailman/listinfo/devel
> or, via email, send a message with subject or body 'help' to
> devel-requ...@rtems.org
>
> You can reach the person managing the list at
> devel-ow...@rtems.org
>
> When replying, please edit your Subject line so it is more specific
> than "Re: Contents of devel digest..."
>
>
> Today's Topics:
>
>1. Re: [PATCH v5 1/4] bsps/stm32f4 Include STM32F4 HAL (Karel Gardas)
>2. Re: [PATCH v5 1/4] bsps/stm32f4 Include STM32F4 HAL
>   (o...@c-mauderer.de)
>3. [PATCH] cpukit/dev/can: Added CAN support (Prashanth S)
>
>
> --
>
> Message: 1
> Date: Sat, 30 Jul 2022 21:41:19 +0200
> From: Karel Gardas 
> To: o...@c-mauderer.de, Duc Doan , devel@rtems.org
> Subject: Re: [PATCH v5 1/4] bsps/stm32f4 Include STM32F4 HAL
> Message-ID: <2af4f901-b157-c0b0-cc75-45921bf788eb@functional.vision>
> Content-Type: text/plain; charset=UTF-8; format=flowed
>
> On 7/30/22 16:32, o...@c-mauderer.de wrote:
> >> ? bsps/arm/include/cmsis_compiler.h |?? 266 +
> >> ? bsps/arm/include/cmsis_gcc.h? |? 3460 +--
> >> ? bsps/arm/include/cmsis_version.h? |??? 39 +
> >> ? bsps/arm/include/core_cm4.h?? |?? 524 +-
> >> ? bsps/arm/include/core_cm7.h?? |? 5186 ++--
> >> ? bsps/arm/include/mpu_armv7.h? |?? 270 +
> >
> > Are the cmsis files from the same source or directly from ARM?
> >
> > The cmsis_gcc.h has a lot of changes compared to the earlier version
> > that has been present in RTEMS. A lot of the changes seem to be
> > whitespace changes. Can these be avoided somehow (for example by using
> > dos2unix before overwriting the file)?
> >
> > In the discord chat there was one suggestion from Ho Kaido to move the
> > files one level down and make them BSP specific. I'm not sure whether
> > I'm for or against that idea. Advantage is that it makes BSPs
> > independant from each other. Disadvantage is that it duplicates code.
> >
> > I think I would try to avoid moving them down due to the code
> > duplication but it raises the question: Which BSPs use the files too and
> > did you try whether they still compile after the upgrade?
>
> We have had this dicussion with Duc on discord IIRC when he started. He
> needed new CMSIS (v5) version due to new HAL which Duc claims depends on
> them. I have not verified that claim personally.
>
> New CMSIS v5 brings obviously:
>
> - by ARM maintained code (v4 is unmaintained IIRC)
>
> but also:
>
> - license change from BSD to Apache-2
>
> At that time I've told Duc to continue with the code and not to worry
> about license changes -- as this would be longer discussion anyway. Not
> sure, but IIRC he also wrote to Sebastian asking for clarification --
> well, not sure about that. Certainly IIRC I suggested that.
>
> Anyway, I took Duc code and try H7 BSPs and to my surprise they compiles
> more or less all without any compilation related issue. Well, I've not
> tried M4 variants. So far I've not run full tester on this. I'll, but
> first I'd like to test his API if it's possible to also use with H7.
>
> BTW: if RTEMS prefer old unmaintained BSD-3 ARM CSMIS code, then it's
> perhaps possible to go in F4 HAL history back and grab just the three
> with the v4 dependency. On the other hand, for ARM Apache-2 seems to be
> the way forward and for some ST.com depended code too -- so I guess
> RTEMS project will need to live with that fact somehow.
>
> Thanks,
> Karel
>
>
> --
>
> Message: 2
> Date: Sat, 30 Jul 2022 22:19:54 +0200
> From: o...@c-mauderer.de
> To: Karel Gardas , Duc Doan
> , devel@rtems.org
> Subject: Re: [PATCH v5 1/4] bsps/stm32f4 Include STM32F4 HAL
> Message-ID: <3628b5b2-0848-304f-9738-f6d64e9bb...@c-mauderer.de>
> Content-Type: text/plain; charset=UTF-8; format=flowed
>
>
>
>
> Am 30.07.22 um 21:41 schrieb Karel Gardas:
> > On 7/30/22 16:32, o...@c-mauderer.de wrote:
> >>> ? bsps/arm/include/cmsis_compiler.h |?? 266 +
> >>> ? bsps/arm/include/cmsis_gcc.h? |? 3460 +--
> >>> ? bsps/a

[PATCH] cpukit/dev/can: Added CAN support

[Prashanth S]

---
 cpukit/dev/can/can.c   | 514 +
 cpukit/include/dev/can/can-msg.h   |  55 +++
 cpukit/include/dev/can/can-queue.h | 127 +++
 cpukit/include/dev/can/can.h   | 102 ++
 spec/build/cpukit/librtemscpu.yml  |   6 +
 5 files changed, 804 insertions(+)
 create mode 100644 cpukit/dev/can/can.c
 create mode 100644 cpukit/include/dev/can/can-msg.h
 create mode 100644 cpukit/include/dev/can/can-queue.h
 create mode 100644 cpukit/include/dev/can/can.h

diff --git a/cpukit/dev/can/can.c b/cpukit/dev/can/can.c
new file mode 100644
index 00..8feec8800b
--- /dev/null
+++ b/cpukit/dev/can/can.c
@@ -0,0 +1,514 @@
+/* SPDX-License-Identifier: BSD-2-Clause */
+
+/**
+ * @file
+ *
+ * @ingroup CANBus
+ *
+ * @brief Controller Area Network (CAN) Bus Implementation
+ *
+ */
+
+/*
+ * Copyright (C) 2022 Prashanth S (fishesprasha...@gmail.com)
+ *
+ * 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 
+#include 
+
+#include 
+#include 
+
+#include 
+#include 
+
+#include 
+
+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 int can_create_sem(struct can_bus *);
+static int try_sem(struct can_bus *);
+static int take_sem(struct can_bus *);
+static int give_sem(struct can_bus *);
+
+/* sem_count this is for debug purpose, for debugging
+the take_sem and give_sem
+*/
+static int sem_count = 0;
+
+static void can_bus_obtain(can_bus *bus)
+{
+  rtems_recursive_mutex_lock(>mutex);
+}
+
+static void can_bus_release(can_bus *bus)
+{
+  rtems_recursive_mutex_unlock(>mutex);
+}
+
+int can_create_sem(struct can_bus *bus)
+{
+  int ret = 0;
+
+  ret = rtems_semaphore_create(rtems_build_name('c', 'a', 'n', bus->index), 
+  CAN_TX_BUF_COUNT, RTEMS_FIFO | RTEMS_COUNTING_SEMAPHORE | RTEMS_LOCAL, 
+  0, >tx_fifo_sem_id);
+
+  if (ret != 0) {
+printf("can_create_sem: rtems_semaphore_create failed %d\n", ret);
+  }
+
+  return ret;
+}
+
+static void can_interrupt_lock_acquire(struct can_bus *bus)
+{
+  can_bus_obtain(bus);
+  bus->can_dev_ops->dev_int(bus->priv, false);
+}
+
+static void can_interrupt_lock_release(struct can_bus *bus)
+{
+  bus->can_dev_ops->dev_int(bus->priv, true);
+  can_bus_release(bus);
+}
+
+static int take_sem(struct can_bus *bus)
+{
+  int ret = rtems_semaphore_obtain(bus->tx_fifo_sem_id, RTEMS_WAIT, 
+RTEMS_NO_TIMEOUT);
+  if (ret == RTEMS_SUCCESSFUL) {
+CAN_DEBUG_LOCK("take_sem: Counting semaphore count = %d\n", ++sem_count);
+if (sem_count > CAN_TX_BUF_COUNT) {
+  printf("take_sem error: sem_count is misleading\n");
+  while (1);
+}
+  }
+
+  return ret;
+}
+
+static int give_sem(struct can_bus *bus)
+{
+  int ret = rtems_semaphore_release(bus->tx_fifo_sem_id);
+  if (ret == RTEMS_SUCCESSFUL) {
+CAN_DEBUG_LOCK("give_sem: Counting semaphore count = %d\n", --sem_count);
+if (sem_count < 0) {
+  printf("give_sem error: sem_count is misleading\n");
+  while (1);
+}
+  }
+
+  return ret;
+}
+
+static int try_sem(struct can_bus *bus)
+{
+  int ret = rtems_semaphore_obtain(bus->tx_fifo_sem_id, RTEMS_NO_WAIT, 
+RTEMS_NO_TIMEOUT);
+  if (ret == RTEMS_SUCCESSFUL) {
+CAN_DEBUG_LOCK("take_sem: Counting semaphore count = %d\n", ++sem_count);
+if (sem_count > CAN_TX_BUF_COUNT) {
+  printf("take_sem error: sem_count is misleading\n");
+  while 

Re: [PATCH] cpukit/dev/can: Added CAN support

[Christian Mauderer]

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 = >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 

Re: [PATCH] cpukit/dev/can: Added CAN support

[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.

> 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.

>> +}
>> +
>> +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.

> +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 = >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.

> 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 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 

Re: [PATCH] cpukit/dev/can: Added CAN support

[oss]

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 00..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 
+#include 
+
+#include 
+
+#include 
+#include 
+#include 
+
+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, 
>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 

[PATCH] cpukit/dev/can: Added CAN support

[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 00..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 
+#include 
+
+#include 
+
+#include 
+#include 
+#include 
+
+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, 
>rx_queue_id);
+}
+
+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) {
+printf("malloc failed\n");
+return RTEMS_NO_MEMORY;
+  }
+
+  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;
+
+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 = >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;
+
+  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 = >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 00..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,