BCDA-APS / apstools

various tools for use with Bluesky at the APS
https://bcda-aps.github.io/apstools/latest/
Other
16 stars 9 forks source link

PVPositionerSoftDone uses invalid subscription event type for simulated devices #1022

Open canismarko opened 1 month ago

canismarko commented 1 month ago

In apstools v 1.7.0, a new subscription was added that uses the event_type="setpoint". In a real device, this works fine, but for simulated devices this fails because "setpoint" is not a valid event type.

In [1]: from apstools.devices import PVPositionerSoftDone

In [2]: from ophyd.sim import instantiate_fake_device

In [3]: instantiate_fake_device(PVPositionerSoftDone, readback_pv="spam:", setpoint_pv="eggs:")
---------------------------------------------------------------------------
UnknownSubscription                       Traceback (most recent call last)
Cell In[3], line 1
----> 1 instantiate_fake_device(PVPositionerSoftDone, readback_pv="sldkjf", setpoint_pv="lldkjf")

File /home/beams1/B268176/miniforge3/envs/haven-test2/lib/python3.10/site-packages/ophyd/sim.py:1307, in instantiate_fake_device(dev_cls, name, prefix, **specified_kw)
   1305 kwargs["name"] = name if name is not None else dev_cls.__name__
   1306 kwargs["prefix"] = prefix
-> 1307 return dev_cls(**kwargs)

File /home/beams1/B268176/miniforge3/envs/haven-test2/lib/python3.10/site-packages/apstools/devices/positioner_soft_done.py:141, in PVPositionerSoftDone.__init__(self, prefix, readback_pv, setpoint_pv, tolerance, use_target, **kwargs)
    139 self.readback.subscribe(self.cb_readback)
    140 self.setpoint.subscribe(self.cb_setpoint)
--> 141 self.setpoint.subscribe(self.cb_update_target, event_type="setpoint")
    143 # cancel subscriptions before object is garbage collected
    144 weakref.finalize(self.readback, self.readback.unsubscribe_all)

File /home/beams1/B268176/miniforge3/envs/haven-test2/lib/python3.10/site-packages/ophyd/ophydobj.py:481, in OphydObject.subscribe(self, callback, event_type, run)
    479 # check that this is a valid event type
    480 if event_type not in self.subscriptions:
--> 481     raise UnknownSubscription(
    482         "Unknown subscription {!r}, must be one of {!r}".format(
    483             event_type, self.subscriptions
    484         )
    485     )
    487 # wrapper for callback to snarf exceptions
    488 def wrap_cb(cb):

UnknownSubscription: "Unknown subscription 'setpoint', must be one of frozenset({'meta', 'value'})"

Not sure what the right solution is, in part because I don't really understand the different subscription event types in ophyd. Perhaps a try…except… block?

prjemian commented 2 weeks ago

This event_type kwarg is new to me. The docstring that describes it does not help me much.

@gfabbris -- Why is event_type="setpoint" needed? What does this do?

prjemian commented 2 weeks ago

More info in docstring of ophyd Device.

prjemian commented 2 weeks ago

The line of code in question: https://github.com/BCDA-APS/apstools/blame/11a97074a737cf63fb3a8462ecc209cd7d646e24/apstools/devices/positioner_soft_done.py#L141

is responsible for ensuring that the soft target of the positioner is updated when the setpoint changes. Why not drop the additional kwarg?

prjemian commented 2 weeks ago

See: https://github.com/bluesky/ophyd/blob/5df3f5694f5c4c0ac049f0e601512f220e85744b/ophyd/signal.py#L1690

Will add check if "setpoint" is one of the device's .event_types before subscribing. This solves the problem.

gfabbris commented 2 weeks ago

I'm on travel, so will take a look at this later this later this week. But the event_type = "setpoint" is needed so that the target is changed when you change the setpoint in Bluesky, but will not change during a ramp when the setpoint is changed in EPICS. If I remember correctly, if you use no event_type it will default to the value which is triggered when the readback changes (so will change when EPICS changes the value).

prjemian commented 2 weeks ago

Good advice. Will keep the subscription but checking if "setpoint" is a supported event type. If it is, then use that kwarg.

prjemian commented 2 weeks ago

BTW: This is tested by test_put_and_stop() which tests interrupted moves.

prjemian commented 2 weeks ago

All tests pass. Ready for review.