PandA Ophyd object fails to process some PV names

[dmgav]

I am testing PandA Ophyd object from the main branch with PandABlocks-ioc installed from dev branch. The PandA object is crashing while connecting to the IOC. The IOC is started as:

$ PandABlocks-ioc softioc xf03idc-panda1.nsls2.bnl.local PANDA ./screens

The code causing the issue:

In [4]: from ophyd_epics_devices.panda import PandA, pvi_get

In [5]: pnd = PandA("PANDA", "pnd")

In [6]: await pnd.connect()
---------------------------------------------------------------------------
AssertionError                            Traceback (most recent call last)
Cell In[6], line 1
----> 1 await pnd.connect()

File ~/repos/ophyd-epics-devices/src/ophyd_epics_devices/panda.py:261, in PandA.connect(self, sim)
    259 if pvi:
    260     for block_name, block_pvi in pvi.items():
--> 261         name, num = block_name_number(block_name)
    263         if name in hints:
    264             block = await self._make_block(name, num, block_pvi["d"])

File ~/repos/ophyd-epics-devices/src/ophyd_epics_devices/panda.py:97, in block_name_number(block_name)
     95 def block_name_number(block_name: str) -> Tuple[str, int]:
     96     m = re.match("^([a-z]+)([0-9]*)$", block_name)
---> 97     assert m, f"Expected '<block_name><block_num>', got '{block_name}'"
     98     name, num = m.groups()
     99     return name, int(num or 1)

AssertionError: Expected '<block_name><block_num>', got 'sfp2_sync_in'

Manually run pvi_get returns the following dictionary:

