Skip to content

Modify GPIB code to use symbolic constants#601

Merged
MatthieuDartiailh merged 17 commits into
pyvisa:mainfrom
DavidCPlatt:gpib-enums-clean
Jul 2, 2026
Merged

Modify GPIB code to use symbolic constants#601
MatthieuDartiailh merged 17 commits into
pyvisa:mainfrom
DavidCPlatt:gpib-enums-clean

Conversation

@DavidCPlatt

@DavidCPlatt DavidCPlatt commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

Create a file of symbolic constants (IntEnum classes) for the various libgpib interface calls. Use these symbols rather than hard-coded integers. Remove
now-extraneous commentary about what the hard-coded integers mean.

The new file gpib_constants.py is derived from the constants file in gpib_ctypes (which isn't a strict dependency of this project). I've reorganized the values as an individual IntEnum class per API or purpose, with the class name based on the API or purpose, and the member names matching those used in gpib_ctypes. This provides a clean visual match between the ib*() call, class name, and member name each time one of the symbols is used, and should help prevent usage mismatches. In the gpib.py code I modified, I have removed the old commentary which explained the intent of each bare integer as being unnecessary.

This change deserves careful eyeball checking before being merged, to make sure I've gotten the translations between bare-integer and symbol correct in every case. I've reviewed it all two or three times, but you know how much author code reviews are worth! :-)

  • Closes # (insert issue number if relevant)
  • Executed black . && isort -c . && flake8 with no errors
  • The change is fully covered by automated unit tests
  • Documented in docs/ as appropriate
  • Added an entry to the CHANGES file

DavidCPlatt and others added 4 commits June 26, 2026 12:24
Create a file of symbolic constants (IntEnum classes)
for the various libgpib interface calls.  Use these
symbols rather than hard-coded integers.  Remove
now-extraneous commentary about what the hard-coded
integers mean.
@codecov

codecov Bot commented Jun 26, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 76.96078% with 47 lines in your changes missing coverage. Please review.
✅ Project coverage is 41.03%. Comparing base (28b0e88) to head (dd28298).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
pyvisa_py/gpib.py 2.08% 47 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #601      +/-   ##
==========================================
+ Coverage   39.18%   41.03%   +1.84%     
==========================================
  Files          27       28       +1     
  Lines        5022     5176     +154     
  Branches      518      517       -1     
==========================================
+ Hits         1968     2124     +156     
+ Misses       3028     3026       -2     
  Partials       26       26              
Flag Coverage Δ
unittests 41.03% <76.96%> (+1.84%) ⬆️

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

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@MatthieuDartiailh MatthieuDartiailh left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks fine to me but I will do another pass later.

Comment thread pyvisa_py/gpib.py Outdated
if ifc.ask(2):
ifc.config(2, attribute_state + 96)
if ifc.ask(gpib_constants.ask.IbaSAD):
ifc.config(gpib_constants.config.IbcSAD, attribute_state + 96)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I wish my comment about the 96 was more explicit...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ach. I didn't even consider those NI-specific SAD masks/constants.

I'll add another commit to the PR for these. Shouldn't take long.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Things being as they are, these days, I'm not absolutely certain that saying "96 86'ed" is entirely safe, but what the hell. Done.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hah! Just realized there was an off-by-one error in the old code - range(96,126) should have been range(96,127). As it was, subaddress 30 wouldn't have been iterated. Fixed now.

DavidCPlatt and others added 4 commits June 26, 2026 15:10
Turn 0, 96, and 126 into symbolic constants for the SAD.
The range() operator which allows iteration through the allowed
GPIB sub-addresses (0 through 30, mapped up to 96 through 126)
needs to have its stop value set to 127 (LAST_SAD + 1) rather
than 126 (LAST_SAD).  Otherwise it will look for subaddresses 0
through 29, and not see a device at subaddress 30.

@MatthieuDartiailh MatthieuDartiailh left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM (minus the changelog formatting)

Comment thread CHANGES
Comment thread pyvisa_py/gpib.py
Co-authored-by: Matthieu Dartiailh <marul@laposte.net>
@MatthieuDartiailh

Copy link
Copy Markdown
Member

Can you address the linting/typin failure ? (mostly a bad import I think)

DavidCPlatt and others added 7 commits July 1, 2026 16:10
Oops... missed one set of cases where I had initially used GPIBwhatever
and now (with imported symbols) need to use gpib_constants.whatever

Also, sort the gpib_constants import statement
Another one I missed:  GPIBconfig -> gpib_constants.config
Apparently, in this case, an IntEnum is not sufficiently like an
int to be type-matched.
@DavidCPlatt

Copy link
Copy Markdown
Contributor Author

OK, this looks better.

I fixed the two enum-name-change sets (GPIBconfig and GPIBerror) I missed during my original restructuring.

I've moved the import into the proper sorted position. There's one other complaint about an old import, but I don't know if I should touch it now (I didn't, before).

I fixed the type mismatch involving the SAD, although it took a couple of tries for me to realize what was actually going on. Apparently IntEnums don't always behave precisely like integers, from a type-checking perspective.

Please take another look.

@MatthieuDartiailh

Copy link
Copy Markdown
Member

Looks good. One more import sorting you can use ruff to fix.

Run ruff on gpib.py, merging two import statements into one.
@DavidCPlatt

Copy link
Copy Markdown
Contributor Author

Done.

@MatthieuDartiailh MatthieuDartiailh merged commit 721c065 into pyvisa:main Jul 2, 2026
18 checks passed
@DavidCPlatt DavidCPlatt deleted the gpib-enums-clean branch July 2, 2026 18:10
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