(Two meta comments: firstly, apologies for lack of proper e-mail quoting, my e-mailer makes it next to impossible, and the quoting in your e-mail is almost
unreadable (nearly impossible to tell which text is quoted), possibly for the same reason. Secondly, why on earth doesn’t “Reply All” work on this mailing list? Is this again my crappy e-mailer, or is this some strage tech-talk configuration?)
In you anxiety to avoid the “undefined” wrapping that occurs with integer arithmetic (technically, undefined for signed, defined as modulo word size for unsigned),
you changed the semantics drastically. This doesn’t feel like a good trade off to me.
Ignoring your boundary checking and the rounding, and with extra casts inserted to make the types involved clearer, the difference between the two versions
of the conversion code boils down to this:
old:
rval = (int32_t) (double) value - (int32_t) roff;
new:
rval = (int32_t) ((double) value - (double) (uint32_t) roff);
Now let’s look at a value of 0 being adjusted with an roff of -512 (for whatever reason, this is what the Hy8402 driver does):
old:
rval = 512;
new:
rval = - (uint32_t) -512 = -4294966784;
Oops.
I do wonder if it is time to act on the proposal of 1998 (a trifle before my time in this field) and remove these conversions altogether. For my own driver
support I gave up on letting EPICS do the ai/ao conversion long ago and use the unconverted val field instead. Much simpler and more predictable!
On 6/11/19 3:28 AM, Ralph Lange via Tech-talk wrote:
Use of the ROFF field has been effectively deprecated since 1998, when its functionality was taken over by the cleaner ESLO/EOFF mechanism. ROFF was mainly kept for compatibility reasons.
It got un-deprecated again later because there are things ROFF can do that EOFF can't (see below). The change that Michael A is objecting to happened as a result of the discussion thread starting at
https://epics.anl.gov/tech-talk/2013/msg00684.php where I pointed out that we were relying on Undefined Behaviour (UB) in some cases, so that code needed to be fixed.
About the Hytec driver support, Michael wrote:
The underlying problem is that the driver support relies on being able to set the ROFF field to a negative value. Unfortunately this is no longer possible, and the result is that ao outputs drive into maximum negative output voltage (potentially very damaging), and the ai inputs can also produce nonsense values (haven't actually tried yet, but the driver code is also now clearly broken).
In the ao record's convert() routine ROFF gets subtracted from the scaled (ESLO+EOFF) and adjusted (ASLO+AOFF) value before it gets rounded and cast to the epicsInt32 RVAL field. Subtraction
of a positive number is necessary here to avoid the UB that would occur if the code relied on value wrap-around to map part of the EGUL-EGUF value range into the signed int32 range of RVAL, which I suspect the Hytec driver may be doing.
Michael: Can you trace the conversion calculations that happen when you set VAL to EGUF, 0 and EGUL to generate RVAL for that driver? Do any of those rely on scaled values greater than 0x7fffffff being wrapped to negative numbers when that negative ROFF gets
subtracted and the result cast to an epicsInt32?
However, I think that not having many users being affected might be related to the ROFF deprecation 17 years before.
ROFF is still used by many device support that talk directly to ADC/DAC registers, but all the device support I have seen that use ROFF set it to a positive number when the hardware has an offset zero — that's why I didn't see much risk
in making this code change back in 2015.
I disagree with Ben's suggestion of 21 years ago that ROFF can easily be replaced by modifying EOFF, for 2 main reasons:
1. EOFF is only used by the record's convert() routine when LINR is set to LINEAR or SLOPE.
2. A user can set AOFF and/or ASLO to correct their ADC/DAC's calibration, which is what those fields were designed for. Any device support that sets EOFF instead of ROFF would need to recalculate the value of EOFF after each adjustment of AOFF/ASLO. EGUL and
EGUF are marked as SPC_LINCONV, but AOFF and ASLO are not, so there's nothing in the record that could trigger that recalculation to happen automatically.
- Andrew
--
Complexity comes for free, Simplicity you have to work for.
This e-mail and any attachments may contain confidential, copyright and or privileged material, and are for the use of the intended addressee only. If you are not the intended addressee or an authorised recipient of the addressee please notify us of receipt by returning the e-mail and do not use, copy, retain, distribute or disclose the information in or attached to the e-mail.
Any opinions expressed within this e-mail are those of the individual and not necessarily of Diamond Light Source Ltd.
Diamond Light Source Ltd. cannot guarantee that this e-mail or any attachments are free from viruses and we cannot accept liability for any damage which you may sustain as a result of software viruses which may be transmitted in or with the message.
Diamond Light Source Limited (company no. 4375679). Registered in England and Wales with its registered office at Diamond House, Harwell Science and Innovation Campus, Didcot, Oxfordshire, OX11 0DE, United Kingdom