Repository: pymeasure/pymeasure

Issue:
Toptica iBeamSmart issue
While some basic functionality is implemented, there is an issue with writing. The laser sends a prompt, to indicate that it is ready for writing as a handshake:

```
CMD>
```

Currently, read is implemented to wait for just `[OK]`.

I would be happy to make a PR, but first need to better understand the pymeasure philosophy. I see a few options where to put this:

- in a State (e.g., ready for write) + 
- in read
- in write

Does anybody have some guidance?

Thanks
Sam

Discussion:

Patch:
diff --git a/CHANGES.rst b/CHANGES.rst
index e85ec3f8..9600d6bb 100644
--- a/CHANGES.rst
+++ b/CHANGES.rst
@@ -9,6 +9,7 @@ Deprecated features
 -------------------
 - HP 34401A: :code:`voltage_ac`, :code:`current_dc`, :code:`current_ac`, :code:`resistance`, :code:`resistance_4w` properties,
   use :code:`function_` and :code:`reading` properties instead.
+- Toptica IBeamSmart: :code:`channel1_enabled`, use :code:`ch_1.enabled` property instead (similar channel2). Also :code:`laser_enabled` is deprecated in favor of :code:`emission`.
 
 Version 0.11.1 (2022-12-31)
 ===========================
diff --git a/docs/api/instruments/toptica/adapters.rst b/docs/api/instruments/toptica/adapters.rst
deleted file mode 100644
index fb551014..00000000
--- a/docs/api/instruments/toptica/adapters.rst
+++ /dev/null
@@ -1,7 +0,0 @@
-################
-Toptica Adapters
-################
-
-.. autoclass:: pymeasure.instruments.toptica.adapters.TopticaAdapter
-    :members:
-    :show-inheritance:
diff --git a/docs/api/instruments/toptica/ibeamsmart.rst b/docs/api/instruments/toptica/ibeamsmart.rst
index 429e6284..4d715a6c 100644
--- a/docs/api/instruments/toptica/ibeamsmart.rst
+++ b/docs/api/instruments/toptica/ibeamsmart.rst
@@ -5,3 +5,7 @@ Toptica IBeam Smart Laser diode
 .. autoclass:: pymeasure.instruments.toptica.ibeamsmart.IBeamSmart
     :members:
     :show-inheritance:
+
+.. autoclass:: pymeasure.instruments.toptica.ibeamsmart.DriverChannel
+    :members:
+    :show-inheritance:
diff --git a/docs/api/instruments/toptica/index.rst b/docs/api/instruments/toptica/index.rst
index a977ef9c..5cfbb6ca 100644
--- a/docs/api/instruments/toptica/index.rst
+++ b/docs/api/instruments/toptica/index.rst
@@ -9,5 +9,4 @@ This section contains specific documentation on the Toptica Photonics instrument
 .. toctree::
    :maxdepth: 2
 
-   adapters
    ibeamsmart
diff --git a/pymeasure/instruments/toptica/__init__.py b/pymeasure/instruments/toptica/__init__.py
index 893c0142..edb07d94 100644
--- a/pymeasure/instruments/toptica/__init__.py
+++ b/pymeasure/instruments/toptica/__init__.py
@@ -22,5 +22,4 @@
 # THE SOFTWARE.
 #
 
-from .adapters import TopticaAdapter
 from .ibeamsmart import IBeamSmart
diff --git a/pymeasure/instruments/toptica/adapters.py b/pymeasure/instruments/toptica/adapters.py
deleted file mode 100644
index f08c9a84..00000000
--- a/pymeasure/instruments/toptica/adapters.py
+++ /dev/null
@@ -1,124 +0,0 @@
-#
-# This file is part of the PyMeasure package.
-#
-# Copyright (c) 2013-2023 PyMeasure Developers
-#
-# Permission is hereby granted, free of charge, to any person obtaining a copy
-# of this software and associated documentation files (the "Software"), to deal
-# in the Software without restriction, including without limitation the rights
-# to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
-# copies of the Software, and to permit persons to whom the Software is
-# furnished to do so, subject to the following conditions:
-#
-# The above copyright notice and this permission notice shall be included in
-# all copies or substantial portions of the Software.
-#
-# THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
-# IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
-# FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
-# AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
-# LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
-# OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
-# THE SOFTWARE.
-#
-
-import re
-import time
-
-from pymeasure.adapters import VISAAdapter
-from pyvisa.errors import VisaIOError
-
-
-class TopticaAdapter(VISAAdapter):
-    """ Adapter class for connecting to Toptica Console via a serial
-    connection.
-
-    :param port: pyvisa resource name of the instrument
-    :param baud_rate: communication speed
-    :param kwargs: Any valid key-word argument for VISAAdapter
-    """
-    # compiled regular expression for finding numerical values in reply strings
-    _reg_value = re.compile(r"\w+\s+=\s+(\w+)")
-
-    def __init__(self, port, baud_rate, **kwargs):
-        kwargs.setdefault('preprocess_reply', self.extract_value)
-        kwargs.setdefault('read_termination', '\r\n')
-        kwargs.setdefault('write_termination', '\r\n')
-        super().__init__(port,
-                         asrl=dict(baud_rate=baud_rate),
-                         **kwargs)
-        # configure communication mode
-        super().write('echo off')
-        super().write('prom off')
-        time.sleep(0.04)
-        # clear the initial messages from the controller
-        self.flush_read_buffer()
-        self.ask('talk usual')
-
-    def extract_value(self, reply):
-        """ preprocess_reply function which tries to extract <value> from 'name
-        = <value> [unit]'. If <value> can not be identified the orignal string
-        is returned.
-
-        :param reply: reply string
-        :returns: string with only the numerical value, or the original string
-        """
-        r = self._reg_value.search(reply)
-        if r:
-            return r.groups()[0]
-        else:
-            return reply
-
-    def _read(self):
-        """ Reads a reply of the instrument which consists of at least two
-        lines. The initial ones are the reply to the command while the last one
-        should be '[OK]' which acknowledges that the device is ready to receive
-        more commands.
-
-        Note: '[OK]' is always returned as last message even in case of an
-        invalid command, where a message indicating the error is returned
-        before the '[OK]'
-
-        :returns: string containing the ASCII response of the instrument.
-        """
-        reply = super()._read()  # read back the LF+CR which is always sent back
-        if reply != "":
-            raise ValueError(
-                "TopticaAdapter.read(1): Error after command "
-                f"'{self.lastcommand}' with message '{reply}'")
-        msg = []
-        try:
-            while True:
-                line = super()._read()
-                if line == '[OK]':
-                    break
-                msg.append(line)
-        except VisaIOError:
-            reply = '\n'.join(msg)
-            self.flush_read_buffer()
-            raise ValueError(
-                "TopticaAdapter.read(2): Error after command "
-                f"'{self.lastcommand}' with message '{reply}'")
-        return '\n'.join(msg)
-
-    def _write(self, command, check_ack=True):
-        """ Writes a command to the instrument. Also reads back a LF+CR which
-        is always sent back.
-
-        :param command: command string to be sent to the instrument
-        :param check_ack: flag to decide if also an acknowledgement from the
-          device is expected. This is the case for set commands.
-        """
-        self.lastcommand = command
-        super()._write(command)
-        # The following lines are used in order to avoid the need of a
-        # complicated Instrument where every property would need to use
-        # check_set/get_errors and many Adapter functions would need to be
-        # reimplemented in the Instrument. See discussion in PR #352
-        if check_ack:
-            reply = self.read()
-            if reply != "":
-                # if any message is returned here this indicates some misuse
-                raise ValueError(
-                    f"TopticaAdapter.write: Error after command '{command}'"
-                    f"with message '{reply}'")
diff --git a/pymeasure/instruments/toptica/ibeamsmart.py b/pymeasure/instruments/toptica/ibeamsmart.py
index ec839b00..49dad866 100644
--- a/pymeasure/instruments/toptica/ibeamsmart.py
+++ b/pymeasure/instruments/toptica/ibeamsmart.py
@@ -22,99 +22,276 @@
 # THE SOFTWARE.
 #
 
-from pymeasure.instruments import Instrument
-from pymeasure.instruments.toptica.adapters import TopticaAdapter
+import logging
+import re
+import time
+from warnings import warn
+
+from pyvisa.errors import VisaIOError
+
+from pymeasure.instruments import Channel, Instrument
 from pymeasure.instruments.validators import strict_discrete_set, strict_range
 
 
+log = logging.getLogger(__name__)
+log.addHandler(logging.NullHandler())
+
+
+def _deprecation_warning(text):
+    def func(x):
+        warn(text, FutureWarning)
+        return x
+    return func
+
+
+def _deprecation_warning_channels(property_name):
+    def func(x):
+        warn(f'Deprecated property name "{property_name}", use the channels '
+             '"enabled" property instead.', FutureWarning)
+        return x
+    return func
+
+
+class DriverChannel(Channel):
+    """A laser diode driver channel for the IBeam Smart laser."""
+
+    power = Channel.setting(
+        "ch {ch} pow %f mic",
+        """Set the output power in µW (float up to 200000).""",
+        check_set_errors=True,
+        validator=strict_range,
+        values=[0, 200000],
+    )
+
+    enabled = Channel.control(
+        "sta ch {ch}",
+        "%s {ch}",
+        """Control the enabled state of the driver channel.""",
+        validator=strict_discrete_set,
+        values=[True, False],
+        get_process=lambda s: True if s == 'ON' else False,
+        set_process=lambda v: "en" if v else "di",
+        check_set_errors=True,
+    )
+
+
 class IBeamSmart(Instrument):
     """ IBeam Smart laser diode
 
-    :param port: pyvisa resource name of the instrument
-    :param baud_rate: communication speed, defaults to 115200
-    :param kwargs: Any valid key-word argument for VISAAdapter
+    For the usage of the different diode driver channels, see the manual
+
+    .. code::
+
+        laser = IBeamSmart("SomeResourceString")
+        laser.emission = True
+        laser.ch_2.power = 1000  # µW
+        laser.ch_2.enabled = True
+        laser.shutdown()
+
+    :param adapter: pyvisa resource name or adapter instance.
+    :param baud_rate: The baud rate you have set in the instrument.
+    :param \\**kwargs: Any valid key-word argument for VISAAdapter.
     """
-    def __init__(self, adapter, baud_rate=115200, **kwargs):
-        if not isinstance(adapter, str):
-            raise TypeError("'adapter' must be a pyvisa resource name string")
+    _reg_value = re.compile(r"\w+\s+=\s+(\w+)")
+
+    channels = Instrument.ChannelCreator(DriverChannel, (1, 2, 3, 4, 5))
+
+    def __init__(self, adapter, name="Toptica IBeam Smart laser diode",
+                 baud_rate=115200,
+                 **kwargs):
         super().__init__(
-            TopticaAdapter(adapter, baud_rate, **kwargs),
-            "toptica IBeam Smart laser diode",
+            adapter,
+            name,
             includeSCPI=False,
+            read_termination='\r\n',
+            write_termination='\r\n',
+            asrl={'baud_rate', baud_rate},
+            **kwargs
         )
