[Xenomai-core] [ANNOUNCE] RTDM driver for CAN devices

[Xenomai-core] [ANNOUNCE] RTDM driver for CAN devices

[Wolfgang Grandegger]

[-- Attachment #1: Type: text/plain, Size: 977 bytes --]

Hello,

at "ftp://ftp.denx.de/pub/xenomai/rtcan" you can find a first version of
RTCAN, an Open Source hard real-time protocol stack for CAN devices
based on BSD sockets. It is based on the SJA1000 socket-based CAN driver
for RTDM by Sebastian Smolorz (see CREDITS file).

Currently it consist of a RTDM driver patch for Xenomai and a tar
archive with the RTCAN utilities. I have attached the README for further
details. It would be nice if the RTCAN patch could be integrated into
Xenomai. As this driver is bigger and more complex than the other RTDM
drivers, it rises a few issues with RTDM driver integration and maintenance:

- Where to put a few utility or test programs
- Where to put miscellaneous files like README, CREDITS, etc.
- Where to discuss RTCAN related question.

For the moment, please report CAN related bugs, questions and comments
to the Socketcan mailing list "Socketcan-users@domain.hid" at
http://developer.berlios.de/projects/socketcan/

Wolfgang.



[-- Attachment #2: README --]
[-- Type: text/plain, Size: 9048 bytes --]

RTCAN is an Open Source hard real-time protocol stack for CAN devices
based on BSD sockets. This implementation is for RTDM, the Real-Time-
Driver-Model. Note that there is a similar variant being developed for
standard Linux using the Linux networking stack.


Status:
------

Note: this is a preliminary version of the RTCAN RTDM driver!

It is available for the following CAN controller and devices:

   SJA1000 ISA devices
   SJA1000 PEAK PCI card
   SJA1000 PEAK parallel port Dongle
   MSCAN for MPC5200 boards

Currently it consist of a RTDM driver patch for Xenomai and a tar 
archive with the RTCAN utilities:

   rtcan-xenomai-0.20.1.patch
   rtcan-utils-0.20.1.tar.bz2

Likely some internals will change sooner than later, e.g. it might be
integrated into Xenomai. Nevertheless, the API is already settled well
as a result of discussions on the "Socketcan" mailing list 
(Socketcan-users@domain.hid)


Installation:
------------

This example installation is for the DENX "linuxppc_2_4_devel" tree
(Linux 2.4.25) using the ELDK (see http://www.denx.de). It works in a
similar way for other kernels and distributions including Linux 2.6.


o General part:

  - Download the Xenomai distribution from the SVN repository, apply 
    the RTCAN patch and run "bootstrap":

    $ svn co http://svn.gna.org/svn/xenomai/trunk xenomai
    $ export XENO_ROOT=$PWD/xenomai
    $ cd $XENO_ROOT
    $ patch -p1 < /tmp/rtcan-xenomai-0.20.1.patch
    $ scripts/bootstrap

    The "bootstrap" is necessary to get "include/rtdm/rtcan.h" copied
    to the installation directory with make install. Alternatively,
    you can copy it by hand. 
    

o Kernel space part:

  - Please install the Xenomai kernel space part as described in the
    README.INSTALL. The following variable should point to the Xenomai
    patched kernel:

    $ export KERNEL_ROOT=/home/wolf/test/linuxppc_2_4_devel-xenomai

  - Configure RTCAN as kernel modules as required by your hardware 
    (and make sure that loadable module support is enabled):

    $ cd $KERNEL_ROOT
    $ export CROSS_COMPILE=ppc_82xx-
    $ make menuconfig
    ... Select "Loadable module support  --->" 
    [*] Enable loadable module support 
    ... Exit
    ... Select "Real-time sub-system --->"
               "Real-time drivers --->" 
                 "CAN bus controller --->"
    [M] RTCAN raw socket interface (NEW)
    (1024) Size of receive ring buffers (must be 2^N) (NEW)
    (4) Maximum number of devices (NEW)
    (16) Maximum number of receive filters per device (NEW)
    [M] MSCAN driver for MPC5200 (NEW)
    [*] Enable CAN 1 (NEW)
    [*] Enable CAN 2 (NEW)
    (66000000) Clock Frequency in Hz (NEW)
    (I2C1/TMR01) Pin Configuration
    <M> Philips SJA1000 CAN controller (NEW)
    <M>   Standard ISA devices
    (4)   Maximum number of ISA devices (NEW)
    <M>   PEAK PCI cards
    ... Exit and save

    Note: you can also statically link the MSCAN drivers into 
    the kernel.


  - Make the Linux kernel and RTCAN modules and copy them to the root
    filesystem:

    $ make dep
    $ make uImage
    $ cp -p arch/ppc/boot/images/uImage /tftpboot/icecube/uImage-rtcan
    $ make modules
    $ export DESTDIR=/opt/eldk/ppc_82xx
    $ make modules_install INSTALL_MOD_PATH=$DESTDIR
    $ find $DESTDIR/lib/modules/2.4.25/kernel/drivers/xenomai/rtcan
    .../rtcan
    .../rtcan/xeno_rtcan.o
    .../rtcan/mscan
    .../rtcan/mscan/xeno_rtcan_mscan.o
    .../rtcan/sja1000/xeno_rtcan_sja1000.o
    .../rtcan/sja1000/xeno_rtcan_peak_pci.o
    .../rtcan/sja1000/xeno_rtcan_isa.o


o User space part:

  - Please install the Xenomai user space part as described in the
    README.INSTALL. You need the path to the Xenomai installation 
    directory to make the RTCAN utilities.

  - Unpack the RTCAN utility distribution:

    $ tar xjf rtcan-utils-0.20.1.tar.bz2
    $ export RTCAN_ROOT $PWD/rtcan-utils-0.20.1

  - Make the RTCAN utilities and install them:

    $ cd $RTCAN_ROOT
    $ export XENO_INSTDIR=/opt/eldk/ppc_82xx/usr/xenomai
    $ make
    $ make install


Running and using RTCAN:
-----------------------

Now boot the Xenomai enabled kernel on your target system.

In case RTCAN is built as kernel modules, you need to load them using
modprobe or insmod, e.g. for this example build:

  # export MODDIR=/lib/modules/2.4.25/kernel/drivers/xenomai/rtcan
  # insmod $MODDIR/xeno_rtcan.o
  # insmod $MODDIR/mscan/xeno_rtcan_mscan.o
  # insmod $MODDIR/sja1000/xeno_rtcan_sja1000.o
  # insmod $MODDIR/sja1000/xeno_rtcan_peak_pci.o

Note that various kernel module parameters can be passed with insmod.
Please use "modinfo" to list them or check the corresponding source 
code files for further information

There are a few RTCAN utilities to configure RTCAN and to send and 
receive CAN messages, which have been installed in the Xenomai
installation directory with "make install":

  # export XENO_ROOT=/usr/xenomai
  # export PATH=$PATH:$XENO_ROOT/bin
  # export LD_LIBRARY_PATH=$XENO_ROOT/lib

  # rtcanconfig --help
  Usage: rtcanconfig <can-interface> [Options] [up|down|start|stop|sleep]
  Options:
   -v, --verbose            be verbose
   -h, --help               this help
   -c, --ctrlmode=M1:M2:... listenonly or loopback mode
   -b, --baudrate=BPS       baudrate in bits/sec
   -B, --bittime=BTR0:BTR1  BTR or standard bit-time
   -B, --bittime=BRP:PROP_SEG:PHASE_SEG1:PHASE_SEG2:SJW:SAM

  # rtcanrecv --help
  Usage: rtcanrecv <can-interface> [Options]
  Options:
   -f  --filter=id:mask[:id:mask]... apply filter
   -e  --error=mask      receive error messages
   -t, --timeout=MS      timeout in ms
   -v, --verbose         be verbose
   -p, --print=MODULO    print every MODULO message
   -n, --name=STRING     name of the RT task
   -h, --help            this help

  # rtcansend --help
  Usage: rtcansend <can-interface> [Options] <can-msg>
  <can-msg> can consist of up to 8 bytes given as a space separated list
  Options:
   -i, --identifier=ID   CAN Identifier (default = 1)
   -r  --rtr             send remote request
   -e  --extended        send extended frame
   -l  --loop=COUNT      send message COUNT times
   -c, --count           message count in data[0-3]
   -d, --delay=MS        delay in ms (default = 1ms)
   -t, --timeout=MS      timeout in ms
   -v, --verbose         be verbose
   -p, --print=MODULO    print every MODULO message
   -h, --help            this help

Here are a few self-explanary commands:

  # rtcanconfig rtcan0 --baudrate=125000 start

  # rtcansend rtcan2 --verbose --identifier=0x123 0xde 0xad
  <0x123> [2] de ad

  # rtcanrecv rtcan0 --verbose
  #1: <0x123> [2] de ad

  bash-2.05b# rtcanrecv rtcan0 --filter=0x120:0x120
  Filter #0: id=0x00000120 mask=0x00000120
  #0: <0x124> [2] de ad
  #1: <0x123> [3] 12 34 56
  #2: <0x133> [4] 11 22 33 44

  # rtcanrecv rtcan0 --error=0xffff
  #1: !0x00000008! [8] 00 00 80 19 00 00 00 00 ERROR


PROC filesystem: the followingfiles provide useful information
on the status of the CAN controller, filter settings, registers,
etc.

  # cat /proc/rtcan/devices
  Name___________ _Baudrate State___ TX_Counter RX_Counter ____Errors
  rtcan0             125000 active            0          8          0
  rtcan1             125000 active            0          8          0
  rtcan2             125000 passive           8          0      14714

  # cat /proc/rtcan/sockets
  fd Name___________ Filter ErrMask RX_Timeout_ns TX_Timeout_ns RX_BufFull
   0 rtcan0               1 0x0ffff      infinite      infinite          0
   1 rtcan0               1 0x00000      infinite      infinite          0

  # cat /proc/rtcan/rtcan2/info
  Device     rtcan2
  Controller SJA1000
  Board      PEAK-PCI
  Clock-Hz   8000000
  Baudrate   125000
  Bit-time   brp=4 prop_seg=0 phase_seg1=13 phase_seg2=2 sjw=1 sam=0
  Ctrl-Mode
  State      passive
  TX-Counter 3
  RX-Counter 0
  Errors     45424
  Refcount   0

  # cat /proc/rtcan/rtcan0/filters
  fd __CAN_ID__ _CAN_Mask_ MatchCount
   0 0x00000000 0x00000000          0
   1 0x00000120 0x00000120          3

  # cat /proc/rtcan/rtcan0/registers
  MSCAN registers at f0000900
  canctl0  0x90 rxfrm synch
  canctl1  0xc0 cane clksrc
  ...

  # cat /proc/rtcan/rtcan2/registers
  SJA1000 registers
  00: 00 00 4c 00 ff 00 03 1c 1a 00 00 02 d6 60 14 88
  10: 02 26 60 de ad 04 04 00 ef c7 ef ef 40 00 00 c7


Documentation:
-------------

Temoporarily, The RTDM CAN profile is documented using Doxygen in
"api_doc/index.html"


Feedback:
--------

Please report Xenomai related bugs and comments to the Xenomai mailing 
list "xenomai@xenomai.org".

Please report CAN related bugs and comments to the "Socketcan" mailing 
list (Socketcan-users@domain.hid) or directly to the main authors 
Wolfgang Grandegger (wg@domain.hid) or Sebastian Smolorz 
(Sebastian.Smolorz@domain.hid). 


Credits:
-------

See CREDITS file.


License:
-------

RTCAN is free software, and you are welcome to redistribute it under
the terms of the GNU General Public License. This program comes with
ABSOLUTELY NO WARRANTY. See "COPYING" for details.

[Xenomai-core] Re: [ANNOUNCE] RTDM driver for CAN devices

[Bernhard Walle]

Hi Wolfgang,

Wolfgang Grandegger <wg@domain.hid> [2006-08-01]:
> 
> at "ftp://ftp.denx.de/pub/xenomai/rtcan" you can find a first version of
> RTCAN, an Open Source hard real-time protocol stack for CAN devices
> based on BSD sockets. It is based on the SJA1000 socket-based CAN driver
> for RTDM by Sebastian Smolorz (see CREDITS file).

Has this something to do with http://sourceforge.net/projects/rtcan/?
It seems no, so maybe it would be better to find another name for your
driver.


Regards,
  Bernhard
-- 
"The last good thing written in C was Franz Schubert's Symphony No. 9."
        -- Werner Trobin

Re: [Xenomai-core] Re: [ANNOUNCE] RTDM driver for CAN devices

[Wolfgang Grandegger]

Bernhard Walle wrote:
> Hi Wolfgang,
> 
> Wolfgang Grandegger <wg@domain.hid> [2006-08-01]:
>> at "ftp://ftp.denx.de/pub/xenomai/rtcan" you can find a first version of
>> RTCAN, an Open Source hard real-time protocol stack for CAN devices
>> based on BSD sockets. It is based on the SJA1000 socket-based CAN driver
>> for RTDM by Sebastian Smolorz (see CREDITS file).
> 
> Has this something to do with http://sourceforge.net/projects/rtcan/?
> It seems no, so maybe it would be better to find another name for your
> driver.

No, the project you mention seem not to be maintained for a long time 
now and I'm going to contact the author. Please regard the name as 
preliminary. We already thought of renaming it to RT-Socketcan.

Wolfgang.

[Xenomai-core] Re: [Xenomai-help] [ANNOUNCE] RTDM driver for CAN devices

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 4905 bytes --]

Hi Wolfgang,

Wolfgang Grandegger wrote:
> Hello,
> 
> at "ftp://ftp.denx.de/pub/xenomai/rtcan" you can find a first version of
> RTCAN, an Open Source hard real-time protocol stack for CAN devices
> based on BSD sockets. It is based on the SJA1000 socket-based CAN driver
> for RTDM by Sebastian Smolorz (see CREDITS file).

I started a review of the patch, and here are some first results. A
deeper look into critical paths is pending. But given that your
driver(s) already proved to work fine for us here and for your own
scenarios, only very sneaking issues can persist - if at all. :)

- AF_CAN is still set to 30 which meanwhile became TIPC. The Socket-CAN
project moved to 29 now. However, this could become a nice race until
Socket-CAN is upstream and reserved a final ID. How to cope with this?
We just had some fun here after moving our AF_TIMS from 0 to 27,
breaking many installation on our robots due to the ABI change. Should
we better move the ID beyond AF_MAX for now? Also a question for the
socketcan-core list...

- Out-commented debug messages: If they can be useful for driver
development / hardware testing, I would say make them selectable via
some CONFIG-switch. The rest should be removed.

- rtcan_mscan_mode_stop() contains some #if 1-#else-#endif block. What
are the alternatives here? Is it an open issue or just a pending cleanup?

- rtcan_mscan_init_one() contains a #ifdef FIXME. What needs to be
fixed? Comments for non-obvious pending issues would be helpful.

- Can rtcan_mscan_exit() be tagged with __exit?

- Is rtcan_mscan_proc_regs() a kind of debug option or useful for normal
operation as well? If it's the former, we may bundle it with the debug
print CONFIG-switch.

- Private data alignment in rtcan_dev_alloc(): you tapped on something
that looks ugly in RTnet as well. We copied this from Linux which still
does magic 32-byte alignment (encapsulated by NETDEV_ALIGN today). But
it also uses a smarter priv lookup now:

#define NETDEV_ALIGN            32
#define NETDEV_ALIGN_CONST      (NETDEV_ALIGN - 1)

static inline void *netdev_priv(struct net_device *dev)
{
        return (char *)dev + ((sizeof(struct net_device)
                                        + NETDEV_ALIGN_CONST)
                                & ~NETDEV_ALIGN_CONST);
}

Maybe something that should be adopted as well (for both stacks).
Nothing critical though.

- rtcan_dev_unregister() looks racy: down(&rtcan_devices_nrt_lock) comes
after the device state check. Is this ok?

- EXPORT_SYMBOL vs. EXPORT_SYMBOL_GPL: For me this makes no practical
difference anymore (kernel modules, specifically drivers must be GPL, no
matter what kind of symbols they use). Anyway, shouldn't we better go
the kernel path and introduce new APIs under _GPL? Just takes a wrapper
for 2.4 I guess.

- struct rtcan_device: what the heck are max_brp and max_sjw
(comments...)? This isn't something generic, right? That's why "FIXME" I
guess.

- LIST_POISON1 and some other stuff in rtcan_internal.h should
definitely be moved to the nucleus's wrapper. Nothing critical, just a
to-do. Likely the whole header will be obsolete in the end.

- [This is a note for me] RTCAN_ASSERT: Looks like we should introduce
RTDM_ASSERT() for this, maybe accepting some driver subsystem ID to
control this separately from the rest of Xenomai.

- RTCAN_PROC_... requires some thoughts: Generalise it? Some users
(haven't checked RTCAN for this, but RTnet has some) should better be
converted to seq_operations. And the rest could benefit from a generic
interface here. Uncritical for now.

- rtcan_dev_get_state_name(): Mm, having some array lookup for the name
here may shorten things.

- rtcan_read_proc_info: Should be done as in rtcan_mscan_proc_regs. Even
better: By increasing the buffer block in RTCAN_PROC_PRINT_VARS() you
could combine several PROC_PRINTs to a single one.

- rtcan_raw_remove_filter(): "if (sock->flistlen < 0)", a real check or
rather an internal ASSERT?

- struct rtcan_rb_frame differs from struct can_frame in the type of
can_dlc. The VW guys find __u8 for this nicer than a natural unsigned
int, but I stopped arguing on this. If we follow or not, it should be
consistent.

- rtcan_socket_context(): container_of?

- [Another note for me] module_param_array vs. MODULE_PARM-array: there
is some not that bad wrapper in RTnet now. I should push it to Xenomai
to simplify such blocks.

- Well known issue: the RTCAN name. This should definitely get resolved
before we merge. Any feedback already?

- Low prio (as long as my own code doesn't follow this ;)): Linux coding
style.


No guarantee that I found every critical point (most of the stuff above
is nitpicking anyway). I will try to repeat this more in details when
time permits. Hope my comments help nevertheless.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 250 bytes --]

[Xenomai-core] Re: [Xenomai-help] [ANNOUNCE] RTDM driver for CAN devices

[Wolfgang Grandegger]

Jan Kiszka wrote:
> Hi Wolfgang,
> 
> Wolfgang Grandegger wrote:
>> Hello,
>>
>> at "ftp://ftp.denx.de/pub/xenomai/rtcan" you can find a first version of
>> RTCAN, an Open Source hard real-time protocol stack for CAN devices
>> based on BSD sockets. It is based on the SJA1000 socket-based CAN driver
>> for RTDM by Sebastian Smolorz (see CREDITS file).
> 
> I started a review of the patch, and here are some first results. A
> deeper look into critical paths is pending. But given that your
> driver(s) already proved to work fine for us here and for your own
> scenarios, only very sneaking issues can persist - if at all. :)

Puh, still plenty of issues. I wanted to make RTCAN public a.s.a.p. 
because a few people already ask for it.

> - AF_CAN is still set to 30 which meanwhile became TIPC. The Socket-CAN
> project moved to 29 now. However, this could become a nice race until
> Socket-CAN is upstream and reserved a final ID. How to cope with this?
> We just had some fun here after moving our AF_TIMS from 0 to 27,
> breaking many installation on our robots due to the ABI change. Should
> we better move the ID beyond AF_MAX for now? Also a question for the
> socketcan-core list...

For the time being I fixed it to 29.

> - Out-commented debug messages: If they can be useful for driver
> development / hardware testing, I would say make them selectable via
> some CONFIG-switch. The rest should be removed.

OK, I'm going to implement a general "debug" option selectable via 
kernel parameter and proc filesystem during run time some time later.

> - rtcan_mscan_mode_stop() contains some #if 1-#else-#endif block. What
> are the alternatives here? Is it an open issue or just a pending cleanup?
> 
> - rtcan_mscan_init_one() contains a #ifdef FIXME. What needs to be
> fixed? Comments for non-obvious pending issues would be helpful.

There are a few open issues with the MSCAN hardware, especially entering 
sleep mode (it does not work as expected/documented). I will cleanup anyhow.

> - Can rtcan_mscan_exit() be tagged with __exit?

Of course, fixed.

> 
> - Is rtcan_mscan_proc_regs() a kind of debug option or useful for normal
> operation as well? If it's the former, we may bundle it with the debug
> print CONFIG-switch.

It's only useful for development and debugging. I will enable it with 
the debug option mentioned above sometime later.

> - Private data alignment in rtcan_dev_alloc(): you tapped on something
> that looks ugly in RTnet as well. We copied this from Linux which still
> does magic 32-byte alignment (encapsulated by NETDEV_ALIGN today). But
> it also uses a smarter priv lookup now:
> 
> #define NETDEV_ALIGN            32
> #define NETDEV_ALIGN_CONST      (NETDEV_ALIGN - 1)
> 
> static inline void *netdev_priv(struct net_device *dev)
> {
>         return (char *)dev + ((sizeof(struct net_device)
>                                         + NETDEV_ALIGN_CONST)
>                                 & ~NETDEV_ALIGN_CONST);
> }
> 
> Maybe something that should be adopted as well (for both stacks).
> Nothing critical though.

I removed alignment because I think it's better to have all data close 
together. What's the benefit of such an alignment. Note that the priv 
structures are normally small.

> - rtcan_dev_unregister() looks racy: down(&rtcan_devices_nrt_lock) comes
> after the device state check. Is this ok?

That's copied from rtdev.c. Indeed, it should be:

     RTCAN_ASSERT(atomic_read(&dev->refcount) == 0,
		 printk("RTCAN: dev reference counter < 0!\n"););
     rtdm_lock_put_irqrestore(&rtcan_devices_rt_lock, context);
     up(&rtcan_devices_nrt_lock);

> - EXPORT_SYMBOL vs. EXPORT_SYMBOL_GPL: For me this makes no practical
> difference anymore (kernel modules, specifically drivers must be GPL, no
> matter what kind of symbols they use). Anyway, shouldn't we better go
> the kernel path and introduce new APIs under _GPL? Just takes a wrapper
> for 2.4 I guess.

Fine for me. Other comments?

> - struct rtcan_device: what the heck are max_brp and max_sjw
> (comments...)? This isn't something generic, right? That's why "FIXME" I
> guess.

These variables might be necessary for some other CAN controllers to 
calculate the bit-timing parameters. Anyhow, I will remove them as long 
as they are not needed.

> - LIST_POISON1 and some other stuff in rtcan_internal.h should
> definitely be moved to the nucleus's wrapper. Nothing critical, just a
> to-do. Likely the whole header will be obsolete in the end.

OK.

> - [This is a note for me] RTCAN_ASSERT: Looks like we should introduce
> RTDM_ASSERT() for this, maybe accepting some driver subsystem ID to
> control this separately from the rest of Xenomai.

Fine for me. Let me know when it will be available.

> - RTCAN_PROC_... requires some thoughts: Generalise it? Some users
> (haven't checked RTCAN for this, but RTnet has some) should better be
> converted to seq_operations. And the rest could benefit from a generic
> interface here. Uncritical for now.

OK, I will leave it for the moment.

> - rtcan_dev_get_state_name(): Mm, having some array lookup for the name
> here may shorten things.

Fixed.

> - rtcan_read_proc_info: Should be done as in rtcan_mscan_proc_regs. Even
> better: By increasing the buffer block in RTCAN_PROC_PRINT_VARS() you
> could combine several PROC_PRINTs to a single one.

Fixed.

> - rtcan_raw_remove_filter(): "if (sock->flistlen < 0)", a real check or
> rather an internal ASSERT?

Should be an ASSERT, of course. Fixed.

> - struct rtcan_rb_frame differs from struct can_frame in the type of
> can_dlc. The VW guys find __u8 for this nicer than a natural unsigned
> int, but I stopped arguing on this. If we follow or not, it should be
> consistent.

I don't like the __u8 either. What do you mean with the last sentence? 
What should be consistent?

> - rtcan_socket_context(): container_of?

OK, I will use container_of() instead of rtcan_socket_context()?

> - [Another note for me] module_param_array vs. MODULE_PARM-array: there
> is some not that bad wrapper in RTnet now. I should push it to Xenomai
> to simplify such blocks.

And accordingly for byte and short, at least. I will add them to 
rtcan_internal.h for the time being.

> 
> - Well known issue: the RTCAN name. This should definitely get resolved
> before we merge. Any feedback already?

I contacted the author. If I will not get an answer soon, I tend 
changing the global name to RT-Socket-CAN (rtsocketcan).

> - Low prio (as long as my own code doesn't follow this ;)): Linux coding
> style.

Thanks ;-).

> No guarantee that I found every critical point (most of the stuff above
> is nitpicking anyway). I will try to repeat this more in details when
> time permits. Hope my comments help nevertheless.

Thanks a lot for your time. I'm going to provide a revised patch a.s.a.p.

A few other issues:

Till now I do not use rt_owner. Is it necessary?

And how can I generate Doxygen documentation to check the integration of 
the RTCAN device profile description?

Wolfgang.

Re: [Xenomai-core] Re: [Xenomai-help] [ANNOUNCE] RTDM driver for CAN devices

[Philippe Gerum]

On Sat, 2006-08-05 at 15:45 +0200, Wolfgang Grandegger wrote:

> > - EXPORT_SYMBOL vs. EXPORT_SYMBOL_GPL: For me this makes no practical
> > difference anymore (kernel modules, specifically drivers must be GPL, no
> > matter what kind of symbols they use). Anyway, shouldn't we better go
> > the kernel path and introduce new APIs under _GPL? Just takes a wrapper
> > for 2.4 I guess.
> 
> Fine for me. Other comments?
> 

It's ok with me too.

> And how can I generate Doxygen documentation to check the integration of 
> the RTCAN device profile description?
> 

Passing --enable-dox-doc to configure will cause Doxygen to be run on
the sources as part of the build process. Doxygen.in should be updated
so that new documented contents appear in the INPUT stanza.

-- 
Philippe.

[Xenomai-core] Re: [Xenomai-help] [ANNOUNCE] RTDM driver for CAN devices

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 5501 bytes --]

Wolfgang Grandegger wrote:
> Jan Kiszka wrote:
>> Hi Wolfgang,
>>
>> Wolfgang Grandegger wrote:
>>> Hello,
>>>
>>> at "ftp://ftp.denx.de/pub/xenomai/rtcan" you can find a first version of
>>> RTCAN, an Open Source hard real-time protocol stack for CAN devices
>>> based on BSD sockets. It is based on the SJA1000 socket-based CAN driver
>>> for RTDM by Sebastian Smolorz (see CREDITS file).
>>
>> I started a review of the patch, and here are some first results. A
>> deeper look into critical paths is pending. But given that your
>> driver(s) already proved to work fine for us here and for your own
>> scenarios, only very sneaking issues can persist - if at all. :)
> 
> Puh, still plenty of issues. I wanted to make RTCAN public a.s.a.p.
> because a few people already ask for it.

Most of my points are either small changes or just notes for future
cleanups, i.e. no show stoppers. The name, that one is critical.

> 
>> - AF_CAN is still set to 30 which meanwhile became TIPC. The Socket-CAN
>> project moved to 29 now. However, this could become a nice race until
>> Socket-CAN is upstream and reserved a final ID. How to cope with this?
>> We just had some fun here after moving our AF_TIMS from 0 to 27,
>> breaking many installation on our robots due to the ABI change. Should
>> we better move the ID beyond AF_MAX for now? Also a question for the
>> socketcan-core list...
> 
> For the time being I fixed it to 29.

Ok.

> 
>> - Out-commented debug messages: If they can be useful for driver
>> development / hardware testing, I would say make them selectable via
>> some CONFIG-switch. The rest should be removed.
> 
> OK, I'm going to implement a general "debug" option selectable via
> kernel parameter and proc filesystem during run time some time later.

Fine with me, but I would skip the work for a /proc switch.

>> - Private data alignment in rtcan_dev_alloc(): you tapped on something
>> that looks ugly in RTnet as well. We copied this from Linux which still
>> does magic 32-byte alignment (encapsulated by NETDEV_ALIGN today). But
>> it also uses a smarter priv lookup now:
>>
>> #define NETDEV_ALIGN            32
>> #define NETDEV_ALIGN_CONST      (NETDEV_ALIGN - 1)
>>
>> static inline void *netdev_priv(struct net_device *dev)
>> {
>>         return (char *)dev + ((sizeof(struct net_device)
>>                                         + NETDEV_ALIGN_CONST)
>>                                 & ~NETDEV_ALIGN_CONST);
>> }
>>
>> Maybe something that should be adopted as well (for both stacks).
>> Nothing critical though.
> 
> I removed alignment because I think it's better to have all data close
> together. What's the benefit of such an alignment. Note that the priv
> structures are normally small.

This needs a closer look indeed. We adopted what Linux does without
checking if it makes sense for RTnet. The CAN devices' private data
structures are different from RT-NICs, so a different alignment may make
sense as well. Let's keep it as it is and push it on the to-do list.

> 
>> - rtcan_dev_unregister() looks racy: down(&rtcan_devices_nrt_lock) comes
>> after the device state check. Is this ok?
> 
> That's copied from rtdev.c. Indeed, it should be:
> 
>     RTCAN_ASSERT(atomic_read(&dev->refcount) == 0,
>          printk("RTCAN: dev reference counter < 0!\n"););
>     rtdm_lock_put_irqrestore(&rtcan_devices_rt_lock, context);
>     up(&rtcan_devices_nrt_lock);

Argh, that locking is indeed like RTnet. And it makes me sick though I'm
likely responsible for most of this mess, thus also for this confusion
here. Please revert it for now until I sorted things in RTnet. The
locking must be simplified - and documented...

In any case, we may only suffer from theoretical races between
rtcanconfig and rmmod here. Uncritical.

>> - struct rtcan_rb_frame differs from struct can_frame in the type of
>> can_dlc. The VW guys find __u8 for this nicer than a natural unsigned
>> int, but I stopped arguing on this. If we follow or not, it should be
>> consistent.
> 
> I don't like the __u8 either. What do you mean with the last sentence?
> What should be consistent?

The types for can_dlc in both of our own structures. If can_frame uses
unsigned int, rtcan_rb_frame should do so as well.

>>
>> - Well known issue: the RTCAN name. This should definitely get resolved
>> before we merge. Any feedback already?
> 
> I contacted the author. If I will not get an answer soon, I tend
> changing the global name to RT-Socket-CAN (rtsocketcan).

I would really hate to have a drivers/rtsocketcan or a
rtdm/rtsocketcan.h. The short name is so much nicer.

> 
>> - Low prio (as long as my own code doesn't follow this ;)): Linux coding
>> style.
> 
> Thanks ;-).
> 
>> No guarantee that I found every critical point (most of the stuff above
>> is nitpicking anyway). I will try to repeat this more in details when
>> time permits. Hope my comments help nevertheless.
> 
> Thanks a lot for your time. I'm going to provide a revised patch a.s.a.p.

Again, the naming should be resolved, but then I think we could merge.
Philippe, ok for you as well?

> 
> A few other issues:
> 
> Till now I do not use rt_owner. Is it necessary?

That's for locking the driver module against removal. I guess this is
not needed for RTCAN if you can unregister a device at any time, maybe
just by spinning on the reference counter release.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 249 bytes --]

[Xenomai-core] Re: [Xenomai-help] [ANNOUNCE] RTDM driver for CAN devices

