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

ERASynth drivers #98

Merged
merged 24 commits into from
Nov 4, 2021
Merged

ERASynth drivers #98

merged 24 commits into from
Nov 4, 2021

Conversation

caenrigen
Copy link
Contributor

@caenrigen caenrigen commented Jun 21, 2021

Hi!

This is a driver using the new style implemented in microsoft/Qcodes#3098.

  • I still need to add an example notebook.

For now I have a question, what is the proper way of making sure a certain command for a parameter was received by an instrument?

For this particular instrument I implemented the driver with manual control over the serial communication (instead of a Visa instrument) because from experience it was very unreliable in setting the parameters and some experiments require to as fast as possible and at the same time to be 100% sure that a certain parameter has been already set to the desired value.

To deal with this issue this driver always reads back the value it sets and sets it again until it succeeds... not very elegant nor ideal but at least makes it reliable.

Let me know your thoughts on this.


PS The link to the docs of this repo is a bit hidden on the top, and I think most people will be looking for it in the README. I suggest in the PR to have it there as well.

@FarBo
Copy link
Contributor

FarBo commented Jun 22, 2021

@caenrigen

Thank you for the driver. Regarding your question, I think you should use ask_raw method in the Instrument class. This method writes a command to the instrument and returns a response.
You should override this method since you are defining a new hardware communication.

I don't have a personal experience myself to guide you in detail, but I wanted to give you some insight.
You could have a look at qcodes drivers, where some of them have used ask_raw, for instance this one:
https://github.com/QCoDeS/Qcodes/blob/master/qcodes/instrument_drivers/Harvard/Decadac.py

Then you could probably use ask_raw to make your parameters set values reliable.

docs/conf.py Outdated Show resolved Hide resolved
qcodes_contrib_drivers/drivers/ERAInstruments/erasynth.py Outdated Show resolved Hide resolved
qcodes_contrib_drivers/drivers/ERAInstruments/erasynth.py Outdated Show resolved Hide resolved
@FarBo
Copy link
Contributor

FarBo commented Jun 28, 2021

@caenrigen
We had a fix for doc building in the respoitory files, so I merged the master branch to have that fix for this PR. CI should not complain about that error anymore. But we have other problems that CI complains about. Let me know if I could help fixing those?

@caenrigen
Copy link
Contributor Author

@caenrigen
We had a fix for doc building in the respoitory files, so I merged the master branch to have that fix for this PR. CI should not complain about that error anymore. But we have other problems that CI complains about. Let me know if I could help fixing those?

Thank you @FarBo

The rest of the errors depend on microsoft/Qcodes#3098 being merged.

@caenrigen
Copy link
Contributor Author

@FarBo @astafan8 This one should also be ready for review now :)

@caenrigen caenrigen force-pushed the erasynth branch 2 times, most recently from 15adca7 to 00b5c5e Compare November 1, 2021 02:59
@caenrigen caenrigen changed the title WIP: ERASynth drivers ERASynth drivers Nov 1, 2021
@caenrigen
Copy link
Contributor Author

@FarBo @astafan8 I have updated this PR to make use of the latest qcodes and the new feature for documenting parameters.

Could you have a look and merge it?

If possible would appreciate if we can have a new release as well in order to be able to ship this new driver to PyPi

@AdriaanRol @dcrielaard

@caenrigen
Copy link
Contributor Author

@astafan8 thank you for the quick review, feel free to merge :)

@FarBo
Copy link
Contributor

FarBo commented Nov 1, 2021

@caenrigen
Could you check the CI errors?
Seems mypy is not happy about bunch of lines in the code. Let me know if I could help passing it :)

@caenrigen
Copy link
Contributor Author

@caenrigen
Could you check the CI errors?
Seems mypy is not happy about bunch of lines in the code. Let me know if I could help passing it :)

Hi! Just checked and tried to fix some of them :)

Please give me a hand if it does not pass again 😇

Kind regards,
Victor

@caenrigen
Copy link
Contributor Author

@FarBo I hope my last commit solves most issues

@FarBo
Copy link
Contributor

FarBo commented Nov 3, 2021

@caenrigen
I am working on mypy,

Will push a fix today :)

@FarBo
Copy link
Contributor

FarBo commented Nov 3, 2021

@caenrigen
I pushed a few things to deal with mypy issues. seems the code is passing mypy check
there is a problem in the CI of the repository, which is unrelated to this PR. That should be fixed, and this branch is updated with that fix to have the CI gets completed for this PR. I will handle that.
Could you test my latest commits to see if they didn't break anything?

@caenrigen
Copy link
Contributor Author

@caenrigen
I pushed a few things to deal with mypy issues. seems the code is passing mypy check
there is a problem in the CI of the repository, which is unrelated to this PR. That should be fixed, and this branch is updated with that fix to have the CI gets completed for this PR. I will handle that.
Could you test my latest commits to see if they didn't break anything?

Thank you for the help, fixed it with my last commit (the attribute needs to be queried every time)

@jenshnielsen
Copy link
Collaborator

@FarBo @caenrigen I submitted a pr over here caenrigen#1 to fix the formatting in the copper mountain driver

@jenshnielsen jenshnielsen merged commit 8cc993a into QCoDeS:master Nov 4, 2021
@caenrigen
Copy link
Contributor Author

Thank you for the review and help!

Quick question, when would it be possible to have a release?

@FarBo
Copy link
Contributor

FarBo commented Nov 4, 2021

No worries

There is abit of work left for the release step in the CI. Once that is done, we will release a version right after.

@caenrigen
Copy link
Contributor Author

No worries

There is abit of work left for the release step in the CI. Once that is done, we will release a version right after.

Awesome! Thanks :)

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.

4 participants