Conversation
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
This PR introduces a new BECProcessedSignal class that computes its value based on signals from other devices in BEC's device manager, along with supporting changes to sim signals to properly trigger subscription callbacks.
Changes:
- Added
ophyd_devices/devices/processed_signal.pywithProcessedSignalModel(Pydantic config model),DeviceSignalMapping, andBECProcessedSignal(a read-only ophyd signal that subscribes to other device signals and computes a derived value via a user-defined function). - Modified
SetableSignal.get()andReadOnlySignal.get()insim_signals.pyto update_readbackand fire_run_subson everyget()call, enabling subscription-based updates needed byBECProcessedSignal. - Removed an unused
BECDeviceBaseimport and a commented-out line fromsim_signals.py.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
ophyd_devices/devices/processed_signal.py |
New module defining BECProcessedSignal — a signal that computes its value from other device signals via a configurable model. Includes validation, subscription management, and an example __main__ block. |
ophyd_devices/sim/sim_signals.py |
Updates SetableSignal.get() and ReadOnlySignal.get() to properly set _readback and trigger _run_subs callbacks; removes unused import and dead code. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ec810a5 to
6192dad
Compare
6192dad to
471286c
Compare
471286c to
b92e6ca
Compare
d-perl
left a comment
There was a problem hiding this comment.
This looks great! I am basically okay with this implementation, I think it should work well, but I had a crazy idea about how to change it a bit which I would like your feedback on:
- is it important that the model can be dynamically changed? if not, I would consider just requiring it on init and getting rid of
set_model, but if it's important, thenset_modelshould refresh the things set up in the connection (see comments in code)- to make that work with the pseudopositioner I think it should be possible if the model
devicescould be adict[str, str] | Callable[[BECProcessedSignal], dict[str, str]]which would be called explicitly with the signalselfbeforeget_device_objects_from_device_managerinwait_for_connection - this could then do
signal.parent.get_positioner_objectsinPSIPseudoMotorBaseto keep the simple reference by device name in the concrete subclasses
- to make that work with the pseudopositioner I think it should be possible if the model
I think the tradeoff is that creating base classes like the PSIPseudoMotorBase becomes a bit more complicated, but we remove some potential sources of runtime errors and making the subclasses like the VirtualSlitCenter would be even a tiny bit simpler
Summary
This PR introduces an implementation for a ProcessedSignal as well as an alternative to write PseudoMotors in BEC.
ProcessedSignals
ProcessedSignals can either be implemented by having a fully defined config passed to the initialization or similar to the implementation here, where string arguments are used to look for the respective signal/devices in the devicemanager of BEC. Device references must be given as dotted names, and the signal will subscribe to the
_default_subof the given object. The compute method will receive the objects and can in return compute/calculate whatever it desires with the given object. However, upon connecting we will test that the return_value matches the type specified in the model for the signal.Virtual Motors
Using the processed signals, a
PSIPseudoMotorbase class is introduced. It relies on the signals and uses them to generate a pseudo motor interface with at least readback/user_readback, setpoint/user_setpoint and motor_is_moving. A center and width implementation is introduced to devices, whereas an example for an alternative integration for a center based on sub-signals is shown here.Note
To test
You may add the following devices to your demo_config.yaml to test the implementation.