[Philippe Gerum]

On Sat, 2006-08-05 at 18:44 +0200, Jan Kiszka wrote:
> > 
> > Thanks a lot for your time. I'm going to provide a revised patch a.s.a.p.
> 
> Again, the naming should be resolved, but then I think we could merge.
> Philippe, ok for you as well?
> 

Yes.

> > 
> > A few other issues:
> > 
> > Till now I do not use rt_owner. Is it necessary?
> 
> That's for locking the driver module against removal. I guess this is
> not needed for RTCAN if you can unregister a device at any time, maybe
> just by spinning on the reference counter release.
> 
> Jan
> 
-- 
Philippe.

[Xenomai-core] Re: [Xenomai-help] [ANNOUNCE] RTDM driver for CAN devices

[Wolfgang Grandegger]

Jan Kiszka wrote:
> Wolfgang Grandegger wrote:
>> Jan Kiszka wrote:
>>> Hi Wolfgang,
>>>
>>> Wolfgang Grandegger wrote:
>>>> Hello,
>>>>
>>>> at "ftp://ftp.denx.de/pub/xenomai/rtcan" you can find a first version of
>>>> RTCAN, an Open Source hard real-time protocol stack for CAN devices
>>>> based on BSD sockets. It is based on the SJA1000 socket-based CAN driver
>>>> for RTDM by Sebastian Smolorz (see CREDITS file).
>>> I started a review of the patch, and here are some first results. A
>>> deeper look into critical paths is pending. But given that your
>>> driver(s) already proved to work fine for us here and for your own
>>> scenarios, only very sneaking issues can persist - if at all. :)
>> Puh, still plenty of issues. I wanted to make RTCAN public a.s.a.p.
>> because a few people already ask for it.
> 
> Most of my points are either small changes or just notes for future
> cleanups, i.e. no show stoppers. The name, that one is critical.

See below.

> 
>>> - AF_CAN is still set to 30 which meanwhile became TIPC. The Socket-CAN
>>> project moved to 29 now. However, this could become a nice race until
>>> Socket-CAN is upstream and reserved a final ID. How to cope with this?
>>> We just had some fun here after moving our AF_TIMS from 0 to 27,
>>> breaking many installation on our robots due to the ABI change. Should
>>> we better move the ID beyond AF_MAX for now? Also a question for the
>>> socketcan-core list...
>> For the time being I fixed it to 29.
> 
> Ok.
> 
>>> - Out-commented debug messages: If they can be useful for driver
>>> development / hardware testing, I would say make them selectable via
>>> some CONFIG-switch. The rest should be removed.
>> OK, I'm going to implement a general "debug" option selectable via
>> kernel parameter and proc filesystem during run time some time later.
> 
> Fine with me, but I would skip the work for a /proc switch.

There will be no separate switch for the PROC output. I was thinking
about one global debug option.

> 
>>> - Private data alignment in rtcan_dev_alloc(): you tapped on something
>>> that looks ugly in RTnet as well. We copied this from Linux which still
>>> does magic 32-byte alignment (encapsulated by NETDEV_ALIGN today). But
>>> it also uses a smarter priv lookup now:
>>>
>>> #define NETDEV_ALIGN            32
>>> #define NETDEV_ALIGN_CONST      (NETDEV_ALIGN - 1)
>>>
>>> static inline void *netdev_priv(struct net_device *dev)
>>> {
>>>         return (char *)dev + ((sizeof(struct net_device)
>>>                                         + NETDEV_ALIGN_CONST)
>>>                                 & ~NETDEV_ALIGN_CONST);
>>> }
>>>
>>> Maybe something that should be adopted as well (for both stacks).
>>> Nothing critical though.
>> I removed alignment because I think it's better to have all data close
>> together. What's the benefit of such an alignment. Note that the priv
>> structures are normally small.
> 
> This needs a closer look indeed. We adopted what Linux does without
> checking if it makes sense for RTnet. The CAN devices' private data
> structures are different from RT-NICs, so a different alignment may make
> sense as well. Let's keep it as it is and push it on the to-do list.

IMHO, the alignment makes sense, if the private structure is used in a
different context. This is not true for RTCAN. I will put it to the TODO 
list.

> 
>>> - rtcan_dev_unregister() looks racy: down(&rtcan_devices_nrt_lock) comes
>>> after the device state check. Is this ok?
>> That's copied from rtdev.c. Indeed, it should be:
>>
>>     RTCAN_ASSERT(atomic_read(&dev->refcount) == 0,
>>          printk("RTCAN: dev reference counter < 0!\n"););
>>     rtdm_lock_put_irqrestore(&rtcan_devices_rt_lock, context);
>>     up(&rtcan_devices_nrt_lock);
> 
> Argh, that locking is indeed like RTnet. And it makes me sick though I'm
> likely responsible for most of this mess, thus also for this confusion
> here. Please revert it for now until I sorted things in RTnet. The
> locking must be simplified - and documented...

OK.

> In any case, we may only suffer from theoretical races between
> rtcanconfig and rmmod here. Uncritical.

Dito.

>>> - struct rtcan_rb_frame differs from struct can_frame in the type of
>>> can_dlc. The VW guys find __u8 for this nicer than a natural unsigned
>>> int, but I stopped arguing on this. If we follow or not, it should be
>>> consistent.
>> I don't like the __u8 either. What do you mean with the last sentence?
>> What should be consistent?
> 
> The types for can_dlc in both of our own structures. If can_frame uses
> unsigned int, rtcan_rb_frame should do so as well.

Ah, of course, will fix that.

>>> - Well known issue: the RTCAN name. This should definitely get resolved
>>> before we merge. Any feedback already?
>> I contacted the author. If I will not get an answer soon, I tend
>> changing the global name to RT-Socket-CAN (rtsocketcan).
> 
> I would really hate to have a drivers/rtsocketcan or a
> rtdm/rtsocketcan.h. The short name is so much nicer.

He did not say, that we cannot use the name RTCAN but he prefers that we 
use a different name to avoid confusion. Therefore I suggest to use the 
offical name "RT-Socket-CAN" for the project, but leave almost all 
internal rtcan prefixes as they are apart from:

   drivers/rtsocketcan
   rtdm/rtsocketcan.h

Note that the API does use the Linux naming in most cases (with the 
prefix can).

Another possibility would be to use rtscan as short form for rtsocketcan 
as prefix.

What do you think? Well, it's just a name.

> 
>>> - Low prio (as long as my own code doesn't follow this ;)): Linux coding
>>> style.
>> Thanks ;-).
>>
>>> No guarantee that I found every critical point (most of the stuff above
>>> is nitpicking anyway). I will try to repeat this more in details when
>>> time permits. Hope my comments help nevertheless.
>> Thanks a lot for your time. I'm going to provide a revised patch a.s.a.p.
> 
> Again, the naming should be resolved, but then I think we could merge.
> Philippe, ok for you as well?
> 
>> A few other issues:
>>
>> Till now I do not use rt_owner. Is it necessary?
> 
> That's for locking the driver module against removal. I guess this is
> not needed for RTCAN if you can unregister a device at any time, maybe
> just by spinning on the reference counter release.

Yep.

Wolfgang.

[Xenomai-core] Re: [Xenomai-help] [ANNOUNCE] RTDM driver for CAN devices

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1769 bytes --]

Wolfgang Grandegger wrote:
>>>> - Well known issue: the RTCAN name. This should definitely get resolved
>>>> before we merge. Any feedback already?
>>> I contacted the author. If I will not get an answer soon, I tend
>>> changing the global name to RT-Socket-CAN (rtsocketcan).
>>
>> I would really hate to have a drivers/rtsocketcan or a
>> rtdm/rtsocketcan.h. The short name is so much nicer.
> 
> He did not say, that we cannot use the name RTCAN but he prefers that we
> use a different name to avoid confusion. Therefore I suggest to use the
> offical name "RT-Socket-CAN" for the project, but leave almost all
> internal rtcan prefixes as they are apart from:
> 
>   drivers/rtsocketcan
>   rtdm/rtsocketcan.h
> 
> Note that the API does use the Linux naming in most cases (with the
> prefix can).
> 
> Another possibility would be to use rtscan as short form for rtsocketcan
> as prefix.
> 
> What do you think? Well, it's just a name.

