Have Device.write accept a full byte sequence, not just a single byte
#79
Conversation
Codecov Report
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more @@ Coverage Diff @@
## main #79 +/- ##
==========================================
+ Coverage 82.88% 84.80% +1.91%
==========================================
Files 16 18 +2
Lines 1011 1033 +22
Branches 153 152 -1
==========================================
+ Hits 838 876 +38
+ Misses 135 120 -15
+ Partials 38 37 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
| # Can't figure this one out. Getting ValueError: bytes must in in range(0, 256) | ||
| # If I knew more about the purpose of `iter_bytes` then maybe I could | ||
| # reconcile it, but I don't have time to investigate right now. | ||
| pytest.param( | ||
| b"1234", | ||
| 3, | ||
| True, | ||
| b"1234", | ||
| marks=pytest.mark.xfail( | ||
| reason=( | ||
| "ValueError bytes must be in range(0, 256). TODO: figure" | ||
| " out correct 'want' value." | ||
| ) | ||
| ), | ||
| ), |
There was a problem hiding this comment.
Thanks for uncovering a bug !
iter_bytes is meant to clip values to the right number of bits (since serial may use from 5 to 8 bits). When send_end is True the highest data bits should only be set when the message is over. The problem here is that ~ gives us a signed binary number. So basically the function is bugged (and it is annoying since it is duplicated in pyvisa-py).
To make it less error prone it is we could refactor it to take the number of data bits and send_end. That way we could write out the mask directly. Can you make the change or do you prefer me to take care of it ?
There was a problem hiding this comment.
I'll have to take a deeper dive to make sure I grok it. I've opened #80 to track, but I don't think that fixing the bug should block this PR.
I'll take a stab at it when I can. If I can't figure it out I'll bounce it back to you 😆
|
@MatthieuDartiailh is there anything blocking this from getting merged? |
|
I am just slow. Just so that it does not get lost could you add a change log entry and I will merge ? By curiosity, will you be able to address #80 ? |
|
No worries! I completely understand. And yes I expect to be able to take a look at #80. |
| Unreleased | ||
| ---------- |
There was a problem hiding this comment.
Changelog updated. Not sure how you like to run things. I typically build out an "Unreleased" section during WIP and then rename the section to the release version.
LMK if you want something else done.
Fixes #72.
While working on this, I noticed that this block
pyvisa-sim/pyvisa_sim/sessions/serial.py
Lines 84 to 88 in 2aab58b
was not touched by unit tests. So I added a test for it, as the proposed change to
device.Device.writewill impact that code.And then while creating the test for
serial.py, I found thatcommon.iter_byteswas also not covered by tests, so I made a test case for that.I also add a test to ensure that logging is recording the full string, not just 1 character at a time.
Now that all impacted code is covered by tests, I implemented the minor change to
device.Device.write,session.MessageBasedSession.writeandserial.SerialInstrumentSession.writeto fix the logging issue.