In [14]: await pvi_get(pnd._init_prefix + ":PVI", pnd.ctxt)
Out[14]: 
{'sfp2_sync_in': {'d': 'PANDA:SFP2_SYNC_IN:PVI'},
 'srgate2': {'d': 'PANDA:SRGATE2:PVI'},
 'srgate1': {'d': 'PANDA:SRGATE1:PVI'},
 'sfp3_sync_out1': {'d': 'PANDA:SFP3_SYNC_OUT1:PVI'},
 'sfp3_sync_out': {'d': 'PANDA:SFP3_SYNC_OUT:PVI'},
 'sfp3_sync_in1': {'d': 'PANDA:SFP3_SYNC_IN1:PVI'},
 'sfp3_sync_in': {'d': 'PANDA:SFP3_SYNC_IN:PVI'},
 'sfp2_sync_out1': {'d': 'PANDA:SFP2_SYNC_OUT1:PVI'},
 'sfp2_sync_out': {'d': 'PANDA:SFP2_SYNC_OUT:PVI'},
 'sfp2_sync_in1': {'d': 'PANDA:SFP2_SYNC_IN1:PVI'},
 'srgate3': {'d': 'PANDA:SRGATE3:PVI'},
 'seq2': {'d': 'PANDA:SEQ2:PVI'},
 'seq1': {'d': 'PANDA:SEQ1:PVI'},
 'pulse4': {'d': 'PANDA:PULSE4:PVI'},
 'pulse3': {'d': 'PANDA:PULSE3:PVI'},
 'pulse2': {'d': 'PANDA:PULSE2:PVI'},
 'pulse1': {'d': 'PANDA:PULSE1:PVI'},
 'pgen2': {'d': 'PANDA:PGEN2:PVI'},
 'pgen1': {'d': 'PANDA:PGEN1:PVI'},
 'pcomp2': {'d': 'PANDA:PCOMP2:PVI'},
 'ttlout1': {'d': 'PANDA:TTLOUT1:PVI'},
 'ttlout9': {'d': 'PANDA:TTLOUT9:PVI'},
 'ttlout8': {'d': 'PANDA:TTLOUT8:PVI'},
 'ttlout7': {'d': 'PANDA:TTLOUT7:PVI'},
 'ttlout6': {'d': 'PANDA:TTLOUT6:PVI'},
 'ttlout5': {'d': 'PANDA:TTLOUT5:PVI'},
 'ttlout4': {'d': 'PANDA:TTLOUT4:PVI'},
 'ttlout3': {'d': 'PANDA:TTLOUT3:PVI'},
 'ttlout2': {'d': 'PANDA:TTLOUT2:PVI'},
 'ttlout10': {'d': 'PANDA:TTLOUT10:PVI'},
 'pcomp1': {'d': 'PANDA:PCOMP1:PVI'},
 'ttlin6': {'d': 'PANDA:TTLIN6:PVI'},
 'ttlin5': {'d': 'PANDA:TTLIN5:PVI'},
 'ttlin4': {'d': 'PANDA:TTLIN4:PVI'},
 'ttlin3': {'d': 'PANDA:TTLIN3:PVI'},
 'ttlin2': {'d': 'PANDA:TTLIN2:PVI'},
 'ttlin1': {'d': 'PANDA:TTLIN1:PVI'},
 'system1': {'d': 'PANDA:SYSTEM1:PVI'},
 'system': {'d': 'PANDA:SYSTEM:PVI'},
 'srgate4': {'d': 'PANDA:SRGATE4:PVI'},
 'counter5': {'d': 'PANDA:COUNTER5:PVI'},
 'inenc2': {'d': 'PANDA:INENC2:PVI'},
 'inenc1': {'d': 'PANDA:INENC1:PVI'},
 'filter2': {'d': 'PANDA:FILTER2:PVI'},
 'filter1': {'d': 'PANDA:FILTER1:PVI'},
 'div2': {'d': 'PANDA:DIV2:PVI'},
 'div1': {'d': 'PANDA:DIV1:PVI'},
 'counter8': {'d': 'PANDA:COUNTER8:PVI'},
 'counter7': {'d': 'PANDA:COUNTER7:PVI'},
 'counter6': {'d': 'PANDA:COUNTER6:PVI'},
 'inenc3': {'d': 'PANDA:INENC3:PVI'},
 'counter4': {'d': 'PANDA:COUNTER4:PVI'},
 'counter3': {'d': 'PANDA:COUNTER3:PVI'},
 'counter2': {'d': 'PANDA:COUNTER2:PVI'},
 'counter1': {'d': 'PANDA:COUNTER1:PVI'},
 'clock2': {'d': 'PANDA:CLOCK2:PVI'},
 'clock1': {'d': 'PANDA:CLOCK1:PVI'},
 'calc2': {'d': 'PANDA:CALC2:PVI'},
 'calc1': {'d': 'PANDA:CALC1:PVI'},
 'bits1': {'d': 'PANDA:BITS1:PVI'},
 'lvdsin1': {'d': 'PANDA:LVDSIN1:PVI'},
 'pcap1': {'d': 'PANDA:PCAP1:PVI'},
 'pcap': {'d': 'PANDA:PCAP:PVI'},
 'outenc4': {'d': 'PANDA:OUTENC4:PVI'},
 'outenc3': {'d': 'PANDA:OUTENC3:PVI'},
 'outenc2': {'d': 'PANDA:OUTENC2:PVI'},
 'outenc1': {'d': 'PANDA:OUTENC1:PVI'},
 'lvdsout2': {'d': 'PANDA:LVDSOUT2:PVI'},
 'lvdsout1': {'d': 'PANDA:LVDSOUT1:PVI'},
 'lvdsin2': {'d': 'PANDA:LVDSIN2:PVI'},
 'bits': {'d': 'PANDA:BITS:PVI'},
 'lut8': {'d': 'PANDA:LUT8:PVI'},
 'lut7': {'d': 'PANDA:LUT7:PVI'},
 'lut6': {'d': 'PANDA:LUT6:PVI'},
 'lut5': {'d': 'PANDA:LUT5:PVI'},
 'lut4': {'d': 'PANDA:LUT4:PVI'},
 'lut3': {'d': 'PANDA:LUT3:PVI'},
 'lut2': {'d': 'PANDA:LUT2:PVI'},
 'lut1': {'d': 'PANDA:LUT1:PVI'},
 'inenc4': {'d': 'PANDA:INENC4:PVI'}}

Some of the names (keys), for example sfp2_sync_in do not end with a number, and can not be split into (block_name, block_num) pair. This is the reason for the assertion: https://github.com/bluesky/ophyd-epics-devices/blob/72b3517e91e84df59ac2abdaac45db95c3f7a32c/src/ophyd_epics_devices/panda.py#L97

This could be an issue with the IOC (generating invalid PV names) or the PandA Ophyd object.

[rosesyrett]

When this was written, Tom had assumed (and told me to assume also) that all blocks discovered by pvi will have a name and number, with the only exception pretty much being the pcap block.

The regex being used for block name and number can account for pcap, because the function used looks like this:

def block_name_number(block_name: str) -> Tuple[str, int]:
    m = re.match("^([a-z]+)([0-9]*)$", block_name)
    assert m, f"Expected '<block_name><block_num>', got '{block_name}'"
    name, num = m.groups()
    return name, int(num or 1)

i,e. the pcap block has no numbers in it, so it gets picked up and automatically assigned a number of 1 (by default) which never actually gets used in the following logic. However, "sfp2_sync_in" contains a number so the regex doesn't show up as a match.

Perhaps I can change this function so that either it:

1. matches the block name to a regex ending in a number; in which case, we continue as-usual and make a DeviceVector element for it (what currently happens for everything other than pcap)
2. matches the block name to a general regex with a mixture of letters and/or numbers, so that it accepts pcap and sfp2_sync_in.

