PHAS is not always respected when SCAN=Event

Bug #1899697 reported by Bruno Martins
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
EPICS Base
Status tracked in 7.0
3.15
Fix Released
Medium
Martin Konrad
7.0
Fix Released
Medium
Martin Konrad

Bug Description

There is a bug when adding records to an Event scan list. Adding a record to an event scan list with a lower PHAS than any other previously added record will end up putting it at the *end* of the list (not at the beginning). Triggering the bug:

    $ cat test.db
    record(ai, "SECOND") { field(EVNT, "E") field(SCAN, "Event") field(PHAS, "1") }
    record(ai, "FIRST") { field(EVNT, "E") field(SCAN, "Event") field(PHAS, "0") }
    record(ai, "THIRD") { field(EVNT, "E") field(SCAN, "Event") field(PHAS, "2") }

    $ softIoc -d test.db
    Starting iocInit
    ############################################################################
    ## EPICS R3.15.8-1+0~20200608134952.18+debian10~1.gbp3c2d35
    ## EPICS Base built Jun 8 2020
    ############################################################################
    iocRun: All initialization complete
    epics> scanpel
    Event "E"
     Priority Low
        SECOND
        FIRST
        THIRD
    epics>

The expected order is FIRST, SECOND, THIRD. I attached a patch (for 3.15.8) that I think fixes the issue. I found it in 3.15.8 but it seems to affect 7.0 series as well.

Revision history for this message
Bruno Martins (brunoseivam) wrote :
Andrew Johnson (anj)
Changed in epics-base:
status: New → Triaged
importance: Undecided → Medium
Revision history for this message
Andrew Johnson (anj) wrote :

Oops! Accepted by inspecting the code, thanks for the fix! Note that this won't just affect SCAN==Event, all the scan types respect PHAS and could be affected.

This definitely needs to be fixed on the 3.15 branch and merged up.

It ought to be possible to combine the two calls to ellInsert() in that routine. Remove the first call completely and the leading if() statement in Bruno's patch and use (ptemp ? &ptemp->node : NULL) for the second argument.

Bruno's patch also fixes a proto-bug in that the second argument to the current ellAdd() call assumes that offsetof(scan_element, node)==0. I thought we'd fixed most of those but we missed that one and in the similar ellDelete() call lower down - replace (void *)pse with &pse->node.

It would be nice to add some tests to dbScanTest.c for this and similar scan-list manipulations, but that exists only on the 7.0 branch so it would have to be done separately. Codeathon task?

Revision history for this message
Martin Konrad (info-martin-konrad) wrote :

I can confirm that Bruno's patch fixes the problem. Andrew, thanks for confirming that &pse->node is the way to go here. I agree that this should be fixed in deleteFromList() as well. I'll take a look at combining the two ellInsert() calls.

Revision history for this message
Martin Konrad (info-martin-konrad) wrote :

Please see my merge request for Bruno's patch + minor refactoring to get rid of one of the ellInsert() calls.

Revision history for this message
Dirk Zimoch (dirk.zimoch) wrote :

For the record: This bug does not exist in 3.14.

Revision history for this message
Martin Konrad (info-martin-konrad) wrote :

Correct, this regression has accidentally been introduced while refactoring this function to improve performance https://git.launchpad.net/epics-base/commit/?h=3.15&id=56d5a5935625074632ee1e9c1b9a6f18a4e06294.

To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers