Resolves #64 Feature suggestion: Connecting instruments #65
Conversation
pyvisa_sim/component.py
Outdated
| :param source: resource to connect to | ||
| :param source_parameter: resource output parameter |
There was a problem hiding this comment.
The docstring does not match the function signature
There was a problem hiding this comment.
Also it may be worth to validate that the connection is well formed. But we may need to delay this till after building the whole device to validate the parameters and sources.
pyvisa_sim/default.yaml
Outdated
| connections: | ||
| power: | ||
| q: "READ1" | ||
| source_list: [ | ||
| {source_name: "GPIB0::2::65535::INSTR", source_parameter: amplitude}, | ||
| {source_name: "self", source_parameter: frequency} | ||
| ] | ||
| function: "0.23 * (%amplitude% ** 2) - %frequency%" |
There was a problem hiding this comment.
I feel like connections could live with dialogues. There are basically the same but connections have some extra that's all. The r field could be used instead of function.
Furthermore if we go that route we could have connections in get/set responses which could be valuable (think of query whose format depend on some internal setting).
Also if we were to allow some device inheritance, it would be nice be able to override a dialogue response to make it a connection without touching to the query.
There was a problem hiding this comment.
How would a device in the yaml file look doing it this way?
There was a problem hiding this comment.
We could have something like
device2:
base: [device1]There was a problem hiding this comment.
I feel like connections could live with dialogues. There are basically the same but connections have some extra that's all. The r field could be used instead of function.
Trying it out as a dialogue right now but I'm concerned about the _get_pair()- and _get_triplet() functions. Is there a reason for it not being a _get_dialogue() that can handle both pairs and triplets (+connections)?
There was a problem hiding this comment.
_get_triplet is only used for setters of properties. So it won't be a problem right now, but we may want to iterate over things after this PR. In this PR we could stops at getting the capability in dialog and then extend it to properties in a later PR.
There was a problem hiding this comment.
Dialogues now use _get_dialogue() instead which can handle connections as well. Please let me know if you want further changes regarding this.
|
Any idea why my connection test fails? It works fine locally. |
|
Tests fail on master too sadly (your pyvisa version may be outdated since the GPIB secondary address is not part of the normalized resource name anymore if the secondary address is not set). I am preparing a modernized version of the code base. I may ask you to rebase your work on it so that we get tests to pass. |
|
I merged my clean up PR so you will need to update your changes. Since this is my fault for not updating ths project for so long let me know if you do not have the bandwidth to do the changes and I can do them. |
|
I will have a look at this tomorrow. Thank you for the fast update! |
|
It is not often I get somebody interested to put work in so I try to keep the momentum |
|
I'm having problems installing the latest version. |
|
Do you have internet access ? pip errors if it cannot look up build dependencies (even if it has local caches, which is extremely annoying but it did not yet dive to figure out the option to avoid that issue). |
|
Could it be that I'm using Python 3.6? The newest version of Setuptools I get is 59.6.0. This is the version we use in our project so I'd rather not update to 3.8. |
|
3.8 fixed my issue fyi. |
|
I am sorry but I do not have the bandwidth to keep supporting 3.6 which is EOL. |
|
There are still conflict and the history is change set is weird. It may be worth simply cherry-picking your changes on top of main (modulo the required changes). |
|
As mentioned before, this is my first time working open source like this. What does module mean in this context? Is there anything more I can do to get this approved? If you prefer, you're welcome to do the required fixes. |
|
In order for this to get merged, you need to first address the conflicts. However I am a bit concerned by the fact that your PR looks like it contains many more changes than I would expect. It may be easier to redo your changes on top of main. Feel free to force push to this branch if necessary. And by modulo (not module), I simply implied that because of the many changes I made you are unlikely to be able to only cherry-pick your changes on top of main because there will be conflicts that will need to be handled. |
Missing devices in device.add_channels(...)
Removed connection class. Dialogues no longer use _get_pair(), they use get_dialogue(). A dialogue with a "sources" key expects response to be a function.
Updated to latest dev version + some optimizations when finding connections
|
I'm having trouble with the MyPy errors. Do you have any suggestions for fixing them? |
|
One legitimate complaint from mypy is that Channel.add_dialogue signature does not match the signature in the base class. You can probably through some minor refactoring re-use the Component implementation in the Channel one (since the only difference is storing the result under |
Fixed: Incompatible default for argument "sources" (default has type "None", argument has type "Dict[Any, Any]") [assignment]
This should be fixed now along with the formatting complaints. |
New feature: Connecting instruments.
By adding connections in the yaml-file an instrument can have connections to others (or itself) and apply a function when the query is called.
An example can be found in device 5 in default.yaml.
connections: power: q: "READ1" source_list: [ {source_name: "GPIB0::2::65535::INSTR", source_parameter: amplitude}, {source_name: "self", source_parameter: frequency} ] function: "0.23 * (%amplitude% ** 2) - %frequency%"When using query "READ1" the device will fetch amplitude from another source and frequency from itself then return power, applying the function.
I have also added a new test in test_all.py for connections.
This is the way my team have requested connection functionality. Please give any feedback as we would like this to be as valuable for the community as possible!