[rosesyrett]

Is this a common use case, for panda blocks to contain mixtures of numbers and letters (with the number not necessarily being at the end)?

[rosesyrett]

Another thing I've noticed; at the moment even if the above discussion weren't a problem and you weren't getting this error, I'd suspect the panda you'd get at the end would not have any of the blocks properly attributed to the panda, unless you subclass the PandA and add typehints to the extra blocks.

What I mean by this, is the following:

At the moment the PandA contains only 3 blocks; the Pcap block, pulse and seq blocks. pulse and seq are defined on the PandA as DeviceVectors meaning they can be accessed by panda.seq[1] corresponding to seq1 for example, in the pvi information.

The panda has the ability to add new signals, and make new blocks based on pvi information. e.g. here, pvi information says we should have 'lut' blocks; the

if pvi:
    for block_name, block_pvi in pvi.items():
    name, num = block_name_number(block_name)

loop will go from 'lut1' to 'lut8' for example. It will correctly make the block in 'self._make_untyped_block' however, in self.set_attribute it will set self.lut to be equal to the block, not self.lut[1], self.lut[2], ... etc.

This is a bug. I am going to fix this as well.

[rosesyrett]

I have now uploaded a PR to address this; please try it out and let me know if the issue(s) persist. Also if you could review it that'd be lovely :)

[dmgav]

Thank you @RAYemelyanova What looks suspicious to me is that Panda box has only SFP2_SYNC_IN, SFP2_SYNC_OUT, SFP3_SYNC_IN and SFP3_SYNC_OUT, but the dictionary above contains both SFP2_SYNC_IN and SFP2_SYNC_IN1, SFP2_SYNC_OUT and SFP2_SYNC_OUT1 etc. The same happens for SYSTEM and SYSTEM1, PCAP and PCAP1, BITS and BITS1. Is this intentional behavior (two PVs are generated for non-numbered block)? Those blocks are not numbered in Panda web interface:

[rosesyrett]

hm... this definitely shouldn't be happening. My only guess is that the PVI information being loaded from the .db file for your panda may need reconfiguring; but I wouldn't be the right person to ask here; @GDYendell have you come across this before? @dmgav could you share with us the db file being used for your panda, or at least the PVI blocks defined in the .db file?

[dmgav]

@RAYemelyanova I have very limited experience with Panda. Where can I find the .db file?

[rosesyrett]

This is something that @GDYendell could probably advise on; my (probably incorrect) guess would be if you could find the machine running the panda IOC then you might be able to locate the startup script and therefore find the .db file that way. But for this I think we need an actual controls engineer (I am only a software developer :monkey: )

[GDYendell]

I am also lacking in PandA knowledge, but have done some digging. Running the IOC up in a debugger, I can see the PandABlocks-ioc calls introspect_panda here, which returns changes including these fields:

dict((k, v) for k, v in changes.values.items() if "LABEL" in k and v)
'*METADATA.LABEL_BITS1': 'Soft inputs and constant bits'
'*METADATA.LABEL_SYSTEM1': 'System control FPGA'
'*METADATA.LABEL_PULSE3': 'One-shot pulse delay and stretch 3'
'*METADATA.LABEL_PULSE1': 'One-shot pulse delay and stretch 1'
'*METADATA.LABEL_PCAP1': 'Position capture control'
'*METADATA.LABEL_TTLOUT2': 'TTL output 2'
'*METADATA.LABEL_TTLOUT1': 'TTL output 1'

These labels cause PVI Devices BITS1/PCAP1 to be created with just the LABEL attached, rather than LABEL being added to the existing BITS/PCAP blocks.The extra sfp{2,3}_sync_{in,out}1 entries with have the same issue although the value is an empty string:

dict((k, v) for k, v in changes.values.items() if "LABEL" in k and "SFP" in k)
'*METADATA.LABEL_SFP3_SYNC_OUT1': ''
'*METADATA.LABEL_SFP2_SYNC_OUT1': ''
'*METADATA.LABEL_SFP2_SYNC_IN1': ''
'*METADATA.LABEL_SFP3_SYNC_IN1': ''

So I guess the response from the PandA should not have the numbers on these LABEL fields, for these blocks. Maybe we can have special cases to strip the 1 off, but that doesn't seem ideal.

In any case, the behaviour is the same with the PandA in the lab at DLS, so I don't think this is something you can fix on your side @dmgav, and I don't think it can / should be fixed in the ophyd layer either.

[dmgav]

Thanks @GDYendell I also was looking at introspect_panda. Since the core issue is difficult to fix, may be we can implement a simple temporary solution in PandA Ophyd object. The idea is to ignore numbered blocks if non-numbered block exists: https://github.com/bluesky/ophyd-epics-devices/pull/39#issuecomment-1675069844 A meaningful warning message could be printed.

