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

Fix to use endpoints MaxPacketSize instead of hardcoded RECV_CHUNK size #417

Merged
merged 1 commit into from
Mar 6, 2024

Conversation

sessl3r
Copy link
Contributor

@sessl3r sessl3r commented Mar 6, 2024

The issue was found to be not able to download screenshots from a RigolDS1074Z.
From this issue it was seen there is some workaround but not a fix.

As the same issue existed in the linux-usbtmc driver module it was easy to refix it here.

The main point is that those scopes have a limited MaxPacketSize which is reported by the USB device correctly and the TMC protocol is relying on using this MaxPacketSize.

Copy link

codecov bot commented Mar 6, 2024

Codecov Report

Attention: Patch coverage is 0% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 24.92%. Comparing base (13bda1d) to head (6bcbaa3).

❗ Current head 6bcbaa3 differs from pull request most recent head 6b3c851. Consider uploading reports for the commit 6b3c851 to get more accurate results

Files Patch % Lines
pyvisa_py/protocols/usbtmc.py 0.00% 5 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main     #417   +/-   ##
=======================================
  Coverage   24.91%   24.92%           
=======================================
  Files          25       25           
  Lines        3516     3515    -1     
  Branches      489      489           
=======================================
  Hits          876      876           
+ Misses       2623     2622    -1     
  Partials       17       17           
Flag Coverage Δ
unittests 24.92% <0.00%> (+<0.01%) ⬆️

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.

@MatthieuDartiailh
Copy link
Member

Wow a fix that helps with Rigol devices is definitively welcome ! The formatting failure is unrelated to your changes so do not worry about it. Could you add a changelog entry ? Once it is done I will merge.

@Fellypao it may be worth checking if this fixes your issue.

This was found using a Rigol DS1074Z scope which has a limited MaxPacketSize of 64.
@sessl3r
Copy link
Contributor Author

sessl3r commented Mar 6, 2024

added changelog (hope 0.7.2 is ok for the version)
at least on my ds1054z the :display:data? on,off,png now works without monkeypatching pyvisa_py - hope also for others :)

@MatthieuDartiailh
Copy link
Member

Perfect ! Thanks a ton !

@MatthieuDartiailh MatthieuDartiailh merged commit 8e8c1a1 into pyvisa:main Mar 6, 2024
@Fellypao
Copy link

Fellypao commented Mar 6, 2024

@Fellypao it may be worth checking if this fixes your issue.

@MatthieuDartiailh Yes, this definitely fixes my issue (#414). Thank you. And many thanks to @sessl3r 👍

@MatthieuDartiailh
Copy link
Member

Amazing !!! I will try to cut a release ASAP

@sessl3r
Copy link
Contributor Author

sessl3r commented Mar 13, 2024

Only as a little late FYI: From the usbtmc PR I got the info that some Agilent scopes will suffer performance from this as they report a smaller MaxPacketSize then they can do (1k vs 4k).
I totally dislike the way usbtmc has solved this - however eventually - if someone reports problems a parameter could be added to overwrite it.

@MatthieuDartiailh
Copy link
Member

Could you open an issue so that the information is less likely to be lost ?

Honestly I take the win of people having Rigol scope working out of the box over maximal performance for Agilent scope.

@MatthieuDartiailh
Copy link
Member

Thanks @sessl3r

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.

3 participants