[Bug 578] New: Unable determine comedi device validity

For more infomation about this bug, visit
Summary: Unable determine comedi device validity
Product: OCL
Version: trunk
Platform: All
OS/Version: All
Status: NEW
Severity: normal
Priority: P2
Component: Hardware
AssignedTo: orocos-dev [..] ...
ReportedBy: kiwi [dot] net [..] ...
CC: orocos-dev [..] ...
Estimated Hours: 0.0

The existing code hides the actual comedi device within a class, and so you
can't check for a valid connection. Attached patch allows access to the device
(without exposing it to the interface) and also provides a validity check of
the subdevice.

[Bug 578] Unable determine comedi device validity

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

--- Comment #9 from S Roderick <kiwi [dot] net [..] ...> 2009-07-10 20:20:33 ---
What can be done to get this patch accepted? Would really like to see it on the
mainline. Were all the objections dealt with?

[Bug 578] Unable determine comedi device validity

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

--- Comment #8 from S Roderick <kiwi [dot] net [..] ...> 2009-05-22 14:12:41 ---
Did we ever come to any conclusion on this patch? Would love to see this
functionality included in OCL. Are any further mod's to the patch requested?
S

[Bug 578] Unable determine comedi device validity

For more infomation about this bug, visit

--- Comment #7 from Klaas Gadeyne <klaas [dot] gadeyne [..] ...> 2008-08-14 13:54:27 ---
(In reply to comment #6)
[...]
> We had this discussion of wether 1. a constructor should throw when receiving
> bad arguments (for example a null pointer or wrong configuration data) or 2.
> just let an 'invalid' object exist that does not respond to the method calls.
>
> The latter (#2) is 'dangerous' because if you don't check isValid() you're
> assuming the device/object works and get wrong or unsafe behaviour. A throw
> from a constructor (#1) avoids this (and is OO-speaking the best thing to do)
> but will not allow this code to be compiled with --fno-exceptions. A flag that
> gains considerable code size reductions.
>
> A third option is to have rule #2 for the RTT, such that the RTT can be used in
> exception-free applications, while the OCL, consisting of all optional,
> cherry-picked by the user components, may choose freely which method to
> implement, and preferably uses #1.
>
> Does this sound acceptable ?

Yep. Moreover, 99.99% [*] of the people using comedi won't compile their code
using -fno-exceptions, and in this case 1. really makes sense.

This is also in sync with
, the numerical
recipes in C between the c++-faqs.

[*] Not to say 100%. If you do belong to the 0.01%, please speak up :-)

[Bug 578] Unable determine comedi device validity

For more infomation about this bug, visit

--- Comment #6 from Peter Soetens

<peter [dot] soetens [..] ...> 2008-08-14 13:24:17 ---
(In reply to comment #5)
> (In reply to comment #4)
> > Created an attachment (id=338)
--> (https://www.fmtc.be/bugzilla/orocos/attachment.cgi?id=338) [details] [details]
> > Full featured patch
>
> Your IsValid method doesn't check validity of the subdevice, only of the device
> itself, e.g.

As the code shows, when the subdevice is invalid, myCard == 0, hence isValid()
returns false.

We had this discussion of wether 1. a constructor should throw when receiving
bad arguments (for example a null pointer or wrong configuration data) or 2.
just let an 'invalid' object exist that does not respond to the method calls.

The latter (#2) is 'dangerous' because if you don't check isValid() you're
assuming the device/object works and get wrong or unsafe behaviour. A throw
from a constructor (#1) avoids this (and is OO-speaking the best thing to do)
but will not allow this code to be compiled with --fno-exceptions. A flag that
gains considerable code size reductions.

A third option is to have rule #2 for the RTT, such that the RTT can be used in
exception-free applications, while the OCL, consisting of all optional,
cherry-picked by the user components, may choose freely which method to
implement, and preferably uses #1.

Does this sound acceptable ?

Peter

[Bug 578] Unable determine comedi device validity

> --- Comment #6 from Peter Soetens

<peter [dot] soetens [..] ...> > 2008-08-14 13:24:17 ---
> (In reply to comment #5)
>> (In reply to comment #4)
>>> Created an attachment (id=338)
> --> (https://www.fmtc.be/bugzilla/orocos/attachment.cgi?id=338)
> [details] [details]
>>> Full featured patch
>>
>> Your IsValid method doesn't check validity of the subdevice, only
>> of the device
>> itself, e.g.
>
> As the code shows, when the subdevice is invalid, myCard == 0, hence
> isValid()
> returns false.

I guess that Klaas's additional check is just to ensure that you get
the correct kind of device. Could getSubDeviceType() really return the
wrong type? (yes it could, reviewing the code)

> We had this discussion of wether 1. a constructor should throw when
> receiving
> bad arguments (for example a null pointer or wrong configuration
> data) or 2.
> just let an 'invalid' object exist that does not respond to the
> method calls.
>
> The latter (#2) is 'dangerous' because if you don't check isValid()
> you're
> assuming the device/object works and get wrong or unsafe behaviour.
> A throw
> from a constructor (#1) avoids this (and is OO-speaking the best
> thing to do)
> but will not allow this code to be compiled with --fno-exceptions. A
> flag that
> gains considerable code size reductions.

Personally, I prefer the no-exception approach (#2). I would prefer to
check for validity explicitly.

> A third option is to have rule #2 for the RTT, such that the RTT can
> be used in
> exception-free applications, while the OCL, consisting of all
> optional,
> cherry-picked by the user components, may choose freely which method
> to
> implement, and preferably uses #1.
>
> Does this sound acceptable ?

I do understand the desire to do this, but it might make things
confusing for new users who get exceptions in one place and not in
another. This would have to be well documented.

S

[Bug 578] Unable determine comedi device validity

For more infomation about this bug, visit

--- Comment #5 from Klaas Gadeyne <klaas [dot] gadeyne [..] ...> 2008-08-13 15:13:54 ---
(In reply to comment #4)
> Created an attachment (id=338)
--> (https://www.fmtc.be/bugzilla/orocos/attachment.cgi?id=338) [details]
> Full featured patch

Your IsValid method doesn't check validity of the subdevice, only of the device
itself, e.g.

The current constructors all use this kind of code

void ComediSubDeviceAIn::init()
{
if ( !myCard) {
log(Error) << "Error creating ComediSubDeviceAIn: null ComediDevice
given." < return;
}
if ( myCard->getSubDeviceType( _subDevice ) != COMEDI_SUBD_AI )
{
log(Error) << "comedi_get_subdevice_type failed" << endlog();
myCard = 0;
return;
}

AFAIS
- either we should throw an exception when this fails (but I know Peter doesn't
like that idea :-) instead of just returning.
- either I would suggest to replace your IsValid() code with

bool ComediSubdeviceAIn::isValid()
{
if ( !myCard) {
return false;
}
else
if ( myCard->getSubDeviceType( _subDevice ) != COMEDI_SUBD_AI )
{
return false;
}
else return true;
}

And use the isValid method in the ::init() methods in order to avoid code
duplication.

thoughts?

Klaas

[Bug 578] Unable determine comedi device validity

For more infomation about this bug, visit

S Roderick <kiwi [dot] net [..] ...> changed:

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

--- Comment #4 from S Roderick <kiwi [dot] net [..] ...> 2008-08-13 14:51:22 ---
Created an attachment (id=338)
--> (https://www.fmtc.be/bugzilla/orocos/attachment.cgi?id=338)
Full featured patch

[Bug 578] Unable determine comedi device validity

For more infomation about this bug, visit

Peter Soetens

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

What |Removed |Added
--------------------------------------------------------------------------
CC| |peter [dot] soetens [..] ...

--- Comment #3 from Peter Soetens

<peter [dot] soetens [..] ...> 2008-08-13 13:56:02 ---
(In reply to comment #2)
> NB, our code to use these checks is ...
> {{{
> // create comedi devices
> comediDev = new OCL::ComediDevice(
> comediDev_prop.rvalue() );
> rc = (NULL != comediDev->getDevice()) &&
> (NULL != comediDev->getDevice()->it);
> if (rc)
> {
> comediSubdev = new OCL::ComediSubDeviceAIn(
> comediDev, getName(), comediSubdev_prop.rvalue());
> rc = comediSubdev->isValid();
> if (rc)
> {
> }}}
>

The reason comedi_internal.h is called like that is because it really is
required to be internal, especially for targets like RTAI/LXRT. Why don't you
use isValid() on the comediDevice itself (which checks in turn 'it') ? That
offers a more uniform API.
The isValid() function is a good extension, but it should be applied on all
comedi (sub)device classes (matter of copy/paste).

Peter

[Bug 578] Unable determine comedi device validity

For more infomation about this bug, visit

--- Comment #2 from S Roderick <kiwi [dot] net [..] ...> 2008-08-11 14:30:25 ---
NB, our code to use these checks is ...
{{{
// create comedi devices
comediDev = new OCL::ComediDevice(
comediDev_prop.rvalue() );
rc = (NULL != comediDev->getDevice()) &&
(NULL != comediDev->getDevice()->it);
if (rc)
{
comediSubdev = new OCL::ComediSubDeviceAIn(
comediDev, getName(), comediSubdev_prop.rvalue());
rc = comediSubdev->isValid();
if (rc)
{
}}}

[Bug 578] Unable determine comedi device validity

For more infomation about this bug, visit

--- Comment #1 from S Roderick <kiwi [dot] net [..] ...> 2008-08-11 14:28:06 ---
Created an attachment (id=334)
--> (https://www.fmtc.be/bugzilla/orocos/attachment.cgi?id=334)
Provide device access and add validity check