+        # configure communication mode: no repeating and no command prompt
+        self.write('echo off')
+        self.write('prom off')
+        time.sleep(0.04)
+        # clear the initial messages from the controller
+        try:
+            self.adapter.flush_read_buffer()
+        except AttributeError:
+            log.warning("Adapter does not have 'flush_read_buffer' method.")
+        self.ask('talk usual')
+
+    def read(self):
+        """Read a reply of the instrument and extract the values, if possible.
+
+        Reads a reply of the instrument which consists of at least two
+        lines. The initial ones are the reply to the command while the last one
+        should be '[OK]' which acknowledges that the device is ready to receive
+        more commands.
+
+        Note: '[OK]' is always returned as last message even in case of an
+        invalid command, where a message indicating the error is returned
+        before the '[OK]'
+
+        Value extraction: extract <value> from 'name = <value> [unit]'.
+        If <value> can not be identified the orignal string is returned.
+
+        :return: string containing the ASCII response of the instrument (without '[OK]').
+        """
+        reply = super().read()  # read back the LF+CR which is always sent back
+        if reply != "":
+            raise ValueError(
+                "TopticaAdapter.read(1): Error after command "
+                f"'{self.lastcommand}' with message '{reply}'")
+        msg = []
+        try:
+            while True:
+                line = super().read()
+                if line == '[OK]':
+                    break
+                msg.append(line)
+        except VisaIOError:
+            reply = '\n'.join(msg)
+            try:
+                self.adapter.connection.flush_read_buffer()
+            except AttributeError:
+                log.warning("Adapter does not have 'flush_read_buffer' method.")
+            raise ValueError(
+                "TopticaAdapter.read(2): Error after command "
+                f"'{self.lastcommand}' with message '{reply}'")
+        reply = '\n'.join(msg)
+        r = self._reg_value.search(reply)
+        if r:
+            return r.groups()[0]
+        else:
+            return reply
+
+    def write(self, command):
+        self.lastcommand = command
+        super().write(command)
+
+    def check_errors(self):
+        """Check communication after setting a value.
+
+        Checks if the last reply is only '[OK]', otherwise a ValueError is
+        raised and the read buffer is flushed because one has to assume that
+        some communication is out of sync.
+        """
+        reply = self.read()
+        if reply:
+            # anything else than '[OK]'.
+            self.adapter.connection.flush_read_buffer()
+            raise ValueError(
+                f"IBeamSmart: Error after command '{self.lastcommand}' with "
+                f"message '{reply}'")
 
     version = Instrument.measurement(
-           "ver", """ Firmware version number """,
+        "ver", """Get Firmware version number.""",
     )
 
     serial = Instrument.measurement(
-           "serial", """ Serial number of the laser system """,
+        "serial", """Get Serial number of the laser system.""",
     )
 
     temp = Instrument.measurement(
-            "sh temp",
-            """ temperature of the laser diode in degree centigrade.""",
+        "sh temp",
+        """Measure the temperature of the laser diode in degree centigrade.""",
     )
 
     system_temp = Instrument.measurement(
-            "sh temp sys",
-            """ base plate (heatsink) temperature in degree centigrade.""",
+        "sh temp sys",
+        """Measure base plate (heatsink) temperature in degree centigrade.""",
+    )
+
+    current = Instrument.measurement(
+        "sh cur",
+        """Measure the laser diode current in mA.""",
+    )
+
+    emission = Instrument.control(
+        "sta la", "la %s",
+        """Control emission status of the laser diode driver (bool).""",
+        validator=strict_discrete_set,
+        values=[True, False],
+        get_process=lambda s: True if s == 'ON' else False,
+        set_process=lambda v: "on" if v else "off",
+        check_set_errors=True,
     )
 
     laser_enabled = Instrument.control(
-            "sta la", "la %s",
-            """ Status of the laser diode driver.
-                This can be True if the laser is on or False otherwise""",
-            validator=strict_discrete_set,
-            values=[True, False],
-            get_process=lambda s: True if s == 'ON' else False,
-            set_process=lambda v: "on" if v else "off",
+        "sta la", "la %s",
+        """Control emission status of the laser diode driver (bool).
+
+        .. deprecated:: 0.12 Use attr:`emission` instead.
+        """,
+        validator=strict_discrete_set,
+        values=[True, False],
+        get_process=lambda s: True if s == 'ON' else False,
+        set_process=lambda v: "on" if v else "off",
+        check_set_errors=True,
+        command_process=_deprecation_warning(
+            "Property `laser_enabled` is deprecated, use `emission` instead."),
     )
 
     channel1_enabled = Instrument.control(
-            "sta ch 1", "%s",
-            """ Status of Channel 1 of the laser.
-                This can be True if the laser is on or False otherwise""",
-            validator=strict_discrete_set,
-            values=[True, False],
-            get_process=lambda s: True if s == 'ON' else False,
-            set_process=lambda v: "en 1" if v else "di 1",
+        "sta ch 1", "%s",
+        """Control status of Channel 1 of the laser (bool).
+
+        .. deprecated:: 0.12 Use :attr:`ch_1.enabled` instead.
+        """,
+        validator=strict_discrete_set,
+        values=[True, False],
+        get_process=lambda s: True if s == 'ON' else False,
+        set_process=lambda v: "en 1" if v else "di 1",
+        check_set_errors=True,
+        command_process=_deprecation_warning_channels("channel1_enabled"),
     )
 
     channel2_enabled = Instrument.control(
-            "sta ch 2", "%s",
-            """ Status of Channel 2 of the laser.
-                This can be True if the laser is on or False otherwise""",
-            validator=strict_discrete_set,
-            values=[True, False],
-            get_process=lambda s: True if s == 'ON' else False,
-            set_process=lambda v: "en 2" if v else "di 2",
+        "sta ch 2", "%s",
+        """Control status of Channel 2 of the laser (bool).
+
+        .. deprecated:: 0.12 Use :attr:`ch_2.enabled` instead.""",
+        validator=strict_discrete_set,
+        values=[True, False],
+        get_process=lambda s: True if s == 'ON' else False,
+        set_process=lambda v: "en 2" if v else "di 2",
+        check_set_errors=True,
+        command_process=_deprecation_warning_channels("channel2_enabled"),
     )
 
     power = Instrument.control(
-            "sh pow", "ch pow %f mic",
-            """ Actual output power in uW of the laser system. In pulse mode
-            this means that the set value might not correspond to the readback
-            one.""",
-            validator=strict_range,
-            values=[0, 200000],
+        "sh pow", "ch pow %f mic",
+        """Control actual output power in µW of the laser system. In pulse mode
+        this means that the set value might not correspond to the readback
+        one (float up to 200000).""",
+        validator=strict_range,
+        values=[0, 200000],
+        check_set_errors=True,
     )
 
     def enable_continous(self):
