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

adds support for configuring commands and channels using YAML #1034

Open
wants to merge 3 commits into
base: xml_yaml_conversion
Choose a base branch
from

Conversation

elmjag
Copy link
Contributor

@elmjag elmjag commented Oct 2, 2024

Adds support for configuring commands and channels using YAML. Currently it's possible to configure Tango, exporter and EPICS commands and channels.

This implements roughly the format discussed in: #1023
The actual format implement is documented here: https://mxcubecore--1034.org.readthedocs.build/en/1034/dev/commands_channels.html

This is still work in progress. Still on the todo list:

  • write some tests
  • do a bit more "manual" testing

I would like to get some feedback, before spending more time on this.

@elmjag elmjag added the YAML Work on supporting YAML configuration files. label Oct 2, 2024
Copy link
Contributor

@fabcor-maxiv fabcor-maxiv left a comment

Choose a reason for hiding this comment

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

Good stuff.

A first review pass, a first batch of comments and improvement suggestions.

| property | purpose | default |
|-----------|-----------------------------------|---------------------|
| attribute | tango attribute name | MXCuBE channel name |
| poll | polling periodicity, milliseconds | polling is disabled |
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
| poll | polling periodicity, milliseconds | polling is disabled |
| polling_period | polling periodicity, milliseconds | polling is disabled |

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed to polling_period


The mxcubecore provides a hardware object-level abstraction
for communicating using various flavors of control system protocols.
A hardware object can utilize instances of `Command` and `Channel` objects.
Copy link
Contributor

Choose a reason for hiding this comment

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

Link to the documentation for the actual classes, please.
(I always forget how to do it, look up the Myst doc: https://myst-parser.readthedocs.io/en/latest/syntax/cross-referencing.html).

Also isn't it mxcubecore.CommandContainer.ChannelObject and mxcubecore.CommandContainer.CommandObject?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it's mxcubecore.CommandContainer.ChannelObject and mxcubecore.CommandContainer.ChannelObject. Added links to API docs for these objects.

for communicating using various flavors of control system protocols.
A hardware object can utilize instances of `Command` and `Channel` objects.
These objects provide a uniform API for accessing a specific control system.
Mxcubecore provides support for using protocols such as _tango_, _EPICS_, _exporter_ and more.
Copy link
Contributor

Choose a reason for hiding this comment

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

Links, links, links! Documentation should have lots of links. Typically the first occurrence of every term should have a link to its definition.

Suggested change
Mxcubecore provides support for using protocols such as _tango_, _EPICS_, _exporter_ and more.
Mxcubecore provides support for using protocols such as [_tango_](https://pytango.readthedocs.io/), _EPICS_, _exporter_ and more.

Choose a reason for hiding this comment

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

https://docs.epics-controls.org/en/latest/ for the official EPICS docs (that is being reworked now as far as I know, but at least it's not the APS website from the 80's anymore 😄)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added links to Tango and EPICS web sites.

The general format for specifying `Command` and `Channel` objects is as follows:

```yaml
class: MegaCommunicator
Copy link
Contributor

Choose a reason for hiding this comment

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

To keep consistent with the rest of the example:

Suggested change
class: MegaCommunicator
class: <hardware-object-class>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed

The format for specifying _tango_ `Command` and `Channel` objects is as follows:

```yaml
class: TangoCommunicator
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
class: TangoCommunicator
class: <hardware-object-class>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed

Comment on lines 11 to 13
"""
an EPICS channel configuration
"""
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"""
an EPICS channel configuration
"""
"""EPICS channel configuration."""

Copy link
Contributor Author

Choose a reason for hiding this comment

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

reluctantly changed!

Comment on lines 1 to 4
"""
Models the `epics` section of YAML hardware configuration file.
Provides an API to read configured EPICS channels.
"""
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"""
Models the `epics` section of YAML hardware configuration file.
Provides an API to read configured EPICS channels.
"""
"""Models for the `epics` section of YAML hardware configuration file.
Provides an API to read configured EPICS channels.
"""

Copy link
Contributor Author

Choose a reason for hiding this comment

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

stupid google!

Comment on lines 81 to 83
"""
get all tango devices specified in this section
"""
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"""
get all tango devices specified in this section
"""
"""Get all Tango devices specified in this section."""

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

"""
add the Command and Channel objects configured in the specified protocol section

parameters:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the sections are case-sensitive.

Suggested change
parameters:
Parameters:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

works fine with lower case p:

image

mxcubecore/protocols_config.py Show resolved Hide resolved
Comment on lines 256 to 274
```yaml
class: EpicsCommunicator
epics:
"FOO:B:x1.":
channels:
State:
suffix: stat
Vol:
```

We have an `FOO:B:x1.` prefix specified, with two `Channel` objects `State` and `Vol`.
`State` will use `FOO:B:x1.stat` PV name, specified by section's prefix and the `suffix` configuration property.
`Vol` will use `FOO:B:x1.Vol` PV name, specified by section's prefix and channels MXCuBE name.
Copy link

@henrique-silva henrique-silva Oct 4, 2024

Choose a reason for hiding this comment

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

So, I think this might be a bit problematic, because the modeling of PV is usually $(PREFIX):$(DEVICE).$(PROPERTY), so one record - or PV - ( $(PREFIX):$(DEVICE)) will have multiple properties, for example an analog out has, .DESC (description), .VAL (value), .ASLO (slope).
But in this case that you're modelling it is way more likely that this will be separated into multiple PVs, so you'd likely have:

class: EpicsCommunicator
epics:
  "FOO:B:":
    channels:
      State:
        suffix: pv_1.STAT
      Vol:
        suffix: volume.VAL

In the end I think the code will handle just fine if you're just appending, but the prefix is only the part before teh last colon (in most naming conventions of EPICS that I've seen)

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure I am understanding you correctly, but would #1023 (comment) answer your question?

Choose a reason for hiding this comment

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

Yes, that's pretty much it.

It's just the non-obvious separation between PV/record and properties in EPICS that is a bit weird for modelling, but the simple concatenation should handle all cases

Copy link
Contributor

Choose a reason for hiding this comment

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

@elmjag I think it is worth modifying the epics example to show a more complex and complete example, that shows how to combine all those things.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will this example make things more clear?

class: EpicsCommunicator
epics:
  "FOO:B:":
    channels:
      State:
        suffix: pv_1.STAT
      Vol:
        suffix: volume.VAL
      Freq:

Regarding Freq channel, I want to include an example channel, without the optional config property suffix.

This way, it's possible to link to generated API documentation for
CommandContainer and ChannelObject classes.

Without this change, sphix refuses to create a link for
CommandContainer and ChannelObject classes, from other section of
the documentation.
Adds a section the describes how to configure hardware object's
commands and channels using YAML configuration files.

Documents the format for Tango, exporter and EPICS protocols.
Adds support for 'tango', 'exporter' and 'epics' sections to YAML
configuration files.

These section are used to configure Command and Channel objects for
hardware objects.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
YAML Work on supporting YAML configuration files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants