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

Declarative EPICS and StandardReadable Devices #598

Merged
merged 12 commits into from
Nov 4, 2024
Merged

Declarative EPICS and StandardReadable Devices #598

merged 12 commits into from
Nov 4, 2024

Conversation

coretl
Copy link
Collaborator

@coretl coretl commented Oct 1, 2024

Add optional Declarative Device support

Includes:

  • An ADR for optional Declarative Devices
  • Support for StandardReadable Declarative Devices via StandardReadableFormat annotations
  • Support for EPICS Declarative Devices via an EpicsDevice baseclass and PvSuffic annotations
  • Updates to the EPICS and Tango demo devices to use them

Changes

pvi structure changes

Structure read from .value now includes DeviceVector support. Requires at least PandABlocks-ioc 0.11.2

Epics signal module moves

ophyd_async.epics.signal moves to ophyd_async.epics.core with a backwards compat module that emits deprecation warning.

# old
from ophyd_async.epics.signal import epics_signal_rw
# new
from ophyd_async.epics.core import epics_signal_rw

StandardReadable wrappers change to StandardReadableFormat

StandardReadable wrappers change to enum members of StandardReadableFormat (normally imported as Format)

# old
from ophyd_async.core import ConfigSignal, HintedSignal
class MyDevice(StandardReadable):
    def __init__(self):
        self.add_readables([sig1], ConfigSignal)
        self.add_readables([sig2], HintedSignal)
        self.add_readables([sig3], HintedSignal.uncached)
