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

Change tango decorators to Annotations #636

Closed
coretl opened this issue Nov 4, 2024 · 5 comments · Fixed by #658
Closed

Change tango decorators to Annotations #636

coretl opened this issue Nov 4, 2024 · 5 comments · Fixed by #658
Assignees
Labels
design Discussion about the internal design of the repo looks easy
Milestone

Comments

@coretl
Copy link
Collaborator

coretl commented Nov 4, 2024

After #598 we should change the tango decorators to match.

E.g.

@tango_polling({"counts": (1.0, 0.1, 0.1), "sample_time": (0.1, 0.1, 0.1)})
class TangoCounter(TangoReadable):
    counts: A[SignalR[int], Format.HINTED_SIGNAL]
    sample_time: A[SignalRW[float], Format.CONFIG_SIGNAL]
    start: SignalX
    reset_: SignalX

to

class TangoCounter(TangoReadable):
    counts: A[SignalR[int], Format.HINTED_SIGNAL, TangoPolling(1.0, 0.1)]
    sample_time: A[SignalRW[float], Format.CONFIG_SIGNAL, TangoPolling(1.0, 0.1)]
    start: SignalX
    reset_: SignalX

@burkeds this would allow you to specify:

  • per-attribute polling at class definition time, that you could override at class init
  • if you don't specify an attribute's polling at class definition time, then you would have to enable it via class init

What are the requirements in terms of polled and non-polled Devices? Do you require the same Ophyd Device to be used with both a polled and a non-polled Tango Server? Also, instead of TangoPolling(allow_polling, polling_period, abs_change, rel_change), should we have a TangoPolling.SLOW_POLL and TangoPolling.FAST_POLL where the numbers attached to each of those are specified at init or via globals?

Acceptance Criteria

  • tango_polling decorator is replaced with TangoPolling annotation class
@coretl coretl added this to the 1.0 milestone Nov 4, 2024
@burkeds
Copy link
Collaborator

burkeds commented Nov 4, 2024

Hi @coretl . I added the ability to poll from the ophyd side because Tango servers at DESY have polling disabled by default and there are no plans to support tango servers with polling. As I understand it, this is not typical in the Tango world. A Tango device server with polling enabled may emit events from any number of attributes, so it is possible that some attributes may use Tango events while other attributes will need to be manually polled for the same Tango device server.

It is possible that a device server may be operated in polling or non-polling mode. It is conceivable that one facility may use a non-polling version of a server and another facility a polling version without any other differences in the attributes/commands.

I would say that it is required that, on initialization, the user can specify which signals if any need to be polled and set the polling rate. Polling is really a signal-level problem more than it is a device-level problem as one does not want to by default poll every signal and risk effectively DDOS'ing the device server. It would be nice if this signal level polling configuration could be done without having to modify the class.

On the subject of SLOW_POLL vs FAST_POLL, this is convenient in setting two polling rates but it doesn't allow for setting change thresholds to trigger callbacks. Is this functionality not desired?

@coretl
Copy link
Collaborator Author

coretl commented Nov 4, 2024

On the subject of SLOW_POLL vs FAST_POLL, this is convenient in setting two polling rates but it doesn't allow for setting change thresholds to trigger callbacks. Is this functionality not desired?

Ahh, that's what abs_change, rel_change are... I guess this is a per signal thing anyway.

It is possible that a device server may be operated in polling or non-polling mode. It is conceivable that one facility may use a non-polling version of a server and another facility a polling version without any other differences in the attributes/commands.

Ok, from that I would suggest:

  • At class definition time we annotate with TangoPolling(ophyd_polling_period: float, abs_change: T, rel_change: T | None = None) those signals that need polling at the Ophyd level if they are not polled at the Tango level
  • At TangoDevice.__init__ we specify support_events: bool which tells us whether to use events or ophyd polling when doing Signal.set_callback.
  • We only add signal level overrides and SLOW_POLL/FAST_POLL overrides if we find a use case for them (in the spirit of not adding features until we have a fair change of getting the interface right)

So the example becomes:

class TangoCounter(TangoReadable):
    counts: A[SignalR[int], Format.HINTED_SIGNAL, TangoPolling(1.0, 0.1)]
    sample_time: A[SignalRW[float], Format.CONFIG_SIGNAL, TangoPolling(1.0, 0.1, 0.05)]
    start: SignalX
    reset_: SignalX

counter = TangoCounter(trl, support_events=False)  # no other polling options can be passed here

Would that work for you?

@burkeds
Copy link
Collaborator

burkeds commented Nov 4, 2024

@coretl That sounds ok to me. If I were to speculate how community Ophyd devices might be used for Tango, I would guess that most shared devices will not specify polling as I imagine most groups take advantage of Tango events. For groups that need ophyd to poll, they would define a new inheriting class which overwrites signal behavior and adds polling as desired. Your suggestion works well with this speculation.

@stan-dot stan-dot added the design Discussion about the internal design of the repo label Nov 18, 2024
@stan-dot
Copy link
Contributor

@coretl could this be excluded from the 1.0 milestone?

@coretl
Copy link
Collaborator Author

coretl commented Nov 19, 2024

@coretl could this be excluded from the 1.0 milestone?

No, this is public API, and this isn't a big ticket to do

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design Discussion about the internal design of the repo looks easy
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants