Issue #20 - TeraTerm XModem file transfers with W command.

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.print(F("debugRxDuplicates: "));
-    Serial.println(debugStartChar, HEX);
+    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

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

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

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
+// The original TommyPROM code used XModem CRC protocol but this requires parameters to be
-// programs do not seem to support this correctly.  The default is now to use basic XModem
+// 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.
-
+//#define XMODEM_CRC_PROTOCOL
-// 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
 
 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;
-            }
+                ++seq;
-            else
+            } 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 (unsigned ms = 30000; (ms); --ms)
-        for (int ms = 1000; (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;
 }
 
 

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);
 };
 

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