Repository: pyvisa/pyvisa-py

Issue:
Port pyvisa-sim `iter_bytes` changes for serial communication
https://github.com/pyvisa/pyvisa-sim/pull/81#issuecomment-1567565412

/assign @dougthor42

Discussion:
dougthor42: Updated, PTAL
codecov[bot]: ## [Codecov](https://app.codecov.io/gh/pyvisa/pyvisa-py/pull/389?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pyvisa) Report
Attention: `9 lines` in your changes are missing coverage. Please review.
> Comparison is base [(`ea7fa43`)](https://app.codecov.io/gh/pyvisa/pyvisa-py/commit/ea7fa43edabcca496016c72e3e66209fa442144b?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pyvisa) 23.14% compared to head [(`423e274`)](https://app.codecov.io/gh/pyvisa/pyvisa-py/pull/389?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pyvisa) 24.22%.

<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##             main     #389      +/-   ##
==========================================
+ Coverage   23.14%   24.22%   +1.07%     
==========================================
  Files          23       24       +1     
  Lines        3318     3356      +38     
  Branches      458      469      +11     
==========================================
+ Hits          768      813      +45     
+ Misses       2532     2525       -7     
  Partials       18       18              
```

| [Flag](https://app.codecov.io/gh/pyvisa/pyvisa-py/pull/389/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pyvisa) | Coverage Δ | |
|---|---|---|
| [unittests](https://app.codecov.io/gh/pyvisa/pyvisa-py/pull/389/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pyvisa) | `24.22% <83.33%> (+1.07%)` | :arrow_up: |

Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pyvisa#carryforward-flags-in-the-pull-request-comment) to find out more.

| [Files](https://app.codecov.io/gh/pyvisa/pyvisa-py/pull/389?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pyvisa) | Coverage Δ | |
|---|---|---|
| [pyvisa\_py/testsuite/test\_common.py](https://app.codecov.io/gh/pyvisa/pyvisa-py/pull/389?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pyvisa#diff-cHl2aXNhX3B5L3Rlc3RzdWl0ZS90ZXN0X2NvbW1vbi5weQ==) | `100.00% <100.00%> (ø)` | |
| [pyvisa\_py/common.py](https://app.codecov.io/gh/pyvisa/pyvisa-py/pull/389?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pyvisa#diff-cHl2aXNhX3B5L2NvbW1vbi5weQ==) | `88.09% <93.33%> (+13.09%)` | :arrow_up: |
| [pyvisa\_py/serial.py](https://app.codecov.io/gh/pyvisa/pyvisa-py/pull/389?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pyvisa#diff-cHl2aXNhX3B5L3NlcmlhbC5weQ==) | `5.97% <0.00%> (+0.25%)` | :arrow_up: |

... and [1 file with indirect coverage changes](https://app.codecov.io/gh/pyvisa/pyvisa-py/pull/389/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pyvisa)


</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/pyvisa/pyvisa-py/pull/389?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pyvisa).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pyvisa).

MatthieuDartiailh: Isort and flake8 are unhappy. CI do not run automatically because you are a first time contributor.

Patch:
diff --git a/CHANGES b/CHANGES
index 6a454f7..8494603 100644
--- a/CHANGES
+++ b/CHANGES
@@ -7,6 +7,8 @@ PyVISA-py Changelog
 - addd URL-support to ASLR devices PR #386
 - add support for GPIB secondary addresses
 - fix missing sock.close() in rpc _connect()
+- Adjusted how `iter_bytes` works to be more accurate to the VISA spec and removed
+  it from the `serial` module (it can still be found in `common`)
 - fix HiSLIP message tracking after read timeout PR #376
 - handle read_termination of null in tcipip PR #394
 - fix tcpip keepalive PR #396
diff --git a/pyvisa_py/common.py b/pyvisa_py/common.py
index 884ba63..478042a 100644
--- a/pyvisa_py/common.py
+++ b/pyvisa_py/common.py
@@ -6,6 +6,7 @@
 
 """
 import logging
+from typing import Iterator, Optional
 
 from pyvisa import logger
 
@@ -28,3 +29,75 @@ class NamedObject(object):
 
 
 int_to_byte = lambda val: val.to_bytes(1, "big")
+
+
+# TODO(anyone): This is copypasta from `pyvisa-sim` project - find a way to
+#   reduce duplication, probably in that project instead of here.
+def _create_bitmask(bits: int) -> int:
+    """Create a bitmask for the given number of bits."""
+    mask = (1 << bits) - 1
+    return mask
+
+
+# TODO(anyone): This is copypasta from `pyvisa-sim` project - find a way to
+#   reduce duplication, probably in that project instead of here.
+def iter_bytes(
+    data: bytes, data_bits: Optional[int] = None, send_end: Optional[bool] = None
+) -> Iterator[bytes]:
+    """Clip values to the correct number of bits per byte.
+    Serial communication may use from 5 to 8 bits.
+    Parameters
+    ----------
+    data : The data to clip as a byte string.
+    data_bits : How many bits per byte should be sent. Clip to this many bits.
+        For example: data_bits=5: 0xff (0b1111_1111) --> 0x1f (0b0001_1111).
+        Acceptable range is 5 to 8, inclusive. Values above 8 will be clipped to 8.
+        This maps to the VISA attribute VI_ATTR_ASRL_DATA_BITS.
+    send_end :
+        If None (the default), apply the mask that is determined by data_bits.
+        If False, apply the mask and set the highest (post-mask) bit to 0 for
+        all bytes.
+        If True, apply the mask and set the highest (post-mask) bit to 0 for
+        all bytes except for the final byte, which has the highest bit set to 1.
+    References
+    ----------
+    + https://www.ivifoundation.org/downloads/Architecture%20Specifications/vpp43_2022-05-19.pdf,
+    + https://www.ni.com/docs/en-US/bundle/ni-visa/page/ni-visa/vi_attr_asrl_data_bits.html,
+    + https://www.ni.com/docs/en-US/bundle/ni-visa/page/ni-visa/vi_attr_asrl_end_out.html
+
+    """
+    if send_end and data_bits is None:
+        raise ValueError("'send_end' requires a valid 'data_bits' value.")
+
+    if data_bits is None:
+        for d in data:
+            yield bytes([d])
+    else:
+        if data_bits <= 0:
+            raise ValueError("'data_bits' cannot be zero or negative")
+        if data_bits > 8:
+            data_bits = 8
+
+        if send_end is None:
+            # only apply the mask
+            mask = _create_bitmask(data_bits)
+            for d in data:
+                yield bytes([d & mask])
+        elif bool(send_end) is False:
+            # apply the mask and set highest bits to 0
+            # This is effectively the same has reducing the mask by 1 bit.
+            mask = _create_bitmask(data_bits - 1)
+            for d in data:
+                yield bytes([d & mask])
+        elif bool(send_end) is True:
+            # apply the mask and set highest bits to 0
+            # This is effectively the same has reducing the mask by 1 bit.
+            mask = _create_bitmask(data_bits - 1)
+            for d in data[:-1]:
+                yield bytes([d & mask])
+            # except for the last byte which has it's highest bit set to 1.
+            last_byte = data[-1]
+            highest_bit = 1 << (data_bits - 1)
+            yield bytes([(last_byte & mask) | highest_bit])
+        else:
+            raise ValueError(f"Unknown 'send_end' value '{send_end}'")
diff --git a/pyvisa_py/serial.py b/pyvisa_py/serial.py
index f438cf3..3319026 100644
--- a/pyvisa_py/serial.py
+++ b/pyvisa_py/serial.py
@@ -7,7 +7,7 @@
 
 """
 import sys
-from typing import Any, List, Optional, Tuple
+from typing import Any, List, Tuple
 
 from pyvisa import attributes, constants, logger, rname
 from pyvisa.constants import (
@@ -34,24 +34,6 @@ except ImportError as e:
 IS_WIN = sys.platform == "win32"
 
 
-def iter_bytes(data: bytes, mask: Optional[int] = None, send_end: bool = False):
-    if send_end and mask is None:
-        raise ValueError("send_end requires a valid mask.")
-
-    if mask is None:
-        for d in data:
-            yield bytes([d])
-
-    else:
-        for d in data[:-1]:
-            yield bytes([d & ~mask])
-
-        if send_end:
-            yield bytes([data[-1] | ~mask])
-        else:
-            yield bytes([data[-1] & ~mask])
-
-
 def to_state(boolean_input: bool) -> constants.LineState:
     """Convert a boolean input into a LineState value."""
     if boolean_input:
@@ -184,20 +166,20 @@ class SerialSession(Session):
 
         """
         logger.debug("Serial.write %r" % data)
-        end_out, _ = self.get_attribute(ResourceAttribute.asrl_end_out)
         send_end, _ = self.get_attribute(ResourceAttribute.send_end_enabled)
+        end_out, _ = self.get_attribute(ResourceAttribute.asrl_end_out)
+        data_bits, _ = self.get_attribute(constants.ResourceAttribute.asrl_data_bits)
 
-        if end_out in (SerialTermination.none, SerialTermination.termination_break):
+        if end_out == SerialTermination.none:
             pass
         elif end_out == SerialTermination.last_bit:
-            last_bit, _ = self.get_attribute(ResourceAttribute.asrl_data_bits)
-            mask = 1 << (last_bit - 1)
-            data = bytes(iter_bytes(data, mask, send_end))
-
+            data = b"".join(common.iter_bytes(data, data_bits, send_end))
         elif end_out == SerialTermination.termination_char:
             term_char, _ = self.get_attribute(ResourceAttribute.termchar)
+            data = b"".join(common.iter_bytes(data, data_bits, send_end=None))
             data = data + common.int_to_byte(term_char)
-
+        elif end_out == SerialTermination.termination_break:
+            data = b"".join(common.iter_bytes(data, data_bits, send_end=None))
         else:
             raise ValueError("Unknown value for VI_ATTR_ASRL_END_OUT: %s" % end_out)
 


Test patch:
diff --git a/pyvisa_py/testsuite/test_common.py b/pyvisa_py/testsuite/test_common.py
new file mode 100644
index 0000000..a1f1b08
--- /dev/null
+++ b/pyvisa_py/testsuite/test_common.py
@@ -0,0 +1,102 @@
+from typing import List, Optional
+
+import pytest
+
+from pyvisa_py import common
+
+
+# TODO(anyone): This is copypasta from `pyvisa-sim` project - find a way to
+#   reduce duplication, probably in that project instead of here.
+@pytest.mark.parametrize(
+    "bits, want",
+    [
+        (0, 0b0),
+        (1, 0b1),
+        (5, 0b0001_1111),
+        (7, 0b0111_1111),
+        (8, 0b1111_1111),
+        (11, 0b0111_1111_1111),
+    ],
+)
+def test_create_bitmask(bits, want):
+    got = common._create_bitmask(bits)
+    assert got == want
+
+
+# TODO(anyone): This is copypasta from `pyvisa-sim` project - find a way to
+#   reduce duplication, probably in that project instead of here.
+@pytest.mark.parametrize(
+    "data, data_bits, send_end, want",
+    [
+        (b"\x01", None, False, b"\x01"),
+        (b"hello world!", None, False, b"hello world!"),
+        # Only apply the mask
+        (b"\x03", 2, None, b"\x03"),  # 0b0000_0011 --> 0b0000_0011
+        (b"\x04", 2, None, b"\x00"),  # 0b0000_0100 --> 0b0000_0000
+        (b"\xff", 5, None, b"\x1f"),  # 0b1111_1111 --> 0b0001_1111
+        (b"\xfe", 7, None, b"\x7e"),  # 0b1111_1110 --> 0b0111_1110
+        (b"\xfe", 8, None, b"\xfe"),  # 0b1111_1110 --> 0b1111_1110
+        (b"\xff", 9, None, b"\xff"),  # 0b1111_1111 --> 0b1111_1111
+        # Always set highest bit *of data_bits* to 0
+        (b"\x04", 2, False, b"\x00"),  # 0b0000_0100 --> 0b0000_0000
+        (b"\x04", 3, False, b"\x00"),  # 0b0000_0100 --> 0b0000_0000
+        (b"\x05", 3, False, b"\x01"),  # 0b0000_0101 --> 0b0000_0001
+        (b"\xff", 7, False, b"\x3f"),  # 0b1111_1111 --> 0b0011_1111
+        (b"\xff", 8, False, b"\x7f"),  # 0b1111_1111 --> 0b0111_1111
+        # Always set highest bit *of data_bits* to 1
+        (b"\x04", 2, True, b"\x02"),  # 0b0000_0100 --> 0b0000_0010
+        (b"\x04", 3, True, b"\x04"),  # 0b0000_0100 --> 0b0000_0100
+        (b"\x01", 3, True, b"\x05"),  # 0b0000_0001 --> 0b0000_0101
+        (b"\x9f", 7, True, b"\x5f"),  # 0b1001_1111 --> 0b0101_1111
+        (b"\x9f", 8, True, b"\x9f"),  # 0b1001_1111 --> 0b1001_1111
+        # data_bits >8 bits act like data_bits=8, as type(data) is "bytes"
+        # which is limited 8 bits per character.
+        (b"\xff", 9, None, b"\xff"),
+        (b"\xff", 9, False, b"\x7f"),
+        (b"\xff", 9, True, b"\xff"),
+        # send_end=None only applies the mask everywhere and doesn't touch the
+        # highest bit
+        # 0x6d: 0b0110_1101 (m) --> 0x0d: 0b0000_1101 (\r)
+        # 0x5e: 0b0101_1110 (^) --> 0x0e: 0b0000_1110
+        # 0x25: 0b0010_0101 (%) --> 0x05: 0b0000_0101
+        # 0x25: 0b0010_0101 (%) --> 0x05: 0b0000_0101
+        (b"\x6d\x5e\x25\x25", 4, None, b"\r\x0e\x05\x05"),
+        # send_end=False sets highest post-mask bit to 0 for all
+        # 0x6d: 0b0110_1101 (m) --> 0x05: 0b0000_0101
+        # 0x5e: 0b0101_1110 (^) --> 0x06: 0b0000_0110
+        # 0x25: 0b0010_0101 (%) --> 0x05: 0b0000_0101
+        # 0x25: 0b0010_0101 (%) --> 0x05: 0b0000_0101
+        (b"\x6d\x5e\x25\x25", 4, False, b"\x05\x06\x05\x05"),
+        # send_end=True sets highest bit to 0 except for final byte
+        # 0x6d: 0b0110_1101 (m) --> 0x05: 0b0000_0101
+        # 0x5e: 0b0101_1110 (^) --> 0x06: 0b0000_0110
+        # 0x25: 0b0010_0101 (%) --> 0x05: 0b0000_0101
+        # 0x25: 0b0010_0101 (%) --> 0x0d: 0b0000_1101
+        (b"\x6d\x5e\x25\x25", 4, True, b"\x05\x06\x05\x0d"),
+        # 0x61: 0b0110_0001 (a) --> 0x21: 0b0010_0001 (!)
+        # 0xb1: 0b1011_0001 (±) --> 0x31: 0b0011_0001 (1)
+        (b"a\xb1", 6, None, b"\x21\x31"),
+        # 0x61: 0b0110_0001 (a) --> 0x01: 0b0000_0001
+        # 0xb1: 0b1011_0001 (±) --> 0x11: 0b0001_0001
+        (b"a\xb1", 6, False, b"\x01\x11"),
+        # 0x61: 0b0110_0001 (a) --> 0x01: 0b0000_0001
+        # 0xb1: 0b1011_0001 (±) --> 0x31: 0b0011_0001 (1)
+        (b"a\xb1", 6, True, b"\x011"),
+    ],
+)
+def test_iter_bytes(
+    data: bytes, data_bits: Optional[int], send_end: bool, want: List[bytes]
+) -> None:
+    got = b"".join(common.iter_bytes(data, data_bits=data_bits, send_end=send_end))
+    assert got == want
+
+
+def test_iter_bytes_with_send_end_requires_data_bits() -> None:
+    with pytest.raises(ValueError):
+        # Need to wrap in list otherwise the iterator is never called.
+        list(common.iter_bytes(b"", data_bits=None, send_end=True))
+
+
+def test_iter_bytes_raises_on_bad_data_bits() -> None:
+    with pytest.raises(ValueError):
+        list(common.iter_bytes(b"", data_bits=0, send_end=None))