search

epics-modules / std

APS BCDA synApps module:
http://epics-modules.github.io/std/
Other
5 stars 9 forks source link

Epid (using EpidSoft support) does not work if ODEL==0 #12

Closed ralphlange closed 5 years ago

ralphlange commented 5 years ago

A change in c2f4981 introduced a bug in the devSoftEpid Device Support that leads to OVAL never being changed (i.e. controller is disfunctional) when ODEL == 0.

ralphlange commented 5 years ago

Fix:

Index: stdApp/src/devEpidSoft.c
===================================================================
--- stdApp/src/devEpidSoft.c    (working copy)
+++ stdApp/src/devEpidSoft.c    (working copy)
@@ -207,7 +207,7 @@
     pepid->dt   = dt;
     pepid->err  = e;
     pepid->cval  = cval;
-    if ((pepid->odel != 0) && (fabs(pepid->oval - oval) > pepid->odel)) {
+    if ((pepid->odel == 0) || (fabs(pepid->oval - oval) > pepid->odel)) {
         pepid->oval  = oval;
     }
     pepid->p  = p;
MarkRivers commented 5 years ago

Thanks for finding this. Clearly I never tested it after this commit. :-(

ralphlange commented 5 years ago

This might even justify a quick bugfix release - @keenanlang?

MarkRivers commented 5 years ago

Should be fixed with https://github.com/epics-modules/std/commit/caabd2e941ce93f9a8bab0a393a5bd1a775d22e9

@ralphlange can you test it?

ralphlange commented 5 years ago

I did - this change works for the ODEL==0 case.

Note that I did not test the ODEL functionality, for which the documentation seems to be out-of-date, so I can't be 100% sure what the correct behavior is.

ralphlange commented 5 years ago

Awwww. As @jeonghanlee points out in #13: the above fix is fine, the caabd2e commit is not.

jeonghanlee commented 5 years ago

@ralphlange , please check the pull request.

ralphlange commented 5 years ago

Not fixed by caabd2e, correct.

jeonghanlee commented 5 years ago

It was fixed with missing {. At the same time, I was working for ESS std module in order to upgrade the latest version. :)

ralphlange commented 5 years ago

I did the same upgrade for CODAC Core System today, that's when I found epid had stopped working. (CCS adds a few tests to verify basic functionality.) Obviously, I had fixed the issue for the CCS code (see above diff), so I didn't follow the details of merging it upstream. Issue fixed with merging #13 by 7aabaf1.

@keenanlang: sorry that jump was too short...

keenanlang commented 5 years ago

Updated the bugfix release to point to correctly fixed commit.

ralphlange commented 5 years ago

Thanks a lot. Yesterday might not make it into the great book of QA History.

MarkRivers commented 5 years ago

I can't believe I did the commit without even typing make!

Lessons learned:

ralphlange commented 5 years ago

Actually, std does have a Travis build with a working badge on the README - none of us checked and saw that it failed. (Pretty pathetic.)

ralphlange commented 5 years ago

The epid record being disfunctional since May without anyone noticing is also a bad sign. Of tests missing, in this case.

MarkRivers commented 5 years ago

Yes, I realized that there was a Travis failure right after I said we needed to add Travis!