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

Tango support #437

Merged
merged 156 commits into from
Oct 8, 2024
Merged

Tango support #437

merged 156 commits into from
Oct 8, 2024

Conversation

burkeds
Copy link
Contributor

@burkeds burkeds commented Jul 8, 2024

Enable support for asynchronous control of Tango servers.

The Tango support backend complies with the Signal object to interface with Tango servers via PyTango device proxies.

The TangoDevice is an optional device class which inherits from Device. It includes attributes and methods to accommodate PyTango device proxies and to manage device polling for servers that do not support events. TangoReadable is an extension of the StandardReadable class to support Tango.

matveyev and others added 30 commits February 26, 2024 09:54
made generic TangoDevice and test for it
…ve additional methods to get last written value
…ve to be awaited from the startup script. This appears to only be working for the OmsVME58Motor object as of writing. Timers and Counters are not connecting properly.
…ables to comply with changes to StandardReadable
…ice. By passing a dictionary of signals upon initializing, it will read the dictionary to add the appropriate signals. position, _stop and _state signals are required.
@burkeds burkeds requested a review from coretl September 18, 2024 11:18
@burkeds
Copy link
Contributor Author

burkeds commented Sep 19, 2024

@callumforrester Are there any other changes you would like to see before merging?

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.

LGTM, thanks!

@coretl
Copy link
Collaborator

coretl commented Sep 19, 2024

@burkeds please could you merge main into this branch once more? pyright doesn't seem to be running on the last CI run for some reason...

