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.

Sign up for GitHub

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

Jump to bottom

Provide a better interface to logging #48

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Provide a better interface to logging #48

wants to merge 4 commits into from

Conversation

ericonr
Copy link
Member

@ericonr ericonr commented Nov 18, 2024

Implements some of the improvements discussed in lnls-sirius/hla#720

Also adds an NTTable to provide atomic access to the timestamp and event waveforms.

erico.rolim@s-gie01-l:~$ pvinfo DE-23RaBPM:TI-EVG:EventTable-Mon
DE-23RaBPM:TI-EVG:EventTable-Mon
Server: 10.20.26.163:5075
Type:
    epics:nt/NTTable:1.0
        structure record
            structure _options
                int queueSize
                boolean atomic
        string[] labels
        structure value
            double[] A
            double[] B
 
erico.rolim@s-gie01-l:~$ pvmonitor DE-23RaBPM:TI-EVG:EventTable-Mon
DE-23RaBPM:TI-EVG:EventTable-Mon
  Timestamp Event
1.73161e+09   118
1.73194e+09   118
1.73194e+09   118

@ericonr
Copy link
Member Author

ericonr commented Nov 21, 2024

@henriquesimoes and @gustavosr8, could you take a look at this, please?

@gustavosr8
Copy link

Nit: maybe rewrite the last part of 3ef2ccc message as

This simplifies interaction for both humans (via GUI) and scripts.

henriquesimoes
henriquesimoes reviewed Nov 22, 2024
View reviewed changes
Copy link

@henriquesimoes henriquesimoes left a comment

Choose a reason for hiding this comment

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

I'm still learning how these definitions work, but they look fine.

I missed the reference (and explanation) for why we are not using a pure JSON specification of the group. Having group definition scattered around records seems pretty hard to maintain and understand as the number of groups increases.

timingApp/Db/event_log.db Show resolved Hide resolved
timingApp/Db/event_log.db Show resolved Hide resolved
@ericonr
Copy link
Member Author

ericonr commented Nov 22, 2024

I missed the reference (and explanation) for why we are not using a pure JSON specification of the group. Having group definition scattered around records seems pretty hard to maintain and understand as the number of groups increases.

epics-base/pvxs#88

For this IOC, since it's all in the same file, I didn't find using info(Q:group ...) so bad, and I don't think it's that awful to maintain (with few records at least). It was much worse with BPM data.

@henriquesimoes
Copy link

For this IOC, since it's all in the same file, I didn't find using info(Q:group ...) so bad, and I don't think it's that awful to maintain (with few records at least). It was much worse with BPM data.

I agree. Considering we couldn't make JSON work at all, I'm fine with keeping it that way. Once it gets to work, though, I wonder if it won't be better to standardize in a usually-more-readable format.

@ericonr
Copy link
Member Author

ericonr commented Nov 25, 2024

This PR might have broken log reset. Still need to take a look at that before merging.

@ericonr ericonr force-pushed the improve-logging branch 3 times, most recently from 528fb64 to 7861ea1 Compare December 10, 2024 16:16
@ericonr
event_log: clean up event log reset command.
58a8203 
The reset command was using the sseq record unnecessarily, because
"Wait" values for WAITx fields only work when using CA links, which
can't be used here, because they don't cause the compress records to be
processed.

We are enquiring about the reliability of the reset mechanism used here
in tech-talk [1]. We could use a single write to RES, without PP, but
that would mean the interface is only updated when the next event
arrives into the log, instead of having immediate effect, which doesn't
make for the best user experience.

[1] https://epics.anl.gov/tech-talk/2024/msg01373.php
@ericonr
event_log: add TSbuffer with whole timestamp.
5f828ca 
Instead of requiring that clients combine the UTC and SUBSEC information
into something meaningful, we can do it ourselves. This simplifies
interaction both for humans (via GUI) and for scripts.

Since we use a double value as the whole timestamp, it only supports up
to microsecond accuracy.
@ericonr
timingApp: build with PVXS module.
81a99f0 
This also requires adding libevent to RUNTIME_PACKAGES, at least for
now. epics-in-docker should consider bundling these dependencies by
default.
@ericonr
event_log: add EventTable NTTable.
9c91813 
Instead of requiring clients to combine multiple waveforms themselves,
provide a table with timestamp and event information.

It was necessary to add a top level trigger mapping to rstSoftBuff,
otherwise, when resetting the buffers, the table would have one element
from each buffer, due to their spurious processing; so we needed to
fetch the values again after the buffer reset was completed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@henriquesimoes henriquesimoes henriquesimoes approved these changes

@anacso17 anacso17 anacso17 approved these changes

@mmdonatti mmdonatti mmdonatti approved these changes

@lsperissinotto lsperissinotto Awaiting requested review from lsperissinotto

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ericonr @gustavosr8 @henriquesimoes @mmdonatti @anacso17