https://www.fmtc.be/bugzilla/orocos/show_bug.cgi?id=639

--- Comment #7 from Klaas Gadeyne <klaas [dot] gadeyne [..] ...> 2009-08-12 13:31:22 ---
(In reply to comment #6)
> (In reply to comment #5)
> > I had a quick look at this code. Most of it seems ok, but
> >
> > - I really _dislike_ the naming of the controller classes. Actually
> > RTCANcontroller is _also_ a (kind of) socketcan controller, only it is using
> > the RTDM socket API. I would really have this reflected in the name. I won´t
> > make a suggestion (yet) because of the next point.
> > - Although there are some differences between most files, I wonder wether both
> > CAN controllers couldn't reuse more code or even be merged into one
> > SocketCANController class.
> > E.g. I've now already seen that one of the classes
> > * doesn't check the return value when performing an (rt_dev) ioctl call
> > * doesn't check wether the CANbus pointer is null
> > and the other does.
> >
> > Thoughts?
>
> You're right that there's plenty of double code, causing divergence. I'm always
> open to refactoring, but the code had to be written first. The win will still
> be minimal and won't improve readability imho (code spread over two classes
> (base/sub) instead of inside one.)

ACK. Maybe the easiest win is then to just rename the rtcancontroller into
rtdm_socketcancontroller for now.

BTW, I also noted that some #ifdef constructions were not properly terminated.

--
Configure bugmail: https://www.fmtc.be/bugzilla/orocos/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.
You are the assignee for the bug.
--
Orocos-Dev mailing list
Orocos-Dev [..] ...
http://lists.mech.kuleuven.be/mailman/listinfo/orocos-dev

https://www.fmtc.be/bugzilla/orocos/show_bug.cgi?id=639

--- Comment #6 from Peter Soetens <peter [..] ...> 2009-08-10 15:14:00 ---
(In reply to comment #5)
> I had a quick look at this code. Most of it seems ok, but
>
> - I really _dislike_ the naming of the controller classes. Actually
> RTCANcontroller is _also_ a (kind of) socketcan controller, only it is using
> the RTDM socket API. I would really have this reflected in the name. I won´t
> make a suggestion (yet) because of the next point.
> - Although there are some differences between most files, I wonder wether both
> CAN controllers couldn't reuse more code or even be merged into one
> SocketCANController class.
> E.g. I've now already seen that one of the classes
> * doesn't check the return value when performing an (rt_dev) ioctl call
> * doesn't check wether the CANbus pointer is null
> and the other does.
>
> Thoughts?

You're right that there's plenty of double code, causing divergence. I'm always
open to refactoring, but the code had to be written first. The win will still
be minimal and won't improve readability imho (code spread over two classes
(base/sub) instead of inside one.)

Peter

--
Configure bugmail: https://www.fmtc.be/bugzilla/orocos/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.
You are the assignee for the bug.
--
Orocos-Dev mailing list
Orocos-Dev [..] ...
http://lists.mech.kuleuven.be/mailman/listinfo/orocos-dev

https://www.fmtc.be/bugzilla/orocos/show_bug.cgi?id=639

--- Comment #5 from Klaas Gadeyne <klaas [dot] gadeyne [..] ...> 2009-08-07 14:35:27 ---
I had a quick look at this code. Most of it seems ok, but

- I really _dislike_ the naming of the controller classes. Actually
RTCANcontroller is _also_ a (kind of) socketcan controller, only it is using
the RTDM socket API. I would really have this reflected in the name. I won´t
make a suggestion (yet) because of the next point.
- Although there are some differences between most files, I wonder wether both
CAN controllers couldn't reuse more code or even be merged into one
SocketCANController class.
E.g. I've now already seen that one of the classes
* doesn't check the return value when performing an (rt_dev) ioctl call
* doesn't check wether the CANbus pointer is null
and the other does.

Thoughts?

--
Configure bugmail: https://www.fmtc.be/bugzilla/orocos/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.
You are the assignee for the bug.
--
Orocos-Dev mailing list
Orocos-Dev [..] ...
http://lists.mech.kuleuven.be/mailman/listinfo/orocos-dev

https://www.fmtc.be/bugzilla/orocos/show_bug.cgi?id=639

Peter Soetens <peter [..] ...> changed:

What |Removed |Added
----------------------------------------------------------------------------
Target Milestone|--- |1.10.0
CC| |peter [..] ...
Status|NEW |ASSIGNED

--- Comment #4 from Peter Soetens <peter [..] ...> 2009-06-16 16:49:07 ---
(In reply to comment #3)
> (In reply to comment #0)
> > Two new CAN controllers are added: SocketCANcontroller and RTCANcontroller.
> >
> > The first one uses the PCAN driver, compiled with NETDEV_SUPPORT. The second
> > one uses the real-time xenomai CAN driver.
> >
> > NodeId was the only CANOpen functionality in the CANopenBus class. So this
> > functionality is moved to the NodeGuard class and CANopenBus is renamed into
> > CANBus. Sync (CANOpen specific) in CANBusInterface is moved to SyncWriter.
>
> Update of the patch. Tested on 1.6 and everything seems to be working. Maybe
> some modifications in the build system are required if you want to build the
> socket CAN controller on linux kernels that have no NETDEV_SUPPORT (linux
> kernel
> < 2.6.25).

I applied it on my git OCL master branch. I'll schedule it for inclusion in OCL
1.10.0

I'll take a look at the cmake logic to make this compile/skip files on older
kernels.

Peter

https://www.fmtc.be/bugzilla/orocos/show_bug.cgi?id=639

--- Comment #2 from Steven Kauffmann <steven [dot] kauffmann [..] ...> 2009-06-12 11:26:38 ---
Created an attachment (id=444)
--> (https://www.fmtc.be/bugzilla/orocos/attachment.cgi?id=444)
CAN patch file

https://www.fmtc.be/bugzilla/orocos/show_bug.cgi?id=639

--- Comment #3 from Steven Kauffmann <steven [dot] kauffmann [..] ...> 2009-06-12 11:30:49 ---
(In reply to comment #0)
> Two new CAN controllers are added: SocketCANcontroller and RTCANcontroller.
>
> The first one uses the PCAN driver, compiled with NETDEV_SUPPORT. The second
> one uses the real-time xenomai CAN driver.
>
> NodeId was the only CANOpen functionality in the CANopenBus class. So this
> functionality is moved to the NodeGuard class and CANopenBus is renamed into
> CANBus. Sync (CANOpen specific) in CANBusInterface is moved to SyncWriter.

Update of the patch. Tested on 1.6 and everything seems to be working. Maybe
some modifications in the build system are required if you want to build the
socket CAN controller on linux kernels that have no NETDEV_SUPPORT (linux
kernel
< 2.6.25).

Does this patch is easily integrable with a CANOpen implementation like
CANFestival ?

2009/3/20 Steven Kauffmann <steven [dot] kauffmann [..] ...>

> For more infomation about this bug, visit <
> https://www.fmtc.be/bugzilla/orocos/show_bug.cgi?id=639>
> Summary: Revision of the OCL CAN classes
> Product: OCL
> Version: trunk
> Platform: All
> OS/Version: All
> Status: NEW
> Severity: normal
> Priority: P3
> Component: Hardware
> AssignedTo: orocos-dev [..] ...
> ReportedBy: steven [dot] kauffmann [..] ...
> CC: orocos-dev [..] ...
> Estimated Hours: 0.0
>
>
> Two new CAN controllers are added: SocketCANcontroller and RTCANcontroller.
>
> The first one uses the PCAN driver, compiled with NETDEV_SUPPORT. The
> second
> one uses the real-time xenomai CAN driver.
>
> NodeId was the only CANOpen functionality in the CANopenBus class. So this
> functionality is moved to the NodeGuard class and CANopenBus is renamed
> into
> CANBus. Sync (CANOpen specific) in CANBusInterface is moved to SyncWriter.
>
> --
> Configure bugmail:
> https://www.fmtc.be/bugzilla/orocos/userprefs.cgi?tab=email
> ------- You are receiving this mail because: -------
> You are on the CC list for the bug.
> You are the assignee for the bug.
> --
> Orocos-Dev mailing list
> Orocos-Dev [..] ...
> http://lists.mech.kuleuven.be/mailman/listinfo/orocos-dev
>
> Disclaimer: http://www.kuleuven.be/cwis/email_disclaimer.htm
>
>