Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make longout record only write on change #63

Closed
wants to merge 5 commits into from

Conversation

joaopaulosm
Copy link
Contributor

This pull request refers to bug 1398215 on Launchpad (https://bugs.launchpad.net/epics-base/+bug/1398215), which is "Make output records only write on change". The first record to have this feature implemented is the longout record.

Two fields were added to the record: OOPT and OVAL. The first one is a menu field with usual options from other records, the second holds the last value written and its used for the comparison during the record process routine.

OOPT can assume the following options:

  • Every Time (default)
  • On Change
  • When Zero
  • When non-zero
  • When Transition to Zero
  • When Transition to non-zero
  • Write Once then On Change (*)

If the OUT field is changed during runtime and OOPT is "On Change", the record special function will force OOPT to be "Write Once then On Change". This will cause output record write to new destination even if the value is equal the previous one.

The second commit refers to test routines of the feature that was implemented.

@AppVeyorBot
Copy link

Build EPICS Base base-7.0-546 failed (commit d0cb53d184 by @)

@anjohnson
Copy link
Member

Group review 2020-03-25: Needs testing, approve in principle before merging.

@anjohnson
Copy link
Member

This PR now has conflicts in longoutRecord.c file, probably from 36a8b51. More importantly though the two additional fields and their behavior need to be documented in the longoutRecord.dbd.pod reference text, and it should also have a brief entry in the Release Notes. Maybe at the upcoming codeathon?

@joaopaulosm
Copy link
Contributor Author

Sounds good! Maybe expand to the remaining records. Looking forward to the codeathon!

@bfrk
Copy link
Contributor

bfrk commented Mar 2, 2021

I would prefer to add a separate field to store the "write once" bit, rather than adding an option to the menu. Rationale:

  1. It is cleaner than fiddling with the nominally user defined OOPT.
  2. Whether a change in OUT should trigger a write should be decided by the database developer, not by the record support.

The semantics of this field would be "consider a change in OUT as a value change wrt the OOPT 'on change' value", type could be menuYesNo.

@joaopaulosm
Copy link
Contributor Author

Fair enough, it also keeps OOPT consistent with the other records that already use it.

@joaopaulosm joaopaulosm changed the title Make longout record only write on change WIP: Make longout record only write on change Mar 8, 2021
@AppVeyorBot
Copy link

@joaopaulosm
Copy link
Contributor Author

Ok, I've done some modification on the implementation.
Three fields were added: OOPT, PVAL and OOCH. The first one is a menu field with the same options as CALCOUT record, the second holds the previous value written and its used for the comparison during the record process routine. The third (OOCH) is the flag that tells the record support if it should consider a change in OUT as a value change (default to YES).

So, the default behaviour when OOPT is "On Change" and the OUT link is changed is to write to the output during the next time the record is processed even if VAL is the same. To avoid this behaviour, OOCH must be set to NO.

I've also added more test cases in longoutTest.c to validate all cases. The tests are done using a simple longout record that writes to the B field of CALC records. These calc records are configured as counters (CALC = A + 1, with A being the calc record itself). This way, we can measure how many times the longout has written to the output link by checking the value of the calc fields.

If the implementation, nomenclature and documentation are OK, I can extend this feature to the remaining output records:

  • bo
  • ao
  • mbbo
  • mbboDirect
  • int64out
  • lso
  • stringout
  • ???

@joaopaulosm joaopaulosm changed the title WIP: Make longout record only write on change Make longout record only write on change Mar 9, 2021
Copy link
Member

@anjohnson anjohnson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love all the tests, thank-you for writing them!

I'm requesting a few minor changes but I think this looks good otherwise.

modules/database/src/std/rec/longoutRecord.dbd.pod Outdated Show resolved Hide resolved
modules/database/src/std/rec/longoutRecord.c Outdated Show resolved Hide resolved
modules/database/src/std/rec/longoutRecord.c Outdated Show resolved Hide resolved
@joaopaulosm
Copy link
Contributor Author

joaopaulosm commented Mar 10, 2021

Thanks @anjohnson! All your requests were implemented.

@joaopaulosm
Copy link
Contributor Author

We may have a problem with the "On Change" OOPT mode: the first (initial) processing. Since the PVAL (previous val) is being initialised with the same value as VAL in the init_record function, the first time record is processed it won't write to its output (OUT) link.

What should we do?

  • Create another field so the database developer decides if the IOC should force an initial write when OOPT = On Change;
  • Initialise PVAL with a different value than VAL when OOPT = On Change, assuming this behaviour;
  • Nothing (?)

@anjohnson
Copy link
Member

There would seem to be overlap with the OOCH and OUTPVT field in that problem, could you maybe initialize OUTPVT to OUT_LINK_CHANGED in the init_record() method instead, or use the OOCH setting to determine the behavior?

@joaopaulosm
Copy link
Contributor Author

Done! Added a few tests cases for that scenario.

@AppVeyorBot
Copy link

@anjohnson
Copy link
Member

Group meeting 7/28 Re-run tests against latest.
ANJ to do final approval.

@anjohnson anjohnson self-assigned this Jun 8, 2022
anjohnson added a commit to anjohnson/epics-base that referenced this pull request Jan 1, 2023
@anjohnson
Copy link
Member

Merged after rebase and some minor edits, at 413f14e.

Sorry for the long delay, at least it got in before 2023!

@anjohnson anjohnson closed this Jan 1, 2023
@joaopaulosm
Copy link
Contributor Author

Thank you Andrew! I should probably find some time to extend this feature to the other output records. In fact, I think I already did it for the stringout.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants