From e4703f4a882970f561c1ce89defe633669de5f10 Mon Sep 17 00:00:00 2001 From: Tom Nisbet Date: Sat, 14 Nov 2020 13:26:08 -0500 Subject: [PATCH] Issue #20 - TeraTerm XModem file transfers with W command. --- TommyPROM/PromDevice.cpp | 15 +++++-- TommyPROM/PromDevice.h | 5 ++- TommyPROM/TommyPROM.ino | 2 +- TommyPROM/XModem.cpp | 93 +++++++++++++++++++++++++++++----------- TommyPROM/XModem.h | 2 +- docs/index.md | 13 +----- 6 files changed, 88 insertions(+), 42 deletions(-) diff --git a/TommyPROM/PromDevice.cpp b/TommyPROM/PromDevice.cpp index 1c8d5ea..23d464e 100644 --- a/TommyPROM/PromDevice.cpp +++ b/TommyPROM/PromDevice.cpp @@ -71,7 +71,10 @@ void PromDevice::resetDebugStats() { debugLastAddress = 0; debugLastExpected = 0; debugLastReadback = 0; - debugStartChar = 0; + debugRxDuplicates = 0; + debugExtraChars = 0; + debugRxStarts = 0; + debugRxSyncErrors = 0; } void PromDevice::printDebugStats() { Serial.print(F("debugBlockWrites: ")); @@ -82,8 +85,14 @@ void PromDevice::printDebugStats() { Serial.println(debugLastExpected, HEX); Serial.print(F("debugLastReadback: ")); Serial.println(debugLastReadback, HEX); - Serial.print(F("debugStartChar: ")); - Serial.println(debugStartChar, HEX); + Serial.print(F("debugRxDuplicates: ")); + Serial.println(debugRxDuplicates); + Serial.print(F("debugExtraChars: ")); + Serial.println(debugExtraChars); + Serial.print(F("debugRxStarts: ")); + Serial.println(debugRxStarts); + Serial.print(F("debugRxSyncErrors: ")); + Serial.println(debugRxSyncErrors); } // BEGIN PRIVATE METHODS diff --git a/TommyPROM/PromDevice.h b/TommyPROM/PromDevice.h index 4105abe..4e1d581 100644 --- a/TommyPROM/PromDevice.h +++ b/TommyPROM/PromDevice.h @@ -32,7 +32,10 @@ class PromDevice uint32_t debugLastAddress; // Last address with an issue uint8_t debugLastExpected; // Last expected readback value uint8_t debugLastReadback; // Last actual readback value - uint8_t debugStartChar; // XModem start char sent or received + uint32_t debugRxDuplicates; // XModem received packet duplicates + uint32_t debugExtraChars; // XModem received bytes beyond first packet + uint8_t debugRxStarts; // XModem start transfer requests + uint8_t debugRxSyncErrors; // XModem unexpected characters after packet protected: uint32_t mSize; // Size of the device, in bytes diff --git a/TommyPROM/TommyPROM.ino b/TommyPROM/TommyPROM.ino index 1c5cf7b..07889cd 100644 --- a/TommyPROM/TommyPROM.ino +++ b/TommyPROM/TommyPROM.ino @@ -19,7 +19,7 @@ #include "XModem.h" -static const char * MY_VERSION = "2.6"; +static const char * MY_VERSION = "2.7"; // Global status diff --git a/TommyPROM/XModem.cpp b/TommyPROM/XModem.cpp index 0f7ff11..37e3991 100644 --- a/TommyPROM/XModem.cpp +++ b/TommyPROM/XModem.cpp @@ -2,17 +2,13 @@ #include "XModem.h" #include "CmdStatus.h" -// The original TommyPROM code used XModem CRC protocol but some Linux communication -// programs do not seem to support this correctly. The default is now to use basic XModem +// The original TommyPROM code used XModem CRC protocol but this requires parameters to be +// changed for the host communication program. The default is now to use basic XModem // with the 8-bit checksum instead of the 16-bit CRC error checking. This shouldn't // matter because communication errors aren't likely to happen on a 3 foot USB cable like // they did over the long distance dail-up lines that XModem was designed for. Uncomment // the XMODEM_CRC_PROTOCOL line below to restore the original XMODEM CRC support. - -// Update!!! - Teraterm does not seem to like sending files to TommyPROM with plain -// XModem, so the default is back to CRC. For Linux, comment out the line below to use -// checksum. -#define XMODEM_CRC_PROTOCOL +//#define XMODEM_CRC_PROTOCOL enum { @@ -37,6 +33,7 @@ enum PKTLEN = 128 }; +enum { RX_OK, RX_ERROR, RX_IGNORE }; uint32_t XModem::ReceiveFile(uint32_t address) { @@ -45,6 +42,7 @@ uint32_t XModem::ReceiveFile(uint32_t address) uint8_t seq = 1; uint32_t numBytes = 0; bool complete = false; + int status; #ifdef XMODEM_CRC_PROTOCOL Serial.println(F("Send the image file using XMODEM CRC")); @@ -70,15 +68,16 @@ uint32_t XModem::ReceiveFile(uint32_t address) { case XMDM_SOH: // Start of a packet - if (ReceivePacket(buffer, PKTLEN, seq++, address)) - { + status = ReceivePacket(buffer, PKTLEN, seq, address); + if (status == RX_OK) { + // Good packet - advance the buffer numBytes += PKTLEN; address += PKTLEN; - } - else - { + ++seq; + } else if (status == RX_ERROR) { return 0; } + // else ignore the duplicate packet break; case XMDM_EOT: @@ -129,7 +128,6 @@ bool XModem::SendFile(uint32_t address, uint32_t fileSize) { rxChar = GetChar(); } - promDevice.debugStartChar = rxChar; if (rxChar != XMDM_TRANSFER_START) { #ifdef XMODEM_CRC_PROTOCOL @@ -182,13 +180,14 @@ void XModem::Cancel() // Private functions int XModem::GetChar(int msWaitTime) { + msWaitTime *= 4; do { if (Serial.available() > 0) { return Serial.read(); } - delay(1); + delayMicroseconds(250); } while (msWaitTime--); return -1; @@ -219,21 +218,22 @@ uint16_t XModem::UpdateCrc(uint16_t crc, uint8_t data) bool XModem::StartReceive() { - for (int retries = 30; (retries); --retries) + delay(2000); + for (int retries = 20; (retries); --retries) { // Send the XMDM_TRANSFER_START until the sender of the file responds with // something. The start character will be sent once a second for a number of // seconds. If nothing is received in that time then return false to indicate // that the transfer did not start. + ++promDevice.debugRxStarts; Serial.write(XMDM_TRANSFER_START); - promDevice.debugStartChar = XMDM_TRANSFER_START; - for (int ms = 1000; (ms); --ms) + for (unsigned ms = 30000; (ms); --ms) { + delayMicroseconds(100); if (Serial.available() > 0) { return true; } - delay(1); } } @@ -241,7 +241,7 @@ bool XModem::StartReceive() } -bool XModem::ReceivePacket(uint8_t buffer[], unsigned bufferSize, uint8_t seq, uint32_t destAddr) +int XModem::ReceivePacket(uint8_t buffer[], unsigned bufferSize, uint8_t seq, uint32_t destAddr) { int c; uint8_t rxSeq1, rxSeq2; @@ -263,7 +263,7 @@ bool XModem::ReceivePacket(uint8_t buffer[], unsigned bufferSize, uint8_t seq, u cmdStatus.error("Timeout waiting for next packet char."); cmdStatus.setValueDec(0, "seq", seq); Serial.write(XMDM_CAN); - return false; + return RX_ERROR; } buffer[ix] = (uint8_t) c; calcCrc = UpdateCrc(calcCrc, buffer[ix]); @@ -275,7 +275,52 @@ bool XModem::ReceivePacket(uint8_t buffer[], unsigned bufferSize, uint8_t seq, u #else rxCrc = GetChar(); #endif - if ((calcCrc != rxCrc) || (rxSeq1 != seq) || ((rxSeq1 ^ rxSeq2) != 0xff)) + + // At this point in the code, there should not be anything in the receive buffer + // because the sender has just sent a complete packet and is waiting on a response. If + // the XModem transfer is not started quickly on the host side, TeraTerm seems to + // buffer the NAK character that starts the transfer and use it as an indication that + // the first packet should be sent again. That can cause the Arduino's 64 byte buffer + // to overflow and gets the transfer out of sync. The normal processing will detect + // the first packet and will process it correctly, but the partial packet in the + // buffer will cause the processing of the next packet to fail. + delay(10); + if (Serial.available()) { + ++promDevice.debugRxSyncErrors; + if (seq == 1) { + // If any characters are in the buffer after the first packet, quietly eat + // them to get back on a packet boundary and send a NAK to cause a retransmit + // of the packet. + while (Serial.available() > 0) { + delay(1); + Serial.read(); + ++promDevice.debugExtraChars; + } + delay(1); + Serial.write(XMDM_NAK); + return RX_IGNORE; + } else { + // Sync issues shouldn't happen at any other time so fail the transfer. + cmdStatus.error("RX sync error."); + cmdStatus.setValueDec(0, "seq", seq); + cmdStatus.setValueDec(1, "rxSeq1", rxSeq1); + cmdStatus.setValueDec(2, "rxSeq2", rxSeq2); + cmdStatus.setValueHex(3, "calcCrc", calcCrc); + cmdStatus.setValueHex(4, "rxCrc", rxCrc); + Serial.write(XMDM_CAN); + return RX_ERROR; + } + } + + if ((calcCrc == rxCrc) && (rxSeq1 == seq - 1) && ((rxSeq1 ^ rxSeq2) == 0xff)) + { + // Resend of previously processed packet. + delay(10); + ++promDevice.debugRxDuplicates; + Serial.write(XMDM_ACK); + return RX_IGNORE; + } + else if ((calcCrc != rxCrc) || (rxSeq1 != seq) || ((rxSeq1 ^ rxSeq2) != 0xff)) { // Fail if the CRC or sequence number is not correct or if the two received // sequence numbers are not the complement of one another. @@ -286,7 +331,7 @@ bool XModem::ReceivePacket(uint8_t buffer[], unsigned bufferSize, uint8_t seq, u cmdStatus.setValueHex(3, "calcCrc", calcCrc); cmdStatus.setValueHex(4, "rxCrc", rxCrc); Serial.write(XMDM_CAN); - return false; + return RX_ERROR; } else { @@ -295,12 +340,12 @@ bool XModem::ReceivePacket(uint8_t buffer[], unsigned bufferSize, uint8_t seq, u { cmdStatus.error("Write failed"); cmdStatus.setValueHex(0, "address", destAddr); - return false; + return RX_ERROR; } Serial.write(XMDM_ACK); } - return true; + return RX_OK; } diff --git a/TommyPROM/XModem.h b/TommyPROM/XModem.h index b4e6824..b388904 100644 --- a/TommyPROM/XModem.h +++ b/TommyPROM/XModem.h @@ -33,7 +33,7 @@ class XModem int GetChar(int msWaitTime = 3000); uint16_t UpdateCrc(uint16_t crc, uint8_t data); bool StartReceive(); - bool ReceivePacket(uint8_t buffer[], unsigned bufferSize, uint8_t seq, uint32_t destAddr); + int ReceivePacket(uint8_t buffer[], unsigned bufferSize, uint8_t seq, uint32_t destAddr); void SendPacket(uint32_t address, uint8_t seq); }; diff --git a/docs/index.md b/docs/index.md index 0230f13..7db83e8 100755 --- a/docs/index.md +++ b/docs/index.md @@ -94,22 +94,11 @@ received file size. Once the READ or WRITE command is issued to the programmer, the transfer must be started on the host program. -~~Note that previous versions of TommyPROM used the XMODEM-CRC protocol to complete the file +Note that previous versions of TommyPROM used the XMODEM-CRC protocol to complete the file transfers for the READ and WRITE commands. This did not work well with minicom and other Linux programs that rely on the sz/rz commands. Versions 2.5 and later of TommyPROM now use basic XModem with the 8-bit checksum. The XModem-CRC support is still available as a compile-time option. See [issue #19](https://github.com/TomNisbet/TommyPROM/issues/19) -for details.~~ - -**UPDATE: XModem troubles continue!** - -It seems that TeraTerm does not like to send files to TommyPROM in XModem checksum mode. -The default transfer has been restored to XModem-CRC mode. -* For TeraTerm, be sure to select the XModem-CRC option in the dialog box when receiving files. -* For Linux/minicom, comment out the XMODEM_CRC_PROTOCOL in XModem.cpp and use checksum mode. - -Given that there are now problems with both minicomm and TeraTerm, it seems likely that the -actual problem lies somewhere in the TommyPROM code. See [issue #20](https://github.com/TomNisbet/TommyPROM/issues/20) for details. The files used for READ and WRITE are simple binary images. This can be created directly