Siglent #1237
Siglent #1237
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1237 +/- ##
==========================================
+ Coverage 59.11% 59.15% +0.04%
==========================================
Files 273 274 +1
Lines 19001 19020 +19
==========================================
+ Hits 11233 11252 +19
Misses 7768 7768
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| @@ -25,3 +25,4 @@ | |||
| from .siglent_spd1168x import SPD1168X | |||
| from .siglent_spd1305x import SPD1305X | |||
| from .siglent_sds1072cml import SDS1072CML | |||
| from .siglent_sdm3045x import SiglentSDM3045X | |||
There was a problem hiding this comment.
The other devices are just called "SDS...", so "SDM3045X" would be more fitting, but for other manufacturers, it's different. So do it as you wish.
| Args: | ||
| adapter: VISA Adapter used to communicate with the instrument. | ||
| name: Optional name for the instrument (default: "Siglent SDM3045X Multimeter"). | ||
| """ |
There was a problem hiding this comment.
We use sphinx for generating documentation, so use:
| Args: | |
| adapter: VISA Adapter used to communicate with the instrument. | |
| name: Optional name for the instrument (default: "Siglent SDM3045X Multimeter"). | |
| """ | |
| :param adapter: VISA Adapter used to communicate with the instrument. | |
| :param name: Optional name for the instrument (default: "Siglent SDM3045X Multimeter"). | |
| """ |
There was a problem hiding this comment.
By the way, there should be no init docstring. This information should be in the class docstring.
| super().__init__( | ||
| adapter, | ||
| name, | ||
| includeSCPI=True, |
There was a problem hiding this comment.
We use a different technique to add SCPI instruments, now: Please inherit from "Instrument" and from "SCPIMixin".
Then you can remove this line:
| measurement_mode = Instrument.control( | ||
| "CONFigure?", # Query command to get the current measurement mode | ||
| "CONFigure:%s", # Command format to set the measurement mode | ||
| "A string property to set the measurement mode. Supported values are " |
There was a problem hiding this comment.
We changed the way, how to write docstrings:
| "A string property to set the measurement mode. Supported values are " | |
| "Control the measurement mode. Supported values are " |
See https://pymeasure.readthedocs.io/en/latest/dev/coding_standards.html#docstrings
There was a problem hiding this comment.
Thanks for adding tests.
| measurement_mode = Instrument.control( | ||
| "CONFigure?", | ||
| "CONFigure:%s", | ||
| "Set the measurement mode " |
There was a problem hiding this comment.
If it is a control, please use "Control:
| "Set the measurement mode " | |
| "Control the measurement mode " |
Set is for a setting and Get/Measure for a measurement.
| dc_voltage_range = Instrument.control( | ||
| "VOLT:RANGe?", | ||
| "VOLT:RANGe %g", | ||
| "Set the DC voltage range in volts.", |
There was a problem hiding this comment.
You can specify the limits (and the strictness), here a suggestion:
| "Set the DC voltage range in volts.", | |
| "Set the DC voltage range in volts (float strictly between 0.1 and 1000).", |
| def reset(self): | ||
| """ | ||
| Resets the instrument to its default state. | ||
| This is equivalent to pressing the reset button on the instrument. | ||
| """ | ||
| self.write("*RST") | ||
|
|
||
| def identify(self): | ||
| """ | ||
| Queries the device identity. | ||
|
|
||
| Returns: | ||
| str: The identity string of the instrument, typically including | ||
| manufacturer, model, and serial number. | ||
| """ | ||
| return self.ask("*IDN?") |
There was a problem hiding this comment.
These commands are already defined in SCPIMixin, so no need to repeat.
| def close(self): | ||
| """ | ||
| Closes the instrument connection. | ||
|
|
||
| Ensures the connection is properly terminated. | ||
| """ | ||
| self.adapter.connection.close() |
There was a problem hiding this comment.
That is already in the instrument itself, if I remember correctly.
Added Support for siglent sdm3045x