-        """ enable countinous emmission mode """
+        """Enable countinous emmission mode."""
         self.write('di ext')
-        self.laser_enabled = True
-        self.channel2_enabled = True
+        self.check_errors()
+        self.emission = True
+        self.ch_2.enabled = True
 
     def enable_pulsing(self):
-        """ enable pulsing mode. The optical output is controlled by a digital
-        input signal on a dedicated connnector on the device """
-        self.laser_enabled = True
-        self.channel2_enabled = True
+        """Enable pulsing mode.
+
+        The optical output is controlled by a digital
+        input signal on a dedicated connnector on the device."""
+        self.emission = True
+        self.ch_2.enabled = True
         self.write('en ext')
+        self.check_errors()
 
     def disable(self):
-        """ shutdown all laser operation """
-        self.write('di ext')
-        self.channel2_enabled = False
-        self.laser_enabled = False
+        """Shutdown all laser operation."""
+        self.write('di 0')
+        self.check_errors()
+        self.emission = False
+
+    def shutdown(self):
+        """Brings the instrument to a safe and stable state."""
+        self.disable()
+        super().shutdown()


Test patch:
diff --git a/tests/instruments/test_all_instruments.py b/tests/instruments/test_all_instruments.py
index 2b47398b..e5090b4a 100644
--- a/tests/instruments/test_all_instruments.py
+++ b/tests/instruments/test_all_instruments.py
@@ -50,7 +50,7 @@ for manufacturer in dir(instruments):
                 devices.append(d)
 
 # Instruments unable to accept an Adapter instance.