Never underestimate naming. Ok, I have this proposal now:

 o drivers/can/ - That's consistent with the existing subdir naming
   anyway.

 o rtdm/rtcan.h - The "rtdm/" prefix clearly defines the context: It's
   THE standard real-time CAN profile for RTDM.

 o All references to "RTCAN" in comments, READMEs, headers, etc. must be
   changed to RT-Socket-CAN. So it should be clear that this has nothing
   to do with the existing "rtcan" project.

 o Variable, type, and function names remain as they are.

Jan


PS: Another point for the long-term to-do list :-> : The nested locking
and the global scope of certain locks. It's safe, it's harmless for
current primary target platforms (UP), but it's not really beautiful
when considering SMP setups. A bit tricky, for sure.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 249 bytes --]

Re: [Xenomai-core] Re: [Xenomai-help] [ANNOUNCE] RTDM driver for CAN devices

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 2694 bytes --]

Jan Kiszka wrote:
> ...
> PS: Another point for the long-term to-do list :-> : The nested locking
> and the global scope of certain locks. It's safe, it's harmless for
> current primary target platforms (UP), but it's not really beautiful
> when considering SMP setups. A bit tricky, for sure.
> 

I put Sebastian in the CC. Maybe he sees issues I may ignore in the
following.

Here is some first idea on a new locking scheme. It reduces the number
of locks by one and turns them either into per-device or per-socket in
the end:


rtcan_socket.lock
----------------------
Protects:
 - concurrent read access to socket FIFO
 - /maybe/ also timeouts (but I think the user should better take care
   to not set a timeout and make use of it at the same time - not that
   hard to achieve in practice...)

This means that we need lock-less FIFOs wrt/ reader/writer
synchronisation. Some pattern like RTnet's rtskb_fifo e.g.


rtcan_device.lock
------------------------
Protects:
 - hardware access of related CAN port
 - device reception list

So we only take a single lock in the IRQ handler (+eventually the nklock
when waking someone up). But it requires that a socket, or more
precisely its FIFO is exclusively assigned to a single device.

And this raises the question how to deal with ifindex=0, i.e. attaching
a socket to all devices. Given that in this case the socket may run into
troubles anyway due to the fixed FIFO size, I would say that we better
create multiple FIFOs here (or additional rtcan_socket structures), one
per device of interest. The wakeup would then need some indirection in
order to use a common rtdm_event for all FIFOs of a socket.

Ok, this may complicate things, so we could do this in several steps:

1. Decouple FIFO reader from writer protection. A *global* device lock
   protects the writers + the reception lists, a socket recv_lock the
   read access. Make FIFOs lock-less wrt/ reader/writer concurrency.
2. Make the ifindex=0 support optional, fall back to global locking if
   it's on. If it's off (should be default), use per-device locking
   (again) as we then have exclusive assignments of sockets to a single
   device.
3. Create multiple FIFOs if ifindex is 0, always use per-device locks.
   As we already have the all-devices-support configurable at that time,
   we could let the user decide if the additional code for this mode
   should get compiled in or not.

Once this is implemented, the whole pattern could nicely be reused for
RTnet. I have in mind to shorten the reception path in the future and
make it more SMP friendly. So I'm with you here to gain experience! :)

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 249 bytes --]

[Xenomai-core] Re: [Xenomai-help] [ANNOUNCE] RTDM driver for CAN devices

[Wolfgang Grandegger]

Jan Kiszka wrote:
> Wolfgang Grandegger wrote:
>>>>> - Well known issue: the RTCAN name. This should definitely get resolved
>>>>> before we merge. Any feedback already?
>>>> I contacted the author. If I will not get an answer soon, I tend
>>>> changing the global name to RT-Socket-CAN (rtsocketcan).
>>> I would really hate to have a drivers/rtsocketcan or a
>>> rtdm/rtsocketcan.h. The short name is so much nicer.
>> He did not say, that we cannot use the name RTCAN but he prefers that we
>> use a different name to avoid confusion. Therefore I suggest to use the
>> offical name "RT-Socket-CAN" for the project, but leave almost all
>> internal rtcan prefixes as they are apart from:
>>
>>   drivers/rtsocketcan
>>   rtdm/rtsocketcan.h
>>
>> Note that the API does use the Linux naming in most cases (with the
>> prefix can).
>>
>> Another possibility would be to use rtscan as short form for rtsocketcan
>> as prefix.
>>
>> What do you think? Well, it's just a name.
> 
> Never underestimate naming. Ok, I have this proposal now:
> 
>  o drivers/can/ - That's consistent with the existing subdir naming
>    anyway.

I was also thinking about that.

>  o rtdm/rtcan.h - The "rtdm/" prefix clearly defines the context: It's
>    THE standard real-time CAN profile for RTDM.
> 
>  o All references to "RTCAN" in comments, READMEs, headers, etc. must be
>    changed to RT-Socket-CAN. So it should be clear that this has nothing
>    to do with the existing "rtcan" project.
> 
>  o Variable, type, and function names remain as they are.

OK, thats fine for me. The important thing is, that the official project 
name is different from RTCAN.

> Jan
> 
> 
> PS: Another point for the long-term to-do list :-> : The nested locking
> and the global scope of certain locks. It's safe, it's harmless for
> current primary target platforms (UP), but it's not really beautiful
> when considering SMP setups. A bit tricky, for sure.

Yes, it's also not nice, that it is in the HW specific driver. I did not 
  touch it because I have no SMP system for testing.

Wolfgang.

[Xenomai-core] Re: [Xenomai-help] [ANNOUNCE] RTDM driver for CAN devices

[Wolfgang Grandegger]

Jan Kiszka wrote:
> Wolfgang Grandegger wrote:
>>>>> - Well known issue: the RTCAN name. This should definitely get resolved
>>>>> before we merge. Any feedback already?
>>>> I contacted the author. If I will not get an answer soon, I tend
>>>> changing the global name to RT-Socket-CAN (rtsocketcan).
>>> I would really hate to have a drivers/rtsocketcan or a
>>> rtdm/rtsocketcan.h. The short name is so much nicer.
>> He did not say, that we cannot use the name RTCAN but he prefers that we
>> use a different name to avoid confusion. Therefore I suggest to use the
>> offical name "RT-Socket-CAN" for the project, but leave almost all
>> internal rtcan prefixes as they are apart from:
>>
>>   drivers/rtsocketcan
>>   rtdm/rtsocketcan.h
>>
>> Note that the API does use the Linux naming in most cases (with the
>> prefix can).
>>
>> Another possibility would be to use rtscan as short form for rtsocketcan
>> as prefix.
>>
>> What do you think? Well, it's just a name.
> 
> Never underestimate naming. Ok, I have this proposal now:
> 
>  o drivers/can/ - That's consistent with the existing subdir naming
>    anyway.
> 
>  o rtdm/rtcan.h - The "rtdm/" prefix clearly defines the context: It's
>    THE standard real-time CAN profile for RTDM.
> 
>  o All references to "RTCAN" in comments, READMEs, headers, etc. must be
>    changed to RT-Socket-CAN. So it should be clear that this has nothing
>    to do with the existing "rtcan" project.
> 
>  o Variable, type, and function names remain as they are.
> 
> Jan
> 
> 
> PS: Another point for the long-term to-do list :-> : The nested locking
> and the global scope of certain locks. It's safe, it's harmless for
> current primary target platforms (UP), but it's not really beautiful
> when considering SMP setups. A bit tricky, for sure.

I just realized another issue. Where to put README and CREDITS file? The 
README should go into doc/txt/can-driver.txt. Should I add the credits 
to this file as well?

Wolfgang.

[Xenomai-core] Re: [Xenomai-help] [ANNOUNCE] RTDM driver for CAN devices

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 2344 bytes --]

