[Bug 488] New: AnalogIn/OutInterface is an atrocity.

For more infomation about this bug, visit
Summary: AnalogIn/OutInterface is an atrocity.
Product: RTT
Version: 1.4.0
Platform: All
OS/Version: All
Status: NEW
Severity: normal
Priority: P2
Component: Operating System Abstraction - Portability
AssignedTo: orocos-dev [..] ...
ReportedBy: peter [dot] soetens [..] ...
CC: orocos-dev [..] ...
Estimated Hours: 0.0

In contrast to the DigitalIn/OutInterface classes, the AnalogIn/OutInterface
takes a template parameter. Furthermore, the AnalogOutput helper class takes
the same parameter. It was meant to allow the choice between unsigned/signed
int or even float or double as the interface to an IO card with analog in or
out channels.

In reality the drivers return an int, or an unsigned int and uses max 16 bits,
but 12 bits ADC/DAC are more common. The AnalogOutput object allows conversion
between doubles and this integer value. I believe RTT 1.0 should have used
'int' as the data type for communicating with the analog interfaces. This would
have allowed devices with 31 bit unsigned int ADC/DAC and 32bit signed ADC/DAC,
far more than necessary.

Usercode would have changed from:

AnalogOutInterface* maout = AnalogOutInterface int>::nameserver.getObject("Ana_Out");

AnalogOutput output(maout, ANA_CHANNEL);
output.value( v ); // in volts.

to:

AnalogOutInterface* maout =
AnalogOutInterface::nameserver.getObject("Ana_Out");

AnalogOutput output(maout, ANA_CHANNEL);
output.value( v ); // in volts.

Which is consistent with the DigitalIn/OutInterface.

I don't have any idea how to solve this with respect to backwards
compatibility, since these classes also appear in user code.

[Bug 488] AnalogIn/OutInterface is an atrocity.

For more infomation about this bug, visit

Peter Soetens

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

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

--- Comment #11 from Peter Soetens

<peter [dot] soetens [..] ...> 2008-06-11 23:10:57 ---
I'm closing this bug. Open a new bug report in case of troubles.

[Bug 488] AnalogIn/OutInterface is an atrocity.

For more infomation about this bug, visit

--- Comment #10 from Peter Soetens

<peter [dot] soetens [..] ...> 2008-05-04 22:20:15 ---
Applied on trunk.

$ svn ci rtt/src/ rtt/tests/ -m\"Applying patch for bug #488:
AnalogIn/OutInterface is an atrocity.
> Make analog interface raw type signed
> \"
Sending rtt/src/dev/AnalogInInterface.hpp
Sending rtt/src/dev/AnalogInput.hpp
Sending rtt/src/dev/AnalogOutInterface.hpp
Sending rtt/src/dev/AnalogOutput.hpp
Sending rtt/src/dev/AxisInterface.hpp
Sending rtt/tests/FakeAnalogDevice.hpp
Transmitting file data ......
Committed revision 29223.
+ sspr@lt00129:~/src/Orocos/trunk
$ svn ci ocl -m"Applying patch for bug #488: AnalogIn/OutInterface is an
atrocity.
> Updates OCL for signed raw types.
> "
Sending ocl/hardware/comedi/dev/ComediSubDeviceAIn.cpp
Sending ocl/hardware/comedi/dev/ComediSubDeviceAIn.hpp
Sending ocl/hardware/comedi/dev/ComediSubDeviceAOut.cpp
Sending ocl/hardware/comedi/dev/ComediSubDeviceAOut.hpp
Transmitting file data ....
Committed revision 29224.

[Bug 488] AnalogIn/OutInterface is an atrocity.

For more infomation about this bug, visit

--- Comment #9 from Peter Soetens