-proper_adapters = ["IBeamSmart", "ANC300Controller"]
+proper_adapters = ["ANC300Controller"]
 # Instruments with communication in their __init__, which consequently fails.
 need_init_communication = [
     "ThorlabsPM100USB",
@@ -61,6 +61,7 @@ need_init_communication = [
     "AWG401x_AFG",
     "VARX",
     "HP8116A",
+    "IBeamSmart",
 ]
 
 
diff --git a/tests/instruments/toptica/test_ibeamsmart.py b/tests/instruments/toptica/test_ibeamsmart.py
new file mode 100644
index 00000000..fe73a381
--- /dev/null
+++ b/tests/instruments/toptica/test_ibeamsmart.py
@@ -0,0 +1,107 @@
+#
+# This file is part of the PyMeasure package.
+#
+# Copyright (c) 2013-2023 PyMeasure Developers
+#
+# Permission is hereby granted, free of charge, to any person obtaining a copy
+# of this software and associated documentation files (the "Software"), to deal
+# in the Software without restriction, including without limitation the rights
+# to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+# copies of the Software, and to permit persons to whom the Software is
+# furnished to do so, subject to the following conditions:
+#
+# The above copyright notice and this permission notice shall be included in
+# all copies or substantial portions of the Software.
+#
+# THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+# IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+# FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+# AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+# LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+# OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+# THE SOFTWARE.
+#
+
+import pytest
+
+from pymeasure.test import expected_protocol
+
+
+from pymeasure.instruments.toptica import IBeamSmart
+
+# Note: This communication does not contain the first two device responses, as they are
+# ignored due to `adapter.flush_read_buffer()`.
+# The device communication depends on previous commands and whether the device power cycled.
+init_comm = [("echo off", None), ("prom off", None), ("talk usual", ""), (None, "[OK]")]
+
+
+def test_init():
+    with expected_protocol(IBeamSmart,
+                           init_comm,
+                           ):
+        pass  # verify init
+
+
+def test_version():
+    with expected_protocol(
+            IBeamSmart,
+            init_comm + [("ver", ""), (None, "iBPs-001A01-05"), (None, "[OK]")],
+    ) as inst:
+        assert inst.version == "iBPs-001A01-05"
+
+
+def test_power():
+    with expected_protocol(
+            IBeamSmart,
+            init_comm + [("sh pow", ""), (None, "PIC  = 000001 uW  "), (None, "[OK]")],
+    ) as inst:
+        assert inst.power == 1
+
+
+def test_disable_emission():
+    with expected_protocol(
+            IBeamSmart,
+            init_comm + [("la off", ""), (None, "[OK]")],
+    ) as inst:
+        inst.emission = False
+
+
+def test_setting_failed():
+    with expected_protocol(
+            IBeamSmart,
+            init_comm + [("la off", ""), (None, "abc"), (None, "[OK]")],
+    ) as inst:
+        with pytest.raises(ValueError):
+            inst.laser_enabled = False
+
+
+def test_enable_channel():
+    with expected_protocol(
+            IBeamSmart,
+            init_comm + [("en 3", ""), (None, "[OK]"),
+                         ("sta ch 3", ""), (None, "ON"), (None, "[OK]"),
+                         ("di 2", ""), (None, "[OK]"),
+                         ("sta ch 2", ""), (None, "OFF"), (None, "[OK]"),
+                         ]
+    ) as inst:
+        inst.ch_3.enabled = True
+        assert inst.ch_3.enabled is True
+        inst.ch_2.enabled = False
+        assert inst.ch_2.enabled is False
+
+
+def test_channel_power():
+    with expected_protocol(
+            IBeamSmart,
+            init_comm + [("ch 2 pow 100.000000 mic", ""), (None, "[OK]")]
+    ) as inst:
+        inst.ch_2.power = 100
+
+
+def test_shutdown():
+    with expected_protocol(
+            IBeamSmart,
+            init_comm + [("di 0", ""), (None, "[OK]"), ("la off", ""), (None, "[OK]")]
+    ) as inst:
+        inst.shutdown()
+        assert inst.isShutdown is True