Fix DeviceVector introspection

[shihab-dls]

Closes #1197

This PR mainly creates a PviTree data structure, to recursively explore a PVI structure, providing enough information forDevice/DeviceVector type selection when creating devices.

[shihab-dls]

I'm confused about the py:class reference target not found error; I thought this was because I added docstrings but did not expose PviTree and SignalDetails, but now Entry which is just Entry = dict[str, str] is failing docs.Will request an initial review, but @coretl any ideas?

[coretl]

@claude review and answer #1198 (comment)

[claude]

Claude finished @coretl's task in 3m 0s —— View job

---

Code Review Complete

• Read repository guidelines (CLAUDE.md)
• Read the specific comment about the docs error
• Review main changes in _pvi_connector.py
• Investigate Entry type alias and docs issue
• Review security warnings about mixed returns
• Provide inline code review comments
• Answer the documentation error question
• Summarize findings

---

Answer to Documentation Error

The py:class reference target not found: ophyd_async.epics.core._pvi_connector.Entry error occurs because:

1. Entry is used in a public API: The type alias Entry = dict[str, str] at line 25 is referenced in the public method signature SignalDetails.from_entry(entry: Entry)
2. SignalDetails is exported but Entry is not: You correctly exposed SignalDetails in __all__, but Sphinx also needs Entry to resolve the cross-reference in the method signature
3. This codebase exports type aliases: Following existing patterns (see Primitive, SignalDatatype, Callback, EnumTypes in core/__init__.py), type aliases used in public APIs should be exported

Solution: Add Entry to both the import and __all__ list in src/ophyd_async/epics/core/__init__.py:

from ._pvi_connector import PviDeviceConnector, PviTree, SignalDetails, Entry

__all__ = [
    "PviDeviceConnector",
    "PviTree", 
    "SignalDetails",
    "Entry",  # Add this
    ...
]

Code Review Summary

Architecture: The PviTree data structure is well-designed with clear separation of concerns. The recursive tree-building approach using post-order traversal is appropriate for PVI introspection.

Key findings (see inline comments for details):

• Security warnings about mixed returns are false positives - the code is correct
• Hardcoded timeout value (10s) on line 245 differs from parent timeout parameter
• Type annotations could be clearer given nested dict structure
• Logic coupling between regex extraction and vector index casting works but could be more explicit

Positive aspects:

• Excellent docstring with concrete examples for both regular and vector device cases
• Clean refactoring removes the old PVI vector handling code
• Good use of structural pattern matching for signal type detection
• Pydantic models provide good validation

Tests: Test coverage looks comprehensive, including mock mode, recursive typing, and edge cases.

---

[shihab-dls]

@claude re-review, please.

[claude]

Claude finished @shihab-dls's task in 2m 18s —— View job

---

Re-review Complete

• Read repository guidelines (CLAUDE.md)
• Check if Entry export issue was resolved
• Review changes in __init__.py
• Review changes in _pvi_connector.py
• Review test changes
• Provide inline comments for any issues
• Post summary findings

---

Summary

Documentation error: ✅ Fixed - Entry is now properly exported in __init__.py (line 3 import, line 19 in __all__). This resolves the Sphinx cross-reference error.

Code quality: The implementation is solid:

• PviTree data structure is well-designed with excellent docstrings showing both regular and vector device cases
• Post-order traversal approach for recursive tree building is appropriate
• Clean refactoring removes old PVI vector handling code
• Structural pattern matching for signal type detection is elegant
• Test coverage is comprehensive (mock mode, recursive typing, edge cases)

Addressed from previous review:

• Timeout consistency: Now uses parent timeout parameter throughout (line 250)
• Vector child logic: Clear flow from regex extraction (line 267) to root_node assignment (line 269) to int cast (line 99)
• Security warnings: Correctly identified as false positives - all code paths either return or raise

Minor observations (see inline comments):

• Type annotation dict[str, Entry] is technically correct but could be clearer given nested structure
• All inline comments provided reference specific implementation details

The PR successfully implements PVI introspection with proper DeviceVector type selection. Ready for merge pending CI checks.

---

[shihab-dls]

@coretl, this is ready for a re-review now.

[coretl]

Almost there! Take it or leave it on the nit, but the trailing comma should go...

[coretl]

It occurs to me that we could relatively easily still support this structure for backwards compatibility, which would make it easier for people to upgrade...

[shihab-dls]

I've pushed changes to support this, and amended panda.db to have a block that uses this legacy structure. Do you see a point in supporting:

# The older PVI structure has vectors of the form
# structure[] ttlout
#     (none)
#     structure
#         string d PANDABLOCKS_IOC:TTLOUT1:PVI
#     structure
#         string d PANDABLOCKS_IOC:TTLOUT2:PVI
#     structure
#         string d PANDABLOCKS_IOC:TTLOUT3:PVI

as well?

[coretl]

Nope, let's drop the old old style...

[coretl]

Suggested change

	int(entry_name.split("v")[1]): cls.build_device_tree(
	int(entry_name.split("v")[1:]): cls.build_device_tree(

[shihab-dls]

Since we must've passed the regex re.match(r"^v(\d+)$", child_name), split('v') must return 2 sub-strings, one with v and the other with all other digits, hence directly [1] (which also avoids making a list object)

[coretl]

Do we test __1 somewhere else? If not, then can we keep both the legacy and the new style tests?

[shihab-dls]

We do! we test __1 with all the other panda blocks, for example, we now do:

record(stringin, "$(IOC_NAME=PANDAQSRV):SEQ:_PVI")
{
    field(VAL,  "$(IOC_NAME=PANDAQSRV):SEQ:PVI")
    info(Q:group, {
        "$(IOC_NAME=PANDAQSRV):PVI": {
            "value.seq.d": {
                "+channel": "VAL",
                "+type": "plain",
                "+putorder":18
            }
        }
    })
}

record(stringin, "$(IOC_NAME=PANDAQSRV):SEQ:1:_PVI")
{
    field(VAL,  "$(IOC_NAME=PANDAQSRV):SEQ:1:PVI")
    info(Q:group, {
        "$(IOC_NAME=PANDAQSRV):SEQ:PVI": {
            "value.__1.d": {
                "+channel": "VAL",
                "+type": "plain",
                "+putorder":18
            }
        }
    })
}

which makes SEQ:PVI, and attaches SEQ:1:PVI as __1 onto it. So all the tests that connect to this IOC will test the new structure, except for test_panda_with_extra_blocks_and_signals, which will check the legacy structure. I'll add a comment onto EXTRA to make this explicit, but otherwise, is that good enough for testing?

EDIT: test_panda_gets_types_from_common_class, for example. accesses seq[1], which would've been parsed from the new PVI vector structure

[shihab-dls]

Commenting for clarity.

This PR now adds support for introspecting DeviceVectors via the new FastCS __<digits> PVI structure, and the legacy format used by pandablocks-ioc. We are also now handling DeviceVectors of Signals.