I've tried to fix it, however I can't get it write. I'm trying to do it with the wrong tools. Is there a procedure for developers? I mean, using it with CLI I miss a lot of features, such as debug capabilities, watch variables and skip in and out of functions.

I saw your question on IRC (guessing it was you)

I don't think, that SCPI_CMD_OPC is an appropriate replacement for WAV:STAT?
It looks like the Rigol scopes (PROTOCOL_V3) are internally rather slow copying memory to read it and thus need WAV:STAT?
If the Keysight is faster it might be worth trying without the WAV:STAT? statement, i.e. effectively bypassing rigol_ds_block_wait (but still: rigol_ds_set_wait_event(devc, WAIT_NONE);)
Should that work, we need to find a way to distinguish between Rigol and Keysight scopes.
As I have neither of the scopes, I cannot test it.


If you really want to debug the code you can use gdb or any IDE around it
(https://visualgdb.com/gdbreference/commands/set_stop-on-solib-events)

#Optional: set LD_LIBRARY_PATH
$ gdb sigrok-cli -d rigol-ds:conn=vxi/192.168.1.10/5555 -c data_source=Memory --frames 1 -o test.sr
(gdb) set stop-on-solib-events 1
(gdb) run
(gdb) continue # until libsigrok was loaded
(gdb) break dev_acquisition_start
(gdb) set stop-on-solib-events 0
(gdb) continue

Happy debugging
[I prefer printf debugging and -l 5 output to analyze program flow]

BTW: I am looking for testers to confirm, that my PR does not break existing functionality for non MSO5000 devices.
Any feedback welcome.
https://github.com/sigrokproject/libsigrok/pull/95
https://github.com/jr-oss/libsigrok/tree/rigol-ds_mso5000

Thanks. I was reading API hopping to help with this bug.
As you mentioned, the simple way to identify between Rigol and Keysight is to create another driver. Despite my efforts of creating a patch. All failed. I will try my best to change original driver. If that work, I can share my results.

I've tried as you suggested, however it didn't work. It is slow as well.

I managed to get it working changing:

do {
   if (time(NULL) - start >= 3) {
      sr_dbg("Timeout waiting for data block");
      return SR_ERR_TIMEOUT;
   }

   g_usleep(devc->analog_frame_size < (15 * 1000) ? (100 * 1000) : (1000 * 1000));

   if (sr_scpi_get_string(sdi->conn, "*OPC?", &buf) != SR_OK)
      return SR_ERR;
   if (parse_int(buf, &len) != SR_OK)
      return SR_ERR;
} while (buf[0] != '1' && len < (1000 * 1000)); 


I also need to add a sleep in line 690

if (devc->model->series->protocol >= PROTOCOL_V3) {
   if (rigol_ds_config_set(sdi, ":WAV:BEG") != SR_OK)
      return TRUE;
   g_usleep(devc->analog_frame_size < (15 * 1000) ? (100 * 1000) : (1000 * 1000));
   if (sr_scpi_send(sdi->conn, ":WAV:DATA?") != SR_OK)
      return TRUE;
}

*OPC? is to query the current command completion status.
*OPC? returns "0" or "1" w/o quotes, thus len (parse_int(buf, &len)) is always < 1000*1000

Rigol DS2000 :WAV:STAT? returns: "Query and return the current waveform reading state." / "The quey returns IDLE,n or READ,n" / "n: the current number of waveform points to be read."
(https://beyondmeasure.rigoltech.com/acton/attachment/1579/f-0508/1/-/-/-/-/MSO2000A%26DS2000A_ProgrammingGuide.pdf)


The referenced Keysight programming manual says on p. 33 "Delaying Before Reading Waveform Data":

With the :WAVeform:DATA? query, the amount of delay needed depends on
the :WAVeform:POINts:MODE and the :WAVeform:FORMat settings. In the
RAW waveform points mode, BYTE format data needs about 200 ms delay,
WORD format data about 300 ms, and ASCII format data about 2 s.


Mabe that's the reason that you have to add the delay in line 690

So for me OPC? and WAV:STAT? are completely independent commands and I still assume that you should just replace the :WAV:STAT? command by a plain delay of ? ms

I haven't compared the programming manuals from Keysight and Rigol thoroughly, but I noticed that there are differences like no :WAV:STAT? and no :WAV:BEG commands.
Maybe there are some more differences that need adjustment.

BTW: IRC is for discussion AFAIK and posting many source code lines is deprecated, but I am also rather new to IRC and the sigrok project.
I just wanted to get my MSO5000 fully supported, but use it also with other LA.

Sorry for posting code lines.

With just a delay, it work as well, but I can notice a slower update.

> len (parse_int(buf, &len)) is always < 1000*1000

I know, I just don't know how to add and remove code without creating a complete mess around it. That is way I leave it.

> Mabe that's the reason that you have to add the delay in line 690

For sure.

> So for me OPC? and WAV:STAT? are completely independent commands and I still assume that you should just replace the :WAV:STAT? command by a plain delay of ? ms

With just a delay, it work as well, but I can notice a slower data update. Just curios if this is the clever way to do it, since *OPC? loops around 3 times before change from 0 to 1 for the first call. The others always show "1" as result. I assume the lag is just for the first call, but can arise further.

>I noticed that there are differences like no :WAV:STAT? and no :WAV:BEG commands. Maybe there are some more differences that need adjustment.

I'm using for reference:
https://www.keysight.com/upload/cmc_upload/All/1000_series_prog_guide.pdf
http://sdpha2.ucsd.edu/Lab_Equip_Manuals/Rigol_DS2000_ProgrammingGuide_EN.pdf

You are correct, there is no :WAV:BEG and I didn't notice. I've commented the line and it is even faster.

However, I still confused. After this tests, and not being part of dev team. I'm not sure how to push it. Better yet, in a way that does not conflict with Rigol with WAV:STAT?.

I am also not a member of the dev/core team.
Just started with MSO5000 and trying to get it in mainline.
But since my PR is rather big and might cause regressions for other models, it is not that easy.


Your problem is probably simpler to review without testing all supported devices.

For rigol_ds_block_wait() an easy solution would be to change the "if" to something like that:
if (devc->model->series->protocol == PROTOCOL_V3 && strcmp(devc->model->series->name, "DSO1000") == 0) {
/* Just wait */
} else if (devc->model->series->protocol == PROTOCOL_V3) {
/*Use :WAV:STAT?*/
}

Hopefully you can find a better/easier-to-understand condition.


To get rid of the WAV:BEG command would get more complicated.
I had the similar situation for MSO5000 and my proposed fix is in this commit, which does not work easily for PROTOCOL_V3.
https://github.com/sigrokproject/libsigrok/commit/0a3ce954d1cd79406368398de492f3afd25de7c7

Maybe it is possible to make use of the struct rigol_ds_commands (api.c).
Currently there are sparse definitions for std_cmds and mso7000s_cmds.
Adding agilent_dso1000 and extending the other cmd-arrays.

Hadn't noticed the cmd-arrays before I made my implementation.


Once you are happy with your implementation, review file HACKING (again), place the source on a publicly accessible git repository and tell the core team on IRC to review.
It is not required to create a github pull request AFAIK. The maintainers have different opinions on that workflow.
Don't expect an immediate answer on IRC. Many users have a re-bouncer running to read the discussions later.

Thanks Ralf, as you suggested. My code can be found here:

https://pastebin.com/Mm1QubJu

It's working, however, channels are inverted, showing negative values.
I'm not sure if this is a user problem, but I can't config trigger from pulseview.

Maybe I did not describe it properly, how to contribute. Either git or mailing list should be used AFAIK. Nice git commit messages help, also.
Please refer to section "Contributions" in HACKING for an official statement how to provide patches.

I am not the maintainer, but I already learned, that these points will (most likely) prevent your patch from being accepted:
- keep line endings (yours is crlf, sigrok is lf)
- trailing spaces (do git diff on console and see red lines)
- fix indentations (opening/closing braces)
- if you copy code/comments, adjust it to your situation. Wrong comments are worse than no comments

You should make the maintainers' work as easy as possible to review and accept your patch.
It is not about getting it to work somehow, but it should fit into the existing code base without breaking anything.


To investigate your problem with negative samples, you could look at the mapping from raw byte data to analog values.
Line 763 in your file "devc->data[i] = ((int)devc->buffer[i] - vref - origin) * vdiv;"
Maybe for your scope the version in line 766 works better (<=PROTOCOL_V2).
Compare against the programmer's manual p. 285: "(YREFerence – data_value) * YINCrement -YORigin"

Hey Ralf, thanks again. This are getting difficult again. I've edited in VS Code, that's why you probably find different endline.

I will do my best to fix as you suggested. However, I'm still not sure how to provide the code. Should I clone entire repo on github and send it, or just .c file?

(In reply to Thales Maia from comment #10)
> 
> I will do my best to fix as you suggested. However, I'm still not sure how
> to provide the code. Should I clone entire repo on github and send it, or
> just .c file?
see https://github.com/sigrokproject/libsigrok/blob/master/HACKING:
 - In order to contribute you should ideally clone the git repository and
   let us know (preferably via IRC, or via the mailing list) from where to
   pull/review your changes. You can use github.com, or any other public git
   hosting site.

I prefer to have a dedicated branch for each modification/PR.
Commit messages should reflect *why* you want to change the code (problem, solution), not what you changed.

On github you can check your modifications again before asking on IRC, to see if there is something in the style you haven't noticed in your local setup.