Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use fewest transfers possible in read_binary_values #874

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

aWZHY0yQH81uOYvH
Copy link

@aWZHY0yQH81uOYvH aWZHY0yQH81uOYvH commented Nov 30, 2024

See pyvisa/pyvisa-py#472 for more details. Essentially, I have an instrument that will lose data when too many small reads are issued. This PR makes it so read_binary_values, when not passed an explicit chunk_size, uses one transfer of default length to read the data header, then if there is more data to read, it reads it all in one more transfer. This should be more efficient and solves a problem with my instrument. Behavior for users that specify chunk_size is unaffected.

In order for this to work with the pyvisa-py backend, pyvisa/pyvisa-py#470 needs to be merged since it currently further breaks up reads.

  • Executed ruff check . && ruff format -c . --check with no errors
  • The change is fully covered by automated unit tests
  • Documented in docs/as appropriate
  • Added an entry to the CHANGES file

Copy link

codecov bot commented Nov 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.54%. Comparing base (2c8677f) to head (0bbbf1c).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #874   +/-   ##
=======================================
  Coverage   79.54%   79.54%           
=======================================
  Files          27       27           
  Lines        5378     5378           
  Branches      335      335           
=======================================
  Hits         4278     4278           
  Misses       1077     1077           
  Partials       23       23           
Flag Coverage Δ
unittests 79.45% <ø> (ø)

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@MatthieuDartiailh MatthieuDartiailh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Could you update the release notes with the PR number ? Also could you add a note in the docs about the new behavior ?

CHANGES Outdated Show resolved Hide resolved
@aWZHY0yQH81uOYvH
Copy link
Author

Sorry, didn't notice the other items in the changelog had PR numbers. I did update the docstring for the read_binary_values function to describe the new default behavior, which shows up in the docs. Would you like it mentioned somewhere else also?

@MatthieuDartiailh
Copy link
Member

You need to adjust the section Reading binary values in docs/source/introduction/rvalues.rst

@aWZHY0yQH81uOYvH
Copy link
Author

Done.

@MatthieuDartiailh
Copy link
Member

Thanks, it looks good. I am wondering if this could break when transferring very long responses in which case it may be better to cap the maximum chunck_size. I will try to test either tomorrow or next week before merging.

@aWZHY0yQH81uOYvH
Copy link
Author

aWZHY0yQH81uOYvH commented Dec 6, 2024

I wonder that as well.

Looks like the length field in the USBTMC header is a 4-byte value (up to 4GiB), while the IEEE header represents the length with up to 9 decimal digits (up to 999MB), so it should be impossible to overflow the length field of the USB header. Similarly, looks like the HP header is a 2-byte length value, so that should also be fine.

I have tested this with 1MiB transfers, but I don't have other equipment to test this with, so further testing would be great to see if there are issues below these limits.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants