Partial fixes for Zmodem RX buffering problems.

include/zmodem.h

@@ -147,6 +147,23 @@
 #  define CONFIG_SYSTEM_ZMODEM_MAXERRORS 20
 #endif
 
+/* Some MMC/SD drivers may fail if large transfers are attempted. As a bug
+ * workaround, you can set the maximum write size with this configuration.
+ * The default value of 0 means no write limit.
+ */
+
+#ifndef CONFIG_SYSTEM_ZMODEM_WRITESIZE
+#  define CONFIG_SYSTEM_ZMODEM_WRITESIZE 0
+#endif
+
+/* Absolute pathes in received file names are not accepted.  This
+ * configuration value must be set to provide the path to the file storage
+ * directory (such as a mountpoint directory).
+ *
+ * Names of file send by the sz commond, on the other hand, must be absolute
+ * paths beginning with '/'.
+ */
+
 #ifndef CONFIG_SYSTEM_ZMODEM_MOUNTPOINT
 #  define CONFIG_SYSTEM_ZMODEM_MOUNTPOINT "/tmp"
 #endif
@@ -314,23 +331,6 @@ int zms_send(ZMSHANDLE handle, FAR const char *filename,
 
 int zms_release(ZMSHANDLE handle);
 
-/****************************************************************************
- * Name: zms_hwflowcontrol
- *
- * Description:
- *   If CONFIG_SYSTEM_ZMODEM_FULLSTREAMING is defined, then the system
- *   must provide the following interface in order to enable/disable hardware
- *   flow control on the device used to communicate with the remote peer.
- *
- * Returned Value:
- *   Zero on success; a negated errno value on failure.
- *
- ****************************************************************************/
-
-#ifdef CONFIG_SYSTEM_ZMODEM_FULLSTREAMING
-int zms_hwflowcontrol(int remfd, bool enable);
-#endif
-
 #undef EXTERN
 #if defined(__cplusplus)
 }

system/zmodem/Kconfig

@@ -9,15 +9,23 @@ config SYSTEM_ZMODEM
 	---help---
 		This selection enables the 'sz' and 'rz' NSH commands.
 
+		NOTE:  Hardware flow control is required on the selected device in
+		order to throttle the rates of data transfer to fit within the
+		allocated buffers.  This cannot be checked here in the configuration
+		because the device that you use may be selected at runtime.
+
 if SYSTEM_ZMODEM
 
 config SYSTEM_ZMODEM_DEVNAME
 	string "Default Zmodem device"
 	default "/dev/console"
-	---help--
+	---help---
 		The default device used by the Zmodem commands if the -d option is
 		not provided on the sz or rz command line.  Default:  "/dev/console".
 
+		NOTE:  This device *MUST* support Hardware flow control in order to
+		throttle the rates of data transfer to fit within the allocated buffers.
+
 config SYSTEM_ZMODEM_RCVBUFSIZE
 	int "Receive buffer size"
 	default 512
@@ -128,6 +136,14 @@ config SYSTEM_ZMODEM_MAXERRORS
 	---help---
 		Max receive errors before canceling the transfer.
 
+config SYSTEM_ZMODEM_WRITESIZE
+	int "Write size limit"
+	default 0
+	---help---
+		Some MMC/SD drivers may fail if large transfers are attempted.
+		As a bug workaround, you can set the maximum write size with
+		this configuration.  The default value of 0 means no write limit.
+
 config DEBUG_ZMODEM
 	bool "Zmodem debug"
 	default n

system/zmodem/README.txt

@@ -1,6 +1,59 @@
 README
 ======
 
