SDO block download process doesn't continued right after sequence break

[lukegluke]

Here the monitor log for block download process (SDO client is the third party device, SDO server is 0x29 CANopenNode from git master branch):

As you see SDO client broke sequence on 107 segment (caused by not relevant internal issue in SDO client). CANopenNode correctly sent block download response with last successfully received segment number 107. SDO client started resend segments from 108, but when it get up to last segment in block (but not in the whole data) CANopenNode not response anything and then sent SDO abort after timeout.

The reason:
After seq break CO_SDO_process goes into CO_SDO_ST_DOWNLOAD_BL_SUB_RESP state, where it sends response and also clears SDO->sequence and return to CO_SDO_ST_DOWNLOAD_BL_SUBBLOCK state. But all further messages will be ignored like sequence was not started and client should retransmit block from the beginning - it is not right.

...
seqno=107       SDO->sequence=107
seqno=123       SDO->sequence=107
seqno=108       SDO->sequence=0
seqno=109       SDO->sequence=0
seqno=110       SDO->sequence=0
...
seqno=127       SDO->sequence=0

So in case of sequence break in CO_SDO_ST_DOWNLOAD_BL_SUB_RESP state SDO->sequence must not be cleared. But it must if it is last segment in whole data transfer or also last segment in current block (SDO->sequence >= SDO->blksize).

But zeroed SDO->sequence also is used to decide to do SDO abort timeout or just download response in first time.
How we can do this accurately? Should the timeoutSubblockDownolad be the "global" variable to indicate of abort needed instead of zeroed SDO->sequence?

[lukegluke]

I'm working on the fix. Adding additional state CO_SDO_ST_DOWNLOAD_BL_SUB_RESP_2, if resting sequence is not needed, seems nice to me.
But I wonder about this code

    if(SDO->timeoutTimer >= SDOtimeoutTime){
        if((SDO->state == CO_SDO_ST_DOWNLOAD_BL_SUBBLOCK) && (SDO->sequence != 0U) && (!SDO->CANtxBuff->bufferFull)){
            timeoutSubblockDownolad = true;
            state = CO_SDO_ST_DOWNLOAD_BL_SUB_RESP_2;
        }
        else{
            CO_SDO_abort(SDO, CO_SDO_AB_TIMEOUT); /* SDO protocol timed out */
            return -1;
        }
    }

@CANopenNode, could you please point me where in specifications this is mentioned that abort code is sent only on the second timeout?

Update: In fact I agree with this logic, cause client can send last segment in block and waiting for response, but if it was lost it is not right to abort whole transaction.

[lukegluke]

Made pull request.
Thanks to different issues with hardware buffer of external SDO client I was able to test sequence break case:

and also timeout case I was mentioned:

Now they are handled right and SDO block download process is continued.

[CANopenNode]

Thank you for this. It is merged.

I'm currently working on split-driver. Will check other SDO issues later.

[lukegluke]

Thanks for your work.
I'm going to move to spilt-driver one time. Is it already in a sufficient degree of readiness? It will be merged in master someday, right?
I am going to fix #39, because it vital to our system. Should I do this in current master, current split-driver or wait a little bit?

[CANopenNode]

Yes, split-driver will be next mayor release. It is working, but I will make some more tests. There is no extra new features, no deeper changes, it is reorganized and clarified.
Then next mayor release will be newObjectDictionary and then I don't plan any more reorganization.

I will fix bugs also in current master and all changes in master will be also updated in split-driver.

So if it is easier for you to stay on master some time and do changes there it is OK.