Dev/sds1072cml #1080
Dev/sds1072cml #1080
Conversation
…ing set_process and get_process.
…kward and should be managed as a channel. Module is nonetheless usable
There was a problem hiding this comment.
Thanks for adding tests. However, please see the writing tests section of our documentation.
The test_instrument.py file contains protocol tests (without a device), while test_instrument_with_device contains tests with an actual device. For a device test, we have a fixture, which allows to define the resource name as a command line parameter.
pymeasure/instruments/siglenttechnologies/siglent_sds1072cml.py
Outdated
Show resolved
Hide resolved
|
Thanks @fthouin for your contribution and welcome to the contributors. Two small comments during a first quick view. |
Co-authored-by: Benedikt Burger <67148916+BenediktBurger@users.noreply.github.com>
|
Hi Benedict, Thanks for the feedback, I have finalized the control of the instrument (trigger and all) I would now like to add tests using the test generator. However, when invoking Is this feature still supported or have I forgotten to configure something? Thanks for your help! |
I do not know by heart, whether the test generator is already part of the latest release or not. Do you have pymeasure installed as a release or from github? |
There was a problem hiding this comment.
A few comments how to improve your code, which is mainly code style according to python standards.
For more information see https://peps.python.org/pep-0257/
| ############################# | ||
| Siglent SDS1072CML Oscilloscope | ||
| ############################# |
There was a problem hiding this comment.
The marks have to be as long as the text itself:
| ############################# | |
| Siglent SDS1072CML Oscilloscope | |
| ############################# | |
| ############################### | |
| Siglent SDS1072CML Oscilloscope | |
| ############################### |
| """ | ||
| vertical_division=Channel.control( | ||
| "C{ch}:VDIV?","C{ch}:VDIV %s", | ||
| "Sets or gets the vertical sensitivity of a channel.", |
There was a problem hiding this comment.
Docstrings in general should be in imperative mode (so "Set or Get"...)
We in pymeasure use "Control" for "Set or get" as the first word of a control. For a measurement it is "Measure" or "Get" and for a setting it is "Set".
| "Sets or gets the vertical sensitivity of a channel.", | |
| "Control the vertical sensitivity of a channel.", |
| ) | ||
| coupling=Channel.control( | ||
| "C{ch}:CPL?","C{ch}:CPL %s1M", | ||
| "Sets and gets the channel coupling mode. (see UM p. 35)", |
There was a problem hiding this comment.
The reference to the user manual is not as useful for those, who do not have it. It is better, if you add the options ("DC" and "AC").
| "Sets and gets the channel coupling mode. (see UM p. 35)", | |
| "Sets and gets the channel coupling mode to "AC" or "DC". (see UM p. 35)", |
| Returns the waveforms displayed in the channel. | ||
| return: |
There was a problem hiding this comment.
A docstring should be in imperative mode. The first sentence should be separated by the rest by an empty line:
| Returns the waveforms displayed in the channel. | |
| return: | |
| Return the waveforms displayed in the channel. | |
| return: |
| map_values=True, | ||
| get_process= lambda v : v.split(" ",1)[-1][0], | ||
| ) | ||
| def getWaveform(self): |
There was a problem hiding this comment.
normally for python the style is get_waveform for methods.
| timetags=[i*descriptorDictionnary["horizInterval"]+descriptorDictionnary["horizOffset"] for i in range(len(rawWaveform))] | ||
| return timetags,waveform | ||
|
|
||
| def getDesc(self): |
There was a problem hiding this comment.
| def getDesc(self): | |
| def get_desc(self): |
| "descriptorOffset":descriptorOffset | ||
| } | ||
| return descriptorDictionnary | ||
| class TriggerChannel(Channel): |
There was a problem hiding this comment.
Two empty lines between definitions outside of classes, please.
| class TriggerChannel(Channel): | |
| class TriggerChannel(Channel): |
| } | ||
| triggerSetupDict=get_process(self.ask("TRSE?")) | ||
| return triggerSetupDict | ||
| def getLevel(self): |
There was a problem hiding this comment.
One empty line between class definitions, please.
| def getLevel(self): | |
| def getLevel(self): |
| class SDS1072CML(Instrument): | ||
| """ Represents the SIGLENT SDS1072CML Oscilloscope | ||
| and provides a high-level for interacting with the instrument | ||
| """ | ||
| def __init__(self, adapter, name="Siglent SDS1072CML Oscilloscope", **kwargs): | ||
| super().__init__( | ||
| adapter, | ||
| name, | ||
| **kwargs |
There was a problem hiding this comment.
Please state, whether the device supports SCPI (in this case inherit SCPIMixin before Instrument) or not (in this case set includeSCPI=False).
First draft of a module to control the Siglent SDS1072CML oscilloscope
_ Unit tests are still lacking
_ the module is usable but awkward (especially the trigger setting)