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 panda-specific save load plans #63

Merged
merged 5 commits into from
Dec 19, 2023
Merged

Add panda-specific save load plans #63

merged 5 commits into from
Dec 19, 2023

Conversation

olliesilvester
Copy link
Contributor

@olliesilvester olliesilvester commented Nov 7, 2023

Adds plans to save and load a panda device, including loading the phases in the correct order. This uses the functions added in #36

This also fixes an issue with hanging subprocceses in unit tests, and fixes a bug where devices wouldn't load if one of the saved PV's were ignored (set to None in the yaml file)

Comment on lines 15 to 24
signalRW_and_value = yield from get_signal_values(walk_rw_signals(panda), ignore)
phase_1 = {}
phase_2 = {}
for signal_name in signalRW_and_value.keys():
if signal_name[-5:] == "units":
phase_1[signal_name] = signalRW_and_value[signal_name]
else:
phase_2[signal_name] = signalRW_and_value[signal_name]

return [phase_1, phase_2]
Copy link
Collaborator

Choose a reason for hiding this comment

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

I already wrote this yesterday for i22, so I'll paste in what I had in case you want to use any of it:

def save_the_panda():
    signals = walk_rw_signals(panda)
    values = yield from get_signal_values(signals, ignore=["seq1.table"])
    units = {n: values.pop(n) for n in list(values) if n.endswith("units")}
    save_to_yaml([units, values], "/tmp/thing.yaml")

Comment on lines 35 to 41
def load_panda(panda: PandA, path: str):
"""
Sets all PV's to a PandA using a previously saved configuration
"""
values = load_from_yaml(path)
signals_to_set = walk_rw_signals(panda)
yield from set_signal_values(signals_to_set, values)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thinking about this, the load is completely generic, it is not specifically for PandA, so I think we should remove it from here...

Copy link
Collaborator

@rosesyrett rosesyrett left a comment

Choose a reason for hiding this comment

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

I'm not sure I like the fact we have plans in ophyd-async. I think this needs a broader discussion...

In general I think this looks good if we are heading in that direction, but personally I'd prefer if we could move this into dls-bluesky-core and keep ophyd-async bluesky agnostic where possible...

src/ophyd_async/core/device_save_loader.py Show resolved Hide resolved
src/ophyd_async/core/device_save_loader.py Outdated Show resolved Hide resolved
src/ophyd_async/panda/panda_utils.py Outdated Show resolved Hide resolved
@rosesyrett rosesyrett added the design Discussion about the internal design of the repo label Nov 14, 2023
@rosesyrett
Copy link
Collaborator

Gonna improve the test coverage and fix the conflict with main

@rosesyrett
Copy link
Collaborator

rosesyrett commented Dec 14, 2023

@coretl and @olliesilvester I've just been adding a test case for this:

def set_signal_values(
    signals: Dict[str, SignalRW[Any]], values: Sequence[Dict[str, Any]]
) -> Generator[Msg, None, None]:
    """Maps signals from a yaml file into device signals.

    ``values`` contains signal values in phases, which are loaded in sequentially
    into the provided signals, to ensure signals are set in the correct order.

    Parameters
    ----------
    signals : Dict[str, SignalRW[Any]]
        Dictionary of named signals to be updated if value found in values argument.
        Can be the output of :func:`walk_rw_signals()` for a device.

    values : Sequence[Dict[str, Any]]
        List of dictionaries of signal name and value pairs, if a signal matches
        the name of one in the signals argument, sets the signal to that value.
        The groups of signals are loaded in their list order.
        Can be the output of :func:`load_from_yaml()` for a yaml file.

    See Also
    --------
    :func:`ophyd_async.core.load_from_yaml`
    :func:`ophyd_async.core.walk_rw_signals`
    """
    # For each phase, set all the signals,
    # load them to the correct value and wait for the load to complete
    for phase_number, phase in enumerate(values):
        # Key is signal name
        for key, value in phase.items():
            # Skip ignored values
            if value is None:
                continue

            if key in signals:
                yield from abs_set(
                    signals[key], value, group=f"load-phase{phase_number}"
                )

        yield from wait(f"load-phase{phase_number}")

Specifically, for the 'continue' line. I've just noticed; isn't the purpose of this, so that yaml files with 'null' entries are ignored? If so, why not just remove that line from the yaml file, especially considering the confusion this can cause with enums that have 'None' values? Edit: on the other side of this, we can ensure that when we are saving to yaml, we don't save any pv's that have 'None' values outside of enums.

@coretl
Copy link
Collaborator

coretl commented Dec 14, 2023

Specifically, for the 'continue' line. I've just noticed; isn't the purpose of this, so that yaml files with 'null' entries are ignored? If so, why not just remove that line from the yaml file, especially considering the confusion this can cause with enums that have 'None' values? Edit: on the other side of this, we can ensure that when we are saving to yaml, we don't save any pv's that have 'None' values outside of enums.

If the value is None, this means the signal was present on the Device, but ignored. This means you can tell the different between a new signal being added to a Device (which should be saved if you re-save), and an ignored Signal

The continue looks correct to me, if you see a null in the file (which will be translated to a None in the python dict), then don't set that value

olliesilvester and others added 5 commits December 15, 2023 11:24
include None type in load tests
* Add generic save_device function

Change panda_utils -> utils.py only, as it already resides
in a panda submodule, and have this define the sorting
function needed to save a generic device.

* Update src/ophyd_async/panda/utils.py

Co-authored-by: Tom C (DLS) <[email protected]>

* Update src/ophyd_async/core/device_save_loader.py

Co-authored-by: Tom C (DLS) <[email protected]>

* lint

* Modify docstrings and make docs build

---------

Co-authored-by: Tom C (DLS) <[email protected]>
@rosesyrett
Copy link
Collaborator

Just rebased

@rosesyrett rosesyrett merged commit 2ae845f into main Dec 19, 2023
14 checks passed
@rosesyrett rosesyrett deleted the add_panda_save_load branch December 19, 2023 09:58
olliesilvester added a commit that referenced this pull request Jan 19, 2024
* Add panda-specific save load plans

* Make load generic, fix loading ignored PVs

* Wait for subprocesses to terminate in conftest

include None type in load tests

* Add generic save_device function (#95)

* Add generic save_device function

Change panda_utils -> utils.py only, as it already resides
in a panda submodule, and have this define the sorting
function needed to save a generic device.

* Update src/ophyd_async/panda/utils.py

Co-authored-by: Tom C (DLS) <[email protected]>

* Update src/ophyd_async/core/device_save_loader.py

Co-authored-by: Tom C (DLS) <[email protected]>

* lint

* Modify docstrings and make docs build

---------

Co-authored-by: Tom C (DLS) <[email protected]>

* Attempt to improve test coverage

---------

Co-authored-by: Rose Syrett <[email protected]>
Co-authored-by: Tom C (DLS) <[email protected]>
Co-authored-by: Rose Syrett <[email protected]>
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants