Added check of block-writing return status

[0r10nV]

It was noticed that there is no feedback on block writing actions, even writing to 'read only' commands seems like action is succeded. Moreover some vendor specific 'writable' commands (i.e. authentication ones) can accept only defined size blocks, sending to them blocks of other lengthes would not be accepted and usually NACKed (virtually returned status can be different from data length being sent in those cases). So I have added check whether block-writing succeded successfully or not.

[karosium]

Thanks, but

if (status>=0 && status!=strlen(block)/2)

will never be true right now. The result is either the full length of the block upon success or the libusb error code for when the firmware returns FALSE. I guess this would be feedback for which byte the controller starts NACKing at but it would have to be implemented first in both the firmware and the library.

Without that the best would be to just add the missing

                if (status >=0) {
                    if (verbose) printf("OK\n");
                    exit(0);
                } else {
                    printf("Error %d\n",status);
                    exit(status);
                }

to both WriteByte and WriteBlock (or move it outside the if chain and just add status= to the write commands)

[0r10nV]

Yes, in practice I have not seen cases that condition

if (status>=0 && status!=strlen(block)/2)

to be true, and proposed short form of check is sufficient.

It was added just for virtual cases if SMB Slave writable command will not accept block length other then specified for that command (i.e. SHA-1 Authentication commands, which accept 20 data bytes strings only, and NACKed if another length string was sent). For the controllers which were checked, in such cases they normally NACKed, but another ones could return status which is not equall to byte string being sent (I'm not sure here, need more controllers for testing, so added that option for just in case).

2018-05-02 16:14 GMT+03:00 Viktor notifications@github.com:

Thanks, but

if (status>=0 && status!=strlen(block)/2)

will never be true right now. The result is either the full length of the block upon success or the libusb error code for when the firmware returns FALSE. I guess this would be feedback for which byte the controller starts NACKing at but it would have to be implemented first in both the firmware and the library.

Without that the best would be to just add the missing

          if (status >=0) {
              if (verbose) printf("OK\n");
              exit(0);
          } else {
              printf("Error %d\n",status);
              exit(status);
          }

to both WriteByte and WriteBlock (or move it outside the if chain and just add status= to the write commands)

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/karosium/smbusb/pull/8#issuecomment-385973331, or mute the thread https://github.com/notifications/unsubscribe-auth/Ak-Xg5WH3bWPunv7YoIDLedl_Fbw86-sks5tubEhgaJpZM4TvU5b .

[karosium]

That makes sense and I agree that this would be a nice feature to have but the firmware does not currently support this. The status value is all or nothing in the current implementation. Either the entire block write succeeds (return = blocksize) or it fails (return = error code). Status will be the same error code if the controller NACKs at the length byte or any other byte. What you're trying to use is not currently implemented in the firmware nor the library so that message is relying on functionality that doesn't exist and will never get called. I'm closing this but feel free to either implement the missing functionality for that message or submit a pull request without it. Otherwise I'll add the error message fix eventually.

[0r10nV]

I have looked a little into firmware sources to check whether it possible to add this new functionality but there is a question whether we could return arbitrary data back to host when using ENDPOINT OUT transfer function, 'cause EP buffer can not be used as it's taken by host.

===== (extract from smbusb.c for SMBWriteBlock() function) ===== status = libusb_control_transfer(device, LIBUSB_ENDPOINT_OUT | LIBUSB_REQUEST_TYPE_VENDOR | LIBUSB_RECIPIENT_DEVICE, SMB_WRITE_BLOCK, address, command, (void)(tmp+(i64)), 64, 100);

===== (extract from smbusb_firmware.c for SMBWriteBlock() case) ===== case SMB_WRITE_BLOCK: EP0BCL=0; // read from the host

[karosium]

Yeah, the same issue came up with PEC error location.

A similar workaround would be using a global var, clearing it at the beginning of every r/w call and incrementing it for every ACK. Then you'd just need to add a command to the firmware for querying that variable and modify the library to call it and return the value in case it gets a fail status back for the read/write command (making sure to let other, relevant libusb error codes through).