epics-modules / mrfioc2

EPICS driver for Micro Research Finland event timing system devices
http://epics-modules.github.io/mrfioc2/
Other
8 stars 29 forks source link

EVG Sequencer: UCHAR for eventCodes doesn't seem to work correctly #118

Closed agaget closed 6 months ago

agaget commented 6 months ago

When I am trying to caput the SoftSeq0EvtCodePVs, that is used to set the sequence of event code, I've just noticed that when I use a value greater than 128 it goes negative. As if it is not signed.

agaget@agaget-dev:~$ caput -a Labo:titan-EVM:SoftSeq0EvtCode-SP 2 129 120      
Old : Labo:titan-EVM:SoftSeq0EvtCode-SP 2047 1 2 0 0 0 0 0 ...
...
New : Labo:titan-EVM:SoftSeq0EvtCode-SP 2047 -127 120 0 0

The guilty PV is this one

record(waveform, "$(P)EvtCode-SP") {
    field( DTYP, "Obj Prop waveform out")
    field( DESC, "Sequence event code array")
    field( INP,  "@OBJ=$(EVG):SEQ$(seqNum), CLASS=SeqManager, PARENT=$(EVG):SEQMGR, PROP=CODES")
    field( NELM, "$(NELM)")
    field( FTVL, "UCHAR")
    info( autosaveFields_pass1, "VAL")
}
    void setEventCode(const epicsUInt8* arr, epicsUInt32 count)
    {
        codes_t codes(count);
        std::copy(arr,
                  arr+count,
                  codes.begin());
       //Up to this point it's still the correct values, but after that I'm not sure to understand the code so ...
        {
            SCOPED_LOCK(mutex);
            scratch.codes.swap(codes);
            is_committed = false;
        }
        DEBUG(4, ("Set events\n"));
        scanIoRequest(changed);
    }

PS :I understand the use of the UCHAR to limit the count of event to 0..255, but I have tested to replace UCHAR by short and by associated type in the C, it seems to work fine. Note that by using a waveform[SHORT] you can initialize more easily the waveform with a dbpf Labo:titan-EVM:SoftSeq0EvtCode-SP "[1 , 2 , 3]" , it doesn't work with a waveform[UCHAR]... We can imagine to check that the event is not greater than 255 in the previous code.... but it's not really the issue here

I suppose it's @mdavidsaver 's code ?

agaget commented 6 months ago

@agaget , all is ok:

caput -S TDL-JJ:Ctrl-EVG-1:SoftSeq0EvtCode-SP 2 129 120
Old : TDL-JJ:Ctrl-EVG-1:SoftSeq0EvtCode-SP 2 129 120
New : TDL-JJ:Ctrl-EVG-1:SoftSeq0EvtCode-SP 2 129 120

dbpf should work the same for UCHAR, so If you find the issue there, open a relevant ticket to the epics base team. ok

what is your EPICS version ? you have test on mrfioc2 2.5.0 or master ?

My test (I forgot to mention earlier) is on epics 7.07 and mrfioc2 2.5.0

jerzyjamroz commented 6 months ago

@agaget , I recommend to use it like that:

pvput TDL-JJ:Ctrl-EVG-1:SoftSeq0EvtCode-SP 2 129 120
Old : 2024-01-15 10:00:16.203  [129,120]
New : 2024-01-15 10:01:36.196  [129,120]

P.S. I was editing the comment and it got corrupted somehow, so this is the continuation.

jerzyjamroz commented 6 months ago

epics 7.07 and mrfioc2 2.5.0

I use the same.

agaget commented 6 months ago

@agaget , I recommend to use it like that:

pvput TDL-JJ:Ctrl-EVG-1:SoftSeq0EvtCode-SP 2 129 120
Old : 2024-01-15 10:00:16.203  [129,120]
New : 2024-01-15 10:01:36.196  [129,120]

P.S. I was editing the comment and it got corrupted somehow, so this is the continuation.

We don't use PVAccess for now.

I will try cleaner test today to understand where does this error come from.

mdavidsaver commented 6 months ago

I've just noticed that when I use a value greater than 128 it goes negative. As if it is not signed.

@agaget CA protocol does not support unsigned integer types. (PVA protocol does) So what you describe is expected when a client does C style cast before sending.

$ cainfo TST{EVR-Out:FP4}Pat:Rise-SP
TST{EVR-Out:FP4}Pat:Rise-SP
    State:            connected
    Host:             10.127.127.1:5064
    Access:           read, write
    Native data type: DBF_CHAR
    Request type:     DBR_CHAR
    Element count:    20
agaget commented 6 months ago

I've just noticed that when I use a value greater than 128 it goes negative. As if it is not signed.

@agaget CA protocol does not support unsigned integer types. (PVA protocol does) So what you describe is expected when a client does C style cast before sending.

$ cainfo TST{EVR-Out:FP4}Pat:Rise-SP
TST{EVR-Out:FP4}Pat:Rise-SP
    State:            connected
    Host:             10.127.127.1:5064
    Access:           read, write
    Native data type: DBF_CHAR
    Request type:     DBR_CHAR
    Element count:    20

Interesting to know. I hope that we will migrate to PVA soon.

But in that case it means it's not fully adapted to the CA ? Ok it wrotes the correct binary number in the cards registers, but it's wrongly displayed (in Phoebus and Caget at least) and a potential cause of error. Shouldn't we use another type (short) and test the condition of <255 in the function instead ?

I think I can propose easily a PR to test something if you're ok.

jerzyjamroz commented 6 months ago

@agaget , this is not the issue of the mrfioc2 module.

agaget commented 6 months ago

@agaget , this is not the issue of the mrfioc2 module.

I think it depends, if it's an issue in the epics-base and that is planned to be corrected, I agree. If it's a behaviour known and that will never be changed, I think the driver needs to adapt. @mdavidsaver can probably tell us more ;)