Wolfgang Grandegger wrote:
> Jan Kiszka wrote:
>> Wolfgang Grandegger wrote:
>>>>>> - Well known issue: the RTCAN name. This should definitely get
>>>>>> resolved
>>>>>> before we merge. Any feedback already?
>>>>> I contacted the author. If I will not get an answer soon, I tend
>>>>> changing the global name to RT-Socket-CAN (rtsocketcan).
>>>> I would really hate to have a drivers/rtsocketcan or a
>>>> rtdm/rtsocketcan.h. The short name is so much nicer.
>>> He did not say, that we cannot use the name RTCAN but he prefers that we
>>> use a different name to avoid confusion. Therefore I suggest to use the
>>> offical name "RT-Socket-CAN" for the project, but leave almost all
>>> internal rtcan prefixes as they are apart from:
>>>
>>>   drivers/rtsocketcan
>>>   rtdm/rtsocketcan.h
>>>
>>> Note that the API does use the Linux naming in most cases (with the
>>> prefix can).
>>>
>>> Another possibility would be to use rtscan as short form for rtsocketcan
>>> as prefix.
>>>
>>> What do you think? Well, it's just a name.
>>
>> Never underestimate naming. Ok, I have this proposal now:
>>
>>  o drivers/can/ - That's consistent with the existing subdir naming
>>    anyway.
>>
>>  o rtdm/rtcan.h - The "rtdm/" prefix clearly defines the context: It's
>>    THE standard real-time CAN profile for RTDM.
>>
>>  o All references to "RTCAN" in comments, READMEs, headers, etc. must be
>>    changed to RT-Socket-CAN. So it should be clear that this has nothing
>>    to do with the existing "rtcan" project.
>>
>>  o Variable, type, and function names remain as they are.
>>
>> Jan
>>
>>
>> PS: Another point for the long-term to-do list :-> : The nested locking
>> and the global scope of certain locks. It's safe, it's harmless for
>> current primary target platforms (UP), but it's not really beautiful
>> when considering SMP setups. A bit tricky, for sure.
> 
> I just realized another issue. Where to put README and CREDITS file? The
> README should go into doc/txt/can-driver.txt. Should I add the credits
> to this file as well?

I would maintain that file under drivers/can. Who knows if it doesn't
grow noticeably over the time... Some reference from the main CREDITS
would be good then. Something like "See drivers/<subsystem>/CREDITS for
further contributors".

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 250 bytes --]

[Xenomai-core] Re: [Xenomai-help] [ANNOUNCE] RTDM driver for CAN devices

[Philippe Gerum]

On Mon, 2006-08-07 at 16:17 +0200, Wolfgang Grandegger wrote:
> Jan Kiszka wrote:
> > Wolfgang Grandegger wrote:
> >>>>> - Well known issue: the RTCAN name. This should definitely get resolved
> >>>>> before we merge. Any feedback already?
> >>>> I contacted the author. If I will not get an answer soon, I tend
> >>>> changing the global name to RT-Socket-CAN (rtsocketcan).
> >>> I would really hate to have a drivers/rtsocketcan or a
> >>> rtdm/rtsocketcan.h. The short name is so much nicer.
> >> He did not say, that we cannot use the name RTCAN but he prefers that we
> >> use a different name to avoid confusion. Therefore I suggest to use the
> >> offical name "RT-Socket-CAN" for the project, but leave almost all
> >> internal rtcan prefixes as they are apart from:
> >>
> >>   drivers/rtsocketcan
> >>   rtdm/rtsocketcan.h
> >>
> >> Note that the API does use the Linux naming in most cases (with the
> >> prefix can).
> >>
> >> Another possibility would be to use rtscan as short form for rtsocketcan
> >> as prefix.
> >>
> >> What do you think? Well, it's just a name.
> > 
> > Never underestimate naming. Ok, I have this proposal now:
> > 
> >  o drivers/can/ - That's consistent with the existing subdir naming
> >    anyway.
> > 
> >  o rtdm/rtcan.h - The "rtdm/" prefix clearly defines the context: It's
> >    THE standard real-time CAN profile for RTDM.
> > 
> >  o All references to "RTCAN" in comments, READMEs, headers, etc. must be
> >    changed to RT-Socket-CAN. So it should be clear that this has nothing
> >    to do with the existing "rtcan" project.
> > 
> >  o Variable, type, and function names remain as they are.
> > 
> > Jan
> > 
> > 
> > PS: Another point for the long-term to-do list :-> : The nested locking
> > and the global scope of certain locks. It's safe, it's harmless for
> > current primary target platforms (UP), but it's not really beautiful
> > when considering SMP setups. A bit tricky, for sure.
> 
> I just realized another issue. Where to put README and CREDITS file? The 
> README should go into doc/txt/can-driver.txt.

Would be nice.

>  Should I add the credits 
> to this file as well?

Why not, but please also add an entry to the central CREDITS file, so
that maintainers are easily identified.

> 
> Wolfgang.
> 
> 
-- 
Philippe.

[Xenomai-core] Re: [Xenomai-help] [ANNOUNCE] RTDM driver for CAN devices

[Philippe Gerum]

On Mon, 2006-08-07 at 16:31 +0200, Jan Kiszka wrote:
> > 
> > I just realized another issue. Where to put README and CREDITS file? The
> > README should go into doc/txt/can-driver.txt. Should I add the credits
> > to this file as well?
> 
> I would maintain that file under drivers/can. Who knows if it doesn't
> grow noticeably over the time...

Yes, thinking a bit more about this, this would be even better than
maintaining it in the doc section, so that the CAN driver could be
easily extracted from the Xenomai stack without having to chase each and
every bit of documentation throughout the tree.

-- 
Philippe.

[Xenomai-core] Re: [Xenomai-help] [ANNOUNCE] RTDM driver for CAN devices

[Wolfgang Grandegger]

