Bug 1750 - SCPI over TCP, multiple receive chunks for one response
Summary: SCPI over TCP, multiple receive chunks for one response
Status: CONFIRMED
Alias: None
Product: libsigrok
Classification: Unclassified
Component: Common: SCPI handling code (show other bugs)
Version: unreleased development snapshot
Hardware: All All
: Normal normal
Target Milestone: ---
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-11-18 12:51 CET by Folkert van Heusden
Modified: 2021-11-29 21:19 CET (History)
3 users (show)



Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Folkert van Heusden 2021-11-18 12:51:59 CET
When an instrument sends the response of '*IDN?' as multiple TCP-packets (by invoking multiple write()s with e.g. naggle disabled), then the SCPI code fails to recognize the *IDN response. I've seen this with libscpi.
The fix seems to be relatively simple:

diff --git a/src/scpi/scpi.c b/src/scpi/scpi.c
index 0e74b32..914df85 100644
--- a/src/scpi/scpi.c
+++ b/src/scpi/scpi.c
@@ -289,7 +289,7 @@ static int scpi_get_data(struct sr_scpi_dev_inst *scpi,
 
        response = *scpi_response;
 
-       while (!sr_scpi_read_complete(scpi)) {
+       while (!memchr(response->str, '\n', response->len)) {
                /* Resize the buffer when free space drops below a threshold. */
                space = response->allocated_len - response->len;
                if (space < 128) {

I did a tiny bit of testing and this fixes at least the problem I saw with libscpi and things still work (I tried '--scan') with a rigol DG1022Z.


Side-note:
                if (space < 128) {
                        int oldlen = response->len;
                        g_string_set_size(response, oldlen + 1024);
                        g_string_set_size(response, oldlen);
                }

Is this correct? I'm puzzled about the g_string_set_size invocations.
Comment 1 Folkert van Heusden 2021-11-18 16:39:14 CET
To clarify; this problem can occur when the instrument is connected via TCP and:

- sends a response with multiple write() commands (with the naggle algorithm disabled so that multiple writes are not always combined into one packet)

- sends a response that is rather large fragmentation takes place (I think this can happen at > 512 bytes but I'm not entirely sure)
Comment 2 Jan Breuer 2021-11-19 08:20:50 CET
SCPI is little bit tricky and `\n` is not always the end of the response (see ARBITRARY PROGRAM DATA).

Name `sr_scpi_read_complete` sounds, it should handle the complete response correctly so isn't the bug in this function not waiting for complete response in all cases?
Comment 3 Gerhard Sittig 2021-11-29 21:09:33 CET
As was said several times on IRC, while the OP failed to update this report 
with information that's available to him, is that assuming the ASCII LF char 
'\n' at the SCPI layer is incorrect, and breaks other people's uses. Which 
means that the approach in comment 0 is not acceptable for mainline. Asking 
physical transports from common code whether the receive data is complete 
_is_ the most appropriate thing to do.

The g_string_set_size() calls are not a problem either, in fact they are 
essential and most appropriate as well. Take these lines in their context 
(the receive routine which keeps reading until the end of the response is 
seen) and lookup string buffers in the glib reference, to see why these 
_are_ needed in the very form that's in the current mainline implementation.

Am adjusting the subject to better reflect what's reported here.
Comment 4 Folkert van Heusden 2021-11-29 21:19:09 CET
I did not add that as you also suggested that changes needed to be made to the tcp layer which I find hard to believe that's the way to go.

Yes, checking for \n may not be the way either, but how it works currently is broken too.