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

Is the BadPVName exception enforcing an EPICS naming rule or just a convention? #1196

Open
gazzar opened this issue Jun 19, 2024 · 9 comments

Comments

@gazzar
Copy link

gazzar commented Jun 19, 2024

I'm getting the BadPVName exception
BadPVName: SR05ID01MIR01:t.Y.RBV
raised when attempting a get() on an EpicsMotor.

Although most comments from colleagues suggest that this seems to be a poorly named PV, is it actually illegal in some way?
The original implementation comment suggests it's more of a guideline than a rule 1e973e1
Our legacy code based on caget, which we still need to use, is quite happy to read it and I can't find any rule in the EPICS docs about dots in PV names being disallowed.

I think the current behaviour should be able to be bypassed if necessary if it is enforcing a convention rather than a rule.

We will try to create an alias of the PV for this case.

@prjemian
Copy link
Contributor

prjemian commented Jun 19, 2024

This code is improperly informed:

if pv.count(".") > 1:
raise BadPVName(pv)

While very unusual, EPICS allows more than one . in an EPICS process variable name.

Thanks for uncovering this little gem. I had not come across it before. The last common instances of double-dot PVs were excised from the APS control systems many years ago, before ophyd was conceived.

I'm searching for the naming rules in the EPICS documentation. Time to call in an expert.

@prjemian
Copy link
Contributor

@anjohnson : Can you provide a reference here to the rules about naming of EPICS process variables? If it makes a difference this issue is discussing Channel Access PV names.

@anjohnson
Copy link

anjohnson commented Jun 19, 2024

CA and PVA servers have their own different limitations and rules for PV names (also called channel names, the terms mean the same thing); the rules for an IOC server are different than for a PCAS or PVA servers. Thus the names you can use depend on the server that publishes the PV, and a generic client application should generally accept and send any string that the user enters (unless it only supports talking to an EPICS motor record say).

A motor record is an object that lives in an IOC, so the characters up to the first . specify the record name, and are subject to the limitations described here. A . character is optional at the end of the PV name, if omitted the channel will connect to the record's VAL field.

When pointing to a specific field of an IOC record the . is required as a separator and followed by the field name; field names have to follow the same rules as a C-99 identifier, see here for more information.

When connecting to an IOC running EPICS 3.15 or later, the field name (or the . which is required in this case) may be followed by a field modifier, see here for the modifiers supported by EPICS 7.0.8.

A JSON field modifier may include . characters, although only when inside a JSON value (string or number). Thus the PV name that @gazzar is trying to connect to above couldn't belong to an EPICS motor record, but it might refer to a legal channel on some other CA or PVA server.

HTH!

@prjemian
Copy link
Contributor

@anjohnson Thanks! That helps me. We're only talking here about CA (not PVA).

From the first link, notice that a "record instance" does not include the . (dot) character. (Note also that { and } are absent from this list.) A "PV name" can include both the record instance name and .+field name. When the latter is not provided, the CA server defaults to .VAL.

@prjemian
Copy link
Contributor

@anjohnson - I notice there is no advice about "field names". (Perhaps that advice is here?) I assume that such names do not have . is a convention? I recall some IOC software that preceded the EPICS areaDetector provided fields with embedded . characters.

@anjohnson
Copy link

Field names are set by the record type. I wrote

field names have to follow the same rules as a C-99 identifier, see here for more information.

The standard rules for C identifiers can be found here, but apparently I should have said C-89 instead of C-99 because even that version allowed unicode characters to be used in identifiers (who knew?). On that page you have to set the "Standard revision" pull-down at the top to C-89 to see our definition as we haven't tried using unicode characters in field names, and even if the Perl code processes them correctly the result will almost certainly not be portable to all our target compilers.

@anjohnson
Copy link

Note that I'm reserving the use of subsequent . characters as field modifiers in case we want to add support for DBF_STRUCT field-types and allow substructure access to the IOC in the future. That would be years away though, don't hold your breath…

@ZLLentz
Copy link
Member

ZLLentz commented Jun 19, 2024

Regardless of whether or not these sorts of PV names are correct or legal in any sort of concrete sense, these errors should be customizable at an application level without monkeypatching.

That is: I should be able to remove this restriction, and I should also be able to make it more stringent to enforce local PV name standards via writing a function and registering it similar to how we've done this for instantiation callbacks.

Right now, you could manually override the validate_pv_name that is imported in ophyd.signal as a local workaround. This probably works:

import ophyd.signal

def my_validate(pv):
    ...

ophyd.signal.validate_pv_name = my_validate

@gazzar
Copy link
Author

gazzar commented Jun 20, 2024

Thanks for picking up the discussion around this @prjemian. @anjohnson is correct that this isn't a motor record but a field of a table record (specifically the y translation drive field), which is implemented as a pseudomotor. It has turned out that I can safely redefine my Ophyd device as an EpicsSignalRO in this case, because that position is only being read from for now in my code. However I did try the monkeypatching approach, which works and will be useful if I decide to redefine the device as an EpicsMotor; Thanks @ZLLentz for a practical workaround.

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

No branches or pull requests

4 participants