-
Notifications
You must be signed in to change notification settings - Fork 28
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
Conversation
500a6a3
to
5e4e2a7
Compare
There was a problem hiding this 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/signal.py
Outdated
"""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): |
There was a problem hiding this comment.
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.
5e4e2a7
to
75ff88c
Compare
Decision from the Ophyd meeting:
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 PandaInput = Literal["ZERO", "ONE"]
class SeqBlock(Device):
enable: SignalRW[PandaInput]
def fly(self):
await self.seq.enable.set("ONE") |
Can this PR be closed now? |
I suspect we're going to come across wanting to do this again later, but the branch can be restored when we need it. |
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