[Bug 489] New: PCANController needs cleanup

For more infomation about this bug, visit
Summary: PCANController needs cleanup
Product: OCL
Version: 1.4.0
Platform: All
OS/Version: All
Status: NEW
Severity: normal
Priority: P2
Component: Hardware
AssignedTo: orocos-dev [..] ...
ReportedBy: peter [dot] soetens [..] ...
CC: orocos-dev [..] ...
Estimated Hours: 0.0

The PCANController class for the peak-systems pcan driver needs a cleanup.

* It should be non-periodic, blocking (with a timeout) on the
LINUX_CAN_Read_Timeout function.
* The code in finalize() should not close the device, such that a start() is
possible after a stop().
* The log messages can be clearer.
* Statistics should be printed upon stop() and not in the destructor.

All but the first point can be implemented with backwards compatibility. So the
first point is OCL 1.6.0 while the other points are OCL 1.4.1.

Christian Louboutin

It is named Christian Louboutin for the reason that man who create Christian Louboutin shoes. At the same time, it's helpful to reduce the risk of injury for the player. If you think your feet should be protected well during the strenuous exercise, the Christian Louboutin Pumps are the right choice for you. There are many Cheap Christian Louboutin Heels from our shop. If you want to learn more informations of those Christian Louboutin Boots, please visite our website http://www.louboutinshopsale.com/. Are you looking for a comfortable shoes? We are ready for you to give you great discount Christian Louboutin Sandals and best service.

[Bug 489] PCANController needs cleanup

For more infomation about this bug, visit

Peter Soetens

<peter [dot] soetens [..] ...> changed:

What |Removed |Added
--------------------------------------------------------------------------
Status|ASSIGNED |RESOLVED
Resolution| |FIXED

--- Comment #9 from Peter Soetens

<peter [dot] soetens [..] ...> 2008-02-15 17:16:08 ---
These patches have been applied without further modifications.
I'll open a new bug report for the 'periodic' vs 'nonperiodic'
performance issue.

$ svn ci -m"Fix bug #489: PCANController needs cleanup.
> Both PCAN and CANPie have had similar fixes. They remain
> periodic for now, can only be fixed in OCL 1.6.0"
Sending can/CANPieController.cpp
Sending can/CANPieController.hpp
Sending can/PCANController.cpp
Transmitting file data ...
Committed revision 28976.

ugg shoes

If you are looking for a pair of pretty UGG Boots, you shouldn't miss this chance for purchasing UGG Classic Cardy Boots this season in our shop. Ugg Bailey Button would give people such a deep impression. UGG Classic Short Boots appreciate it not only by the high quality with steady innovations but also by its stylish designs. There is no doubt that it could go well with you casual outfits. There are many kinds of UGG Classic Tall Boots for different ages and tastes online. So pick up your own UGG Sandals now.



[Bug 489] PCANController needs cleanup

For more infomation about this bug, visit

--- Comment #8 from Peter Soetens

