Siglent sds hd support #245

fredzo · 2024-08-29T10:54:20Z

Hi,
This PR is based on the work of Eric Kuzmenko (#176).
It's been discussed on EEVblog forum and tested by different users on SDS 2000X HD, 800X HD, 1000X HD, and 2000X plus : https://www.eevblog.com/forum/testgear/new-sigrokpulseview-hardware-support-(siglent-sds-hd-rigol-dho800-)/
I've tried not to change any behavior of the original driver.
There are known limitations on digital channels of 2000X HD for memory depth > 5Mpts due to a bug in Siglent's firmware. I am exchanging with Siglent support team about that.
Please let me know if you have any comment.
Best regard,
Frederic.

…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.

…0X HD, 3000X HD, 5000, 6000 and 7000.

…ing communication stack to get the data from the device.

Fixed compilation warnings.

…rames.

Read block length from block header for both analog and digital channels. Added use of WAV:POIN to specify block length as suggested in Siglent code exmaples.

…isition. Removed unused SCPI commands for E11 prtocol

…o 5mV instead of 500 uV). Fixed vertical ranges lower than 10mV for E11 models.

doge-gif · 2024-10-10T14:17:00Z

.gitignore

 /tests/*.log
 /tests/*.trs
 /tests/main
+.vscode/


I'd suggest moving this to line 18, it might help accelerate the merging

doge-gif · 2024-10-10T14:18:15Z

src/hardware/siglent-sds/api.c

 static const uint64_t vdivs[][2] = {
 	/* microvolts */
-	{ 500, 100000 },
+	{ 500, 1000000 },


Maybe we'd want to split this to another bugfix PR, although it depends on the maintainers.

doge-gif · 2024-10-10T14:18:52Z

src/hardware/siglent-sds/api.c

-		{ 50, 1 }, { 2, 1000 }, 18, 8, 1400363},
+		{ 50, 1 }, { 2, 1000 }, 18, 8, 25, 1400363},
 	[SDS1000CNL] = {VENDOR(SIGLENT), "SDS1000CNL", NON_SPO_MODEL,
-		{ 50, 1 }, { 2, 1000 }, 18, 8, 1400363},
+		{ 50, 1 }, { 2, 1000 }, 18, 8, 25, 1400363},
 	[SDS1000DL] = {VENDOR(SIGLENT), "SDS1000DL", NON_SPO_MODEL,
-		{ 50, 1 }, { 2, 1000 }, 18, 8, 1400363},
+		{ 50, 1 }, { 2, 1000 }, 18, 8, 25, 1400363},
 	[SDS1000X] = {VENDOR(SIGLENT), "SDS1000X", SPO_MODEL,
-		{ 50, 1 }, { 500, 100000 }, 14, 8, 14000363},
+		{ 50, 1 }, { 500, 1000000 }, 14, 8, 25, 14000363},
 	[SDS1000XP] = {VENDOR(SIGLENT), "SDS1000X+", SPO_MODEL,
-		{ 50, 1 }, { 500, 100000 }, 14, 8, 14000363},
+		{ 50, 1 }, { 500, 1000000 }, 14, 8, 25, 14000363},
 	[SDS1000XE] = {VENDOR(SIGLENT), "SDS1000XE", ESERIES,
-		{ 50, 1 }, { 500, 100000 }, 14, 8, 14000363},
+		{ 50, 1 }, { 500, 1000000 }, 14, 8, 25, 14000363},


Same as above, maybe split into another bugfix PR

doge-gif · 2024-10-10T14:20:35Z

src/hardware/siglent-sds/api.c

+			case 100000:
+				cmd = g_strdup_printf("%" PRIu64 "UV", p);
+				break;


Since we fixed the bug on uV being 10^5, maybe this should be 1,000,000 too?

doge-gif

