Skip to content

Conversation

@rdelagato
Copy link
Contributor

Handles receiving empty file
Retrieves next char from serial if expected char not received
Handles potentially echoed ACK, NAK
Removed second data purge, sender and receiver are usually synced after first purge
Address issue #44

Retrieves next char from serial if expected char not received
Handles potentially echoed ACK, NAK
Removed second data purge, sender and receiver are usually synced after first purge
@lgtm-com
Copy link

lgtm-com bot commented Jan 21, 2021

This pull request introduces 1 alert when merging 64f294e into a9d8514 - view on LGTM.com

new alerts:

  • 1 for Testing equality to None

@coveralls
Copy link

Coverage Status

Coverage decreased (-58.0%) to 0.0% when pulling 576216e on rdelagato:master into a9d8514 on tehmaze:master.

@coveralls
Copy link

coveralls commented Jan 21, 2021

Coverage Status

Coverage decreased (-0.9%) to 57.029% when pulling 576216e on rdelagato:master into a9d8514 on tehmaze:master.

@jquast
Copy link
Collaborator

jquast commented May 1, 2023

I cannot accept the fix, "Handles potentially echoed ACK, NAK". If I understand, you are coping with an endpoint, a sender, that sends a copy of any ACK or NAK we send back to us, is that what this is for? In particular, like a serial line or server with "ECHO" enabled. I have re-reviewed XMODEM draft documents, and our own send() function, has no reason to send ACK or NAK. And so, in the case of the next data packet, what if it also happened to begin with same byte as ACK or NAK? Wouldn't this corrupt that packet by discarding that character??

I would accept the fix, "Removed second data purge, sender and receiver are usually synced after first purge", but I don't understand, this code is implemented and quotes the original 1982 xmodem document, section "6. PROGRAMMING TIPS", so here I have a source that specifies the need to "Purge", but you are saying it is "usually" not needed, but that's just not good enough for me, some automatic test or steps with some other open source software like lrzsz would have to show that the current code fails and that this change fixes it.

@jquast jquast merged commit ebea3fe into tehmaze:master May 1, 2023
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

Successfully merging this pull request may close these issues.

3 participants