Merge ~bhill/epics-base:db-tpro-show-values into epics-base:3.16

Proposed by Bruce Hill
Status: Needs review
Proposed branch: ~bhill/epics-base:db-tpro-show-values
Merge into: epics-base:3.16
Diff against target: 168 lines (+48/-0)
11 files modified
src/std/rec/aSubRecord.c (+2/-0)
src/std/rec/aiRecord.c (+4/-0)
src/std/rec/aoRecord.c (+4/-0)
src/std/rec/biRecord.c (+6/-0)
src/std/rec/boRecord.c (+5/-0)
src/std/rec/calcRecord.c (+5/-0)
src/std/rec/calcoutRecord.c (+5/-0)
src/std/rec/longinRecord.c (+4/-0)
src/std/rec/longoutRecord.c (+4/-0)
src/std/rec/mbbiRecord.c (+4/-0)
src/std/rec/mbboRecord.c (+5/-0)
Reviewer Review Type Date Requested Status
Andrew Johnson Needs Fixing
Ralph Lange Needs Fixing
Review via email: mp+341081@code.staging.launchpad.net

Description of the change

Add diagnostic support for showing record values when TPRO is >= 2

To post a comment you must log in.
Revision history for this message
mdavidsaver (mdavidsaver) wrote :

See previous discussion at https://github.com/epics-base/epics-base/pull/20

I personally like the idea of using TPRO to control debug prints. So much so that I've been adding it to my drivers so the last couple of years.

I don't have a problem with overloading TPRO in this way in Base. I would make the threshold numbers it larger (5 and 10).

As this is Base, maybe we should add a new field instead? Is .DDBG taken (aka Device support DeBuG)?

Revision history for this message
Ralph Lange (ralph-lange) wrote :

I like the idea of separating the Device Support debug setting from Record Support debug setting (as I had stated in the GitHub discussion), so I would be in favor of a new field, and DDBG sounds good as a name.

Larger threshold values sound like a good idea.

I don't understand why the aSub does not get the VAL printed, and the message might be misleading (as OUTx are the output links, not the output values).
Also, for a debug function, I would expect it to print VALx values independent from the event posting setting. (As with all records that don't have such a setting.)

bi/bo should IMHO always print the numerical value, and add the string if ONAM/ZNAM are defined.

mbbi/mbbo should add the string representation if there is one defined.

For many record types, when used with other than Soft Device Support, the RVAL field is as interesting as the VAL field.

aai/aao/compress/dfanout/event/histogram/int64in/int64out/lsi/lso/mbbiDirect/mbboDirect/permissive/sel/seq/state/stringin/stringout/subArray/sub/waveform are missing.

review: Needs Fixing
Revision history for this message
Andrew Johnson (anj) wrote :

A Google search for "EPICS TPRO" came back with a tech-talk discussion:
  https://epics.anl.gov/tech-talk/2014/msg00129.php
This thread contained a response from me contrary to my github answer:

> I do like the idea of using TPRO > 1 to trigger debug messages from
> record processing, and this could also be used by device support. It
> should be up to the specific record/device support to document its usage
> though.

While writing that response in 2014 I also adjusted the Wiki description at
  https://wiki-ext.aps.anl.gov/epics/index.php/RRM_3-14_dbCommon
to say:

> If this field is non-zero a message is printed whenever this record is
> processed, and when any other record in the same lock-set is processed
> by a database link from this record.

Thus (following the legal principle of stare decisis) I rescind my github objection to the use of TPRO to enable debug messages, but I would like to see some kind of standardization in the values used, either globally or at least by the record type. If we're going to do this though we should also do all the record types in Base, not just the ones we use/like the most.

Question: Should the TPRO value be a verbosity level, page selector, a bit-mask or some combination of the 3?

I support Ralph's comments about the record-specific behaviors. The VAL field of an aSub record holds the status value returned by the subroutine and is less useful than that of a sub record, so I can understand not being interested in seeing that (printing SNAM when it changes would be useful though). The seq record has an even less useful VAL field, and I'm sure there are others like it so I think that what gets printed should be up to the record type.

Setting TPRO on a record also prints the record names of any other records in the same lockset that are processed directly by the record, but it does not change their TPRO fields, so those records will not have their debug print mechanisms triggered (unless they also have TPRO set). This is still reasonable behavior.

Setting TPRO to 255 could be used for a DOS attack if it results in a lot of output. We are probably already serializing all our record processing when TPRO is set though (through printing the record name to stdout in the dbProcess() routine) but this is really just another attack surface of which we already have too many for this to change.

review: Needs Fixing
Revision history for this message
Ralph Lange (ralph-lange) wrote :

> Question: Should the TPRO value be a verbosity level, page selector, a bit-mask or some combination of the 3?

Bit-masks may make sense in device support debugging that is often distributed over multiple layers - see ASYN's trace masks.

Record support is practically always one layer, and I see a verbosity level being more useful.

If we split Device Support debugging to a separate field, each device support can use it as it prefers: mask or page or level. In that case I would support TPRO being a verbosity level.

If TPRO is shared between Record and Device Support, things get messy.

Revision history for this message
mdavidsaver (mdavidsaver) wrote :

> Question: Should the TPRO value be a verbosity level, page selector, a bit-mask or some combination of the 3?

I've only used it as a verbosity level, aka. print if TPRO>=N for some value of N.

Revision history for this message
Bruce Hill (bhill) wrote :

I've found uses of TPRO as a verbosity level in several device support modules along w/ some that just enable
diagnostics for any non-zero tpro value.     That was what I was looking to do w/ this commit.

On 03/13/2018 08:12 AM, mdavidsaver wrote:
>> Question: Should the TPRO value be a verbosity level, page selector, a bit-mask or some combination of the 3?
> I've only used it as a verbosity level, aka. print if TPRO>=N for some value of N.

--
Bruce Hill
Member Technical Staff
SLAC National Accelerator Lab
2575 Sand Hill Road M/S 10
Menlo Park, CA 94025

Revision history for this message
Andrew Johnson (anj) wrote :

Core Meeting: Should be done for (and messages consistent from) all record types; TPRO value of 1 should be reserved for just the current trace processing, values for these messages should be sensible (higher numbers may print more stuff, leave room between numbers for device support, link support etc. to use as well); consider adding another message when beginning/ending an asynchronous I/O operation.

review: Needs Fixing

There was an error fetching revisions from git servers. Please try again in a few minutes. If the problem persists, contact Launchpad support.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches