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

LeCroy T3DSO1204 Oscilloscope #697

Closed
wants to merge 122 commits into from
Closed

LeCroy T3DSO1204 Oscilloscope #697

wants to merge 122 commits into from

Conversation

LastStarDust
Copy link
Contributor

This implements a device of a new vendor: LeCroy/Teledyne T3DSO1204 Oscilloscope.

Connection to the device is made through an TCP or USB connection.

I based the pymeasure.instruments.lecroy.LeCroyT3DSO1204 on the pymeasure.instruments.keysight.KeysightDSOX1102G but I added many functions, in particular related to the trigger.

I strived for 100% test coverage. However, to run the tests you need the hardware.

Work in progress


Work in progress


LeCroy T3DSO1204 Oscilloscope


LeCroy T3DSO1204 Oscilloscope
@BenediktBurger
Copy link
Member

Thanks for adding another instrument.
Please add a link in the "instruments/index.rst" to your LeCroy index.rst and please add similarly your "init.py" (which should load your instrument) to "instruments/init.py".

Thanks for the tests with an actual instrument. Please add a few tests, which do not require the instrument (see writing tests). They allow the maintainers to ensure compatibility during code changes without need for the instrument.

Copy link
Member

@bilderbuchi bilderbuchi left a comment

Choose a reason for hiding this comment

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

I did not manage to get through it all, it's just so much code to review, but I left a couple of remarks.

pymeasure/instruments/lecroy/lecroyT3DSO1204.py Outdated Show resolved Hide resolved
pymeasure/instruments/lecroy/lecroyT3DSO1204.py Outdated Show resolved Hide resolved
pymeasure/instruments/lecroy/lecroyT3DSO1204.py Outdated Show resolved Hide resolved
pymeasure/instruments/lecroy/lecroyT3DSO1204.py Outdated Show resolved Hide resolved
pymeasure/instruments/lecroy/lecroyT3DSO1204.py Outdated Show resolved Hide resolved
@LastStarDust
Copy link
Contributor Author

Thanks for adding another instrument. Please add a link in the "instruments/index.rst" to your LeCroy index.rst and please add similarly your "init.py" (which should load your instrument) to "instruments/init.py".

Thanks for the tests with an actual instrument. Please add a few tests, which do not require the instrument (see writing tests). They allow the maintainers to ensure compatibility during code changes without need for the instrument.

Thank you for the quick reply. I have applied the changes you request in the first part of your comment. I am going to write the unit tests without device in the next few days.

Copy link
Member

@BenediktBurger BenediktBurger left a comment

Choose a reason for hiding this comment

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

Here a few comments, mostly regarding the Channel implementation (now we have a more powerful channel syntax in place) and a property, which seems to be more appropriate for the channel than for the instrument itself.

pymeasure/instruments/lecroy/lecroyT3DSO1204.py Outdated Show resolved Hide resolved
pymeasure/instruments/lecroy/lecroyT3DSO1204.py Outdated Show resolved Hide resolved
pymeasure/instruments/lecroy/lecroyT3DSO1204.py Outdated Show resolved Hide resolved
pymeasure/instruments/lecroy/lecroyT3DSO1204.py Outdated Show resolved Hide resolved
pymeasure/instruments/lecroy/lecroyT3DSO1204.py Outdated Show resolved Hide resolved
pymeasure/instruments/lecroy/lecroyT3DSO1204.py Outdated Show resolved Hide resolved
pymeasure/instruments/lecroy/lecroyT3DSO1204.py Outdated Show resolved Hide resolved
pymeasure/instruments/lecroy/lecroyT3DSO1204.py Outdated Show resolved Hide resolved
@bilderbuchi
Copy link
Member

@bmoneke please, can you check your old conversations/comments and resolve them where appropriate?

@LastStarDust
Copy link
Contributor Author

LastStarDust commented Nov 26, 2022

I think that I have made all the changes necessary for the new Channel implementation. Let me know if I missed something.

Copy link
Member

@BenediktBurger BenediktBurger left a comment

Choose a reason for hiding this comment

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

Thanks for your work and your patience with our comments and requests for change.

Two, small changes. One regarding the usage of the channels (a simplification) and one docstring, in both cases I made already suggestions.

Thanks for adding the "ch_X" style. It caused some lines to be not properly indented (see flake hinting).

Please have a look at the few other open conversations. They contain only small changes.

I estimate, we're closing in on merging.

pymeasure/instruments/lecroy/lecroyT3DSO1204.py Outdated Show resolved Hide resolved
pymeasure/instruments/lecroy/lecroyT3DSO1204.py Outdated Show resolved Hide resolved
pymeasure/instruments/lecroy/lecroyT3DSO1204.py Outdated Show resolved Hide resolved
pymeasure/instruments/lecroy/lecroyT3DSO1204.py Outdated Show resolved Hide resolved
@bilderbuchi
Copy link
Member

OK, I had to create #770 and will merge over there. Thank you for the hard work and perseverance!

bilderbuchi added a commit that referenced this pull request Nov 29, 2022
@bilderbuchi
Copy link
Member

OK, this has landed in master now, so closing this manually.

@LastStarDust
Copy link
Contributor Author

@bmoneke @bilderbuchi Thank you so much for maintaining pymeasure and for your support in this pull request!

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.

None yet

4 participants