<peter [dot] soetens [..] ...> 2008-02-05 13:53:00 ---
(In reply to comment #6)
> (In reply to comment #4)
> > Created an attachment (id=231)
--> (https://www.fmtc.be/bugzilla/orocos/attachment.cgi?id=231) [details] [details]
> > Addresses the comments of previous patch
> >
> > * better checks to avoid crashes upon start or cleanup
> > * run in ORO_SCHED_OTHER until further notice.
>
> Patch seems generally fine to me now. One small question remains
>
> void PCANController::step()
> {
> while ( this->readFromBuffer(_CANMsg) ){
> - _CANMsg.origin = this;
> _bus->write(&_CANMsg); // we own _CANMsg;
> }
> }
>
> What's the purpose of removing this line? (was not in the previous patch)
>

Is a dupe, the origin is already set to 'this' in readFromBuffer().

[Bug 489] PCANController needs cleanup

For more infomation about this bug, visit

--- Comment #7 from Klaas Gadeyne <klaas [dot] gadeyne [..] ...> 2008-02-05 13:27:24 ---
(In reply to comment #5)
> Created an attachment (id=232)
--> (https://www.fmtc.be/bugzilla/orocos/attachment.cgi?id=232) [details]
> Cleanups and fixes for the canpie controller class
>
> Backwards compatible fixes for the CANPieController

Patch seems ok.

[Bug 489] PCANController needs cleanup

For more infomation about this bug, visit

--- Comment #6 from Klaas Gadeyne <klaas [dot] gadeyne [..] ...> 2008-02-05 13:25:05 ---
(In reply to comment #4)
> Created an attachment (id=231)
--> (https://www.fmtc.be/bugzilla/orocos/attachment.cgi?id=231) [details]
> Addresses the comments of previous patch
>
> * better checks to avoid crashes upon start or cleanup
> * run in ORO_SCHED_OTHER until further notice.

Patch seems generally fine to me now. One small question remains

void PCANController::step()
{
while ( this->readFromBuffer(_CANMsg) ){
- _CANMsg.origin = this;
_bus->write(&_CANMsg); // we own _CANMsg;
}
}

What's the purpose of removing this line? (was not in the previous patch)

[Bug 489] PCANController needs cleanup

For more infomation about this bug, visit

--- Comment #5 from Peter Soetens

<peter [dot] soetens [..] ...> 2008-02-05 11:25:07 ---
Created an attachment (id=232)
--> (https://www.fmtc.be/bugzilla/orocos/attachment.cgi?id=232)
Cleanups and fixes for the canpie controller class

Backwards compatible fixes for the CANPieController

[Bug 489] PCANController needs cleanup

For more infomation about this bug, visit

Peter Soetens

<peter [dot] soetens [..] ...> changed:

What |Removed |Added
--------------------------------------------------------------------------
Attachment #208 is|0 |1
obsolete| |

--- Comment #4 from Peter Soetens

<peter [dot] soetens [..] ...> 2008-02-05 11:21:57 ---
Created an attachment (id=231)
--> (https://www.fmtc.be/bugzilla/orocos/attachment.cgi?id=231)
Addresses the comments of previous patch

* better checks to avoid crashes upon start or cleanup
* run in ORO_SCHED_OTHER until further notice.

[Bug 489] PCANController needs cleanup

For more infomation about this bug, visit

--- Comment #3 from Klaas Gadeyne <klaas [dot] gadeyne [..] ...> 2008-01-17 16:04:55 ---
(In reply to comment #1)
> Created an attachment (id=208)
--> (https://www.fmtc.be/bugzilla/orocos/attachment.cgi?id=208) [details]
> All backwards compatible fixes.

(In addition to comment #2)

- : PeriodicActivity(ORO_SCHED_OTHER, priority,period),
_handle(NULL),
+ : PeriodicActivity(ORO_SCHED_RT, priority, period), _handle(NULL),

Since you alter the scheduler, this modif is backwards compatible API wise, but
IIRC the parameter "priority" get's a different meaning when changing the
scheduler, so might this not break existing applications?

Furthermore, I don't like this idea very much since, although you give the
impression the thread now runs in HRT, there will be mode switches with the
above statements everytime the PCANController issues a system call.
One of the things I check to verify whether my application is real-time is
verifying if there are no mode switches. This get's harder with the above
change.

Did you check whether this really improves the latency of receiving CAN
messages?

Therefor, I would also leave the

- // This is a Non-Realtime driver!!
- log(Info) << "This is NOT a real-time CAN driver." << endlog();

statement in place.

- log(Error) << "Error opening " << DeviceName << endlog();
- // Add assert here?
+ log(Error) << DeviceName << ": failed. Could not open device."
<< endlog();

should probably become

- log(Error) << "Error opening " << DeviceName << endlog();
- // Add assert here?
+ // Throw exception here.
+ log(Error) << DeviceName << ": failed. Could not open device."
<< endlog();

If you can't open the device, you should throw an exception here and cleanup,
otherwise the destructor will fail I guess
The rest of the patch seems fine (you moron ;-)

[Bug 489] PCANController needs cleanup

For more infomation about this bug, visit

Klaas Gadeyne <klaas [dot] gadeyne [..] ...> changed:

What |Removed |Added
--------------------------------------------------------------------------
CC| |klaas [dot] gadeyne [..] ...

--- Comment #2 from Klaas Gadeyne <klaas [dot] gadeyne [..] ...> 2008-01-17 10:55:49 ---
(In reply to comment #0)
> The PCANController class for the peak-systems pcan driver needs a cleanup.
>
> * It should be non-periodic, blocking (with a timeout) on the
> LINUX_CAN_Read_Timeout function.
> * The code in finalize() should not close the device, such that a start() is
> possible after a stop().
> * The log messages can be clearer.
> * Statistics should be printed upon stop() and not in the destructor.
>
> All but the first point can be implemented with backwards compatibility. So the
> first point is OCL 1.6.0 while the other points are OCL 1.4.1.

I think 3 of the 4 (apart from the finalize thingy) above issues also hold for
CanPieController, so this one should maybe be retitled?

Klaas

[Bug 489] PCANController needs cleanup

For more infomation about this bug, visit

Peter Soetens

<peter [dot] soetens [..] ...> changed:

What |Removed |Added
--------------------------------------------------------------------------
Status|NEW |ASSIGNED
AssignedTo|orocos- |peter [dot] soetens [..] ...
|dev [..] ... |

--- Comment #1 from Peter Soetens

<peter [dot] soetens [..] ...> 2008-01-17 10:50:44 ---
Created an attachment (id=208)
--> (https://www.fmtc.be/bugzilla/orocos/attachment.cgi?id=208)
All backwards compatible fixes.