Repository navigation
fix(eload): cache mode only after the driver confirms set_mode - #485
Merged
hoehner merged 1 commit intoSep 7, 2026
Merged
Conversation
InstroELoad.set_mode assigned self._mode before entering _resource_lock and calling self._driver.set_mode (instro/eload/eload.py:224). When the driver raised, for example a model that does not support CP, the cached mode was left pointing at a mode the instrument was never put into, and the next set_level/set_range passed it straight to the driver instead of hitting the "Mode must be set" guard. open() already rolls the cache back for the _apply_load_config path, so only direct set_mode calls were exposed. Move the assignment inside the lock, after the driver call returns. That matches InstroDMM, which updates _measurement_config only after its driver call succeeds.
Greptile SummaryThis PR corrects ELoad mode caching so the in-memory mode changes only after the driver successfully applies it.
Confidence Score: 5/5The PR appears safe to merge and directly fixes the stale-mode state without introducing an actionable regression. The cached mode is now updated only after successful driver confirmation, and the added test exercises the reported failure path; no blocking or independently actionable quality issues remain. Important Files Changed
Reviews (1): Last reviewed commit: "fix(eload): cache mode only after the dr..." | Re-trigger Greptile |
hoehner
self-requested a review
September 7, 2026 20:43
hoehner
approved these changes
Sep 7, 2026
hoehner
left a comment
Collaborator
There was a problem hiding this comment.
Clean fix, approving after tests run!
Collaborator
|
Thank you @MaxFreedomPollard ! |
6 of 13 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
InstroELoad.set_mode()assignedself._mode = modebefore it enteredself._resource_lockand calledself._driver.set_mode(...)(instro/eload/eload.py:224onmain). If the driver raised, for example a model that answers CP withNotImplementedError, the exception propagated but the cached mode was already updated to a mode the instrument was never put into. The nextset_level()orset_range()then sailed past the"Mode must be set"guard and handed that wrong mode to the driver, so the load was driven in CP units while it was still in whatever mode it was actually in.open()already resetsself._mode = Nonewhen_apply_load_config()fails, so the config-driven path was covered. Directset_mode()calls had no recovery, and the stale value persisted silently.The fix moves the assignment inside the lock, after
self._driver.set_mode(...)returns.InstroDMMalready does it this way: every one of its setters calls the driver first and only then updatesself._measurement_config(instro/dmm/dmm.py:292-365). ELoad was the outlier.Fixes #417
Type of change
fix)feat)feat!/fix!)refactor)docs)chore)Verification
The new test fails on
mainand passes with the fix. Checking out just the unmodified module under the new test:With the fix in place:
mainat b8f262a gives 2234 passed on the same suite, so the only difference is the added test. The change is pure Python and touches no Rust, so I ran the Python half ofjust checkandjust testrather than the full native build. No hardware was used; the test drives aMagicMock(spec=ELoadDriverBase)the same way the surroundingInstroELoadtests do.Tests
Checklist
feat(driver): add support for Keysight E36300)Notes for reviewers
No docs change:
docs/guides/instrumentation/eload.mdxalready says the mode must be set before level or range, and this only makes the code hold to that when the driver rejects the mode.I left the
self._mode = Nonerollback inopen()alone. It still earns its place, because_apply_load_config()can fail atset_rangeorset_levelafterset_modelegitimately succeeded, and the cache has to be cleared for that case too.Scoped to #417 only. #416, the atomicity of
_apply_load_configacross separately locked setters, is untouched here: the assignment now sits inside_resource_lock, which is where #416 would want it, so that work can build on this rather than undo it.