Thanks for the PR!
May I suggest a few changes, so hopefully the merge will be faster?
(And no, I'm not a maintainer. Just another fella owning one of those scopes)

fredzo · 2024-10-10T18:47:04Z

Thanks for the PR! May I suggest a few changes, so hopefully the merge will be faster? (And no, I'm not a maintainer. Just another fella owning one of those scopes)

Thanks for the feedback @doge-gif ! I'll take your proposals (but I'm not creating a separate PR).
I had a few exchanges on Discord with maintainers, they're looking at it.
In the mean time, builds with this patch and the Rigol driver update are available on eevblog forum for Mac and Windows (see link in the PR text).
Best,
Frederic.

swdee · 2025-01-05T07:09:32Z

src/hardware/siglent-sds/protocol.c

+						/* SDS2000X+: Since the LA sample rate is a fraction of the sample rate of the analog channels,
+							* there needs to be repeated "fake" samples inserted after each "real" sample
+							* in order to make the output match the timebase of an enabled analog channel.
+							* The scaling by a factor of 2.5 and 5 appears to be necessary due to an artifact
+							* where the instrument will present half of the entire sample quantity from the screen
+							* within a single block (625000 bytes, or 5000000 bits / samples). Which means there
+							* are some legitimate points missing that are filled with "fake" ones at larger timebases. */
+						for (int i = 0; i < samplerate_ratio-1; i++, ii++)
+							g_array_append_val(tmp_samplebuf, tmp_value);


I get why up-sampling of the digital data has been done here, however it has the unintended consequence of causing the Decoders to produce inaccurate data.

In my test case I have D9 which is approximately a 134kHz PWM square wave generated by MCU and CH1 which is an AC wave in sync with the PWM signal. When no up-sampling is performed the PWM decoder correctly calculates the PWM signal on D9.

�

When a larger time base is used an up-sampling is applied to D9, the PWM decoder outputs the wrong data.
� Is it possible for some type of warning/notification to be drawn over D9 when up-sampling occurs to warn the user?

Hi @swdee , sorry for the late reply, I must have missed the notification.
I'm not aware of any ways in Sigrok API to generate a displayable warning in acquisition process.
I understand that this might be misleading, but it's a limitation of the scope's sampling rate, not much we can do about it sorry...

I find this acceptable. However, I wonder if we could adjust the sampling rate so the encoders get the correct information? I expect the encoder is getting the real sampling rate, but receiving the oversampled data.

Krakonos

I have left a few comments, but overall I find this PR of good quality. Additionally, I did some testing on SDS5034X and it seems to work, although there is one issue: the scope does not resume acquisition automatically and manually needs to be resumed before starting acquisition in pulseview. Is this on purpose to be able to trigger an even on the scope and then trasfer to the PC?

Krakonos · 2025-11-27T14:13:12Z

src/hardware/siglent-sds/api.c


-	sr_scpi_source_add(sdi->session, scpi, G_IO_IN, 7000,
+	int tiemout = (devc->model->series->protocol == E11) ? 50 : 7000;
+	sr_scpi_source_add(sdi->session, scpi, G_IO_IN, tiemout,


Typo here tiemout -> timeout. Also, why is there such a huge difference between the E11 and all the others?

Krakonos · 2025-11-27T14:17:02Z

src/hardware/siglent-sds/protocol.c

+						/* SDS2000X+: Since the LA sample rate is a fraction of the sample rate of the analog channels,
+							* there needs to be repeated "fake" samples inserted after each "real" sample
+							* in order to make the output match the timebase of an enabled analog channel.
+							* The scaling by a factor of 2.5 and 5 appears to be necessary due to an artifact
+							* where the instrument will present half of the entire sample quantity from the screen
+							* within a single block (625000 bytes, or 5000000 bits / samples). Which means there
+							* are some legitimate points missing that are filled with "fake" ones at larger timebases. */
+						for (int i = 0; i < samplerate_ratio-1; i++, ii++)
+							g_array_append_val(tmp_samplebuf, tmp_value);


I find this acceptable. However, I wonder if we could adjust the sampling rate so the encoders get the correct information? I expect the encoder is getting the real sampling rate, but receiving the oversampled data.

Krakonos · 2025-11-27T18:07:56Z

src/hardware/siglent-sds/protocol.c

+			return SR_ERR;
+		if (sr_scpi_get_string(sdi->conn, "TRIG:STAT?", &buf) != SR_OK) {
+			g_free(buf);
+			return SR_ERR;


Should the buf contents be verified? Otherwise, I don't see the reason for calling TRIG:STAT?. I'm not too familiar with SCPI protocol though, so maybe this is something that needs to be called? In that case, perhaps a comment would be worth it.

Krakonos · 2025-11-27T18:42:10Z

src/hardware/siglent-sds/protocol.c

+ *
+ * @return SR_ERR upon failure.
+ */
+static int siglent_get_wave_points(struct sr_dev_inst *sdi,struct dev_context *devc)


Nitpick: I don't fancy the output argument devc. When reading the function usage, it is not immediately clear what member fields are updated. Either num_samples could be passed directly (and devc would never enter the function), or the function could be renamed to reflect it updated num_samples field only (siglent_update_num_samples).

Krakonos · 2025-11-27T18:52:44Z

src/hardware/siglent-sds/protocol.c

+			tmp_samplebuf = g_array_sized_new(FALSE, FALSE, sizeof(uint8_t), len); /* New temp buffer. */
+			for (uint64_t cur_sample_index = 0; cur_sample_index < devc->memory_depth_digital; cur_sample_index++) {
+				char sample = (char)g_array_index(buffdata, uint8_t, cur_sample_index);
+				for (int ii = 0; ii < 8; ii++, sample >>= 1) {


I suggest sample is typed as unsigned char. The shift here is implementation defined, frequently implemented as arithmetic shift. This means after 8 shifts the value would be 0 or 1, depending on the 8th bit of the original byte. Now, I realize this does not cause any issue now, since each iteration extracts the lowest bit and we don't check the remaining value.

Krakonos · 2025-11-27T19:16:41Z

@fredzo Hi! I wonder if you'd be willing to finish the work here? I posted a few comments, nothing major. Also, I wonder if you had time to PR #236 ? It seems to bring some value in parsing the headers, but I haven't spent much time with it. Personally, I'd like to typecast the data to structs to extract the fields.

I'm trying to help out with reviews and get stuff moving to upstream.

fredzo · 2025-11-27T19:52:53Z

Hi @Krakonos ,
Thank you for taking the time to review and comment this PR!
I'm sorry but I moved to a different project (ngscopeclient) and I don't have the environment to build and patch this code anymore, nor the time to set it up :-/
I will not be able to update this PR in the near future, but feel free to take it from here and see if you finally manage to get it to upstream !...
Best,

Krakonos · 2025-11-27T20:35:52Z

@fredzo Ok, no problem! I'll push it over the finish line, since most of the work is done and it would be a shame to let it go. I hope I'll be able to do the same for the Rigol scopes, though I don't have any on hand to test with.

I've seen the ngscopeclient on eevblog forums and might give it a try too, looks like a cool project -- the only thing I'm sad about is that all of the drivers will be reimplemented there with little hope of sharing the codebase; especially given a lof of the quirks and workarounds for different issues in the scope firmwares.

Have fun!

Frederic Borry and others added 12 commits June 27, 2024 00:30

Added declaration of all E11 Siglent oscilocope series : 800X HD, 100…

fa0a72a

…0X HD, 3000X HD, 5000, 6000 and 7000.

Added a timeout while reading data buffer to let time to the underlyl…

d47b871

…ing communication stack to get the data from the device.

Refactored code for better E11 modification isolation.

f9419df

Refactored timebase detection code.

253edb3

Fixed compilation warnings.

Fixed block acquisition for E11 protocol => adds support for 4Mpts+ f…

e91860d

…rames.

Fixed error detection when digital depth > 50Mpts

02fd780

Fixed pagination for analog channels and digital channels.

14ef910

Read block length from block header for both analog and digital channels. Added use of WAV:POIN to specify block length as suggested in Siglent code exmaples.

Fixed triggering for large timebases by forcing stop mode before acqu…

e41e10f

…isition. Removed unused SCPI commands for E11 prtocol

Fixed pagination for digital channels.

8b7eccf

Fixed 500 micro-volt range for all models (there was a typo leading t…

07f9912

…o 5mV instead of 500 uV). Fixed vertical ranges lower than 10mV for E11 models.

Merge branch 'sigrokproject:master' into siglent-sds-hd-support

c53aaac

fredzo mentioned this pull request Aug 30, 2024

Add Siglent SDS E-series Support #247

Open

doge-gif reviewed Oct 10, 2024

View reviewed changes

fredzo and others added 4 commits October 11, 2024 00:49

Merge branch 'sigrokproject:master' into siglent-sds-hd-support

dd5af60

Fixed comment

8392ad3

Fixed .vscode gitignore location

0b9352a

Fixed microvolt multiplier.

aa9b218

doge-gif approved these changes Oct 12, 2024

View reviewed changes

swdee reviewed Jan 5, 2025

View reviewed changes

abraxa added the needs code review label Nov 9, 2025

Krakonos reviewed Nov 27, 2025

View reviewed changes

Krakonos self-assigned this Nov 27, 2025

Siglent sds hd support #245

Are you sure you want to change the base?

Siglent sds hd support #245

Uh oh!

Conversation

fredzo commented Aug 29, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

doge-gif left a comment

Choose a reason for hiding this comment

Uh oh!

fredzo commented Oct 10, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Krakonos left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Krakonos commented Nov 27, 2025

Uh oh!

fredzo commented Nov 27, 2025

Uh oh!

Krakonos commented Nov 27, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants