Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle errors from the read/write method #152

Open
DaveDavenport opened this issue Aug 25, 2023 · 11 comments
Open

Handle errors from the read/write method #152

DaveDavenport opened this issue Aug 25, 2023 · 11 comments

Comments

@DaveDavenport
Copy link

It looks like currently the return value from the read/write method (in scpi_interface_t) is not taken into account.

My use case:

I made an interface to a TCP socket, this all works. But when f.e. the connection gets disconnected while sending (and I return -1 in the write method), the scpi-parser keeps trying to send data like nothing happened.

I think it would be nice if it would handle this more gracefully, so I can recover from it more gracefully at a higher level.

I am on commit: 4e87990

@jancumps
Copy link

Dave, what would the higher level (the SCPI engine) have to do in that case?
Because we're implementing all inputs and outputs in our own code (in your case: integration with TCP sockets), would it work to develop the communication error handling in that layer too?

@DaveDavenport
Copy link
Author

I need to look into it in a bit more details.

At the moment I saw a lot of calls to the write method even after it errored out and I returned -1 (it got aborted in the middle of returning a large array of sample points).

I have not looked at how buffering is done in the library. I am also not sure what to do with the return value in the write method?

If I write 10 out of 20 bytes do I need to block until all 20 are written? or do I return 10 and then I get called again with the next 10 bytes?

In general (from orther libs that have a custom read/write method) I would expect the following:

  • I push in data as I receive it. the library either takes the whole buffer or part of it and tells me how much it consumed.
  • I write data and return how much data I've written. If I written less then the buffer, I get called again with the remainder or I always send the whole buffer (blocking) and return success/failure.
  • If I fail to write data I return an error (-1?) and the library goes into an error mode. How the error is handled (f.e. flush outstanding buffers and return to 'idle' operation) is up to the user (within the capabilities of the library).

I probably take a better look at this in the next few weeks.

@jancumps
Copy link

The tcp example writes 0 (not -1) if no values could be written.

@DaveDavenport
Copy link
Author

Is that taken into account?
If I look here: https://github.com/j123b567/scpi-parser/blob/master/libscpi/src/parser.c#L134 it does not seems to do anything with the result.
Also in other methods, it sums it up, but does not handle the result. (e.g. try resend/error out, etc.).

@jancumps
Copy link

that line above is to take care to send a separator back before the reply, except if it's the first reply.
There's no use for the return value in that situation - no upstream call to give it to.

The SCPI lib handles SCPI errors - and allows you push your your own to the scpi error stack.

It doesn't assume or do anything in the communication layer. I think it should also not assume a right way to handle errors in that layer.

@DaveDavenport
Copy link
Author

DaveDavenport commented Aug 26, 2023

I am confused.

If I fail to write data I want a way to stop the library from trying to push out data.

At the moment it seems to keep going as nothing is wrong. (and if in the middle or a large array this can be a lot of calls).

I could (in my interface function) ignore all followup calls and just discard the data? is this the way to 'handle errors'?

I would expect that if I return an error, the loop (at a higher level) will break out, and this will propagate back up.

@jancumps
Copy link

I think that if you could define

  • what the scpi lib should do, in case the communication layer flags it is in an error state and not able to write data,
  • what it should do with the (maybe lots of?) data while the error status is active, and
  • what it should do after that error state is resolved,

it could be turned into code.

@jancumps
Copy link

thought: an other option would be to flag the comms error the part of your design that generates that data. It can then stop pushing data to the SCPI lib.

@DaveDavenport
Copy link
Author

DaveDavenport commented Aug 26, 2023

I mostly want loops to break out, this is what I currently run in my copy.

(return value is unsigned, so check against 0 for now).

--- a/libscpi/src/parser.c
+++ b/libscpi/src/parser.c
@@ -1613,7 +1613,9 @@ static size_t produceResultArrayBinary(scpi_t * context, const void * array, siz
     if (format == SCPI_FORMAT_ASCII) {\
         size_t i;\
         for (i = 0; i < count; i++) {\
-            result += func(context, array[i]);\
+           size_t res = func(context, array[i]);\
+           if ( res == 0 ) { break; }\
+            result += res;\
         }\
     } else {\
         result = produceResultArrayBinary(context, array, count, sizeof(*array), format);\

This for me improves things a lot already, as it just aborts the write , makes it that I quicker return from my higher level call and I can start handling the disconnect correctly.

(hope I did not make typo, I just quickly tried to redo the patch quickly in one point, as I cannot test it right now and not at pc with the source)

@jancumps
Copy link

I see what you're doing there. That will work to make the functions that return arrays stop faster.
It has to be validated that none of the possible (func) can return 0 in a valid case ...

@DaveDavenport
Copy link
Author

DaveDavenport commented Aug 26, 2023

Yes, I want 'errors' to propagate as quickly as possible to the top level method, so I can start handling the disconnect/error/etc (the interface callback functions should be as simple/short as possible imho.).

I can try to make a PR later (next weeks), if this is acceptable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants