Add support for Siglent's SDS2000X Plus series #176

gralco · 2022-02-17T22:56:38Z

Don't set num_block_bytes before any waveform data is read.

Add support for both analog and digital (logic analyzer) channels.

Make SIGLENT_HEADER_SIZE 361 based on experimentation.

Add a pause after sending a command to enter ARM mode. This is necessary to prevent crashes when the timebase is large.

Move the start point after every full block is read.

Don't wait for the instrument to prepare its output buffers if it's a SDS2000X Plus.

Fix multi-channel support and ensure that the correct sample rate is obtained when the timebase is changed or when channels are enabled/disabled in "display" mode.

If a channel is turned on/off on the oscilloscope then use sigrok's settings instead.

gralco · 2022-02-17T22:58:04Z

src/hardware/siglent-sds/protocol.c

@@ -322,6 +322,7 @@ static int siglent_sds_read_header(struct sr_dev_inst *sdi)
 	ret = sr_scpi_read_data(scpi, buf, SIGLENT_HEADER_SIZE);
 	if (ret < SIGLENT_HEADER_SIZE) {
 		sr_err("Read error while reading data header.");
+		sr_dbg("Device returned %i bytes.", ret);


This line was added when I was trying to debug why capturing a waveform wasn't working over USB. Doing this showed that it was only getting 52 bytes when done over USB. This line should hopefully help with future debugging.

Here's what printing this header shows: ALL,#9000000446WAVEDESC

FYI see these findings on using USB on SDS1104X-E: https://github.com/sigrokproject/libsigrok/pull/114/files#diff-b2e40d0d92f027de6ccfdb0e8581aca4192f2408ec3eb900fcc1a6520d054fd3R39

The USB packet size is 64 bytes with a 12 byte packet header so you get 52 bytes of data per packet. Additionally at least the SDS1104X-E has an internal buffer of 61440 bytes (variable named show_send_buffer_size can be seen on uboot log) which gets refilled only when it has been completely read. And while it's refilling the read will fail.

gralco · 2022-02-17T23:03:46Z

Pinging @marchelh since he implemented the existing Siglent device driver.

gralco · 2022-02-17T23:15:46Z

This was tested against tag libsigrok-0.5.2 with 3decd3b patch on top of it. I only tested it using the analog channels since I do not currently own a logic analyzer probe set for the instrument.

gralco · 2022-02-18T02:51:55Z

@marchelh while I'm at this, I was wondering how you got 363 for SIGLENT_HEADER_SIZE. After some experimentation, I found that if I do 361 for this number then my first few points are not garbage and are actually part of the waveform. At 363 it seems like I might be missing 2 valid sample points, at numbers less than 361 it seems I'm getting some sort of preamble data in the beginning. ~~However, I'm finding that there are a few samples missing when my waveform is captured, exactly the number set in SIGLENT_HEADER_SIZE is absent at the end.~~

For example, if my captured window is set to 2us (2000ns) wide, I am only seeing out to 1819ns in the case where SIGLENT_HEADER_SIZE is 361 (2000ns - 361 * 500ps = 1819.5ns) and 1818ns when its 363 (2000ns - 363 * 500ps = 1818.5ns). Here 500ps comes from my sample rate.

I'm not sure if this is happening for all SIglents or if it only applies to SDS2000X Plus. Missing 361 or 363 samples is not huge deal in most cases. It starts to matter more when your timebase is set to something small, I would say it begins to make a noticeable impact at 200ns/div or less. At my sample rate this means I'm missing around 182ns of my measurement. Experimenting with it showed that these points are missing from the end of the waveform, not the beginning.

Edit: I fixed this and other issues in 54730ea by removing devc->num_block_bytes = len; on line 550.

gralco · 2022-02-19T00:33:08Z

~~I'm finding that for some reason my SDS2000X Plus is refusing to transfer more than 10Mpts if the memory depth is set to something >10Mpts:~~

sr: [00:43.873868] siglent-sds: Waiting 50000 ms for device to prepare the output buffers
sr: [01:33.873940] siglent-sds: New block with header expected.
sr: [01:33.907750] siglent-sds: Device returned 361 bytes.
sr: [01:33.907768] siglent-sds: Received data block header: 'WAVEDESC' -> block length 361.
sr: [01:33.907773] siglent-sds: Requesting: 20000000 bytes.
sr: [01:34.861392] siglent-sds: Received block: 1, 10000002 bytes.
sr: [01:34.944802] session: bus: Received SR_DF_ANALOG packet (10000002 samples).
sr: [01:35.017755] siglent-sds: 10000002 of 20000000 block bytes read.
sr: [01:35.017775] siglent-sds: Requesting: 9999998 bytes.
sr: [01:37.020508] scpi_vxi: Device read failed for 192.168.1.7 with error 15
sr: [01:37.020533] siglent-sds: Read error, aborting capture.
sr: [01:37.020541] session: bus: Received SR_DF_FRAME_END packet.
sr: [01:37.020567] session: bus: Received SR_DF_END packet.
sr: [01:37.020584] session: fd_source_finalize: key 0xffffffffffffffff
sr: [01:37.020600] session: Stopped.

~~This results in only acquiring 5ms (for 2GSa/s) when the window is set to anything wider than 5ms.~~

I've tried hacking it to forcefully set the block sizes it's acquiring to 10MB and 1MB but neither worked, in both cases it just failed after 10MB were read. I also tried adding a long delay between block transfers but that did nothing. Any help with this would be appreciated.

Edit: This has been fixed.

gralco · 2022-04-06T02:56:02Z

I just added support for the digital (logic analyzer) channels as well.

Edit: there seems to be a bug where the SCPI command "WAVeform:STARt <value>" is not obeyed for the digital channels. So I added a workaround where just the first block of the waveform is displayed for large timebases. Currently, this results in half of the screen being displayed at a timebase of 2ms/div, a fifth of the screen displayed at a timebase of 5ms/div, and a tenth of the screen being displayed at timebases >= 10ms. You will capture the entire screen for timebases of 1ms/div or less.

gralco · 2022-06-01T16:51:40Z

@uwehermann If you get a chance, could you please take a look at this merge request. I believe it's ready to be merged.

gralco · 2022-07-22T16:16:27Z

@gsigh Could you help get this merged?

gralco · 2022-09-07T22:08:37Z

My last push just added some fixes to work with the latest firmware (V1.5.2R1).

gralco · 2024-02-01T04:28:32Z

@abraxa @fenugrec @gsigh Could you please have a look at this PR? It would be nice to get it merged upstream.

fenugrec

See comments.

I have issues with some of the changes that would seem to impact the other devices ? If that is the case, they belong in a separate commit with a good explanation and testing that confirms no breakage
lots of strcmp with the model name; in some areas it looks like there is so much difference that it would make sense to split code into separate functions.
I didn't even look at code that looked like for{if{do{if{if{for{if{if... that is way too many indents and almost unreviewable
you replaced some hardcoded constants with something that ends up in the device struct - that is great, but could/should have been a separate commit for those cases that could be applied independantly of adding SDS2000 per se. Small changesets are much prefered.

fenugrec · 2024-02-04T16:34:20Z

src/hardware/siglent-sds/api.c

+		/* SDS2000X+: The channels need to be enabled/disabled prior to
+		 * obtaining the sample rate when the data source is set to display.
+		 * If a channel is turned on/off on the oscilloscope then use Sigrok's settings instead. */
+		if (!strcmp(devc->model->series->name, "SDS2000X+") && devc->data_source == DATA_SOURCE_SCREEN) {


I prefer avoiding to test things like model name strings for feature support... adding a bool flag (or bit-masked enums, I would look at other more mature drivers to see if there is a more common pattern) would be preferable esp. if future models also need this

fenugrec · 2024-02-04T16:35:21Z

src/hardware/siglent-sds/api.c

+					if (!strcmp(channel_state, "OFF") && ch->enabled)
+						if (siglent_sds_config_set(sdi, "C%d:TRA %s", ch->index + 1,
+							ch->enabled ? "ON" : "OFF") != SR_OK)
+							return SR_ERR;


this is getting into some pretty deep indent levels, maybe time to split some functionality into a separate func ?

fenugrec · 2024-02-04T16:37:10Z

src/hardware/siglent-sds/api.c

+					char *channel_state;
+					char *cmd = g_strdup_printf("C%d:TRA?", ch->index + 1);
+					sr_scpi_get_string(sdi->conn, cmd, &channel_state);
+					if (!strcmp(channel_state, "ON") && !ch->enabled)


not checking if sr_scpi_get_string() failed before strcmp (i didnt check if it sets channel_state to null or to an empty string if it fails)

fenugrec · 2024-02-04T16:38:37Z

src/hardware/siglent-sds/api.c

 	switch (devc->model->series->protocol) {
 	case SPO_MODEL:
-		if (siglent_sds_config_set(sdi, "WFSU SP,0,TYPE,1") != SR_OK)
+		if (siglent_sds_config_set(sdi, "WFSU SP,0,NP,0,FP,0") != SR_OK)


you are changing this for all models , is this intended ?

fenugrec · 2024-02-04T16:39:53Z

src/hardware/siglent-sds/protocol.c

 			if (sr_scpi_get_string(sdi->conn, ":INR?", &buf) != SR_OK)
 				return SR_ERR;
 			sr_atoi(buf, &out);
 			g_free(buf);
-			if (out == DEVICE_STATE_TRIG_RDY) {
+			if (out == DEVICE_STATE_TRIG_RDY || out == DEVICE_STATE_STOPPED) {


another change that impacts other devices, would like to hear rationale, testing on other models to confirm no breakage, and separate commit unless it's directly related to sds2000 support

fenugrec · 2024-02-04T16:40:17Z

src/hardware/siglent-sds/protocol.c

-		s = (ch->type == SR_CHANNEL_LOGIC) ? "D%d:WF?" : "C%d:WF? ALL";
-		if (sr_scpi_send(sdi->conn, s, ch->index + 1) != SR_OK)
-			return SR_ERR;
+		if (!strcmp(devc->model->series->name, "SDS2000X+")) {


another strcmp with model name

fenugrec · 2024-02-04T16:41:33Z

src/hardware/siglent-sds/protocol.c

@@ -315,13 +336,25 @@ static int siglent_sds_read_header(struct sr_dev_inst *sdi)
 	struct dev_context *devc = sdi->priv;
 	char *buf = (char *)devc->buffer;
 	int ret, desc_length;
-	int block_offset = 15; /* Offset for descriptor block. */
+	int block_offset; /* Offset for descriptor block. */
+	if (!strcmp(devc->model->series->name, "SDS2000X+"))


maybe would make sense to have this value (and header_size just below) in the device descriptor struct. Less code.

fenugrec · 2024-02-04T16:44:25Z

src/hardware/siglent-sds/protocol.c

-					} else {
-						/* Get previous stored sample from low channel buffer. */
-						tmp_value = g_array_index(data_low_channels, uint8_t, samples_index);
+		if (ch->type == SR_CHANNEL_LOGIC) {


this section is complex and hard to review (I am skipping over it at this time)
Also if there's that many different sub-cases with "if model is SDS2000", it starts making sense to have a separate function entirely.

fenugrec · 2024-02-04T16:47:43Z

src/hardware/siglent-sds/protocol.c

+					/* The older models need more time to prepare the the output buffers due to CPU speed. */
+					wait = (devc->memory_depth_analog * 2.5);
+					sr_dbg("Waiting %.f ms for device to prepare the output buffers", wait / 1000);
+					g_usleep(wait);


I always dislike sleep/wait calls. Is there no way to tweak SCPI read timeouts ?

fenugrec · 2024-02-04T16:49:50Z

src/hardware/siglent-sds/protocol.c

@@ -609,16 +727,18 @@ SR_PRIV int siglent_sds_receive(int fd, int revents, void *cb_data)
 					g_array_free(data, TRUE);
 				}
 				len = 0;
-				if (devc->num_samples == (devc->num_block_bytes - SIGLENT_HEADER_SIZE)) {
+				if (devc->num_samples == devc->num_block_bytes) {


changing common code for non-obvious reason

…c Kuzmenko) in this PR : github.com/sigrokproject/pull/176/ Created a new protocol version "E11" matching the latest Siglent SDS Scope programming guide. Reduced the read-wait time from 7sec to 50ms for better performance (achived by looping on read sample logic to consume the data has it arrives). Tested with SDS2504X HD model connected via Ethernet cable.

Add support for both analog and digital (logic analyzer) channels. Don't set num_block_bytes before any waveform data is read. Add a pause after sending a command to enter ARM mode. This is necessary to prevent crashes when the timebase is large. Move the start point after every full block is read. Don't wait for the instrument to prepare its output buffers if it's a SDS2000X Plus. Fix multi-channel support and ensure that the correct sample rate is obtained when the timebase is changed or when channels are enabled/disabled in "display" mode. If a channel is turned on/off on the oscilloscope then use sigrok's settings instead.

gralco commented Feb 17, 2022

View reviewed changes

gralco force-pushed the master branch 6 times, most recently from 647e32d to 62444c6 Compare February 19, 2022 00:24

gralco force-pushed the master branch 16 times, most recently from 46ece52 to 66d2b87 Compare February 24, 2022 18:48

gralco force-pushed the master branch 3 times, most recently from f6e2248 to ffbd467 Compare March 6, 2022 16:26

gralco force-pushed the master branch 2 times, most recently from 4c7d423 to 54730ea Compare March 6, 2022 21:16

gralco force-pushed the master branch from 54730ea to 36d2f9d Compare April 6, 2022 02:27

gralco force-pushed the master branch 7 times, most recently from 455d96d to fafe491 Compare April 7, 2022 17:50

gralco force-pushed the master branch from fafe491 to 3f9b449 Compare September 7, 2022 21:58

gralco force-pushed the master branch 3 times, most recently from b5f1b52 to 16c651d Compare September 8, 2022 03:19

voneiden mentioned this pull request Dec 17, 2022

siglent-sds: Reimplement eseries acquisition logic and add new scope models #118

Closed

gralco force-pushed the master branch from 16c651d to a661246 Compare February 22, 2023 17:36

abraxa added the needs code review label Sep 27, 2023

gralco force-pushed the master branch 2 times, most recently from 25a46dc to a056b30 Compare February 1, 2024 04:25

gralco mentioned this pull request Feb 1, 2024

siglent-sds: fix header size (some models), fix download speed #236

Open

fenugrec reviewed Feb 4, 2024

View reviewed changes

fredzo mentioned this pull request Aug 29, 2024

Siglent sds hd support #245

Open

gralco force-pushed the master branch from a056b30 to 0f6af22 Compare October 26, 2024 03:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Siglent's SDS2000X Plus series #176

Add support for Siglent's SDS2000X Plus series #176

gralco commented Feb 17, 2022 •

edited

Loading

gralco Feb 17, 2022 •

edited

Loading

voneiden Aug 4, 2022

gralco commented Feb 17, 2022

gralco commented Feb 17, 2022 •

edited

Loading

gralco commented Feb 18, 2022 •

edited

Loading

gralco commented Feb 19, 2022 •

edited

Loading

gralco commented Apr 6, 2022 •

edited

Loading

gralco commented Jun 1, 2022

gralco commented Jul 22, 2022

gralco commented Sep 7, 2022

gralco commented Feb 1, 2024

fenugrec left a comment

fenugrec Feb 4, 2024

fenugrec Feb 4, 2024

fenugrec Feb 4, 2024

fenugrec Feb 4, 2024

fenugrec Feb 4, 2024

fenugrec Feb 4, 2024

fenugrec Feb 4, 2024

fenugrec Feb 4, 2024

fenugrec Feb 4, 2024

fenugrec Feb 4, 2024

Add support for Siglent's SDS2000X Plus series #176

Are you sure you want to change the base?

Add support for Siglent's SDS2000X Plus series #176

Conversation

gralco commented Feb 17, 2022 • edited Loading

gralco Feb 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gralco commented Feb 17, 2022

gralco commented Feb 17, 2022 • edited Loading

gralco commented Feb 18, 2022 • edited Loading

gralco commented Feb 19, 2022 • edited Loading

gralco commented Apr 6, 2022 • edited Loading

gralco commented Jun 1, 2022

gralco commented Jul 22, 2022

gralco commented Sep 7, 2022

gralco commented Feb 1, 2024

fenugrec left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gralco commented Feb 17, 2022 •

edited

Loading

gralco Feb 17, 2022 •

edited

Loading

gralco commented Feb 17, 2022 •

edited

Loading

gralco commented Feb 18, 2022 •

edited

Loading

gralco commented Feb 19, 2022 •

edited

Loading

gralco commented Apr 6, 2022 •

edited

Loading