[-- Attachment #1: Type: text/plain, Size: 2570 bytes --]

Jan Kiszka wrote:
> Wolfgang Grandegger wrote:
>> Jan Kiszka wrote:
>>> Wolfgang Grandegger wrote:
>>>>>>> - Well known issue: the RTCAN name. This should definitely get
>>>>>>> resolved
>>>>>>> before we merge. Any feedback already?
>>>>>> I contacted the author. If I will not get an answer soon, I tend
>>>>>> changing the global name to RT-Socket-CAN (rtsocketcan).
>>>>> I would really hate to have a drivers/rtsocketcan or a
>>>>> rtdm/rtsocketcan.h. The short name is so much nicer.
>>>> He did not say, that we cannot use the name RTCAN but he prefers that we
>>>> use a different name to avoid confusion. Therefore I suggest to use the
>>>> offical name "RT-Socket-CAN" for the project, but leave almost all
>>>> internal rtcan prefixes as they are apart from:
>>>>
>>>>   drivers/rtsocketcan
>>>>   rtdm/rtsocketcan.h
>>>>
>>>> Note that the API does use the Linux naming in most cases (with the
>>>> prefix can).
>>>>
>>>> Another possibility would be to use rtscan as short form for rtsocketcan
>>>> as prefix.
>>>>
>>>> What do you think? Well, it's just a name.
>>> Never underestimate naming. Ok, I have this proposal now:
>>>
>>>  o drivers/can/ - That's consistent with the existing subdir naming
>>>    anyway.
>>>
>>>  o rtdm/rtcan.h - The "rtdm/" prefix clearly defines the context: It's
>>>    THE standard real-time CAN profile for RTDM.
>>>
>>>  o All references to "RTCAN" in comments, READMEs, headers, etc. must be
>>>    changed to RT-Socket-CAN. So it should be clear that this has nothing
>>>    to do with the existing "rtcan" project.
>>>
>>>  o Variable, type, and function names remain as they are.
>>>
>>> Jan
>>>
>>>
>>> PS: Another point for the long-term to-do list :-> : The nested locking
>>> and the global scope of certain locks. It's safe, it's harmless for
>>> current primary target platforms (UP), but it's not really beautiful
>>> when considering SMP setups. A bit tricky, for sure.
>> I just realized another issue. Where to put README and CREDITS file? The
>> README should go into doc/txt/can-driver.txt. Should I add the credits
>> to this file as well?
> 
> I would maintain that file under drivers/can. Who knows if it doesn't
> grow noticeably over the time... Some reference from the main CREDITS
> would be good then. Something like "See drivers/<subsystem>/CREDITS for
> further contributors".

Attached is a revised patch of RT-Socket-CAN for Xenomai ready for 
integration. I fixed most of the issues as discussed. Still a few on the 
TO-DO list, but they could be fixed later on, I think.

Thanks.

Wolfgang.


[-- Attachment #2: xenomai-rtsocketcan.patch.bz2 --]
[-- Type: application/x-bzip, Size: 43292 bytes --]

[Xenomai-core] Re: [Xenomai-help] [ANNOUNCE] RTDM driver for CAN devices

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 2889 bytes --]

Wolfgang Grandegger wrote:
> Jan Kiszka wrote:
>> Wolfgang Grandegger wrote:
>>> Jan Kiszka wrote:
>>>> Wolfgang Grandegger wrote:
>>>>>>>> - Well known issue: the RTCAN name. This should definitely get
>>>>>>>> resolved
>>>>>>>> before we merge. Any feedback already?
>>>>>>> I contacted the author. If I will not get an answer soon, I tend
>>>>>>> changing the global name to RT-Socket-CAN (rtsocketcan).
>>>>>> I would really hate to have a drivers/rtsocketcan or a
>>>>>> rtdm/rtsocketcan.h. The short name is so much nicer.
>>>>> He did not say, that we cannot use the name RTCAN but he prefers
>>>>> that we
>>>>> use a different name to avoid confusion. Therefore I suggest to use
>>>>> the
>>>>> offical name "RT-Socket-CAN" for the project, but leave almost all
>>>>> internal rtcan prefixes as they are apart from:
>>>>>
>>>>>   drivers/rtsocketcan
>>>>>   rtdm/rtsocketcan.h
>>>>>
>>>>> Note that the API does use the Linux naming in most cases (with the
>>>>> prefix can).
>>>>>
>>>>> Another possibility would be to use rtscan as short form for
>>>>> rtsocketcan
>>>>> as prefix.
>>>>>
>>>>> What do you think? Well, it's just a name.
>>>> Never underestimate naming. Ok, I have this proposal now:
>>>>
>>>>  o drivers/can/ - That's consistent with the existing subdir naming
>>>>    anyway.
>>>>
>>>>  o rtdm/rtcan.h - The "rtdm/" prefix clearly defines the context: It's
>>>>    THE standard real-time CAN profile for RTDM.
>>>>
>>>>  o All references to "RTCAN" in comments, READMEs, headers, etc.
>>>> must be
>>>>    changed to RT-Socket-CAN. So it should be clear that this has
>>>> nothing
>>>>    to do with the existing "rtcan" project.
>>>>
>>>>  o Variable, type, and function names remain as they are.
>>>>
>>>> Jan
>>>>
>>>>
>>>> PS: Another point for the long-term to-do list :-> : The nested locking
>>>> and the global scope of certain locks. It's safe, it's harmless for
>>>> current primary target platforms (UP), but it's not really beautiful
>>>> when considering SMP setups. A bit tricky, for sure.
>>> I just realized another issue. Where to put README and CREDITS file? The
>>> README should go into doc/txt/can-driver.txt. Should I add the credits
>>> to this file as well?
>>
>> I would maintain that file under drivers/can. Who knows if it doesn't
>> grow noticeably over the time... Some reference from the main CREDITS
>> would be good then. Something like "See drivers/<subsystem>/CREDITS for
>> further contributors".
> 
> Attached is a revised patch of RT-Socket-CAN for Xenomai ready for
> integration. I fixed most of the issues as discussed. Still a few on the
> TO-DO list, but they could be fixed later on, I think.
> 

Great! Will have a look later and start to merge it.

What's the status of the utils? Philippe and I agreed to put them into
src/utils/can.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 249 bytes --]

[Xenomai-core] Re: [Xenomai-help] [ANNOUNCE] RTDM driver for CAN devices

[Wolfgang Grandegger]

[-- Attachment #1: Type: text/plain, Size: 3182 bytes --]

Jan Kiszka wrote:
> Wolfgang Grandegger wrote:
>> Jan Kiszka wrote:
>>> Wolfgang Grandegger wrote:
>>>> Jan Kiszka wrote:
>>>>> Wolfgang Grandegger wrote:
>>>>>>>>> - Well known issue: the RTCAN name. This should definitely get
>>>>>>>>> resolved
>>>>>>>>> before we merge. Any feedback already?
>>>>>>>> I contacted the author. If I will not get an answer soon, I tend
>>>>>>>> changing the global name to RT-Socket-CAN (rtsocketcan).
>>>>>>> I would really hate to have a drivers/rtsocketcan or a
>>>>>>> rtdm/rtsocketcan.h. The short name is so much nicer.
>>>>>> He did not say, that we cannot use the name RTCAN but he prefers
>>>>>> that we
>>>>>> use a different name to avoid confusion. Therefore I suggest to use
>>>>>> the
>>>>>> offical name "RT-Socket-CAN" for the project, but leave almost all
>>>>>> internal rtcan prefixes as they are apart from:
>>>>>>
>>>>>>   drivers/rtsocketcan
>>>>>>   rtdm/rtsocketcan.h
>>>>>>
>>>>>> Note that the API does use the Linux naming in most cases (with the
>>>>>> prefix can).
>>>>>>
>>>>>> Another possibility would be to use rtscan as short form for
>>>>>> rtsocketcan
>>>>>> as prefix.
>>>>>>
>>>>>> What do you think? Well, it's just a name.
>>>>> Never underestimate naming. Ok, I have this proposal now:
>>>>>
>>>>>  o drivers/can/ - That's consistent with the existing subdir naming
>>>>>    anyway.
>>>>>
>>>>>  o rtdm/rtcan.h - The "rtdm/" prefix clearly defines the context: It's
>>>>>    THE standard real-time CAN profile for RTDM.
>>>>>
>>>>>  o All references to "RTCAN" in comments, READMEs, headers, etc.
>>>>> must be
>>>>>    changed to RT-Socket-CAN. So it should be clear that this has
>>>>> nothing
>>>>>    to do with the existing "rtcan" project.
>>>>>
>>>>>  o Variable, type, and function names remain as they are.
>>>>>
>>>>> Jan
>>>>>
>>>>>
>>>>> PS: Another point for the long-term to-do list :-> : The nested locking
>>>>> and the global scope of certain locks. It's safe, it's harmless for
>>>>> current primary target platforms (UP), but it's not really beautiful
>>>>> when considering SMP setups. A bit tricky, for sure.
>>>> I just realized another issue. Where to put README and CREDITS file? The
>>>> README should go into doc/txt/can-driver.txt. Should I add the credits
>>>> to this file as well?
>>> I would maintain that file under drivers/can. Who knows if it doesn't
>>> grow noticeably over the time... Some reference from the main CREDITS
>>> would be good then. Something like "See drivers/<subsystem>/CREDITS for
>>> further contributors".
>> Attached is a revised patch of RT-Socket-CAN for Xenomai ready for
>> integration. I fixed most of the issues as discussed. Still a few on the
>> TO-DO list, but they could be fixed later on, I think.
>>
> 
> Great! Will have a look later and start to merge it.
> 
> What's the status of the utils? Philippe and I agreed to put them into
> src/utils/can.

Oops, I checked the previous mails and understood, that they should be 
kept external but anyway, I prefer this solution. Nevertheless, I think 
it requires integration with configure and friends. I have attached my 
revised external version of rtsocketcan-utils.

Wolfgang.

> Jan
> 


[-- Attachment #2: rtsocketcan-utils-0.20.2.tar.bz2 --]
[-- Type: application/x-bzip, Size: 18568 bytes --]