+Buffering Notes
+===============
+
+  Hardware Flow Control
+  ---------------------
+  Hardware flow control must be enabled in serial drivers in order to
+  prevent data overrun.  However, in the most NuttX serial drivers, hardware
+  flow control only protects the hardware RX FIFO:  Data will not be lost in
+  the hardware FIFO but can still be lost when it is taken from the FIFO.
+  We can still overflow the serial driver's RX buffer even with hardware
+  flow control enabled! That is probably a bug.  But the workaround solution
+  that I have used is to use lower data rates and a large serial driver RX
+  buffer.
+
+  Those measures should be unnecessary if buffering and hardware flow
+  control are set up and working correctly.
+
+  RX Buffer Size
+  --------------
+  The Zmodem protocol supports a message that informs the file sender of
+  the maximum size of dat that you can buffer (ZRINIT).  However, my
+  experience is that the Linux sz ignores this setting and always sends file
+  data at the maximum size (1024) no matter what size of buffer you report.
+  That is unfortunate because that, combined with the possibilities of data
+  overrun mean that you must use quite large buffering for Zmodem file
+  receipt to be reliable (none of these issues effect sending of files).
+
+  Buffer Recommendations
+  ----------------------
+  Based on the limitations of NuttX hardware flow control and of the Linux
+  sz behavior, I have been testing with the following configuration
+  (assuming UART1 is the Zmodem device):
+
+    1) This setting determines that maximum size of a data packet frame:
+
+       CONFIG_SYSTEM_ZMODEM_PKTBUFSIZE=1024
+
+    2) Input Buffering.  If the input buffering is set to a full frame, then
+       data overflow is less likely.
+
+       CONFIG_UART1_RXBUFSIZE=1024
+
+    3) With a larger driver input buffer, the Zmodem receive I/O buffer can be
+       smaller:
+
+       CONFIG_SYSTEM_ZMODEM_RCVBUFSIZE=256
+
+    4) Output buffering.  Overrun cannot occur on output (on the NuttX side)
+       so there is no need to be so careful:
+
+       CONFIG_SYSTEM_ZMODEM_SNDBUFSIZE=512
+       CONFIG_UART1_TXBUFSIZE=256
+
 Using NuttX Zmodem with a Linux Host
 ====================================
 
@@ -8,12 +61,15 @@ Using NuttX Zmodem with a Linux Host
     --------------------------------------------------
     The NuttX Zmodem commands have been verified against the rzsz programs
     running on a Linux PC.  To send a file to the PC, first make sure that
-    the serial port is configured to work with the board:
+    the serial port is configured to work with the board (Assuming you are
+    using 9600 baud for the data transfers -- high rates may result in data
+    overruns):
 
-      $ sudo stty -F /dev/ttyS0 57600
+      $ sudo stty -F /dev/ttyS0 9600     # Select 9600 BAUD
-      $ sudo stty -F /dev/ttyS0
+      $ sudo stty -F /dev/ttyS0 crtscts  # Enables CTS/RTS handshaking
+      $ sudo stty -F /dev/ttyS0          # Show the TTY configuration
 
-    Start rz on the Linux host:
+    Start rz on the Linux host (using /dev/ttyS0 as an example):
 
       $ sudo rz </dev/ttyS0 >/dev/ttyS0
 
@@ -33,25 +89,35 @@ Using NuttX Zmodem with a Linux Host
     If you don't have the rz command on your Linux box, the package to
     install rzsz (or possibily lrzsz).
 
-    Then on the target:
+    Then on the target (using /dev/ttyS1 as an example).
 
       > sz -d /dev/ttyS1 <filename>
 
     Where filename is the full path to the file to send (i.e., it begins
-    with the '/' character).
+    with the '/' character).  /dev/ttyS1 or whatever device you select
+    *MUST* support Hardware flow control in order to throttle therates of
+    data transfer to fit within the allocated buffers.
 
     Receiving Files on the Target from the Linux Host PC
     ----------------------------------------------------
     To send a file to the target, first make sure that the serial port on the
-    host is configured to work with the board:
+    host is configured to work with the board (Assuming that you are using
+    9600 baud for the data transfers -- high rates may result in data
+    overruns):
 
-      $ sudo stty -F /dev/ttyS0 57600
+      $ sudo stty -F /dev/ttyS0 9600     # Select 9600 BAUD
-      $ sudo stty -F /dev/ttyS0
+      $ sudo stty -F /dev/ttyS0 crtscts  # Enables CTS/RTS handshaking
+      $ sudo stty -F /dev/ttyS0          # Show the TTY configuration
 
-    Start rz on the on the target:
+    Start rz on the on the target.  Here, in this example, we are using
+    /dev/ttyS1 to perform the transfer
 
       nsh> rz -d /dev/ttyS1
 
+    /dev/ttyS1 or whatever device you select *MUST* support Hardware flow
+    control in order to throttle therates of data transfer to fit within the
+    allocated buffers.
+
     Then use the sz command on Linux to send the file to the target:
 
       $ sudo sz <filename> t </dev/ttyS0 >/dev/ttyS0
@@ -68,3 +134,22 @@ Using NuttX Zmodem with a Linux Host
 
     If you don't have the az command on your Linux box, the package to
     install rzsz (or possibily lrzsz).
