epics-containers / pvi

EPICS PV Interface described in YAML
https://epics-containers.github.io/pvi
Apache License 2.0
4 stars 2 forks source link

overly restrictive Schema? #61

Closed gilesknap closed 8 months ago

gilesknap commented 10 months ago

This snippet of device yaml gets a schema error.

The reason is for the . The schema only accepts a-z A-Z 0-9. Is there any good reason to omit - and `` from the alowed signal and group names?

label: ADGenICam
parent: ADDriver
children:

- type: Group
  name: AVT_Mako_1_52
  layout:
    type: Grid
  children:

  - type: SignalW
    name: GC_GevTimConLatch
    pv: GC_GevTimConLatch
    widget:
      type: TextWrite

  - type: SignalW
    name: GC_GevTimConReset
    pv: GC_GevTimConReset
    widget:
      type: TextWrite
gilesknap commented 10 months ago

I see the error when trying to do a regroup on this.

gilesknap commented 10 months ago

@GDYendell can we change the schema to allow these names and at the same time look into why it disallows initial lowercase?

GDYendell commented 10 months ago

The issue is we need to be able to reliably convert from name/label to Title Case to display on the UI, and doing that for any arbitrary string is tricky. This is why it is currently restricted to PascalCase.

I have just tried to use convert-case so we don't have to implement all the conversions ourselves, but this doesn't like GC_GevTimConLatch because it is not one of the valid formats.

There is also case-switcher and inflection which are less restrictive and formats it as GC Gev Tim Con Latch. This is pretty horrible and I am not sure what we want.

Maybe we use convert-case and if it fails we just leave it unchanged?

gilesknap commented 10 months ago

I have converted a portion of the Features by removing the _ in the device file and running regroup. The results would probably be better if the long names were allowed more space. It is true that the mission to make these auto generated screens compact is the source of a few issues.

Would switching to two lines for every signal help ? (label line, value(s) line)

But regarding convert-case we could provide a single regex processing stage before it that tries to make things legal (removing - or _ and perhaps ucasing the next character comes to mind)

image

gilesknap commented 10 months ago

Or just make the columns wider and only present demand/readback on two lines like this

if the label for demand/readback can be pushed down half a line it would look better

label:    DEMAND----------
          READBACK--------
label:    READBACK--------
label:    READBACK--------
label:    DEMAND----------
          READBACK--------
gilesknap commented 10 months ago

It occurs to me that what we really want is a custom regex per device (or group?). For example simply removing the GC_ from all labels on this screen would really help as GC is redundant anyway.

In #60 I suggest an autogenerator customization file - this would be a good place for such a regex.

GDYendell commented 10 months ago

I discussed with @coretl and he pointed out that if we move to pulling the config directly from the camera, the output is like this. The templates we are generating from have already had the names squashed to fit into PVs, but if we do this ourselves we can use the full names for the labels.

GDYendell commented 9 months ago

73 should help with this. The convert should now enforce PascalCase names and make linting/error reporting by the yaml extension now work. However, I think we do need to be restrictive on this or we will end up with badly formatted labels on the generated UIs.