Implement SubControllerVector #192

shihab-dls · 2025-08-15T13:37:33Z

fixes #122

This PR introduces a SubControllerVector, which is a mapping of int to SubController, used in pvi grouping.

codecov · 2025-08-15T13:44:22Z

Codecov Report

❌ Patch coverage is 82.43243% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.58%. Comparing base (9990065) to head (cad6c34).

Files with missing lines	Patch %	Lines
src/fastcs/controller.py	73.80%	11 Missing ⚠️
src/fastcs/attributes.py	0.00%	1 Missing ⚠️
src/fastcs/controller_api.py	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #192      +/-   ##
==========================================
- Coverage   89.03%   88.58%   -0.46%     
==========================================
  Files          46       46              
  Lines        2280     2321      +41     
==========================================
+ Hits         2030     2056      +26     
- Misses        250      265      +15

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

src/fastcs/controller.py

shihab-dls · 2025-08-18T10:47:09Z

Maybe it makes more sense to have self._vector_origin and

@dataclass(frozen=True)
class VectorOrigin:
    name: str
    vector: weakref.ReferenceType[SubControllerVector]

Then, we can dereference the parent vector if we want to do something with knowledge of what controllers are in a group, but also just pass the name to pvi since that is all we need for grouping. I'm unsure of any use cases the vector class has other than pvi grouping though, so I'm not sure is this adds any value.

GDYendell · 2025-08-18T11:31:44Z

@shihab-dls I think the trouble of having to keep track of what vector a sub controller is part of would be avoided if SubControllerVector was a subclass of Controller that gets registered directly with its parent, rather it just being a container and registering its children.

Then with a controller hierarchy like

- Odin
 - FP
  - 1
   - HDF
    - FileName

the path would be ["fp", 1, "hdf"] where integers in the path are treated specially:

For PVs, it does Prefix:FP:1:HDF:FileName
For the pvi structure it does {"fp": "fp1": {"hdf": {...}}} (because groups are not allowed to start with a number)
For GUI groups we could potentially allow names to start with numbers, or just do the same as pvi structure. Probably the latter as otherwise the subscreen for each fp instance would just have "1" as a title...

If this works as I would like, the FP controller vector itself could have attributes and commands. For example here FrameProcessorAdapterController would be a vector with FrameProcessorController elements.

There may be complications with as this, but that is how it looks in my head. Does that make some sense? Happy to pair on this and figure out the details.

shihab-dls · 2025-08-18T14:18:03Z

@shihab-dls I think the trouble of having to keep track of what vector a sub controller is part of would be avoided if SubControllerVector was a subclass of Controller that gets registered directly with its parent, rather it just being a container and registering its children.

Then with a controller hierarchy like
- Odin
 - FP
  - 1
   - HDF
    - FileName 
the path would be ["fp", 1, "hdf"] where integers in the path are treated specially:
* For PVs, it does `Prefix:FP:1:HDF:FileName`

* For the pvi structure it does `{"fp": "fp1": {"hdf": {...}}}` (because groups are not allowed to start with a number)

* For GUI groups we could potentially allow names to start with numbers, or just do the same as pvi structure. Probably the latter as otherwise the subscreen for each fp instance would just have "1" as a title...
If this works as I would like, the FP controller vector itself could have attributes and commands. For example here FrameProcessorAdapterController would be a vector with FrameProcessorController elements.

There may be complications with as this, but that is how it looks in my head. Does that make some sense? Happy to pair on this and figure out the details.

I like this idea (adopting more from ophyds DeviceVector); I had initially started with this line of thinking, but was trying to think of a way to group controllers without relying on their names, given I think we lose information about what controller is or is not a vector by the time we get to structuring the pvi tree. Starting at "A SubControllerVector is a Controller" is probably a more valuable foundation though, so I'll explore some approaches (and probably ask for your input tomorrow :) )

shihab-dls · 2025-11-03T14:33:18Z

@shihab-dls I think the trouble of having to keep track of what vector a sub controller is part of would be avoided if SubControllerVector was a subclass of Controller that gets registered directly with its parent, rather it just being a container and registering its children.
Then with a controller hierarchy like
- Odin
 - FP
  - 1
   - HDF
    - FileName 
the path would be ["fp", 1, "hdf"] where integers in the path are treated specially:
* For PVs, it does `Prefix:FP:1:HDF:FileName`

* For the pvi structure it does `{"fp": "fp1": {"hdf": {...}}}` (because groups are not allowed to start with a number)

* For GUI groups we could potentially allow names to start with numbers, or just do the same as pvi structure. Probably the latter as otherwise the subscreen for each fp instance would just have "1" as a title...
If this works as I would like, the FP controller vector itself could have attributes and commands. For example here FrameProcessorAdapterController would be a vector with FrameProcessorController elements.
There may be complications with as this, but that is how it looks in my head. Does that make some sense? Happy to pair on this and figure out the details.
I like this idea (adopting more from ophyds DeviceVector); I had initially started with this line of thinking, but was trying to think of a way to group controllers without relying on their names, given I think we lose information about what controller is or is not a vector by the time we get to structuring the pvi tree. Starting at "A SubControllerVector is a Controller" is probably a more valuable foundation though, so I'll explore some approaches (and probably ask for your input tomorrow :) )

At this point, pvi trees for CA and P4P are constructed such that a parent controller will have a vector entry as:

"vector": {"d": {prefix}:Vector:PVI}

within which, the vector in p4p will have

"v0": {"d": {prefix}:Vector:0:PVI}",
"v1: {"d": {prefix}:Vector:1:PVI}"

whereas in ca it will have

"vector0": {"d": {prefix}:Vector:0:PVI}",
"vector1: {"d": {prefix}:Vector:1:PVI}"

alongside any vector attributes. I'm unaware if there is a reason we don't want to do vector0:... in p4p as well?

P.S. I've also just tested this in ophyd-async and a ControllerVector will map onto a DeviceVector

shihab-dls · 2025-11-03T15:39:22Z

Unsure about codecov currently, so will request a review to make sure the pvi structure and PV formatting is what we expect

GDYendell

I think the thing that needs to drive this design of this is what we can represent in the PVI tree. I don't think we can have a node in the tree with both named controllers and indexed controllers.

Perhaps add_sub_controller should still only take str and adding indexed controllers to ControllerVector can only be done with __setitem__? We should also disable adding named controllers to ControllerVector and disable Controller from registering controllers with just numbers as the name.

I am wondering if with these restrictions we can revert the controller path being str | int everywhere and instead have the ControllerVector validate the int and then convert it straight to a str. Here and here we are relying on path elements being int to know if it is a vector, but we could just use isdigit instead.

Thoughts?

shihab-dls · 2025-11-05T15:23:34Z

I think the thing that needs to drive this design of this is what we can represent in the PVI tree. I don't think we can have a node in the tree with both named controllers and indexed controllers.

Perhaps add_sub_controller should still only take str and adding indexed controllers to ControllerVector can only be done with __setitem__? We should also disable adding named controllers to ControllerVector and disable Controller from registering controllers with just numbers as the name.

I am wondering if with these restrictions we can revert the controller path being str | int everywhere and instead have the ControllerVector validate the int and then convert it straight to a str. Here and here we are relying on path elements being int to know if it is a vector, but we could just use isdigit instead.

Thoughts?

I think that's reasonable! I've just implemented this. In order to restrict usage of add_sub_controller in ControllerVector and restrict the type of name passed to add_sub_controller in Controller, I've had to make ControllerVector of type BaseController instead of Controller. An example PVA structure output of these changes is:

P4P_TEST_DEVICE:PVI structure 
    alarm_t alarm 
        int severity 0
        int status 0
        string message 
    time_t timeStamp 2025-11-05 15:18:19.649  
        long secondsPastEpoch 1762355899
        int nanoseconds 649368047
    structure display
        string description some controller
    structure value
        structure a
            string rw P4P_TEST_DEVICE:A
        structure b
            string w P4P_TEST_DEVICE:B
        structure table
            string rw P4P_TEST_DEVICE:Table
        structure child_vector
            string d P4P_TEST_DEVICE:ChildVector:PVI

where P4P_TEST_DEVICE:ChildVector:PVI links to:

P4P_TEST_DEVICE:ChildVector:PVI structure 
    alarm_t alarm 
        int severity 0
        int status 0
        string message 
    time_t timeStamp 2025-11-05 15:18:19.649  
        long secondsPastEpoch 1762355899
        int nanoseconds 649484634
    structure display
        string description some child vector
    structure value
        structure vector_attribute
            string r P4P_TEST_DEVICE:ChildVector:VectorAttribute
        structure __1
            string d P4P_TEST_DEVICE:ChildVector:1:PVI
        structure __2
            string d P4P_TEST_DEVICE:ChildVector:2:PVI

where one of the vector children has:

P4P_TEST_DEVICE:ChildVector:1:PVI structure 
    alarm_t alarm 
        int severity 0
        int status 0
        string message 
    time_t timeStamp 2025-11-05 15:18:19.650  
        long secondsPastEpoch 1762355899
        int nanoseconds 649633169
    structure display
        string description some sub controller
    structure value
        structure c
            string w P4P_TEST_DEVICE:ChildVector:1:C
        structure e
            string r P4P_TEST_DEVICE:ChildVector:1:E
        structure f
            string rw P4P_TEST_DEVICE:ChildVector:1:F
        structure g
            string rw P4P_TEST_DEVICE:ChildVector:1:G
        structure h
            string rw P4P_TEST_DEVICE:ChildVector:1:H
        structure j
            string r P4P_TEST_DEVICE:ChildVector:1:J
        structure d
            string x P4P_TEST_DEVICE:ChildVector:1:D
        structure i
            string x P4P_TEST_DEVICE:ChildVector:1:I

Currently, in ophyd-async, this maps to:

p4p_test_device.children() ==
[('a', <ophyd_async.core._signal.SignalRW object at 0x7f5675febc50>), 
('b', <ophyd_async.core._signal.SignalW object at 0x7f5675ebdd10>), 
('table', <ophyd_async.core._signal.SignalRW object at 0x7f5675222d10>), 
('child_vector', <ophyd_async.core._device.Device object at 0x7f5675222f50>)]

So now I'll need to amend ophyd-async to infer a DeviceVector based on the presence of __# children

GDYendell

Some initial comments. I will have to review the p4p bit in an editor to understand what it is doing...

GDYendell · 2025-11-05T16:25:07Z

src/fastcs/controller.py

+        )
+
+    def __getitem__(self, key: int) -> Controller:
+        return self._children[key]


It would be good to catch the KeyError here and re-raise with a better error like "Controller does not have a sub controller ".

GDYendell · 2025-11-05T16:26:09Z

src/fastcs/controller.py

+        for index, child in children.items():
+            super().add_sub_controller(str(index), child)
+
+    def add_sub_controller(self, *args, **kwargs):


I think pyright won't like not having the same function signature here?

GDYendell · 2025-11-05T16:28:03Z

src/fastcs/controller.py

+            yield str(key), child
+
+    def __hash__(self):
+        return hash(id(self))


I am not sure we need to use ControllerVector as a key. This isn't required to fulfill MutableMapping is it?

GDYendell · 2025-11-05T16:29:24Z

src/fastcs/controller.py

        sub_controller.set_path(self.path + [name])
        self.__sub_controller_tree[name] = sub_controller
-        super().__setattr__(name, sub_controller)
+        super().__setattr__(str(name), sub_controller)


There are still a bunch of str() casts that aren't needed now that path is list[str] again

GDYendell · 2025-11-05T16:36:51Z

src/fastcs/controller.py

+    """A collection of SubControllers, with an arbitrary integer index.
+    An instance of this class can be registered with a parent ``Controller`` to include
+    it's children as part of a larger controller. Each child of the vector will keep
+    a string name of the vector.
+    """


I think this can just be

Suggested change

"""A collection of SubControllers, with an arbitrary integer index.

An instance of this class can be registered with a parent ``Controller`` to include

it's children as part of a larger controller. Each child of the vector will keep

a string name of the vector.

"""

"""A controller with a collection of identical sub controllers distinguished by a numeric value"""

GDYendell · 2025-11-05T16:40:15Z

src/fastcs/controller.py

+        self.update(children)
+        super().__init__(description=description, ios=ios)
+        for index, child in children.items():
+            super().add_sub_controller(str(index), child)


Doesn't this hit this check?

GDYendell · 2025-11-05T16:42:52Z

src/fastcs/controller.py

+    def add_sub_controller(self, *args, **kwargs):
+        raise NotImplementedError(
+            "Cannot add named sub controller to ControllerVector. "
+            "Use __setitem__ instead, for indexed sub controllers"


We probably need an explicit example of square brackets indexing as well as saying use __setitem__

GDYendell · 2025-11-05T16:47:21Z

src/fastcs/controller.py

+
+    def children(self) -> Iterator[tuple[str, Controller]]:
+        for key, child in self._children.items():
+            yield str(key), child


It would be be nicer to just return the dict so that the caller can choose to do .items()/.keys()/.values() rather than having to index a tuple.

Actually, do we get in implemented for free by inheriting MutableMapping? In which case maybe we could have children just be Iterator[Controller]?

GDYendell · 2025-11-05T16:47:41Z

src/fastcs/transport/epics/ca/util.py

                arguments["validate"] = _verify_in_datatype
+        case Bool():
+            arguments["ZNAM"] = "False"
+            arguments["ONAM"] = "True"


This seems unrelated?

GDYendell · 2025-11-05T16:48:16Z

src/fastcs/transport/epics/pva/pvi_tree.py

    pv_prefix: str
+    controller_api: ControllerAPI | None
    description: str | None
    device_signal_info: _PviSignalInfo | None


I think these are redundant?

shihab-dls commented Aug 15, 2025

View reviewed changes

src/fastcs/controller.py Outdated Show resolved Hide resolved

GDYendell mentioned this pull request Aug 19, 2025

Handle parameters starting with numbers and convert them to valid names DiamondLightSource/fastcs-odin#71

Open

shihab-dls and others added 3 commits October 28, 2025 15:36

feat: implement initial subcontroller vector

7f0d4b7

WIP Pass bool record *NAM fields

7591e8f

WIP Handle ints in CA pvi

d6bfed5

GDYendell force-pushed the 122_controller_vector branch from f67f90e to d6bfed5 Compare October 28, 2025 15:41

shihab-dls added 8 commits November 3, 2025 13:50

chore: fix typing due to integer in controller path

98fdd76

chore: amend p4p raw value logic to strip suffix

57b411d

chore: update example iocs

d520bbe

tests: amend tests to use controller vetor

016344f

chore: amend controller api path type signature

de036d7

chore: add ios to vector init

c8a6c0a

refactor: remove duplicate pvi tree layer for p4p

480bab8

test: amend p4p tests to use new pvi structure

d882765

shihab-dls added 3 commits November 3, 2025 14:36

Merge branch 'main' into 122_controller_vector

f76fa52

chore: revert type signature for add_sub_controller

3e8e43a

chore: amend controller vector name

a8805b9

shihab-dls force-pushed the 122_controller_vector branch from 1c4e14c to a8805b9 Compare November 3, 2025 15:33

shihab-dls marked this pull request as ready for review November 3, 2025 15:39

shihab-dls requested a review from GDYendell November 3, 2025 15:39

GDYendell reviewed Nov 4, 2025

View reviewed changes

shihab-dls added 3 commits November 5, 2025 10:49

refactor: move vector children up one tree level using controller api

09c9842

refactor: amend pva structure to add intermediate ControllerVector level

c696a97

refactor: revert path to str only and restrict add_sub_controller

b27499b

tests: amend tests to use new vector children naming

ecaf5ec

tests: add tests for restricted add_sub_controller method

cad6c34

shihab-dls requested a review from GDYendell November 5, 2025 15:44

GDYendell reviewed Nov 5, 2025

View reviewed changes

Implement SubControllerVector #192

Are you sure you want to change the base?

Implement SubControllerVector #192

Conversation

shihab-dls commented Aug 15, 2025

Uh oh!

codecov bot commented Aug 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

shihab-dls commented Aug 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GDYendell commented Aug 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shihab-dls commented Aug 18, 2025 • edited by GDYendell Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shihab-dls commented Nov 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shihab-dls commented Nov 3, 2025

Uh oh!

GDYendell left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shihab-dls commented Nov 5, 2025

Uh oh!

GDYendell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Aug 15, 2025 •

edited

Loading

shihab-dls commented Aug 18, 2025 •

edited

Loading

GDYendell commented Aug 18, 2025 •

edited

Loading

shihab-dls commented Aug 18, 2025 •

edited by GDYendell

Loading

shihab-dls commented Nov 3, 2025 •

edited

Loading

GDYendell left a comment •

edited

Loading