+
+    STATUS
+      2013-7-15:  I have tested with the configs/olimex-lpc1766stk
+        configuration.  I have been able to send large and small files with
+        the sz command.  I have been able to receive small files, but there
+        are problems receiving large files:  The Linux SZ does not obey the
+        buffering limits and continues to send data while rz is writing
+        the previously received data to the file and the serial driver's RX
+        buffer is overrun by a few bytes while the write is in progress.
+        As a result, when it reads the next buffer of data, a few bytes may
+        be missing (maybe 10). Either (1) we need a more courteous host
+        application, or (2) we need to greatly improve the target side
+        buffering capability!
+
+        My thought now is to implement the NuttX sz and rz commands as
+        PC side applications as well.  Matching both sides and obeying
+        the handshaking will solve the issues.  Another option might be
+        to fix the serial driver hardware flow control somehow.
+

system/zmodem/zm_receive.c

@@ -357,12 +357,6 @@ static int zmr_zsinit(FAR struct zm_state_s *pzm)
       pzm->flags |= ZM_FLAG_ESCCTRL;
     }
 
-  /* Enable hardware flow control if we will be streaming */
-
-#ifdef CONFIG_SYSTEM_ZMODEM_FULLSTREAMING
-  (void)zms_hwflowcontrol(pzmr->cmn.remfd, true);
-#endif
-
   /* Setup to receive a data packet.  Enter PSTATE_DATA */
 
   zm_readstate(pzm);
@@ -777,12 +771,15 @@ static int zmr_filedata(FAR struct zm_state_s *pzm)
              pzm->pstate, pzm->psubstate, PSTATE_IDLE, PIDLE_ZPAD);
       zmdbg("ZMR_STATE %d->%d\n",  pzm->state, ZMR_FINISH);
 
-      /* Revert to the IDLE stawte and send ZFERR */
+      /* Revert to the IDLE state, send ZFERR, and terminate the transfer
+       * with an error.
+       */
 
       pzm->state     = ZMR_FINISH;
       pzm->pstate    = PSTATE_IDLE;
       pzm->psubstate = PIDLE_ZPAD;
-      return zmr_fileerror(pzmr, ZFERR, (uint32_t)errorcode);
+      (void)zmr_fileerror(pzmr, ZFERR, (uint32_t)errorcode);
+      return -errorcode;
     }
 
   zmdbg("offset: %ld nchars: %d pkttype: %02x\n",

system/zmodem/zm_send.c

@@ -432,12 +432,6 @@ static int zms_zrinit(FAR struct zm_state_s *pzm)
       pzm->flags |= ZM_FLAG_ESCCTRL;
     }
 
-  /* Enable hardware flow control if we will be streaming */
-
-#ifdef CONFIG_SYSTEM_ZMODEM_FULLSTREAMING
-  (void)zms_hwflowcontrol(pzmr->cmn.remfd, true);
-#endif
-
   /* Check if the receiver supports full-duplex streaming
    *
    * ZCRCW:

system/zmodem/zm_state.c

@@ -628,6 +628,8 @@ static int zm_header(FAR struct zm_state_s *pzm, uint8_t ch)
 
 static int zm_data(FAR struct zm_state_s *pzm, uint8_t ch)
 {
+  int ret;
+
   /* ZDLE encountered in this state means that the following character is
    * escaped.  Escaped characters may appear anywhere within the data packet.
    */
@@ -713,7 +715,15 @@ static int zm_data(FAR struct zm_state_s *pzm, uint8_t ch)
      {
        /* We are at the end of the packet.  Check the CRC and post the event */
 
-       zm_dataevent(pzm);
+       ret = zm_dataevent(pzm);
+
+       /* The packet data has been processed.  Discard the old buffered
+        * packet data.
+        */
+
+       pzm->pktlen = 0;
+       pzm->ncrc   = 0;
+       return ret;
      }
    else if (pzm->ncrc > 1)
      {

system/zmodem/zm_utils.c

@@ -245,6 +245,7 @@ int zm_getc(int fd)
 ssize_t zm_write(int fd, FAR const uint8_t *buffer, size_t buflen)
 {
   ssize_t nwritten;
+  size_t wrsize;
   size_t total = 0;
 
   /* Read reading as necessary until the requested buffer is filled or until
@@ -253,10 +254,20 @@ ssize_t zm_write(int fd, FAR const uint8_t *buffer, size_t buflen)
 
   while (total < buflen)
     {
+#if CONFIG_SYSTEM_ZMODEM_WRITESIZE > 0
+       if (buflen > CONFIG_SYSTEM_ZMODEM_WRITESIZE)
+         {
+           wrsize = CONFIG_SYSTEM_ZMODEM_WRITESIZE;
+         }
+       else
+#endif
+         {
+           wrsize = buflen;
+         }
+
       /* Get the next gulp of data from the file */
 
-      nwritten = write(fd, buffer, buflen);
+      nwritten = write(fd, buffer, wrsize);
-
       if (nwritten < 0)
         {
           int errorcode = errno;