j123b567 / scpi-parser

Open Source SCPI device library
BSD 2-Clause "Simplified" License
447 stars 191 forks source link

fix corrupted compound command results #127

Closed MisterHW closed 2 years ago

MisterHW commented 2 years ago

When compound commands are used, e.g. in a scenario where the application controls hardware with multiple channels we found that some some parts go missing. IDN?;IDN? works, other queries that would return "0;1;2" only return "0;".

fix: use netconn_write(u->io, data, len, NETCONN_COPY). NETCONN_NOCOPY is not allowed here, as data doesn't stay valid until sending is complete. See https://doc.ecoscentric.com/ref/lwip-api-sequential-netconn-write.html for details.

enhancement: provide SCPI_ResultCommand() to facilitate prepending results with the command in question.

example: MEAS:VOLT:CH1?;CH2?;CH3?;CH4?:MEAS:CURR:CH1? reply (prepend on): MEAS:VOLT:CH1,0.123;MEAS:VOLT:CH2,2.048;MEAS:VOLT:CH3,1.337;MEAS:VOLT:CH4,0;:MEAS:CURR:CH1,0.42 reply (default): 0.123;2.048;1.337;0;0.42

j123b567 commented 2 years ago

Please keep in mind, that the library itself was never designed with multiple sessions in mind. It can fail hardly in more complex cases.

E.g. if you allow two simultanouse clients and one sends in one TCP packet *ID and in second packet N?\r\n and the other client will send in between MEAS?\r\n, then the result in the buffer for parsing will be *IDMEAS?\r\nN?\r\n which is wrong for both.

So in my opinion, the only correct way of solving this is to introduce real multi session ability. The easiest approach would be to have multiplle scipi_t context for each session and somehow share status of the device between them. So introduce some functions to have some "master" session and for every client, derive new "slave" session, which will be discarded, after client disconnect.

MisterHW commented 2 years ago

I've been wondering about possible benefits for multiple sessions. While it's again not an issue with scpi-parser itself but a defect in the example I aim to resolve, a single line* sent in a single session, like

MEAS:VOLT:CH1?;CH2?;CH3?;CH4?:MEAS:CURR:CH1?\r\n

seems to produce garbled output. When sending

MEAS:VOLT:CH1?\r\n MEAS:VOLT:CH2?\r\n MEAS:VOLT:CH3?\r\n MEAS:VOLT:CH4?\r\n MEAS:CURR:CH1?\r\n

no issue is observed, though I haven't tested sending the commands in one go (only consecutively to ensure correct syntax and results). In its current state I believe the server example implementation has no means to manage the buffers it passes to netconn_write(), and its documentation points out that the buffers need to stay valid for an indeterminate amount of time (when kept in the re-send queue).

Copying buffers via NETCONN_COPY takes more resources, but I think it's the only legitimate way to use netconn_write() in the current example implementation. Other approaches will need a handler to be called when the tcp packet has been successfully received, or output to be written directly to a TX buffer.


(*) IEEE 488.2 allows the designer some discretion on how compound headers are handled within the rules of section 7.6.1.5. SCPI imposes additional requirements regarding how a compound command is parsed. Multiple \<PROGRAM MESSAGE UNIT> elements may be sent in a \<PROGRAM MESSAGE>. The first command is always referenced to the root node. Subsequent commands, however, are referenced to the same tree level as the previous command in a message unit. https://www.ivifoundation.org/docs/scpi-99.pdf

j123b567 commented 2 years ago

NETCONN_NOCOPY vs NETCONN_COPY is deffinitely a problem which sould be solved. Buffer with data is reused, while it is sending, which is wrong.

But, I don't understand the rest of the PR.

SCPI specification allows multiple PROGRAM MESSAGE UNIT in PROGRAM MESSAGE but I don't know, if it allows multiple "queries" (commands ending with ?) in single message. It is possibly this library extension (I hope it is not against the spec to support it).

MisterHW commented 2 years ago

rest of the PR

The other two commits probably need to be in another branch and require some more thought / a separate PR. This PR can then focus on 9afb267.

j123b567 commented 2 years ago

So, can you please remove the other two commits so this can be easilly merged?

MisterHW commented 2 years ago

two commits removed.

provide key-value pair formatting

will be revisited in another PR.