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

Remove Toptica adapter #819

Merged
merged 13 commits into from Jan 19, 2023
Merged

Conversation

BenediktBurger
Copy link
Member

@BenediktBurger BenediktBurger commented Jan 4, 2023

Remove the Toptica Adapter and make the ibeamSmart depending on the VISAAdapter alone.
Takes hints from original PR #352 and the new issue #816.

The tests require empty strings, which are added to the ProtocolAdapter in #818

I added a few tests from the examples @dkriegner gave in #352.

Fixes #816

Copy link
Member

@msmttchr msmttchr left a comment

Choose a reason for hiding this comment

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

Few comment from my side. Thanks

pymeasure/instruments/toptica/ibeamsmart.py Outdated Show resolved Hide resolved
pymeasure/instruments/toptica/ibeamsmart.py Show resolved Hide resolved
pymeasure/instruments/toptica/ibeamsmart.py Outdated Show resolved Hide resolved
@BenediktBurger
Copy link
Member Author

Thanks @msmttchr for your Tipps.
My possibilities to refractor the instrument are limited, as I do not know it.

@msmttchr
Copy link
Member

msmttchr commented Jan 8, 2023 via email

@BenediktBurger
Copy link
Member Author

@waveman68 could you please test this implementation with your device?

Does anyone of you, @dkriegner or @waveman68 have a manual and could help me with the protocol tests (just give me some examples)?

@dkriegner
Copy link
Contributor

dkriegner commented Jan 10, 2023

The manual was delivered as printout. I will ask to get it for you. I'll come back to you.

Edit: The manual was shared with @bmoneke by email

@BenediktBurger BenediktBurger marked this pull request as ready for review January 10, 2023 11:36
@dkriegner
Copy link
Contributor

Thanks for all the work. If you give sufficient time, I will instruct someone (remotely) to test the new code with the hardware.

@BenediktBurger
Copy link
Member Author

Thanks @dkriegner , it is not urgent. Whenever you (and your acquantance) have time, is fine.

Copy link
Contributor

@dkriegner dkriegner left a comment

Choose a reason for hiding this comment

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

I can confirm that these changes fix issue #816.

There are some remaining issues which need to be discussed. Some of them only are important if a translator device (see #815 for a more detailed description) are used. But some are also completely generic.

pymeasure/instruments/toptica/ibeamsmart.py Show resolved Hide resolved
pymeasure/instruments/toptica/ibeamsmart.py Show resolved Hide resolved
pymeasure/instruments/toptica/ibeamsmart.py Outdated Show resolved Hide resolved
tests/instruments/toptica/test_ibeamsmart.py Show resolved Hide resolved
@BenediktBurger
Copy link
Member Author

Thanks for the information regarding the flush.
I don't know yet, how to do it properly (best would be to not use it).

@dkriegner
Copy link
Contributor

I think a flush method is very important to be able to clean up after an error. It's a pity that pyvisa does not implement it for all connection methods.
Since even on the NI webpage they recommend using read to flush a TCPIP socket in Labview I assume there is a technical reason for that.

If you would not want to implement a fallback, the colleagues will keep some local code changes. In the end this is again only an issue when using a TCPIP translator. When using via the serial device, the existing flush_read_buffer is sufficient and the error handling is only needed for your unittests. But likely this should then be mentioned as comment. One could also mention that because of this using any Adapter defined outside is unlikely to work (even SerialAdapter would likely fail) since the communication is screwed up after the __init__

@BenediktBurger
Copy link
Member Author

What do you think of a flush method based on read?
That would be the most complete instrument implementation. And basically, we strive to just use write and read (or their bytes variants) as these are universal to all connection types (and adapters).

@dkriegner
Copy link
Contributor

yes, something based on read (repeated read as shown in my comment above or something equivalent) with the timeout set to minimum seems sensible. Not sure if the string conversion in read could make troubles and the byte equivalent would not be better ?!

@BenediktBurger
Copy link
Member Author

I raised an issue #834 for that missing flush method in pyvisa-py as it does not pertain to this instrument itself, but to the VISAAdapter (and more properly to pyvisa-py).

From my side, the code is ready and any maintainer may merge it.

@BenediktBurger
Copy link
Member Author

@bilderbuchi you might merge at will.

@bilderbuchi
Copy link
Member

Thanks!

@bilderbuchi bilderbuchi merged commit c04f211 into pymeasure:master Jan 19, 2023
@BenediktBurger BenediktBurger deleted the PR-Toptica-Adapter branch January 24, 2023 15:09
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.

Toptica iBeamSmart issue
4 participants