feat(lib): add VISA instrument discovery API, consolidate CLI onto it - #341
Open
dalvarez-204 wants to merge 37 commits into
Open
feat(lib): add VISA instrument discovery API, consolidate CLI onto it#341dalvarez-204 wants to merge 37 commits into
dalvarez-204 wants to merge 37 commits into
Conversation
…added from_json_str() method
…r cosmetic error display
…lishers schema Replaces the flat name/vendor/connection/num_channels shape with a nested config (device: DeviceInfo, driver: VisaDriverConfig, timing, publishers) matching the pattern already used by ModbusConfig/EtherNetIPConfig. PSU_VENDOR_REGISTRY keys are renamed to exact driver class names. Adds a type-tagged publishers union (NominalCorePublisher/FilePublisher) replacing the old flat dataset_rid/output_directory shortcuts. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…figs_parse The new nested PSUConfig schema uses "instrument" as its discriminator, not "protocol" like Modbus/EtherNetIP, so the test's protocol-only check silently pytest.skip()'d every PSU example instead of validating it -- a future schema break in examples/psu/bench_psu.json would have passed CI green. Add an instrument-keyed loader path alongside the existing protocol-keyed one, and declare version/instrument explicitly in the PSU example (matching the Modbus/EtherNetIP examples' own convention of always showing the discriminator field even though it has a default). Verified the fix actually catches breakage: corrupting driver.name in bench_psu.json makes the test fail with a real pydantic ValidationError, not a skip. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
patch("instro.lib.transports.visa.VisaDriver") only overwrites the attribute
on the visa module; instro/psu/drivers/simulated.py already bound its own
local VisaDriver name at import time via `from ... import VisaDriver`, so the
patch silently misses on every test after the first in a pytest session,
leaking a stale mock from whichever test happened to trigger the first
import. Patch instro.psu.drivers.simulated.VisaDriver instead, where the
name is actually looked up.
FilePublisherConfig.format omitted "jsonl", silently blocking the non-deprecated, recommended FilePublisher format for JSON-config-driven PSUs even though FilePublisher itself supports it.
PSU_VENDOR_REGISTRY in config.py is a hand-maintained, lazily-imported mirror of instro.psu.drivers.__all__ (kept lazy so validating a JSON config doesn't force-import every vendor driver module). Add a test asserting the two sets agree so a driver added to one and not the other fails CI instead of silently breaking config-driven construction or discover().
Lost during the JSON-config restructuring, along with the other InstroPSU parameters that got replaced by dataset_rid/output_file/ visa_backend. InstroPSU still accepts **kwargs for default tags (sibling docs like dmm.mdx and eload.mdx kept this bullet verbatim).
The Parameters section sat ambiguously between the direct-constructor example and the JSON config example, so **kwargs/dataset_rid looked like valid JSON config fields. PSUConfig forbids extra fields (extra="forbid"), so JSON configs have no kwargs escape hatch. Move the parameter list next to the constructor it describes and say so explicitly.
Every other model in config.py (VisaDriverConfig, the publisher configs, PSUConfig itself) sets extra="forbid" to catch typo'd JSON keys immediately; TimingConfig was the only one still accepting silently-dropped unknown fields, e.g. a misspelled "pol_interval" would be ignored rather than rejected.
…fig loading The "No vendor-string factory" rule predates the JSON/dict config-loading feature and, read literally, appeared to conflict with PSU_VENDOR_REGISTRY resolving a validated driver.name string to a class for InstroPSU.from_json/ from_dict. Traced the rule back to the repo's very first commit (before the discovery module or the config feature existed) - no prior auto-discovery factory was ever removed, so there's nothing to delete. Clarify instead: the constraint is about the direct Python construction API; the declarative config format's internal string resolution is a different mechanism.
le=10.0 had no documented rationale anywhere in this repo's history and no test or doc depended on it. Some deployments legitimately want a slower background poll than 10s; keep the ge=0.01 floor since a near-zero interval would busy-loop the daemon thread.
…n-instrument-creation
…num_channels Adds a config parameter accepting a PSUConfig, a dict, or a path to a JSON file (str/Path is always treated as a file path, matching ModbusDevice/ EtherNetIPDevice's convention; raw JSON text still goes through the existing from_json_str, which pre-parses to a dict before construction). Mutually exclusive with driver/num_channels; name may still override config.device.name. The resolved PSUConfig is kept on self._config (also matching ModbusDevice), so device metadata (description/manufacturer/model) and other fields that resolve_psu_from_config strips into loose values for construction remain reachable afterward, instead of being discarded once __init__ returns. build_psu_from_config keeps its exact external behavior; from_dict/from_json/ from_json_str are unchanged. The vendor-lookup and driver-construction logic they share now lives in resolve_psu_from_config, extracted so __init__ can use it directly without building a redundant second InstroPSU.
DeviceInfo's description/manufacturer/model fields were already accepted by the schema but unused in bench_psu.json, unlike every Modbus/EtherNetIP example config which populates them. Brings PSU's example in line.
examples/psu/psu_config.py builds a B&K 9115, a Rigol DP800, and the bundled simulator purely from the psu_config_*.json files next to it -- same loop regardless of vendor. A failed connection is reported and skipped rather than aborting the rest of the loop, since only the simulated entry works without real hardware attached. The simulated config also declares a FilePublisher so the example demonstrates config-driven publishing, not just construction. tests/psu/simulated/test_psu_config_example.py runs the simulated config for real against a live SimulatedPSUServer (no mocks) and confirms the declared FilePublisher actually writes data, isolated to tmp_path so it doesn't leave a psu_data/ directory in the repo root. Docs page generated via just gen-examples.
…t-creation' of github.com:nominal-io/instro into dalvarez/instro-75-featpsu-json-config-driven-instrument-creation
Release-As: 1.0.0
VISA discovery module not utilized by cli until later date
Builds device/driver instead of the removed flat name/vendor/connection fields, and uses driver_class_name (already PascalCase) for the registry lookup instead of vendor_key. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ders
instro/cli/discover.py and instro/lib/discover.py had independently
drifted: different _IDN_MAP shapes (2-tuple vs 4-tuple), different
device coverage (CLI had Keysight34461A but not TDKLambdaGenesys or any
scope entries; lib had the reverse), and duplicated VISA scan+match
loops that could silently diverge further.
instro/lib/discover.py is now the merged source of truth for _IDN_MAP
(includes previously-uncommitted scope entries: Keysight1200X,
Tektronix2SeriesMSO, SiglentSDS1000XE) and is the only place that opens
a VisaDriver per resource and does IDN matching. scan_visa_resources()
now accepts an already-open ResourceManager (rm=) so a caller that
already opened one for its own diagnostics (the CLI) doesn't open a
second one -- opening a fresh RM every call for identical-backend
reuse was wasteful and, in the case of a transient failure on the
second open, would have raised out of a call site that had no reason
to expect it.
VisaScanError gains an optional `hint` field: `message` stays the raw
str(exc) unconditionally (so a consumer that just wants results isn't
forced through an interpretation layer), and `hint` carries the CLI's
friendlier guidance ("permission denied - check udev rules", "USB
backend missing - install libusb") only when the exception matches a
known pattern -- callers that want it use hint or message, callers that
don't just use message.
instro/cli/discover.py's own VISA scan+match loop, IDN_MAP, and VisaDriver
usage are deleted; it now calls scan_visa_resources() and renders
VisaScanResult into Rich tables, adding a new ERRORS table since the
old inline per-resource error printing had nowhere to live once errors
became structured data returned after the scan completes rather than
printed live during it. Degraded-interface diagnostics and serial-port
listing stay CLI-only, since they're about explaining an empty VISA
scan to a human, not something other scan_visa_resources() consumers
need.
tests/cli/test_discover.py now mocks scan_visa_resources() directly
for its scan-result tests (that logic is already covered by
tests/lib/test_discover.py) and only exercises real VISA-backend mocks
for the CLI's own backend-resolution/degraded-interface diagnostics,
which it still owns. Also removed the duplicate _IDN_MAP importability
test now that only one _IDN_MAP exists.
Neither the key (vendor, model substrings) nor the value's 4-tuple positions were self-evident from the type annotation alone.
dalvarez-204
changed the title
Cover the recognized-PSU, non-PSU-filtered, and unrecognized/error-ignored paths, plus backend/timeout passthrough. Removed the leftover brainstorming comment above discover() -- filed #343 to track the terminator-retry idea specifically.
dalvarez-204
marked this pull request as ready for review
August 3, 2026 19:47
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
dalvarez-204
commented
Aug 3, 2026
dalvarez-204
commented
Aug 3, 2026
| ("SIGLENT TECHNOLOGIES", "SDS1202X-E"): ("scope", "SiglentSDS1000XE", "siglent_sds1000x_e", 2), | ||
| ("SIGLENT TECHNOLOGIES", "SDS1204X-E"): ("scope", "SiglentSDS1000XE", "siglent_sds1000x_e", 4), | ||
| } | ||
|
|
Contributor
Author
There was a problem hiding this comment.
new _IDN_MAP values have been tested in another PR
dalvarez-204
commented
Aug 3, 2026
| return None | ||
|
|
||
|
|
||
| def scan_visa_resources( |
Contributor
Author
There was a problem hiding this comment.
scan functionality is the same as in original CLI work
dalvarez-204
commented
Aug 3, 2026
No circular-import constraint required deferring these -- instro.lib already imports the same VisaConfig/VisaDriver chain at module level before psu.py's own imports run, confirmed by direct import test. Update the discover() tests' patch target from instro.lib.discover to instro.psu.psu, since the name is now bound at psu.py's import time rather than looked up fresh on each call.
InstroPSU.discover() probed with the requested backend but returned a VisaConfig with no visa_backend set, so reconstructing an InstroPSU from a discovered config would fall back to automatic IVI/py selection -- breaking devices that need the explicitly selected backend.
Base automatically changed from
dalvarez/instro-75-featpsu-json-config-driven-instrument-creation
to
main
August 4, 2026 14:13
PR #131 merged into main and deleted the instro-75 branch, so GitHub retargeted this PR's base to main directly, surfacing the gap between main's now-final PSU config-loading feature and this branch's older draft of the same feature (inherited via its earlier stack on instro-75). Resolution: took main's version wholesale for every file where this branch's differences were just an older draft of the same instro-75 work (AGENTS.md, psu.mdx, examples/psu/*, instro/psu/config.py, tests/psu/test_psu_config.py, tests/test_examples.py) -- confirmed via `git log` that every conflicting commit on this branch's side was a PSU-config-feature commit, not a discover-specific one. Manually merged the two files with real content unique to both sides: - instro/psu/psu.py: kept main's __init__ (autostart, name-fallback fix, config deep-copy fix, from_dict/from_json/from_json_str removal) and this branch's discover() classmethod + hoisted imports + backend fix. - instro/cli/discover.py: kept this branch's lib-backed consolidation entirely -- confirmed main's only change here was 7 scope _IDN_MAP entries already present in this branch's copy of the map. Also fixed a silent auto-merge casualty: tests/cli/test_discover.py's scope-recognition test called the old 3-arg _rm_mock() against this branch's rewritten 0-arg version and patched instro.cli.discover.VisaDriver, which no longer exists post-consolidation. Ported its intent to tests/lib/test_discover.py (where scan-logic coverage now lives) as test_scan_recognized_scope, and deleted the broken original. just check-python, just check-examples, and the full test suite (1640 passed) all pass post-merge.
Contributor
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
nhschwab
reviewed
Aug 4, 2026
Contributor
|
Add docs for lib level discovery method and relevant types |
nhschwab
reviewed
Aug 4, 2026
nhschwab
reviewed
Aug 4, 2026
| if info.category == "psu" and info.vendor_key is not None and info.num_channels is not None: | ||
| configs.append( | ||
| PSUConfig( | ||
| device=DeviceInfo(name=info.resource), |
Contributor
There was a problem hiding this comment.
can we choose a friendlier device name? Don't want every single published channel with the visa resource string as the prefix
nhschwab
reviewed
Aug 4, 2026
…instro.lib scan_visa_resources, VisaScanResult, VisaInstrumentInfo, VisaScanError, and VisaUnrecognizedInstrument were only reachable via the instro.lib.discover submodule, inconsistent with how the rest of instro.lib's public surface (VisaConfig, VisaDriver, ModbusDriver, ...) is re-exported at the top level.
My earlier merge conflict resolution restored this method from the merge commit without checking why it had disappeared from the working tree -- it had been deliberately removed. Re-removing it along with the imports and test file that only existed to support it. instro.lib.scan_visa_resources and its dataclasses remain as the supported discovery API.
nhschwab
reviewed
Aug 5, 2026
dalvarez-204
changed the title
Add an API reference page for instro.lib.discover (scan_visa_resources and its result dataclasses) and a "Discovering programmatically" section to the CLI guide, since scan_visa_resources is now exported as part of instro.lib's public surface. Also comment vendor_key on VisaInstrumentInfo: it's unused today, kept for an anticipated vendor-key-keyed registry lookup once another category (e.g. scope) grows one like PSU_VENDOR_REGISTRY.
vendor_key was live in the original InstroPSU.discover() (fed PSUConfig.vendor directly), but became dead weight once that method was rewritten for the nested PSUConfig schema to resolve drivers via PSU_VENDOR_REGISTRY (keyed by class name) instead -- and InstroPSU.discover() itself is gone now. No category has a vendor-key-keyed registry today, so there's nothing for it to feed. Cheap to reintroduce later, informed by whatever a real consumer actually needs.
The CLI's --backend flag has an equivalent, but there's no --timeout flag for scan_visa_resources' timeout kwarg to mirror -- said so explicitly instead of implying parity that doesn't exist.
Existing tests only exercised the CLI's preference for hint over raw message using a manually-constructed VisaScanError; the classifier's two real branches (VisaIOError w/ SYSTEM_ERROR, missing USB backend) were never triggered by an actual exception. Also assert the miss case (hint is None) explicitly in test_scan_error instead of leaving it unchecked.
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
3 participants
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.
Adds
scan_visa_resources()and its result dataclasses (VisaScanResult,VisaInstrumentInfo,VisaScanError,VisaUnrecognizedInstrument) toinstro.libas a public, programmatic VISA discovery API.instro discover(the CLI) is refactored onto this shared implementation instead of duplicating its own scan loop and_IDN_MAP.(added a new ERRORS table to the CLI, since errors are now structured data returned after the scan rather than printed live during it).
Degraded-interface diagnostics and serial-port listing stay CLI-only.
scan_visa_resources()accepts an already-openResourceManager(rm=).VisaScanErrorgained an optionalhintfield:messagestays the rawstr(exc)unconditionally, so a consumer that just wants results isn't forced through an interpretation layer;hintcarries the CLI's friendlier guidance ("permission denied - check udev rules", "USB backend missing - install libusb") only when a known exception pattern matches, for callers that want to display it.Docs: added a
scan_visa_resourcesreference page (docs/reference/src/reference/discover.md) and a "Discovering programmatically" section to the CLI guide.Known limitation
VISA's resource enumeration (
pyvisa'slist_resources()) can't auto-discover a raw TCP-socket instrument (e.g. the bundled PSU simulator) -- there's no broadcast/mDNS mechanism for a plain socket resource. Discovery only works for instruments VISA can natively enumerate (USB, GPIB, some LAN/VXI-11 instruments). This is a pre-existing limitation flagged by aTODOin the module, not something introduced here.Test plan
just checkandjust testpasspython -m instro.cli.main discoverfor real (no devices connected) -- clean output, no crash