Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Siglent SDS E-series Support #247

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

mike42
Copy link

@mike42 mike42 commented Aug 30, 2024

The current main branch of libsigrok is not able to retrieve samples from Siglent SDS E-series oscilloscopes. This change adds support.

Background

This was previously submitted as sigrokproject/libsigrok#118, but the PR was withdrawn by the author as there was no maintainer interest at the time.

Per the original PR, the following bugs are related to the change.

Bug Description
1355 Impossible to stop capture with Siglent SDS1104X-E
1356 LIBUSB_ERROR_TIMEOUT and segfault when using Siglent SDS1104X-E over usb
1242 Siglent SDS 1104X-E unable to connect
1628 Pulseview - No data acquisition and crashing with Siglent 1104X-E data

Various rebases of this change have been confirmed to work by different users, with a variety of scopes:

Equipment Tested by Date/ref
SDS-1104X-E @voneiden Feb 6 2021
SDS-1104X-U @iommu October 2 2021
SDS-1204X-E @tomba October 29, 2022
SDS-1104X-E @laf0rge November 16, 2022
SDS 1104X-E @mike42 August 30, 2024

These affordable devices are quite popular with hobbyists, so you wont have a problem finding somebody to test siglent-sds driver changes for regressions on this hardware in future.

Lastly, noting that this PR has failed to gain traction before, I'd like to mention that there is considerable interest in these devices from prospective Sigrok contributors, but this is not matched by actual movement in the siglent-sds driver. If those with commit access are still not in a position to enhance/maintain this driver, then that's absolutely fine, but I would like to respectfully request that you expand the group of committers to ensure that the community is able to take this driver forward.

Testing the change

I tested this on Linux (Debian testing).

This setup can be replicated by installing build dependencies, cloning sigrok-util, then building from source using the sigrok-cross-linux script, with the added step of patching some URL's to point to forks before running the build (patch: siglent-sds.txt)

$ git clone git://sigrok.org/sigrok-util
$ cd sigrok-util
$ git apply -f siglent-sds.txt
$ cd cross-compile/linux
$ C=--disable-java ./sigrok-cross-linux

Note: disable-java is to work around bug 1827, and the alternative PulseView URL is just so that I don't need to ignore test failures, see sigrokproject/pulseview#72. Neither of these are related to this change.

Then running:

$ LD_LIBRARY_PATH=$HOME/sr/lib/ $HOME/sr/bin/pulseview --dont-scan

And confirming libsigrok commit ID matches this PR.

image

Testing via network connection

I'm able to scan/connect to my SDS1104X-E using Raw TCP, port 5025.

image

On the scope, I have connected channels 1 & 3 to the calibration signal and pressed 'Auto Setup'. The screen looks like this:

ScreenImg(1)

In sigrok, I can then press 'Run', and it retrieves the information.

image

Testing USB connectivity

I don't have my udev rules working so I'm running as root (sorry).

# LD_LIBRARY_PATH=/home/mike/sr/lib/ /home/mike/sr/bin/pulseview --driver siglent-sds --dont-scan

I had to reboot my scope here because it got stuck in a weird state, but the same capture works, just slowly.

image

Contrast with current behaviour

I want to emphasise that this change does not implement every possible improvement, but does fix compatibility issues which prevent users from acquiring samples from this type of device.

In the current master branch, connecting over Ethernet allows data to be shown for the first channel (albeit very slowly), but the capture never completes, channels other than the first channel are never loaded, and stdout shows zillions of warnings in the form WARNING: Received analog packet with 0 samples.

image

The device can be discovered over USB, but similarly a capture never completes, with the only feedback being the message sr: scpi_usbtmc: USBTMC invalid bulk in header. appearing on stdout.

image

How you can help

  • Testing - Do you own a Siglent SDS -E or -U series? I would appreciate it if you are able to attempt to replicate my testing above and report whether you are also now able to capture samples.
  • Regression testing - The existing siglent-sds driver apparently supports SDS1000X and SDS2000X series devices. If you own one of these older devices (not "-E", "-U," or "HD"), then I would appreciate a comment as to whether the changes in this branch cause any regressions compared to the current libsigrok - master.
  • Code review & comments - If you have feedback on how I could make this code more acceptable to the project then I'd like to hear it!

voneiden and others added 4 commits August 30, 2024 09:31
This commit re-implements the whole acquisition logic for the Siglent
E-series (SDS1000X-E, SDS1000X-U, SDS2000X-E). The acquisition is
based on a state machine to keep the logic simple and robust.
Additionally this improves maintainability as the E-series acquisition
logic is now isolated from the older models which follow a partially
different logic and have other constraints when compared to the
E-series.
@fredzo
Copy link

fredzo commented Aug 30, 2024

Great work @mike42 ! Hope it will not collide with the PR I created yesterday (bad timing...) : #245
@committers, if you are willing to accept both #245 and #247 PRs, I am OK to work on the merge of my code to @mike42 's.

@andrethomas
Copy link

Hi, any plans to get this merged soon?

@andrethomas
Copy link

andrethomas commented Oct 15, 2024

I can confirm that it works on Windows with a Siglent SDS1104X-E

image

If you would like to test in Windows 32bit or 64bit you can find the binaries for this PR here

@UCSIG
Copy link

UCSIG commented Oct 15, 2024

I can also confirm that everything works for a Siglent SDS-1204X-E!
Image 524

@andrethomas
Copy link

@abraxa is there anything else we need to do before this PR can be merged?

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.

6 participants