Fix promptgroup settings in record definitions

I found major parts of the existing set really badly designed.

Instead of being orthogonal to the record types and defining groups of fields "related to the main functionality", "related to simulation mode", "related to disabling" etc., they go with the record types and define "subroutine related fields of the subroutine record", "multi-bit related fields of the multi-bit records", "specific fields of the compress record" etc.
Many are meaningless and not used in a reasonable way ("GUI_CLOCK", "GUI_TIMER", "GUI_MOTOR")

I would propose a complete and clean redesign. Anyone against that?

What should I do to retain compatibility?
- replace existing defs?
- reuse existing defs and add new types at the end?
- abandon (don't use) existing defs and add a complete new set at the end?

I agree with you about the bad design.

Important question: Does VDCT have its own list of GUI_xxx strings, or will it accept anything in a promptgroup() entry? The 3.15 Perl DBD parser will accept anything that begins "GUI_" but even that is easily changed (tools/DBD/Recfield.pm). Other DBD parsers also exist, so it would be worth asking on tech-talk if there are objections to changing the set of allowed strings.

You might also want to check with Kay, I have a vague recollection that SNS might have done something with the record promptgroup values at some time in the past.

It might be nice for the set of prompt group choices to be extensible; currently in dbStaticLib it is stored as a short index number taken from the fixed set given in the guigroup.h header. Options might be:
1. Change flddes.promptgroup to a string, discard guigroup.h and change the dbStaticLib API (replacing the dbGetPromptGroup() routine) to match.
2. Keep the short index and dbGetPromptGroup() but discard guigroup.h and make dbLexRoutines generate the list at load time, adding entries to the list as it sees them. This uses less memory than option 1 and preserves dbStaticLib's dbGetPromptGroup() routine as long as no external code is using the header's index values. We'd add routines to convert both ways between index value and string.
3. Make a menuGuigroups.dbd file and generate the guigroups.h header at build-time. Would need some internal fiddling to use it inside dbStaticLib, but this approach would ensure that typo's don't cause almost-duplicates to appear in the list.

> Does VDCT have its own list of GUI_xxx strings, or will it accept anything in a promptgroup() entry?

https://github.com/epicsdeb/visualdct/blob/master/src/com/cosylab/vdct/dbd/DBDResolver.java#L173

Unfortunately a pre-defined list. It looks like it will ignore unknown groups (not tested).

Personally I never found the grouped listing all that useful. As Ralph says, there isn't much commonality across recordtypes. In VDCT I prefer the simple alphabetical listing.

The usability of the grouped listing is very much linked to having a comprehensive set of groups.
If it reliably helps finding the field faster, it is useful. If not, forget it.

I will test the existing vdct's behavior when prompt groups are changed.
As the usability of the existing mapping is very very limited, I would accept the grouping effectively not working anymore for new Base DBDs (fields disappearing from the grouped listing), as long as the alpha listing works and the tool does not complain or crash.

From Rolf Keitel's response I think his tdct tool would be OK if we changed the set of argument strings as long as the DBD statement's existence means the same thing, which I doubt you were planning to change anyway.

I am leaning towards your option 3 (making it a menu), as this is the only way to add descriptive strings and to have control over the order of the tags. Both features will be required for DCT tools to use promptgroups in a reasonable, consistent way.

Forget about the descriptive strings - that's BS. Sorry.

The complication with option 3 is that dbStaticLib would have to load the menu definition before it can properly parse any record.dbd files. Option 2 would probably be simpler, and tools could control the order of tags by sorting them as long as you start all of the tag names with a number "00 - Scan", "10 - Inputs", "20 - Links", "30 - Alarms", "40 - Convert", "50 - Outputs" etc.

Of course other than the fields defined in dbCommon each record type can use a different set of tag names that are appropriate to that record type, so maybe options 1 or 2 might give a better result than predefining just one set of allowable tags. We'd have to make sure that the DCTs only present those tags for which there are fields defined in this record type, but I some of them may already do that anyway.

Wouldn't it be true for all menus that they have to be read (defined) before they can be used in parsing?

Anyway, I do like the idea that record types can use their own groups in addition to the ones from base. That's actually the way it should be. Especially for things that have a whole separate set of field categories, like the motor record. Good!

The number sorting sounds reasonable.
The groups in base should have a reasonable numerical spread, so that additional records can insert or add.
Tools can throw away the number plus any non-alnum characters for a nicer display if they want to. (That format must be specified and documented.)

@Andrew:
Could you have a look at the linked branch?
I did not update any promptgroup() defs in the records yet - this is just the change of the mechanism in base.
Proving backward compatibility, if you will.

Should I add tests? What kind of? I could read some simple field defs (both old style and new style tags) and make sure the dump output is as expected.

How should we manage deprecation of the guigroup.h header?

HI Ralph,

This will also need a small change in the Perl parser. In src/tools/DBD/Recfield.pm; the %field_attrs hash holds regexp's for validating the attribute strings, and currently it looks for GUI_\w+ for the promptgroup attribute.

Other than that this looks good so far, thanks.

Perl parser fix added, thanks for the hint.

Two remaining issues:

1. Should I add tests?
What kind? I could read some simple field defs (both old style and new style tags) and make sure the dump output is as expected.

2. How should we manage deprecation of the guigroup.h header?
Do a "#pragma message" that gives a hint? Or just remove it?

1. Tests: There are very few tests of attributes in src/tools/test/Recfield.plt (which tests the attribute storage mechanism) and none of the promptgroup attribute as such, but other than the round-trip ability of the parser and output generator there isn't very much that can go wrong there — maybe it should reject group names that contain quote characters though, so instead of /^.*$/ use something like /^[^'"\]*$/. We don't have a dbStatic/test directory and it would probably be easier to test using a full IOC anyway so you could add something to db/test if you feel so inclined, but I would accept this branch without any added tests.

2. "#pragma message" isn't portable to all our targets; I would probably replace the contents of the file with
    #warning included header guigroup.h should no longer be used
and then delete the file in the next major release. Make sure there are Release Notes about the change, and an AppDevGuide update.

Currently the HTML output for a record type generated by src/tools/dbdToHtml.pl has a "DCT" column which just contains "Yes" or "No" depending on whether the promptgroup attribute is set or not; should we change that column to list the group name instead?

- Andrew

1.
Sounds reasonable. I will follow your suggestions.

2.
Interesting. Actually I suggested #pragma message because I read that #warning is not portable (http://stackoverflow.com/questions/171435/portability-of-warning-preprocessor-directive). I will find out what works.

AppDevGuide changes are already done (not pushed yet). Release Notes will follow.

Thanks for your help!

Hmm; we already use #error in several places, but not #warning. Maybe it's simpler to just remove the header file now and document that.

Across GnuCC and MSVC,
    #pragma message ("blabla")
is portable, but does not show up as a warning,
    #warning blabla
is not portable.

Since the empty header file will - if being used - fail the build anyway, I will use
    #error
which is portable.

(I prefer to have the explanations in front of the user instead of failing with an include file not being found, which is more likely to create tech-talk traffic.)

I did some testing and found it not necessary to restrict the character set.

Messed-up solo double-quotes are failing the build early (dbExpand of the DBD file), and backslash-escaped things never get converted to control characters and stay harmless.
Once the promptgroup tag is in the global list and hash table as a zero terminated string, it is not used much anymore, except for searching it through the hash.

What were your concerns?

FYI - here's my starting point for the groups:

10 - Common
20 - Scan
30 - Behaviour
40 - Inputs
45 - Outputs
50 - Links
60 - Conversion
70 - Alarms
80 - Display
90 - Simulation

Behaviour is the group collecting the truly record type specific things, like CALC expressions, SELM masks, etc.
Display gets all client related stuff: GUI hints, units, ranges, strings, deadbands.

I wasn't sure if we handled the attribute values as carefully as other strings ­— great.

The group names are a good start; I wonder if the words should be singular "Input", "Output", "Link" and "Alarm" (so you can talk about "the Input group") and even use the verb form so "Simulate" and "Convert"? How about "Action" instead of "Behaviour"? USians will want it spelled "Behavior" so picking a different word would avoid arguments.

Thanks.