[GDYendell]

That seems like a reasonable workaround for now.

Digging a bit deeper, the command *CHANGES? gets a response containing these entries here:

[v for v in ex.multiline if "LABEL" in v and any(n in v for n in ("PCAP", "BITS", "SFP"))]
0: '*METADATA.LABEL_BITS1=Soft inputs and constant bits'
1: '*METADATA.LABEL_SFP3_SYNC_OUT1='
2: '*METADATA.LABEL_SFP2_SYNC_OUT1='
3: '*METADATA.LABEL_SFP2_SYNC_IN1='
4: '*METADATA.LABEL_SFP3_SYNC_IN1='
5: '*METADATA.LABEL_PCAP1=Position capture control'

which I think is directly from the hardware.

[dmgav]

@GDYendell I pushed a commit that removes 'fake' blocks from loaded PVI: https://github.com/bluesky/ophyd-epics-devices/pull/39#issuecomment-1675302748 This fixes the issue in the sense that PandA object can be created. The label is not represented in the block (it is part of PCAP1 block, which is removed), but it is not important:

In [34]: dir(pnd.pcap)
Out[34]: 
[ ...  # Removed irrelevant attributes
  'active',
 'bits0_capture',
 'bits1_capture',
 'bits2_capture',
 'bits3_capture',
 'connect',
 'enable',
 'enable_delay',
 'gate',
 'gate_delay',
 'health',
 'name',
 'parent',
 'samples_capture',
 'set_name',
 'shift_sum',
 'trig',
 'trig_delay',
 'trig_edge',
 'ts_end_capture',
 'ts_start_capture',
 'ts_trig_capture']

Is it possible to arm/disarm PCAP via Epics? It does not seem to be represented in the block attributes.

[dmgav]

I see that there is a PV: PANDA:PCAP:ARM, which arms/disarms PCAP, but it is not represented as an attribute of the Ophyd object.

[dmgav]

Another issue that I noticed is that when I call the read() method, the key in the returned dictionary is either an empty string or incomplete name:

In [20]: await pnd.pcap.active.read()
Out[20]: {'': {'value': 0, 'timestamp': 0.0, 'alarm_severity': 0}}

In [18]: await pnd.lut[1].out.read()
Out[18]: 
{'-1-out': {'value': <GeneratedChoices.1: '1'>,
  'timestamp': 0.0,
  'alarm_severity': 0}}

This could be a bug.

[GDYendell]

Yeah it does look to me like the PcapBlock should have more attributes and they just haven't been done yet.

For the naming, it looks to me that name and parent_name need to be set in various places in the PandA ophyd device The epics_signal_ methods don't allow setting parent or name. signal.set_name could be called on each signal after creation - is that the expected API?

[dmgav]

I have limited understanding of the system, but it looks like all the attributes of PCAP except arm are included in the Ophyd object. The arm attribute is not in the PVI, so it is not included. In principle, for testing purposes arm can be added manually as follows, but I assume this is not the intention for the production version.

In [24]: class ZeroAndOne(Enum):
    ...:     OFF = '0'
    ...:     ON = '1'
    ...: 

In [25]: pnd.pcap.arm = epics_signal_rw(ZeroAndOne, "PANDA:PCAP:ARM")

In [26]: await pnd.pcap.arm.connect()

In [27]: await pnd.pcap.active.read()
Out[27]: {'': {'value': 0, 'timestamp': 0.0, 'alarm_severity': 0}}

In [28]: await pnd.pcap.arm.set(1)

In [29]: await pnd.pcap.active.read()
Out[29]: {'': {'value': 1, 'timestamp': 0.0, 'alarm_severity': 0}}

In [30]: await pnd.pcap.arm.set(0)

In [31]: await pnd.pcap.active.read()
Out[31]: {'': {'value': 0, 'timestamp': 0.0, 'alarm_severity': 0}}

[GDYendell]

Ah I see yes you are right. Arm isn't returned by introspect_panda either and the record is created manually here... I suppose it could also call add_pvi_info here, but I wonder why this special case is needed.

[rosesyrett]

PANDA:PCAP:ARM is not represented in ophyd yet. It should be dynamically added to the pcap object anyways, so you should still be able to access pnd.pcap.arm after connecting it even though the pcap definition doesn't contain it.

The issue you mention above where the name of pnd.lut[1].out.read() seems wrong is probably dependant on how you instantiated your panda. If you used a DeviceCollector, the name should have been set correctly. Probably what happened is you made calls similar to:

pnd = PandA("my-ioc")
await pnd.connect()

To set the names correctly you should either use the device collector:

async with DeviceCollector():
    pnd = PandA("my-ioc")

or, set the names of children directly:

pnd.set_name("pnd")

Let me know if this works for your use case.