gpib: fix EOS for read#597
Conversation
Use the TERMCHAR and TERM_CHAR_EN attributes to configure the GPIB interface prior to each read. Partial fix for pyvisa#596 This allows the GPIB device class to be used with devices such as the HP 8903B which do not signal EOI at the end of input and must depend on termination-character matching by the interface. It would be possible to cache the results of the config settings, and only re-config when they change, but that would probably be excessive optimization since configuring the interface does not involve round-trips to the device.
for more information, see https://pre-commit.ci
| server to receive VXI-11 `DEVICE_INTR_SRQ` callbacks. This also fixes the | ||
| `create_intr_chan` XDR packer in `protocols/vxi11.py`. | ||
| Other transports (GPIB, USBTMC, HiSLIP, Serial) remain unsupported for now. PR #577 | ||
| - Pass termchar information to USBTMC and GPIO devices when reading. |
There was a problem hiding this comment.
Please mention the PR number
There was a problem hiding this comment.
Please pardon my ignorance - how is that best done? In the normal git process I have to commit the change to the file, before uploading to Gitlab, before creating the pull request (which is when I know what the pull-request number is).
Do I have to split changes into two (one commit and pull request for the actual change, and another for the documentation in CHANGES)? or edit the change after the pull request is created, or ???.
Please educate me! :-)
There was a problem hiding this comment.
You can simply push the changelog change as a separate commit after creating the PR.
| if ( | ||
| self.attrs[ResourceAttribute.termchar] is None | ||
| or not self.attrs[ResourceAttribute.termchar_enabled] | ||
| ): | ||
| ifc.config(0x0C, 0) # IbcEOSrd | ||
| else: | ||
| termchar = self.attrs[ResourceAttribute.termchar] | ||
| ifc.config(0x0F, termchar) # IbcEOSchar | ||
| ifc.config(0x0E, 1) # IbcEOScmp | ||
| ifc.config(0x0C, 1) # IbcEOSrd |
There was a problem hiding this comment.
Rather than doing this at each read, could it be possible to do it once in a handler for the attribute to avoid the systematic overhead.
Also if there not already an enum for the admissible values to config creating one would make sense.
There was a problem hiding this comment.
I will look at this. From what I have seen of the Session and GPIOSession code structure, this would add some complications due to the fact that there are two interacting attributes - neither one alone controls what we must tell the interface to do. I'd need to add several things to GPIOSession: one actual "look at the two, and configure" method, and two custom setter functions for the two attributes which would set them and then call the common work function. We would actually end up going through the configure process twice at a minimum (once each time one of the two attributes is set). This would eliminate most of the overhead (I think) but at the expense of a good deal of additional code complexity. Is there a leaner way, within the Attribute framework?
The other possibility would be to do the attribute lookup and comparison each time through read() as I do now, creating a tuple (actual or otherwise) of the three values for the three configuration settings. The GPIOSession would store a cached value of the tuple. On each read we'd compute the new tuple, compare it to the cached one, call ifc.config for each config item which has changed, and then update the cached tuple. This would eliminate the ifc.config() calls unless something changed, at the expense of some significant data management each time.
The way I looked at it, was that the overhead in the "lazy" approach here ought to be quite small. The configuration updates (changed or not) do go down to the interface driver, but they do not actually trigger any GPIO or USBTMC bus activity - the device itself isn't actually involved at this time. That seemed a good tradeoff for keeping the change small and localized, and the impact on the GPIOSession code and data structure down.
So - what do you think?
There was a problem hiding this comment.
As far as the enums go... well, there's a nice set of them defined in gpib_ctypes/constants, but I wasn't sure whether pyvisa_py is formally dependent on gpib_ctypes - I saw something about installing either this or a different GPIB package at one point. I didn't want to risk introducing a dependency which would break things.
If that's safe, I could work on another PR to import the GPIB constants, clean up this usage, and look for other places in the code where simple numeric values were used in place of a mnemonic, and fix those too.
There was a problem hiding this comment.
Do you expect any kind of cross-talks between those attributes ? Because I do not understand why you would need to re-specify the term-char when enabling the term char.
If not not you should be able to simply manage those in _get_attribute and _set_attribute of the GPIB session.
Regarding the enum, we do not have a strong dependency on gpib-ctypes but we could duplicate the enum to clean up our code.
There was a problem hiding this comment.
Since the cross-talk is strictly an enable/disable issue, I think you're right. I'll work up a different work-branch and set of changes along those lines, test it, and submit a PR with that approach and let you decide what (if anything) you want to approve. Looking back, I think my approach was to keep the code interventions I was doing for GPIB and USBTMC in the same style (ensuring the correct read configuration as part of the read process). That may be an unnecessary consistency, I suppose.
If it's OK with you I'll handle the "create or use a consistent set of enums" as a separate PR. There are a lot of other uses of hard-coded constants-in-place-of-enums and I think it makes sense to clean them all up in a step, rather than pull them in here.
There was a problem hiding this comment.
I'm going to abandon this PR, as the alternate implementation you suggested seems to have worked out very nicely and I think it will address your concerns.
I'll start working on the enum change.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #597 +/- ##
==========================================
- Coverage 39.29% 39.25% -0.05%
==========================================
Files 27 27
Lines 5008 5014 +6
Branches 513 514 +1
==========================================
Hits 1968 1968
- Misses 3014 3020 +6
Partials 26 26
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Closing in favor of #closzd in favorbof #599 |
2 participants
Add actual TERMCHAR support to GPIB interface
Use the TERMCHAR and TERM_CHAR_EN attributes to configure the GPIB interface prior to each read. Partial fix for #596
This allows the GPIB device class to be used with devices such as the HP 8903B which do not signal EOI at the end of input and must depend on termination-character matching by the interface.