Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 27 additions & 2 deletions src/ophyd_async/core/_device_filler.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
from __future__ import annotations

import types
from abc import abstractmethod
from collections.abc import Callable, Iterator, Sequence
from typing import (
Expand All @@ -9,8 +10,10 @@
NoReturn,
Protocol,
TypeVar,
Union,
cast,
get_args,
get_origin,
get_type_hints,
runtime_checkable,
)
Expand Down Expand Up @@ -76,6 +79,7 @@ def __init__(
self._extras: dict[UniqueName, Sequence[Any]] = {}
self._signal_datatype: dict[LogicalName, type | None] = {}
self._vector_device_type: dict[LogicalName, type[Device] | None] = {}
self._optional_devices: set[str] = set()
self.ignored_signals: set[str] = set()
# Backends and Connectors stored ready for the connection phase
self._unfilled_backends: dict[
Expand Down Expand Up @@ -121,6 +125,20 @@ def _scan_for_annotations(self):
self.ignored_signals.add(attr_name)
name = UniqueName(attr_name)
origin = get_origin_class(annotation)
args = get_args(annotation)

if (
get_origin(annotation) is Union
and types.NoneType in args
and len(args) == 2
):
# Annotation is an Union with two arguments, one of which is None
# Make this signal an optional parameter and set origin to T
# so the device is added to unfilled_connectors
self._optional_devices.add(name)
(annotation,) = [x for x in args if x is not types.NoneType]
origin = get_origin_class(annotation)

if (
name == "parent"
or name.startswith("_")
Expand Down Expand Up @@ -241,8 +259,15 @@ def check_filled(self, source: str):
:param source: The source of the data that should have done the filling, for
reporting as an error message
"""
unfilled = sorted(set(self._unfilled_connectors).union(self._unfilled_backends))
if unfilled:
unfilled = set(self._unfilled_connectors).union(self._unfilled_backends)
unfilled_optional = sorted(unfilled.intersection(self._optional_devices))

for name in unfilled_optional:
setattr(self._device, name, None)

unfilled_mandatory = sorted(unfilled.difference(unfilled_optional))

if unfilled_mandatory:
raise RuntimeError(
f"{self._device.name}: cannot provision {unfilled} from {source}"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
f"{self._device.name}: cannot provision {unfilled} from {source}"
f"{self._device.name}: cannot provision {unfilled_mandatory} from {source}"

)
Expand Down
1 change: 1 addition & 0 deletions src/ophyd_async/epics/core/_pvi_connector.py
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,7 @@ def _fill_child(self, name: str, entry: Entry, vector_index: int | None = None):
async def connect_mock(self, device: Device, mock: LazyMock):
self.filler.create_device_vector_entries_to_mock(self.mock_device_vector_len)
# Set the name of the device to name all children
self.filler.check_filled("")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this needed? All optional devices should have been created in mock mode so there should be no need to call this function...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this so that when trying to connect check_filled would be called testing the added code to create the disconnected attr. Otherwise no other part of the mock code calls for filler.check_filled().

Copy link
Collaborator

@coretl coretl Dec 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, please remove this line and add a test that instantiates DeviceFiller manually on a Device with both mandatory and optional Signals, then calls check_filled to check its failure and success paths.

device.set_name(device.name)
return await super().connect_mock(device, mock)

Expand Down
17 changes: 17 additions & 0 deletions tests/unit_tests/epics/pvi/test_pvi.py
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,10 @@ class Block4(StandardReadable):
signal_rw: SignalRW[int]


class Block5(Device):
signal_rw: SignalRW[int] | None


DeviceT = TypeVar("DeviceT", bound=Device)


Expand Down Expand Up @@ -133,6 +137,19 @@ async def test_device_create_children_from_annotations():
assert device.signal_device is top_block_1_device


async def test_device_create_children_from_annotations_filler():
device = with_pvi_connector(Block5, "PREFIX:")

# Makes sure before connecting we create a signal and it exists in the device
assert hasattr(device, "signal_rw")
assert isinstance(device.signal_rw, SignalRW)

await device.connect(mock=True)

# After connecting if the signal is not present it's set to None
assert device.signal_rw is None
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would expect the signal to exist here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, that was my mistake, I'll fix it.



async def test_device_create_children_from_annotations_with_device_vectors():
device = with_pvi_connector(Block4, "PREFIX:", name="test_device")
await device.connect(mock=True)
Expand Down
Loading