# new
from ophyd_async.core import StandardReadableFormat as Format
class MyDevice(StandardReadable):
    def __init__(self):
        self.add_readables([sig1], Format.CONFIG_SIGNAL)
        self.add_readables([sig2], Format.HINTED_SIGNAL)
        self.add_readables([sig3], Format.HINTED_UNCACHED_SIGNAL

Declarative Devices are now available

# old
from ophyd_async.core import ConfigSignal, HintedSignal
from ophyd_async.epics.signal import epics_signal_r, epics_signal_rw

class Sensor(StandardReadable):
    def __init__(self, prefix: str, name="") -> None:
        with self.add_children_as_readables(HintedSignal):
            self.value = epics_signal_r(float, prefix + "Value")
        with self.add_children_as_readables(ConfigSignal):
            self.mode = epics_signal_rw(EnergyMode, prefix + "Mode")
        super().__init__(name=name)
# new
from typing import Annotated as A
from ophyd_async.core import StandardReadableFormat as Format
from ophyd_async.epics.core import EpicsDevice, PvSuffix, epics_signal_r, epics_signal_rw

class Sensor(StandardReadable, EpicsDevice):
    value: A[SignalR[float], PvSuffix("Value"), Format.HINTED_SIGNAL]
    mode: A[SignalRW[EnergyMode], PvSuffix("Mode"), Format.CONFIG_SIGNAL]

@Tom-Willemsen
Copy link
Member

Tom-Willemsen commented Oct 1, 2024

Some thoughts from office conversations at ISIS (@rerpha):

  • We feel the procedural option is nicer / more supportable / less magical (basically in agreement with ADR 6)
  • In most of our cases we probably could write our devices using the declarative approach, but in a few places it would probably get a bit messy and much harder to read than the procedural approach
    • Slightly worried about loss of flexibility / explicitness
  • How would a type-generic device be written using the declarative approach? If I have a Generic[T] device would the T typevar be a valid datatype to pass to the declarative signal datatype? I'm a bit worried about getting tied in typing knots here and it's all very not-obvious I feel, whereas the explicit approach makes it quite obvious...

Would appreciate being involved in the discussion if it looks like we might be going towards declarative route.

@coretl
Copy link
Collaborator Author

coretl commented Oct 1, 2024

One key point that came out of the discussion is that we should always be free to drop to procedural. We should always be able to have an __init__ method that does epics_signal_rw calls. This should be used for any of the DeviceVector cases, and any tricky generics (although I think they could mostly be made to work in the declarative approach). This means we should never be writing a formatted string in the declarative approach, if we need it we should drop to an __init__ method.

The question here is should we support the declarative approach for a subset of the EPICS devices. I think the large number of areaDetector drivers and plugins we are writing are good use case for this, they have no logic, only interface, and are more naturally written in the declarative style. This would allow us to teach the declarative style first, then introduce the procedural style when we need the more complicated examples.

Happy to discuss this in more detail on a zoom call

@Tom-Willemsen
Copy link
Member

@coretl and I discussed on zoom but will add the result of the discussion here:

One key point that came out of the discussion is that we should always be free to drop to procedural.

This is fine from our perspective then - I was worried that the annotation style would be enforced, as long as procedural is still a supported option this is fine for us.

@DominicOram
Copy link
Contributor

I echo @Tom-Willemsen's thoughts. Prefer the more explicit procedural approach but happy if ophyd_async supports both. Could I suggest we add a line in the ADR that explicitly says something like "This is not a step towards only supporting the declarative approach and there are no plans to drop the procedural approach"

@coretl coretl changed the title Device ADR proposal Procedural EPICS and StandardReadable Devices Oct 24, 2024
@coretl coretl changed the base branch from main to signal-typing October 24, 2024 15:58
@coretl coretl changed the title Procedural EPICS and StandardReadable Devices Declarative EPICS and StandardReadable Devices Oct 24, 2024
@coretl coretl force-pushed the adr-device branch 4 times, most recently from 142a2b7 to 2837231 Compare October 29, 2024 15:42
Base automatically changed from signal-typing to main October 30, 2024 10:57
@coretl coretl force-pushed the adr-device branch 2 times, most recently from e16a364 to 5e5e2b0 Compare October 30, 2024 15:26
@coretl coretl marked this pull request as ready for review October 30, 2024 15:32
Copy link
Contributor

@evalott100 evalott100 left a comment

Choose a reason for hiding this comment

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

I'll give a tentative approval, but I think we should have a review from someone from a dodal type layer.

pyproject.toml Outdated Show resolved Hide resolved
Copy link
Contributor

@rtuck99 rtuck99 left a comment

Choose a reason for hiding this comment

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

Deprecation warnings fixed, approved

@coretl coretl merged commit 32afe3d into main Nov 4, 2024
20 checks passed
@coretl coretl deleted the adr-device branch November 4, 2024 11:02
ZohebShaikh pushed a commit that referenced this pull request Nov 14, 2024
Add optional Declarative Device support

Includes:
- An ADR for optional Declarative Devices
- Support for `StandardReadable` Declarative Devices via `StandardReadableFormat` annotations
- Support for EPICS Declarative Devices via an `EpicsDevice` baseclass  and `PvSuffic` annotations
- Updates to the EPICS and Tango demo devices to use them

Structure read from `.value` now includes `DeviceVector` support. Requires at least PandABlocks-ioc 0.11.2

`ophyd_async.epics.signal` moves to `ophyd_async.epics.core` with a backwards compat module that emits deprecation warning.
```python
from ophyd_async.epics.signal import epics_signal_rw
from ophyd_async.epics.core import epics_signal_rw
```

`StandardReadable` wrappers change to enum members of `StandardReadableFormat` (normally imported as `Format`)
```python
from ophyd_async.core import ConfigSignal, HintedSignal
class MyDevice(StandardReadable):
    def __init__(self):
        self.add_readables([sig1], ConfigSignal)
        self.add_readables([sig2], HintedSignal)
        self.add_readables([sig3], HintedSignal.uncached)
from ophyd_async.core import StandardReadableFormat as Format
class MyDevice(StandardReadable):
    def __init__(self):
        self.add_readables([sig1], Format.CONFIG_SIGNAL)
        self.add_readables([sig2], Format.HINTED_SIGNAL)
        self.add_readables([sig3], Format.HINTED_UNCACHED_SIGNAL
```

```python
from ophyd_async.core import ConfigSignal, HintedSignal
from ophyd_async.epics.signal import epics_signal_r, epics_signal_rw

class Sensor(StandardReadable):
    def __init__(self, prefix: str, name="") -> None:
        with self.add_children_as_readables(HintedSignal):
            self.value = epics_signal_r(float, prefix + "Value")
        with self.add_children_as_readables(ConfigSignal):
            self.mode = epics_signal_rw(EnergyMode, prefix + "Mode")
        super().__init__(name=name)
from typing import Annotated as A
from ophyd_async.core import StandardReadableFormat as Format
from ophyd_async.epics.core import EpicsDevice, PvSuffix, epics_signal_r, epics_signal_rw

class Sensor(StandardReadable, EpicsDevice):
    value: A[SignalR[float], PvSuffix("Value"), Format.HINTED_SIGNAL]
    mode: A[SignalRW[EnergyMode], PvSuffix("Mode"), Format.CONFIG_SIGNAL]
```
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

Successfully merging this pull request may close these issues.

5 participants