Arguably, the Continue patch may break for users relying on undocumented/unspecified behavior. The other two patches should be straight-forward improvements.
I didn't think of the server side, as I didn't need it. Will have a look.
A pen scanner I have uses proprietary protocol over OBEX over IrLAP over RS-232/IrDA, but doesn't support any OBEX commands except CONNECT and DISCONNECT. All other requests use the user-defined range. Unfortunately they seem to return Continue rather than Success for every request. It essentially means I have to let the application layer handle the continue logic as it is on a case-to-case basis if it makes sense or not.
It is a bit hacky, I know, but I think overall it makes sense for OpenOBEX not to make make assumptions about behavior that is unspecified.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Actually, IrDA is a kind of transport that is hard to support these days. I have no current hardware that has IrDA. But I will add all the fixes needed to make this hardware work for you :-)
Regards...
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
I've added the same code for the server. (I have no idea what it means for a server to receive a response, so I'll just assume this was the right spot.)
Looking at the code, I don't see why only GET could reach that piece. I'm (trying to) follow IrDA OBEX 1.2, and it seems pretty symmetric when it comes to Continue handling in GET and PUT. If you have any other info, I'm all-ears. :)
I found another issue today with IrOBEX_TransportConnect() not always returning error when it should. (It caused busy-wait in HandleInput() later.) Attached that new patch.
Well, *_request_rx|tx are for the request sending state. From the client's POV, only PUT requests can take more than one OBEX part. IIRC, all other commands from OBEX base specification are required to contain all request parameters in the first packet.
The *_response_rx|tx are for the chatting after the request finished sending. From the client's POV, only GET requests can have more than one part here.
If I'm thinking this wrong, then all types are allowed for CONTINUE and your patch is wrong ;-)
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Apologies for a long post, but here is my reasoning. Short version: We probably agree on GET and PUT, but not how it generalizes to other request types.
I think we agree that *_response_rx() is called for all responses sent by the remote end, no matter what command was used. At least, this is my understanding.
We then have three cases: GET, PUT, and others. I'm only talking of *_response_rx() here since that is where Continue is being handled.
For GET, we agree that any non-last response can be Continue, and then we move back to sending a "continuation request," which is basically going through all non-sent headers and sends them in a new request. For GET, there are no new headers (by specification), so the continuation request is empty. This goes on until a request with the final bit is sent. Then the server responds with OK.
For PUT, I argue we have the same case, just that usually the queue of unsent headers is not empty. The server will send Continue, and the client will respond by sending a continuation request, just like it does for GET. This goes on until the client sends a final PUT, in which case the server responds with OK. This is from the PUT example on p. 28 of IrOBEX 1.2. I think we agree even on this case, but we are not on the same page on how that generalizes to other requests.
So, for GET and PUT (but no others) there are specific details on what should happen when the server sends Continue (sections 3.3.3.2 and 3.3.4). The IrOBEX 1.2 specification mandates a continuation request if a Continue is seen for GET and PUT. However, it does not state that Continue should be handled that way for any other request types. That is left up to the implementation, it seems.
In my case, I have a device that sends Continue even though the host should not immediately send a continuation request. This particular request is a poll to see if there is more data, so I want to send it, receive a reply and then wait some fraction of a second before I send the next request. In a way, they are continuation requests with delays.
This could be implemented in OpenOBEX as a user-configurable policy on when to schedule continuation requests (essentially a rate-limit,) but since the specification does not say how to handle Continue in general, I think it makes more sense for OpenOBEX to simply do hands-off for any request not GET or PUT.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Ah, now I see what you want to achieve. Stopping is already managable using suspend/resume. However, a callback event at the right moment (before obex_client_response_tx_prepare() is called) is missing, so that you can call OBEX_SuspendRequest()
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Looking at the code, I agree suspending right before handling Continue would probably solve my issue. And to clean up the request, CancelRequest() seems valid at that point. (Since the scanner never sends OK, I need a way to just stop when I need to send other requests.)
If you prefer that solution, I can change the patch to insert a new event delivery instead. Any name preference for the event? I see server sends PROGRESS , but that's after the Continue decision. So maybe just something like RESPRECVD right before the if-statements?
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
How about an OBEX_EV_CONTINUE right after the "if (rsp == OBEX_RSP_CONTINUE) {" line? Some checks for suspend and abort are probably needed at some places :-)
I applied patch #4 to my (currently still local) git repository.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Here is a mostly untested version. It works with CancelRequest(nice=false).
I have yet to figure out good ways to test SuspendRequest() and CancelRequest(true).
The general idea is that
object==NULL could happen, and that's bad.
For object->abort==true, response_tx_prepare() handles it for us.
For object->suspended==true, object_append_data() handles it for us, but it will probably cause response_rx() to return RESULT_ERROR, which isn't very nice. OTOH that seems to follow the general pattern for object->suspended==true.
I have no way of testing SuspendRequest() as the device doesn't support GET/PUT-like continuations. It seems a bit hairy to get right with the state/direction to avoid infinite work loops, etc., so I'm simply stating only CancelRequest() is allowed.
Last edit: tgaz 2014-02-18
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Final attempt. Really scoped down to my needs. Adds EV_CONTINUE (client-side) and allows you to do CancelRequest(). I couldn't test the nice==true case fully since no request handles ABORT (it simply doesn't respond.) Sending it works, and I think that's good enough testing. Those 0.0001% out there who need this functionality can hopefully improve the code if needed. ;)
I applied the patches #1 and #3 but I cannot quite agree with patch #2.
Additionally, what are those "Some user-defined commands abuse Continue".
A pen scanner I have uses proprietary protocol over OBEX over IrLAP over RS-232/IrDA, but doesn't support any OBEX commands except CONNECT and DISCONNECT. All other requests use the user-defined range. Unfortunately they seem to return Continue rather than Success for every request. It essentially means I have to let the application layer handle the continue logic as it is on a case-to-case basis if it makes sense or not.
It is a bit hacky, I know, but I think overall it makes sense for OpenOBEX not to make make assumptions about behavior that is unspecified.
Nice :-)
Actually, IrDA is a kind of transport that is hard to support these days. I have no current hardware that has IrDA. But I will add all the fixes needed to make this hardware work for you :-)
Regards...
I've added the same code for the server. (I have no idea what it means for a server to receive a response, so I'll just assume this was the right spot.)
Looking at the code, I don't see why only GET could reach that piece. I'm (trying to) follow IrDA OBEX 1.2, and it seems pretty symmetric when it comes to Continue handling in GET and PUT. If you have any other info, I'm all-ears. :)
I found another issue today with IrOBEX_TransportConnect() not always returning error when it should. (It caused busy-wait in HandleInput() later.) Attached that new patch.
Well, *_request_rx|tx are for the request sending state. From the client's POV, only PUT requests can take more than one OBEX part. IIRC, all other commands from OBEX base specification are required to contain all request parameters in the first packet.
The *_response_rx|tx are for the chatting after the request finished sending. From the client's POV, only GET requests can have more than one part here.
If I'm thinking this wrong, then all types are allowed for CONTINUE and your patch is wrong ;-)
Apologies for a long post, but here is my reasoning. Short version: We probably agree on GET and PUT, but not how it generalizes to other request types.
I think we agree that *_response_rx() is called for all responses sent by the remote end, no matter what command was used. At least, this is my understanding.
We then have three cases: GET, PUT, and others. I'm only talking of *_response_rx() here since that is where Continue is being handled.
For GET, we agree that any non-last response can be Continue, and then we move back to sending a "continuation request," which is basically going through all non-sent headers and sends them in a new request. For GET, there are no new headers (by specification), so the continuation request is empty. This goes on until a request with the final bit is sent. Then the server responds with OK.
For PUT, I argue we have the same case, just that usually the queue of unsent headers is not empty. The server will send Continue, and the client will respond by sending a continuation request, just like it does for GET. This goes on until the client sends a final PUT, in which case the server responds with OK. This is from the PUT example on p. 28 of IrOBEX 1.2. I think we agree even on this case, but we are not on the same page on how that generalizes to other requests.
So, for GET and PUT (but no others) there are specific details on what should happen when the server sends Continue (sections 3.3.3.2 and 3.3.4). The IrOBEX 1.2 specification mandates a continuation request if a Continue is seen for GET and PUT. However, it does not state that Continue should be handled that way for any other request types. That is left up to the implementation, it seems.
In my case, I have a device that sends Continue even though the host should not immediately send a continuation request. This particular request is a poll to see if there is more data, so I want to send it, receive a reply and then wait some fraction of a second before I send the next request. In a way, they are continuation requests with delays.
This could be implemented in OpenOBEX as a user-configurable policy on when to schedule continuation requests (essentially a rate-limit,) but since the specification does not say how to handle Continue in general, I think it makes more sense for OpenOBEX to simply do hands-off for any request not GET or PUT.
Ah, now I see what you want to achieve. Stopping is already managable using suspend/resume. However, a callback event at the right moment (before obex_client_response_tx_prepare() is called) is missing, so that you can call OBEX_SuspendRequest()
:)
Looking at the code, I agree suspending right before handling Continue would probably solve my issue. And to clean up the request, CancelRequest() seems valid at that point. (Since the scanner never sends OK, I need a way to just stop when I need to send other requests.)
If you prefer that solution, I can change the patch to insert a new event delivery instead. Any name preference for the event? I see server sends PROGRESS , but that's after the Continue decision. So maybe just something like RESPRECVD right before the if-statements?
How about an OBEX_EV_CONTINUE right after the "if (rsp == OBEX_RSP_CONTINUE) {" line? Some checks for suspend and abort are probably needed at some places :-)
I applied patch #4 to my (currently still local) git repository.
Here is a mostly untested version. It works with
CancelRequest(nice=false)
.I have yet to figure out good ways to test
SuspendRequest()
andCancelRequest(true)
.The general idea is that
object==NULL
could happen, and that's bad.object->abort==true
,response_tx_prepare()
handles it for us.object->suspended==true
,object_append_data()
handles it for us, but it will probably causeresponse_rx()
to returnRESULT_ERROR
, which isn't very nice. OTOH that seems to follow the general pattern forobject->suspended==true
.I have no way of testing
SuspendRequest()
as the device doesn't support GET/PUT-like continuations. It seems a bit hairy to get right with the state/direction to avoid infinite work loops, etc., so I'm simply stating onlyCancelRequest()
is allowed.Last edit: tgaz 2014-02-18
Final attempt. Really scoped down to my needs. Adds
EV_CONTINUE
(client-side) and allows you to doCancelRequest()
. I couldn't test thenice==true
case fully since no request handlesABORT
(it simply doesn't respond.) Sending it works, and I think that's good enough testing. Those 0.0001% out there who need this functionality can hopefully improve the code if needed. ;)The 4 patches of yours are now in the git repository at gitorious.org/openobex.
Thanks