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

Add create_soft_signal_r and creates_soft_signal_rw methods #217

Merged
merged 7 commits into from
Apr 19, 2024

Conversation

jsouter
Copy link
Contributor

@jsouter jsouter commented Apr 15, 2024

Moved from dodal, have placed in core.signals as seemed logical to prevent circular imports, could be moved to sim_signal_backends.py

@@ -253,6 +253,18 @@ def set_sim_callback(signal: Signal[T], callback: ReadingValueCallback[T]) -> No
return _sim_backends[signal].set_callback(callback)


def create_soft_signal_rw(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Slight preference for:

Suggested change
def create_soft_signal_rw(
def soft_signal_rw(

@@ -119,3 +122,18 @@ async def test_set_and_wait_for_value():
assert not st.done
set_sim_put_proceeds(sim_signal, True)
assert await time_taken_by(st) < 0.1


async def test_create_soft_signal():
Copy link
Contributor

Choose a reason for hiding this comment

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

Could: I would suggest splitting these into separate tests by either:

  • Using pytest.mark.parametrize
  • Have a helper function that is the test and call it with each case

@jsouter jsouter force-pushed the 215_soft_signals branch 2 times, most recently from 8db8da2 to 4551237 Compare April 16, 2024 08:51
@coretl coretl mentioned this pull request Apr 16, 2024
@callumforrester callumforrester linked an issue Apr 17, 2024 that may be closed by this pull request
@jsouter
Copy link
Contributor Author

jsouter commented Apr 17, 2024

@gilesknap @coretl Add in option to pass an initial value to SimSignalBackend in the init method, and also supporting this in the soft_signal_* methods. self._initial_value gets set to the initial_value argument in SimSignalBackend's init, but the converter only converts it when connect is called, though I think this is fine if users are only accessing it through the public methods. soft_signal_r_and_backend still returns the backend, though this may not be required now.

@gilesknap
Copy link
Contributor

@gilesknap @coretl Add in option to pass an initial value to SimSignalBackend in the init method, and also supporting this in the soft_signal_* methods. self._initial_value gets set to the initial_value argument in SimSignalBackend's init, but the converter only converts it when connect is called, though I think this is fine if users are only accessing it through the public methods. soft_signal_r_and_backend still returns the backend, though this may not be required now.

Thanks James, I have rebased and used the new feature successfully.

@jsouter jsouter force-pushed the 215_soft_signals branch 2 times, most recently from bc4e98b to ed969fb Compare April 18, 2024 13:46
@jsouter jsouter requested a review from evalott100 April 18, 2024 13:46
@jsouter
Copy link
Contributor Author

jsouter commented Apr 18, 2024

Following discussion with @coretl, SimSignalBackends no longer take a source/name at init, and all backends now provide source as a method, where the Signal classes pass a name to the backend. source is available from signals as a property.
This shouldn't affect the CA/PVA backend much, but for devices using SimSignalBackend, the source descriptor entry is now based on the device's name, so the source may no longer resemble proper PV names, and will be more like "soft://my-sim-motor" instead of "soft://MY-SIM-MOTOR:X_RBV" e.g.

Copy link
Contributor

@evalott100 evalott100 left a comment

Choose a reason for hiding this comment

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

Looks good!

Comment on lines 262 to 265
signal = SignalRW(
SimSignalBackend(datatype, initial_value)
)
signal.set_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.

Please can we make name=None be passed to these helpers, and if it isn't already there then add it as an optional arg to Signal.__init__

Copy link
Collaborator

Choose a reason for hiding this comment

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

And also add this to epics_signal_* helpers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah! Missed this comment somehow, might have to be a new PR.

test numpy dtype with SimSignalBackend
@jsouter jsouter merged commit 4f58a41 into main Apr 19, 2024
17 of 18 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.

Create helper functions for making soft signals
5 participants