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

Make Signal, Backend generic on read/set bounds #249

Closed
wants to merge 1 commit into from

Conversation

DiamondJoseph
Copy link
Contributor

@DiamondJoseph DiamondJoseph commented Apr 19, 2024

Allows Signals to be put with one datatype and return another.
e.g. for Aravis cameras, where a non-exhaustive enum may live in the python runtime, with a subset of fields that are required, while the signal may extend with additional values that may vary on a hardware/reboot level and are unrealistic to track exhaustively.

Required for #190 #239 closes #238 may close #229

@DiamondJoseph DiamondJoseph force-pushed the assymetric-signals branch 6 times, most recently from 500a6a3 to 5e4e2a7 Compare April 22, 2024 09:32
@DiamondJoseph DiamondJoseph marked this pull request as ready for review April 22, 2024 09:41
Copy link
Contributor

@callumforrester callumforrester left a comment

Choose a reason for hiding this comment

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

Technically I think this works, I had a play here: https://mypy-play.net/?mypy=latest&python=3.12&gist=64e5a5ed3d68622e0ed390e7cadefc39
Feel free to add more test cases if you think of any.

However, this is a somewhat significant design change so I think we should ensure that others are comfortable with it, especially @coretl and @abbiemery. This may even warrant an ADR.

src/ophyd_async/core/utils.py Outdated Show resolved Hide resolved
"""Set the value and return a status saying when it's done"""
if timeout is USE_DEFAULT_TIMEOUT:
timeout = self._timeout
coro = self._backend.put(value, wait=wait, timeout=timeout)
return AsyncStatus(coro)


class SignalRW(SignalR[T], SignalW[T], Locatable):
class SignalRW(SignalR[R], SignalW[S], Locatable):
Copy link
Contributor

Choose a reason for hiding this comment

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

It's a shame to have this for all signals when R and S will be the same >90% of the time. I wonder if it would make things simpler to introduce a new type instead of coopting SignalRW? @coretl may have thoughts.

@coretl
Copy link
Collaborator

coretl commented Apr 22, 2024

Decision from the Ophyd meeting:

  • Use an Enum when you require it to be exhaustive, make connect fail if the enum strings do not match exactly
  • When you require a subset or otherwise, then use a str and return choices in the describe dictionary in case you need to check programmatically

I have one syntactic idea to ease the second point, instead of:

class PandaInput(Enum):
    ZERO = "ZERO"
    ONE = "ONE"
    # but there will be other entries that are only known at runtime

class SeqBlock(Device):
    enable: SignalRW[PandaInput]

def fly(self):
    await self.seq.enable.set(PandaInput.ONE)

then we drop to a str type but say we will restrict it for typing purposes:

PandaInput = Literal["ZERO", "ONE"]

class SeqBlock(Device):
    enable: SignalRW[PandaInput]

def fly(self):
    await self.seq.enable.set("ONE")

@callumforrester
Copy link
Contributor

Can this PR be closed now?

@DiamondJoseph
Copy link
Contributor Author

I suspect we're going to come across wanting to do this again later, but the branch can be restored when we need it.

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.

Support extensible enums Ensure data read from enum signals is picklable
3 participants