Skip to content

feat(dewesoftx): add dewesoftx daq driver - #443

Open
maxleblang wants to merge 12 commits into
mainfrom
ml/dewesoftx-support
Open

feat(dewesoftx): add dewesoftx daq driver#443
maxleblang wants to merge 12 commits into
mainfrom
ml/dewesoftx-support

Conversation

@maxleblang

Copy link
Copy Markdown
Contributor

Summary

This PR implements a DAQ driver to "configure" and stream synchronous data out of a running DewesoftX instance into instro, essentially treating the DewesoftX instance as a physical DAQ. There are more first class ways to integrate Dewesoft support with instro but this is good enough for the time being. Additionally, this driver is pretty universal as it doesn't matter what Dewesoft DAQ is connected as long as its data is going through DewesoftX.

Type of change

  • Bug fix (fix)
  • New feature (feat)
  • Breaking change (feat! / fix!)
  • Refactor (refactor)
  • Documentation (docs)
  • Chore / tooling (chore)

Verification

This video demonstrates a simulated channels in DewesoftX being streamed to Nominal Core via the DewesoftXDriver in instro

Recording.2026-08-26.154311.mp4

Tests

  • Unit tests added or updated
  • Existing tests cover this change
  • No tests — explain why:

Early unstable driver development. This needs more iterations before tests and docs.

Checklist

  • PR title follows Conventional Commits (e.g. feat(driver): add support for Keysight E36300)
  • I have read CONTRIBUTING.md
  • Documentation updated if user-facing behavior changed
  • Code follows the style/conventions of the surrounding code

@maxleblang
maxleblang requested a review from hoehner August 27, 2026 21:38
@claude

claude Bot commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Claude finished @maxleblang's task in 14m 3s —— View job


Code review

  • Gather context (diff vs origin/main, AGENTS.md conventions)
  • Review DewesoftXDriver against DAQ driver conventions
  • Post inline findings
  • Overview

Overview

Sound fit for DAQDriverBase's transport-agnostic contract — COM/DCOM instead of VISA, following the same pattern as NIDAQ/LabJackT7 (vendor SDK, not VISA). Correctly composes rather than subclasses, populates the private channel/timing dicts only inside configure_*, and doesn't reach back into InstroDAQ. pywin32 is properly platform-gated (sys_platform == 'win32') in packages/instro-unstable/pyproject.toml. Good scope discipline landing in instro-unstable given the driver's own admission this needs more iteration.

Posted 3 inline findings (session-restart anchoring bug that drops the first batch on every start(), a float leaking into an int-typed field, and an unbounded error-logging loop on unexpected session stop).

Two things not called out inline:

  • No tests. The PR explains why ("early unstable driver development"), which is a reasonable call for a first landing, but the cursor/resync logic (_seed_cursor, _resync_session) is exactly the kind of non-trivial logic AGENTS.md asks to cover with mocked-transport tests once this stabilizes.
  • README "Supported devices" table (DAQ | InstroDAQ | Keysight 34980A, NI-DAQmx, LabJack T-series, MCC USB-series) doesn't list DewesoftX, unlike other unstable drivers which do get an (unstable) tag there (e.g. DMM's Keithley 2750, AWG's Keysight/Rigol entries). Worth adding per AGENTS.md's docs-sync convention.

@greptile-apps

greptile-apps Bot commented Aug 27, 2026

Copy link
Copy Markdown

Greptile Summary

The PR adds an experimental Windows-only DewesoftX DAQ driver that attaches through COM, binds synchronous analog channels, tracks storage-session timestamps, and emits Instro measurements.

  • Adds DewesoftX lifecycle, channel configuration, buffered acquisition, session resynchronization, and COM thread-attachment behavior.
  • Exports the driver through the unstable DAQ package.
  • Adds a Windows-gated pywin32 dependency and updates the lockfile.

Confidence Score: 4/5

The PR should not merge until ring-buffer wraparound is handled without silently dropping an entire available buffer.

The new read and fetch paths treat equal wrapped positions as proof that no samples arrived, although the producer may have advanced by one or more complete buffer lengths.

Files Needing Attention: packages/instro-unstable/instro/unstable/daq/drivers/dewesoftx.py

Important Files Changed

Filename Overview
packages/instro-unstable/instro/unstable/daq/drivers/dewesoftx.py Implements the complete DewesoftX acquisition driver, but wrapped-position arithmetic can silently miss a full ring of samples.
packages/instro-unstable/instro/unstable/daq/drivers/init.py Exposes DewesoftXDriver from the unstable DAQ driver namespace.
packages/instro-unstable/pyproject.toml Adds pywin32 only on Windows to support the driver's COM integration.
uv.lock Records the Windows pywin32 dependency and workspace package metadata updates.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  A[Instro DAQ] --> B[DewesoftXDriver]
  B --> C[Windows COM]
  C --> D[Running DewesoftX]
  D --> E[Synchronous channel ring buffers]
  E --> B
  B --> F[DewesoftXData]
  F --> G[Instro Measurements]
Loading
Prompt To Fix All With AI
### Issue 1
packages/instro-unstable/instro/unstable/daq/drivers/dewesoftx.py:154-156
**Full wraps lose samples**

When acquisition advances by exactly one full buffer between reads, `(DBPos - pos) % buf_size` evaluates to zero, so `read_analog()` reports no data and `fetch_analog()` keeps waiting despite a full buffer being available. Track producer progress independently of the wrapped position so complete wraps and overruns are not mistaken for an empty buffer.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Reviews (1): Last reviewed commit: "clean up" | Re-trigger Greptile

Comment thread packages/instro-unstable/instro/unstable/daq/drivers/dewesoftx.py Outdated

# Start a storing session
name = f"run_{datetime.now().strftime('%Y-%m-%d_%H-%M-%S')}"
self._app.StartStoring(name)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

start() calls StartStoring() but never re-anchors self._t0_ns (it's only set in open()). Since StartStoreTimeUTC now points at the new session, the next read_analog() hits _resync_session(), sees the anchor changed, logs a restart, and returns DewesoftXData(channels={}) — silently dropping the first batch on every hardware-timed start. Set self._t0_ns from self._app.Data.StartStoreTimeUTC right after StartStoring() returns.

self._ai_hw_timing_config = HWTimingConfig(
sample_rate=rate,
sample_period=round(1e9 / rate),
samples_per_channel=max(1, rate // 10),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

rate is self._app.Data.SampleRate, an untyped COM Any, so rate // 10 can be a float (mypy won't catch it since it's Any). HWTimingConfig.samples_per_channel is typed int. InstroDAQ.configure_ai_hw_sample_rate (instro/daq/daq.py:890) wraps the equivalent expression in int(...) — do the same here: max(1, int(rate // 10)).

target = self._ai_hw_timing_config.samples_per_channel
rate = self._ai_hw_timing_config.sample_rate
last_resync = time.monotonic()
while True:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This inner loop never checks the background-daemon stop event. If DewesoftX stops storing externally, _resync_session() (called every 0.5s at line 194) raises RuntimeError on every daemon cycle indefinitely — _background_daemon logs and continues but there's no path back to a healthy state short of daq.stop(). Consider surfacing a clearer terminal condition or having the driver recover once storing resumes.

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.

1 participant