Repository: pymeasure/pymeasure (language: python)

Issue:
Deprecate implicit includeSCPI definition for mixin based instruments
The transition from our current setup with `Instrument` including SCPI commands per default to a mixin based process (#804 , #905 ) is difficult as `includeSCPI` is True on default, but the future default will be without SCPI.

In order to facilitate the transition, I propose the following:

1. Change the default value to `None`
2. In the init, `None` will be converted to `True`, but will raise a deprecation warning.
3. We could add a test for new instruments that they have `includeSCPI` defined.

That change should prompt people to include a definition into their driver (those, which are in pymeasure and those which they use privately).

What do you think @bilderbuchi , @CasperSchippers , @msmttchr ?


Discussion / hints:

Fix patch:
diff --git a/CHANGES.rst b/CHANGES.rst
index b0c0c5d7e1..591a3f0dfa 100644
--- a/CHANGES.rst
+++ b/CHANGES.rst
@@ -7,6 +7,7 @@ Added features
 Deprecated features
 -------------------
 - Replaced :code:`directory_input` keyword-argument of :code:`ManagedWindowBase` by :code:`enable_file_input` (@CasperSchippers, #964)
+- Make parameter :code:`includeSCPI` obligatory for all instruments, even those which use SCPI (@BenediktBurger, #1007)
 - Replaced :code:`celcius` attribute of :code:`LakeShoreTemperatureChannel` by :code:`celsius` (@afuetterer, #1003)
 - Replaced :code:`error` property of Keithley instruments by :code:`next_error`.
 - Replaced :code:`measurement_time` property of Pendulum CNT-91 by :code:`gate_time`.
diff --git a/pymeasure/instruments/hcp/tc038d.py b/pymeasure/instruments/hcp/tc038d.py
index 9f6e87b949..d29a5b0c7b 100644
--- a/pymeasure/instruments/hcp/tc038d.py
+++ b/pymeasure/instruments/hcp/tc038d.py
@@ -70,7 +70,9 @@ class TC038D(Instrument):
     def __init__(self, adapter, name="TC038D", address=1, timeout=1000,
                  **kwargs):
         """Initialize the device."""
-        super().__init__(adapter, name, timeout=timeout, **kwargs)
+        super().__init__(adapter, name, timeout=timeout,
+                         includeSCPI=False,
+                         **kwargs)
         self.address = address
 
     def write(self, command):
diff --git a/pymeasure/instruments/instrument.py b/pymeasure/instruments/instrument.py
index d173b6d33f..d94dfbe826 100644
--- a/pymeasure/instruments/instrument.py
+++ b/pymeasure/instruments/instrument.py
@@ -24,6 +24,7 @@
 
 import logging
 import time
+from warnings import warn
 
 from .common_base import CommonBase
 from ..adapters import VISAAdapter
@@ -58,7 +59,7 @@ class Instrument(CommonBase):
 
     :param adapter: A string, integer, or :py:class:`~pymeasure.adapters.Adapter` subclass object
     :param string name: The name of the instrument. Often the model designation by default.
-    :param includeSCPI: A boolean, which toggles the inclusion of standard SCPI commands
+    :param includeSCPI: An obligatory boolean, which toggles the inclusion of standard SCPI commands
     :param preprocess_reply: An optional callable used to preprocess
         strings received from the instrument. The callable returns the
         processed string.
@@ -71,7 +72,7 @@ class Instrument(CommonBase):
     """
 
     # noinspection PyPep8Naming
-    def __init__(self, adapter, name, includeSCPI=True,
+    def __init__(self, adapter, name, includeSCPI=None,
                  preprocess_reply=None,
                  **kwargs):
         # Setup communication before possible children require the adapter.
@@ -82,6 +83,10 @@ def __init__(self, adapter, name, includeSCPI=True,
                 raise Exception("Invalid Adapter provided for Instrument since"
                                 " PyVISA is not present")
         self.adapter = adapter
+        if includeSCPI is None:
+            warn("It is deprecated to specify `includeSCPI` implicitly, declare it explicitly.",
+                 FutureWarning)
+            includeSCPI = True
         self.SCPI = includeSCPI
         self.isShutdown = False
         self.name = name
diff --git a/pymeasure/instruments/keithley/keithleyDMM6500.py b/pymeasure/instruments/keithley/keithleyDMM6500.py
index 01865583e0..b9795fef6e 100644
--- a/pymeasure/instruments/keithley/keithleyDMM6500.py
+++ b/pymeasure/instruments/keithley/keithleyDMM6500.py
@@ -220,7 +220,9 @@ class KeithleyDMM6500(Instrument):
     def __init__(
         self, adapter, name="Keithley DMM6500 6½-Digit Multimeter", read_termination="\n", **kwargs
     ):
-        super().__init__(adapter, name, read_termination=read_termination, **kwargs)
+        super().__init__(adapter, name, read_termination=read_termination,
+                         includeSCPI=True,
+                         **kwargs)
         self.command_set = "SCPI"
 
     def __exit__(self, exc_type, exc_value, traceback):
diff --git a/pymeasure/instruments/racal/racal1992.py b/pymeasure/instruments/racal/racal1992.py
index 72b9e3a35e..833cc2dbb5 100644
--- a/pymeasure/instruments/racal/racal1992.py
+++ b/pymeasure/instruments/racal/racal1992.py
@@ -53,6 +53,7 @@ def __init__(self, adapter, name="Racal-Dana 1992", **kwargs):
         super().__init__(
             adapter,
             name,
+            includeSCPI=False,
             **kwargs
         )
 
diff --git a/pymeasure/instruments/redpitaya/redpitaya_scpi.py b/pymeasure/instruments/redpitaya/redpitaya_scpi.py
index 454e91f0a1..c31ff4bea5 100644
--- a/pymeasure/instruments/redpitaya/redpitaya_scpi.py
+++ b/pymeasure/instruments/redpitaya/redpitaya_scpi.py
@@ -197,6 +197,7 @@ def __init__(self,
             name,
             read_termination=read_termination,
             write_termination=write_termination,
+            includeSCPI=True,
             **kwargs)
 
     dioN = Instrument.MultiChannelCreator(DigitalChannelN, list(range(7)), prefix='dioN')


Test patch:
diff --git a/tests/instruments/test_all_instruments.py b/tests/instruments/test_all_instruments.py
index f30b41f230..f763496100 100644
--- a/tests/instruments/test_all_instruments.py
+++ b/tests/instruments/test_all_instruments.py
@@ -188,6 +188,92 @@ def find_devices_in_module(module):
     "ChannelAWG",
     "ChannelAFG",
 ]
+# Instruments which do not YET define `includeSCPI` explicitly
+grandfathered_includeSCPI_instruments = [
+    "AdvantestR624X",
+    "AdvantestR6245",
+    "AdvantestR6246",
+    "AdvantestR3767CG",
+    "AFG3152C",
+    "Agilent33521A",
+    "Agilent8722ES",
+    "AgilentB1500",
+    "AgilentE4980",
+    "AgilentE4408B",
+    "Agilent33500",
+    "Agilent8257D",
+    "Agilent34410A",
+    "Agilent33220A",
+    "Agilent4156",
+    "Ametek7270",
+    "AMI430",
+    "AnritsuMS4645B",
+    "AnritsuMS4647B",
+    "AnritsuMS4644B",
+    "AnritsuMS464xB",
+    "AnritsuMS4642B",
+    "AnritsuMS9740A",
+    "AnritsuMS2090A",
+    "AnritsuMS9710C",
+    "AnritsuMG3692C",
+    "APSIN12G",
+    "ATSBase",
+    "ATS545",
+    "ATS525",
+    "AWG401x_base",
+    "BKPrecision9130B",
+    "CNT91",
+    "ECO560",
+    "ESP300",
+    "HP33120A",
+    "HP34401A",
+    "Keithley2000",
+    "Keithley2200",
+    "Keithley2400",
+    "Keithley2600",
+    "Keithley2260B",
+    "Keithley2306",
+    "Keithley2750",
+    "Keithley6221",
+    "Keithley6517B",
+    "Keithley2450",
+    "KeysightDSOX1102G",
+    "KeysightN7776C",
+    "KeysightN5767A",
+    "KeysightE36312A",
+    "LakeShore211",
+    "LakeShore224",
+    "LakeShore331",
+    "LakeShore421",
+    "LakeShore425",
+    "LeCroyT3DSO1204",
+    "ParkerGV6",
+    "PL303P",
+    "PL303QMTP",
+    "PL303QMDP",
+    "PLBase",
+    "PL068P",
+    "PL601P",
+    "PL155P",
+    "razorbillRP100",
+    "SG380",
+    "SM7045D",
+    "SPDBase",
+    "SPDSingleChannelBase",
+    "SPD1168X",
+    "SPD1305X",
+    "SR860",
+    "SR830",
+    "SR570",
+    "TDS2000",
+    "TeledyneMAUI",
+    "TeledyneOscilloscope",
+    "TexioPSW360L30",
+    "ThorlabsPro8000",
+    "VellemanK8090",
+    "Yokogawa7651",
+    "YokogawaGS200",
+]
 
 
 @pytest.mark.parametrize("cls", devices)
@@ -239,6 +325,22 @@ def test_kwargs_to_adapter(cls):
         cls(SIM_RESOURCE, visa_library="@sim", kwarg_test=True)
 
 
+@pytest.mark.parametrize("cls", devices)
+@pytest.mark.filterwarnings("error:It is deprecated to specify `includeSCPI`:FutureWarning")
+def test_includeSCPI_explicitly_set(cls):
+    if cls.__name__ in (*proper_adapters, *need_init_communication):
+        pytest.skip(f"{cls.__name__} cannot be tested without communication.")
+    elif cls.__name__ in channel_as_instrument_subclass:
+        pytest.skip(f"{cls.__name__} is a channel, not an instrument.")
+    elif cls.__name__ in grandfathered_includeSCPI_instruments:
+        pytest.skip(f"{cls.__name__} is in the codebase and needs information about SCPI.")
+    elif cls.__name__ == "Instrument":
+        pytest.skip("`Instrument` requires a `name` parameter.")
+
+    cls(adapter=MagicMock())
+    # assert that no error is raised
+
+
 def property_name_to_id(value):
     """Create a test id from `value`."""
     device, property_name, prop = value
diff --git a/tests/instruments/test_instrument.py b/tests/instruments/test_instrument.py
index a305316f04..c9f3c55b9b 100644
--- a/tests/instruments/test_instrument.py
+++ b/tests/instruments/test_instrument.py
@@ -104,6 +104,18 @@ def test_fake_instrument():
     assert fake.values("5") == [5]
 
 
+class Test_includeSCPI_parameter:
+    def test_not_defined_includeSCPI_raises_warning(self):
+        with pytest.warns(FutureWarning) as record:
+            Instrument(name="test", adapter=ProtocolAdapter())
+        msg = str(record[0].message)
+        assert msg == "It is deprecated to specify `includeSCPI` implicitly, declare it explicitly."
+
+    def test_not_defined_includeSCPI_is_interpreted_as_true(self):
+        inst = Instrument(name="test", adapter=ProtocolAdapter())
+        assert inst.SCPI is True
+
+
 @pytest.mark.parametrize("adapter", (("COM1", 87, "USB")))
 def test_init_visa(adapter):
     Instrument(adapter, "def", visa_library="@sim")