-
Notifications
You must be signed in to change notification settings - Fork 53
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
adds support for configuring commands and channels using YAML #1034
Conversation
There was a problem hiding this 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.
docs/source/dev/commands_channels.md
Outdated
| property | purpose | default | | ||
|-----------|-----------------------------------|---------------------| | ||
| attribute | tango attribute name | MXCuBE channel name | | ||
| poll | polling periodicity, milliseconds | polling is disabled | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| poll | polling periodicity, milliseconds | polling is disabled | | |
| polling_period | polling periodicity, milliseconds | polling is disabled | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed to polling_period
docs/source/dev/commands_channels.md
Outdated
|
||
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. |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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.
docs/source/dev/commands_channels.md
Outdated
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. |
There was a problem hiding this comment.
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.
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. |
There was a problem hiding this comment.
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 😄)
There was a problem hiding this comment.
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.
docs/source/dev/commands_channels.md
Outdated
The general format for specifying `Command` and `Channel` objects is as follows: | ||
|
||
```yaml | ||
class: MegaCommunicator |
There was a problem hiding this comment.
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:
class: MegaCommunicator | |
class: <hardware-object-class> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed
docs/source/dev/commands_channels.md
Outdated
The format for specifying _tango_ `Command` and `Channel` objects is as follows: | ||
|
||
```yaml | ||
class: TangoCommunicator |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
class: TangoCommunicator | |
class: <hardware-object-class> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed
mxcubecore/model/protocols/epics.py
Outdated
""" | ||
an EPICS channel configuration | ||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
""" | |
an EPICS channel configuration | |
""" | |
"""EPICS channel configuration.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reluctantly changed!
mxcubecore/model/protocols/epics.py
Outdated
""" | ||
Models the `epics` section of YAML hardware configuration file. | ||
|
||
Provides an API to read configured EPICS channels. | ||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
""" | |
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. | |
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stupid google!
mxcubecore/model/protocols/tango.py
Outdated
""" | ||
get all tango devices specified in this section | ||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
""" | |
get all tango devices specified in this section | |
""" | |
"""Get all Tango devices specified in this section.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
docs/source/dev/commands_channels.md
Outdated
```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. |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think it's clearer, but take my opinion with a grain of salt since I don't really know much about MxCube 😅
But yes, it does show that you can map PVs and fields to the mxcube channels
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated example
a7bf566
to
617b67a
Compare
138f62e
to
0becc48
Compare
|
||
channels: Optional[Dict[str, Optional[Channel]]] | ||
|
||
def get_channels(self) -> Iterable[Tuple[str, Channel]]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should be Iterator
and not Iterable
.
def get_channels(self) -> Iterable[Tuple[str, Channel]]: | |
def get_channels(self) -> Iterator[Tuple[str, Channel]]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is valid, generators are Iterables:
>>> from typing import Iterable
>>> def foo():
... yield 1
... yield 3
>>> isinstance(foo(), Iterable)
True
Here I want to specify a more generic return type. It is some kind of iterable. Today it's generator, but in the future perhaps it is a list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, I guess it is fine... I do not know for sure.
The rules I had read some time ago were like this:
- When you produce you should use as precise a type you can.
- When you consume you should use as generic a type as you can that still fulfils your usage.
so that is what I always follow and I never had any issue.
So for example, I would do something like this:
def producer() -> list[int]:
return [1, 2, 3]
def consumer(things: typing.Iterable[int]) -> None
for thing in things:
print(thing)
some_things = producer()
consumer(some_things)
If later the producer
function is refactored into a generator (or iterator or any other iterable), I still do not need to change anything in consumer
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A question purely for my education (you guys have studied more than I have). Reading you example one might be tempted to think that producer promises to return a list. Would it be OK to write code like
list2 = list1 + producer()
Or should one instead write
list2 = list1 + list(producer())
in case producer was later refactored to return something else? Or is this principle only valid for certain contexts?
(I know you could write something like
list2 = []; list2.extend(list1); list2.extend(producer())
but the question is whether you need to assume that the return type of producer() might be(come) any iterable at some later date.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you have
def producer() -> list[int]:
return [1, 2, 3]
Then, it explicitly promises to return a list. Thus you can use return value as a list.
However, if you have:
def producer() -> Iterable[int]:
return [1, 2, 3]
Then it does not promise a list. This can any type of iterable value. Then you should explicitly convert this to a list, if you a doing list operations on it.
Here is documentation on Iterable type annotation: https://docs.python.org/3/library/collections.abc.html#collections.abc.Iterable
However, bear in mind that none of this is enforced during runtime. Nothing in python stops you from doing this:
def producer() -> Iterable[int]:
return False
Python will happily execute this and return a boolean to you.
The only way to find you that your code is not following the type annotations is to run a static checker tool, such as mypy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@elmjag Thanks, that makes sense. I know that Python does not respect type annotations, but I want to be a good boy and follow our rules, so I was trying to make sure I understood what our rules were.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest that we try out pyright: https://github.com/microsoft/pyright
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest that we try out pyright: https://github.com/microsoft/pyright
Yes, I agree. I wrote a new issue: #1053
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.
0becc48
to
e7ba036
Compare
Adds support for configuring commands and channels using YAML. Currently it's possible to configure
Tango
,exporter
andEPICS
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:
I would like to get some feedback, before spending more time on this.