@burkeds burkeds requested a review from coretl September 30, 2024 14:31
def __init__(
self,
trl: str | None = None,
device_proxy: DeviceProxy | None = None,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you ever specify a device_proxy when creating a TangoDevice? If so, why would you do this rather than supplying the trl?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

While I don't have a specific use case, it is conceivable that you might have different parts of a tango server in different ophyd devices. Tango servers are not necessarily always written to control a single device. Like ophyd, they can abstract away pretty much anything. For example, you might pass control signals to one ophyd device but scripts or monitoring to another. Or you might have a soft controller device that can send signals to many other devices. This allows you to create one shared resource that can exist across many devices.

Comment on lines 14 to 25
counters: DeviceVector[TangoCounter]
mover: TangoMover

def __init__(self, trl: str, mover_trl: str, counter_trls: list[str], name=""):
super().__init__(trl, name=name)

# If devices are inferred from type hints, they will be created automatically
# during init. If they are created automatically, their trl must be set before
# they are connected.
self.mover.set_trl(mover_trl)
for i, c_trl in enumerate(counter_trls):
self.counters[i + 1] = TangoCounter(c_trl)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think I prefer making these procedurally if there isn't a way to infer sub-devices from a Tango Device:

Suggested change
counters: DeviceVector[TangoCounter]
mover: TangoMover
def __init__(self, trl: str, mover_trl: str, counter_trls: list[str], name=""):
super().__init__(trl, name=name)
# If devices are inferred from type hints, they will be created automatically
# during init. If they are created automatically, their trl must be set before
# they are connected.
self.mover.set_trl(mover_trl)
for i, c_trl in enumerate(counter_trls):
self.counters[i + 1] = TangoCounter(c_trl)
def __init__(self, trl: str, mover_trl: str, counter_trls: list[str], name=""):
super().__init__(trl, name=name)
# These sub-devices are not part of the Tango device, so make them manually
self.mover = TangoMover(mover_trl)
self.counters = DeviceVector(
{i+1: TangoCounter(c_trl) for i, c_trl in enumerate(counter_trls)}
)

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 think I might have misunderstood the requirements. A single tango device does not carry information about associated devices. For now perhaps we can say that tango will only be able to infer signals.

If your tango database has a device server, its possible that we could infer higher level functionality from that, but a tango server is beyond the scope of this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is possible to infer the existence of devices from a tango Database server which IS accessible via a device server API but this is not something I had considered so far. Can implementing this be its own PR in the future?

from ._mover import TangoMover


class TangoDetector(TangoReadable):
Copy link
Collaborator

Choose a reason for hiding this comment

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

If there is no trl passed in __init__, does it make sense for this to be a TangoReadable? It could be a StandardReadable instead and not take trl in init

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 had kept it as a TangoReadable so it could use the device inference methods in TangoDevice. Since we won't be doing that, I will change it to a StandardReadable

Comment on lines 131 to 158
async def tango_signal_auto(
datatype: type[T] | None = None,
*,
trl: str,
device_proxy: DeviceProxy | None,
timeout: float = DEFAULT_TIMEOUT,
name: str = "",
) -> SignalW | SignalX | SignalR | SignalRW | None:
try:
signal_character = await infer_signal_character(trl, device_proxy)
except RuntimeError as e:
if "Commands with different in and out dtypes" in str(e):
return None
else:
raise e

if datatype is None:
datatype = await infer_python_type(trl, device_proxy)

backend = make_backend(datatype, trl, trl, device_proxy)
if signal_character == "RW":
return SignalRW(backend=backend, timeout=timeout, name=name)
if signal_character == "R":
return SignalR(backend=backend, timeout=timeout, name=name)
if signal_character == "W":
return SignalW(backend=backend, timeout=timeout, name=name)
if signal_character == "X":
return SignalX(backend=backend, timeout=timeout, name=name)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd prefer this to be private, so that people didn't end up using tango_signal_auto everywhere instead of tango_signal_r(datatype). What do you reckon?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could do that although it may not have the desired effect. By default, device init will infer the existence, types, and RW character of all attributes/commands on a tango device. By not annotating any signals they can simply point a tango device to a TRL and it will automatically do tango_signal_auto for everything. I suspect this will be the case for many users that don't really care about type safety. If they don't care about type safety, they probably also don't care about encapsulation and will just use the method if it is private anyway.

…tomatically infer their signals but the tango device server does not necessarily carry information about subdevices. Individual device servers should not have sub-devices but tango device databases can. This may be the subject of a future pr. Made tango_signal_auto private to discourage use.
@coretl
Copy link
Collaborator

coretl commented Oct 7, 2024

@burkeds please can you merge with a suitable commit message?

…ls.org/). This relies on PyTango (https://pypi.org/project/pytango/) ophyd-async devices to asynchronous PyTango DeviceProxy objects. The control strategy relies on a shared resource called `proxy` found in the new TangoDevice class. By passing this proxy to the TangoSignalBackend of its signals, a proxy to attributes or commands of the Tango device can be established.

    1. New TangoDevice and TangoReadable device classes.
    2. Automated inference of the existence unannotated signals
    3. Monitoring via Tango events with optional polling of attributes.
    4. Tango sensitive signals are constructed by attaching a TangoSignalBackend to a Signal object or may be built using new tango_signal_* constructor methods.
    5. Signal objects with a Tango backend and Tango devices should behave the same as those with EPICS or other backends.

    1. As of this commit, typed commands are not supported in Ophyd-Async so Tango command signals with a type other than None are automatically built as SignalRW as a workaround.
    2. Tango commands with different input/output types are not supported.
    3. Pipes are not supported.

    1. Extension of Device and StandardReadable to support shared resources such as the DeviceProxy.
    2. Extension of the Tango backend to support typed commands.
    3. Extension of the Tango backend to support pipes.

Contact:
Devin Burke
Research software scientist
Deutsches Elektronen-Synchrotron (DESY)
[email protected]
…ls.org/). This relies on PyTango (https://pypi.org/project/pytango/) ophyd-async devices to asynchronous PyTango DeviceProxy objects. The control strategy relies on a shared resource called `proxy` found in the new TangoDevice class. By passing this proxy to the TangoSignalBackend of its signals, a proxy to attributes or commands of the Tango device can be established.

    1. New TangoDevice and TangoReadable device classes.
    2. Automated inference of the existence unannotated signals
    3. Monitoring via Tango events with optional polling of attributes.
    4. Tango sensitive signals are constructed by attaching a TangoSignalBackend to a Signal object or may be built using new tango_signal_* constructor methods.
    5. Signal objects with a Tango backend and Tango devices should behave the same as those with EPICS or other backends.

    1. As of this commit, typed commands are not supported in Ophyd-Async so Tango command signals with a type other than None are automatically built as SignalRW as a workaround.
    2. Tango commands with different input/output types are not supported.
    3. Pipes are not supported.

    1. Extension of Device and StandardReadable to support shared resources such as the DeviceProxy.
    2. Extension of the Tango backend to support typed commands.
    3. Extension of the Tango backend to support pipes.

Contact:
Devin Burke
Research software scientist
Deutsches Elektronen-Synchrotron (DESY)
[email protected]
@burkeds
Copy link
Contributor Author

burkeds commented Oct 8, 2024

@burkeds please can you merge with a suitable commit message?

@coretl Done. PyTango 10 was released so I updated pyproject to install pytango 10.0.0 or higher. I also had to comment out the unused Device and DeviceVector inferrence methods to improve converage. I left the comments in as a placeholder for this functionality which can hopefully be added in the future.

@coretl coretl merged commit 1559aa0 into bluesky:main Oct 8, 2024
11 checks passed
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.

3 participants