<peter [dot] soetens [..] ...> 2008-05-04 22:18:44 ---
Created an attachment (id=273)
--> (https://www.fmtc.be/bugzilla/orocos/attachment.cgi?id=273)
Updates OCL for signed raw types.

Patch for OCL in addition to patch 271.

[Bug 488] AnalogIn/OutInterface is an atrocity.

For more infomation about this bug, visit

--- Comment #8 from Peter Soetens

<peter [dot] soetens [..] ...> 2008-05-04 22:18:00 ---
Created an attachment (id=272)
--> (https://www.fmtc.be/bugzilla/orocos/attachment.cgi?id=272)
Make analog interface raw type signed

After much [some mental process], I've changed the 'raw' type of the analog
interfaces to 'int' instead of 'unsigned int'. This allows in practice 32bits
signed AD converters and 31bits unsigned AD converters (only returns positive
numbers). This change was done to support two's complement AD converters as
well. In case your 'unsigned' AD converter has 32bits (does that exist?) your
implementation needs to offset it to a signed raw representation. I'm sure I'm
not bullying anyone with this... but you're free to correct me if I'm wrong.

This patch is for RTT (in addition to patch 270)

Peter

[Bug 488] AnalogIn/OutInterface is an atrocity.

For more infomation about this bug, visit

Peter Soetens

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

What |Removed |Added
--------------------------------------------------------------------------
Target Milestone|--- |1.6.0

--- Comment #7 from Peter Soetens

<peter [dot] soetens [..] ...> 2008-04-29 23:29:37 ---
Applied on trunk/rtt:
$ svn ci -m\"Fix bug #488: AnalogIn/OutInterface is an atrocity.
> Applied analog-interface-cleanup3.patch
> \"
Sending rtt/src/dev/AnalogInInterface.hpp
Sending rtt/src/dev/AnalogInput.hpp
Sending rtt/src/dev/AnalogOutInterface.hpp
Sending rtt/src/dev/AnalogOutput.hpp
Sending rtt/src/dev/io.cpp
Sending rtt/tests/FakeAnalogDevice.hpp
Sending rtt/tests/dev_test.cpp
Transmitting file data .......
Committed revision 29219.

And on trunk/ocl:
$ svn ci hardware/ -m"Fix bug #488: AnalogIn/OutInterface is an atrocity.
All OCL hardware components are adapted (and compile tested).
> "
Sending hardware/axes/dev/AnalogDrive.hpp
Sending hardware/axes/dev/AnalogSensor.hpp
Sending hardware/comedi/dev/ComediDevice.cpp
Sending hardware/comedi/dev/ComediDevice.hpp
Sending hardware/comedi/dev/ComediEncoder.cpp
Sending hardware/comedi/dev/ComediEncoder.hpp
Sending hardware/comedi/dev/ComediPulseTrainGenerator.cpp
Sending hardware/comedi/dev/ComediPulseTrainGenerator.hpp
Sending hardware/comedi/dev/ComediSubDeviceAIn.cpp
Sending hardware/comedi/dev/ComediSubDeviceAIn.hpp
Sending hardware/comedi/dev/ComediSubDeviceAOut.cpp
Sending hardware/comedi/dev/ComediSubDeviceAOut.hpp
Sending hardware/comedi/dev/ComediSubDeviceDIn.cpp
Sending hardware/comedi/dev/ComediSubDeviceDIn.hpp
Sending hardware/comedi/dev/ComediSubDeviceDOut.cpp
Sending hardware/comedi/dev/ComediSubDeviceDOut.hpp
Sending hardware/comedi/dev/ComediThreadScope.hpp
Sending hardware/comedi/dev/comedi_internal.h
Sending hardware/ethercat-demo/EthercatDemonAxesVelocityController.cpp
Sending hardware/ethercat-demo/EthercatDemonAxesVelocityController.hpp
Sending hardware/ethercat-demo/EthercatIO.hpp
Sending hardware/ethercat-demo/dev/AnalogEtherCATInputDevice.hpp
Sending hardware/ethercat-demo/dev/AnalogEtherCATOutputDevice.hpp
Sending hardware/io/IOComponent.hpp
Sending hardware/io/dev/FakeAnalogDevice.hpp
Sending hardware/kuka/Kuka160nAxesVelocityController.cpp
Sending hardware/kuka/Kuka160nAxesVelocityController.hpp
Sending hardware/kuka/Kuka361nAxesTorqueController.cpp
Sending hardware/kuka/Kuka361nAxesTorqueController.hpp
Sending hardware/kuka/Kuka361nAxesVelocityController.cpp
Sending hardware/kuka/Kuka361nAxesVelocityController.hpp
Sending hardware/laserdistance/LaserDistance.cpp
Sending hardware/laserdistance/LaserDistance.hpp
Sending hardware/lias/CRSnAxesVelocityController.cpp
Sending hardware/lias/CRSnAxesVelocityController.hpp
Sending hardware/lias/IP_FastDAC_AOutInterface.cpp
Sending hardware/lias/IP_FastDAC_AOutInterface.hpp
Sending hardware/performer_mk2/PerformernAxesVelocityController.cpp
Sending hardware/performer_mk2/PerformernAxesVelocityController.hpp
Sending hardware/staubli/StaubliRX130nAxesVelocityController.cpp
Sending hardware/staubli/StaubliRX130nAxesVelocityController.hpp
Sending hardware/xyPlatform/xyPlatformAxisHardware.cpp
Sending hardware/xyPlatform/xyPlatformAxisHardware.hpp
Transmitting file data ...........................................
Committed revision 29222.

[Bug 488] AnalogIn/OutInterface is an atrocity.

For more infomation about this bug, visit

Peter Soetens

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

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

--- Comment #6 from Peter Soetens

<peter [dot] soetens [..] ...> 2008-04-29 23:06:42 ---
Created an attachment (id=271)
--> (https://www.fmtc.be/bugzilla/orocos/attachment.cgi?id=271)
Adapt to cleanup3 from RTT.

This patch adapts to the final new Analog* API and also places Comedi in the
OCL namespace.

[Bug 488] AnalogIn/OutInterface is an atrocity.

For more infomation about this bug, visit

Peter Soetens

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

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

--- Comment #5 from Peter Soetens

<peter [dot] soetens [..] ...> 2008-04-29 23:03:08 ---
Created an attachment (id=270)
--> (https://www.fmtc.be/bugzilla/orocos/attachment.cgi?id=270)
user read/write again at Analog*Interface level + fix return values as well

This patch solves the issues pointed at by Klaas.
The return values are now 'int' (to indicate success/failure) in case of the
Analog*Interface classes and the value() names have been replaced by the
read/write equivalents again to be more clear.

AnalogInInterface and AnalogOutInterface are compatible (again) when you wish
to inherit from both classes.

The patch is not ABI compatible.

[Bug 488] AnalogIn/OutInterface is an atrocity.

For more infomation about this bug, visit

--- Comment #4 from Klaas Gadeyne <klaas [dot] gadeyne [..] ...> 2008-04-21 11:59:04 ---
(In reply to comment #3)
> Created an attachment (id=264)
--> (https://www.fmtc.be/bugzilla/orocos/attachment.cgi?id=264) [details]
> Improved the RTT API as well.
>
> Same as previous RTT patch but the API is overall better. Some methods
> (read,write,...) have been deprecated and have been
> given a new name (value, rawValue,...). The OCL patch works with this one.
>
> Any objections ?

None. The new names are an improvement (although I would maybe prefer
*valueGet() for the input channels, and valueSet() for the output channels to
avoid confusion between read/write inputs, but that's probably a matter of
taste), and backwards compatibility seems guaranteed with this patch.

Some small remarks (the whitespace fixes make it hard to read your patch though
:-(

Index: src/dev/AnalogInput.hpp
/**
* Read the raw value of this channel.
*/
- InputType rawValue() const
+ int rawValue() const
{
- InputType r;
- board->read(channel, r);
+ unsigned int r;
+ board->rawValue(channel, r);
return r;
}

Shouldn't the return type of this function be "unsigned int" too?

Similar remark for the rawValue of AnalogOutput.hpp

Furthermore, some methods now have a void return type, while a "real" return
value would make sense, e.g.

/**
* Write the raw value of this channel.
*/
- void rawValue(OutputType i)
+ void rawValue(int i)
{
- if ( i < board->binaryLowest(channel ) )
- i_cache = board->binaryLowest( channel );
- else if ( i > board->binaryHighest( channel ) )
- i_cache = board->binaryHighest( channel ) ;
- else
- i_cache = i;
- board->write(channel, i);
+ board->rawValue(channel, i);
}

where the last line does permit return values. From a first (and probably too
fast) test, it seems such void2type return modifications are even ABI
compatible?

[Bug 488] AnalogIn/OutInterface is an atrocity.

For more infomation about this bug, visit

Peter Soetens

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

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

--- Comment #3 from Peter Soetens

<peter [dot] soetens [..] ...> 2008-04-20 00:02:14 ---
Created an attachment (id=264)
--> (https://www.fmtc.be/bugzilla/orocos/attachment.cgi?id=264)
Improved the RTT API as well.

Same as previous RTT patch but the API is overall better. Some methods
(read,write,...) have been deprecated and have been
given a new name (value, rawValue,...). The OCL patch works with this one.

Any objections ?

[Bug 488] AnalogIn/OutInterface is an atrocity.

For more infomation about this bug, visit

--- Comment #2 from Peter Soetens

<peter [dot] soetens [..] ...> 2008-04-20 00:00:42 ---
Created an attachment (id=263)
--> (https://www.fmtc.be/bugzilla/orocos/attachment.cgi?id=263)
Adapts OCL's AnalogIn/Out use to template-less RTT implementation

This patch fixes all ocl/hardware classes for using the new RTT API.
The template parameter has been dropped. The comedi implementations are
now far more efficient (caching range settings etc.).

[Bug 488] AnalogIn/OutInterface is an atrocity.

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-03-17 10:51:49 ---
Created an attachment (id=252)
--> (https://www.fmtc.be/bugzilla/orocos/attachment.cgi?id=252)
removes the template parameter from RTT classes.

This patch removes the template parameter and assumes that 'int' is used to
read/write to channels.

Only the resolution is an unsigned int, all the other functions use 'int'.

This patch does not fix the OCL Comedi classes.