Fix `common.iter_bytes` #81

dougthor42 · 2023-04-15T22:28:46Z

Fixes #80.

See that Issue for more detail.

Adjust iter_bytes function signature to use data_bits instead of mask. Calculate the mask base on those number of bits.
Update docstring
Update tests

codecov-commenter · 2023-04-15T22:31:54Z

Codecov Report

Merging #81 (7644d02) into main (dccb9ba) will increase coverage by 0.17%.
The diff coverage is 95.23%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##             main      #81      +/-   ##
==========================================
+ Coverage   84.80%   84.97%   +0.17%     
==========================================
  Files          18       18              
  Lines        1033     1058      +25     
  Branches      152      160       +8     
==========================================
+ Hits          876      899      +23     
- Misses        120      121       +1     
- Partials       37       38       +1

Flag	Coverage Δ
unittests	`84.97% <95.23%> (+0.17%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pyvisa_sim/testsuite/test_serial.py	`100.00% <ø> (ø)`
pyvisa_sim/common.py	`91.66% <92.85%> (-2.78%)`	⬇️
pyvisa_sim/sessions/serial.py	`68.75% <100.00%> (ø)`
pyvisa_sim/testsuite/test_common.py	`100.00% <100.00%> (ø)`

MatthieuDartiailh · 2023-04-27T18:21:41Z

I have a slightly different understanding of send_end.

The way I read the specs if we have 7 data bits and send_end is True, then the bit 7 should be true only for the last bytes. In your case it seems you encode send_end in an extra bit compared to the number of allowed data bits.

I checked on my phone (little time this days) so I may have missed something.

dougthor42 · 2023-04-30T21:34:55Z

Hmm... So you're interpreting "transmit the last character with the eighth bit set" as "8th bit == 1", not "8th bit == whatever value it would have been for that character"

Like this? (7 data bits)

Input	`send_end=True`	`send_end=False`
N/A	7th bit set to 1 for last byte no matter what the original value was. 8th bit always removed b/c data_bits=7.	7th bit set to 0 for last byte no matter what the original value was. 8th bit always removed b/c data_bits=7.
`0b1111_1111`	`0b0111_1111`	`0b0011_1111`
`0b0000_1111`	`0b0100_1111`	`0b0000_1111`
`0b1111_1100`, `0b1011_1100`, `0b1011_1100`	`0b0111_1100`, `0b0011_1100`, `0b0111_1100`	`0b0111_1100`, `0b0011_1100`, `0b0011_1100`

Do I have that correct?

(The below is for my own future reference, emphasis and additions mine)

If it is set to VI_ASRL_END_LAST_BIT (pyvisa.constants.SerialTermination.last_bit), and VI_ATTR_SEND_END_EN (pyvisa.constants.ResourceAttribute.send_end_enabled which is the send_end arg) is set to VI_TRUE (True), the write will send all but the last character with the highest bit clear, then transmit the last character with the highest bit set. For example, if VI_ATTR_ASRL_DATA_BITS is set to 8, the write will clear the eighth bit for all but the last character, then transmit the last character with the eighth bit set. If VI_ATTR_SEND_END_EN is set to VI_FALSE, the write will send all the characters with the highest bit clear.

dougthor42 · 2023-04-30T21:37:23Z

pyvisa_sim/common.py

+    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)
+    send_end : If True, send the final byte unclipped (with all 8 bits).


-(with all 8 bits) +(with all `data_bits` bits)

Pending clarification of #81 (comment) - might be different.

MatthieuDartiailh · 2023-05-01T12:57:34Z

Rereading https://www.ni.com/docs/en-US/bundle/ni-visa/page/ni-visa/vi_attr_asrl_end_out.html, I realize I missed one case which calls for taking bool | None as argument:

None: serial termination does not depend on send end, so we simply apply the mask
False: serial termination depends on send end and send end enabled is False, we always clear (set to zero) the highest of the data bits.
True: serial termination depends on send end and send end is enabled, we clear the highest of the data bits for all bytes except the last for which it is set to 1.

Does that make sense ?

dougthor42 · 2023-05-06T22:52:48Z

Yeah I think so. Check out the new test suite and LMK.

MatthieuDartiailh

It looks almost right but I have one small concern for the non terminal bytes when send_end is not None.

pyvisa_sim/common.py

MatthieuDartiailh · 2023-05-09T07:11:30Z

pyvisa_sim/common.py

+        mask = _create_bitmask(data_bits)
+
+        # Send everything but the last byte with the mask applied.
        for d in data[:-1]:
-            yield bytes([d & ~mask])
+            yield bytes([d & mask])
+
+        last_byte = data[-1]

-        if send_end:
-            yield bytes([data[-1] | ~mask])
+        # Send the last byte adjusted by `send_end`
+        if send_end is None:
+            # only apply the mask
+            yield bytes([last_byte & mask])
+        elif bool(send_end) is True:
+            # apply the mask and set highest of data_bits to 1
+            highest_bit = 1 << (data_bits - 1)
+            yield bytes([(last_byte & mask) | highest_bit])
+        elif bool(send_end) is False:
+            # apply the mask and set highest of data_bits to 0
+            # This is effectively the same has reducing the mask by 1 bit.
+            new_mask = _create_bitmask(data_bits - 1)
+            yield bytes([last_byte & new_mask])
        else:
-            yield bytes([data[-1] & ~mask])


If send_end is not None the highest bit should always be cleared since it has a special meaning. The easiest way to do it would make sense to adjust the value of data_bits.

MatthieuDartiailh · 2023-05-09T07:13:09Z

pyvisa_sim/testsuite/test_common.py

+        (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


Those cases need to cover more than a single byte.

See L54-L59 - those cases test multiple bytes.

Yes and they do expose the issue about clearing the highest bit I mentioned in another comment.

MatthieuDartiailh · 2023-05-10T07:03:39Z

pyvisa_sim/testsuite/test_common.py

+        (b"\x6d\x5c\x25\x25", 4, None, b"\r\x0c\x05\x05"),
+        (b"\x6d\x5c\x25\x25", 4, False, b"\r\x0c\x05\x05"),
+        (b"\x6d\x5c\x25\x25", 4, True, b"\r\x0c\x05\x0d"),


The fact that the case for send_end = None match the other two tells me that the highest bit is not cleared as I would expect for a non-None send_end value.

dougthor42

Holy crap, I have no idea why that was so hard for me to grok. But I think (I hope 🤣) I've got it.

pyvisa_sim/common.py

MatthieuDartiailh

One last inconsistency I noticed (but not sure how to test for it) otherwise LGTM

MatthieuDartiailh · 2023-05-14T08:48:03Z

pyvisa_sim/common.py

-    if mask is None:
+
+def iter_bytes(
+    data: bytes, data_bits: Optional[int] = None, send_end: Optional[bool] = False


I think None would make for a more natural default for send_end.

MatthieuDartiailh · 2023-05-15T07:54:53Z

pyvisa_sim/sessions/serial.py

+            data_bits, _ = self.get_attribute(
+                constants.ResourceAttribute.asrl_data_bits
+            )
+            val = b"".join(common.iter_bytes(data, data_bits, send_end))
            self.device.write(val)
        else:
            self.device.write(data)


The fact that this is not affected by asrl_data_bits looks awfully wrong to me, we should go through iter_bytes with a send_end of None.

dougthor42

Sorry for the delay. Both comments addressed.

MatthieuDartiailh · 2023-05-29T22:26:25Z

Thanks !

Could you make a similar PR againt PyVISA-py (I have no good solution to avoid the duplication) ?

dougthor42 added 3 commits April 15, 2023 15:00

Fix iter_bytes implementation

8d0fdcf

Update serial.write for change to iter_bytes

1666643

Update changelog

ef8b3b9

dougthor42 commented Apr 30, 2023

View reviewed changes

dougthor42 added 2 commits May 6, 2023 15:51

Update based on new interpretation of spec

0799f19

Revert a change to send_end in test_serial

7b83721

MatthieuDartiailh requested changes May 9, 2023

View reviewed changes

MatthieuDartiailh reviewed May 10, 2023

View reviewed changes

dougthor42 added 3 commits May 11, 2023 14:47

Add missing blank line at end of docstring

61c2baf

Update tests based on new understanding

62bd65b

Update iter_bytes

d667560

dougthor42 commented May 11, 2023

View reviewed changes

pyvisa_sim/common.py Show resolved Hide resolved

dougthor42 requested a review from MatthieuDartiailh May 11, 2023 22:57

MatthieuDartiailh requested changes May 15, 2023

View reviewed changes

dougthor42 added 2 commits May 29, 2023 14:38

default send_end to None

84a5f15

Pipe asrl_end != last_bit through iter_bytes with send_end=None

7644d02

dougthor42 requested a review from MatthieuDartiailh May 29, 2023 21:44

dougthor42 commented May 29, 2023

View reviewed changes

MatthieuDartiailh approved these changes May 29, 2023

View reviewed changes

MatthieuDartiailh merged commit 17323c6 into pyvisa:main May 29, 2023

dougthor42 mentioned this pull request May 30, 2023

Port pyvisa-sim iter_bytes changes for serial communication pyvisa/pyvisa-py#374

Closed

dougthor42 mentioned this pull request Aug 6, 2023

Update iter_bytes to be the same as what's used in pyvisa-sim pyvisa/pyvisa-py#389

Merged

5 tasks

dougthor42 deleted the fix-iter_bytes branch May 11, 2024 20:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `common.iter_bytes` #81

Fix `common.iter_bytes` #81

dougthor42 commented Apr 15, 2023

codecov-commenter commented Apr 15, 2023 •

edited

Loading

MatthieuDartiailh commented Apr 27, 2023

dougthor42 commented Apr 30, 2023

dougthor42 Apr 30, 2023

MatthieuDartiailh commented May 1, 2023

dougthor42 commented May 6, 2023

MatthieuDartiailh left a comment

MatthieuDartiailh May 9, 2023

MatthieuDartiailh May 9, 2023

dougthor42 May 9, 2023

MatthieuDartiailh May 10, 2023

MatthieuDartiailh May 10, 2023

dougthor42 left a comment

MatthieuDartiailh left a comment

MatthieuDartiailh May 14, 2023

MatthieuDartiailh May 15, 2023

dougthor42 left a comment

MatthieuDartiailh commented May 29, 2023

Fix common.iter_bytes #81

Fix common.iter_bytes #81

Conversation

dougthor42 commented Apr 15, 2023

codecov-commenter commented Apr 15, 2023 • edited Loading

Codecov Report

MatthieuDartiailh commented Apr 27, 2023

dougthor42 commented Apr 30, 2023

Choose a reason for hiding this comment

MatthieuDartiailh commented May 1, 2023

dougthor42 commented May 6, 2023

MatthieuDartiailh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dougthor42 left a comment

Choose a reason for hiding this comment

MatthieuDartiailh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dougthor42 left a comment

Choose a reason for hiding this comment

MatthieuDartiailh commented May 29, 2023

Fix `common.iter_bytes` #81

Fix `common.iter_bytes` #81

codecov-commenter commented Apr 15, 2023 •

edited

Loading