From 2ec900deababd3ae9be28e9673764866f1a72fbc Mon Sep 17 00:00:00 2001 From: Gregory Nutt Date: Mon, 15 Jul 2013 12:33:35 -0600 Subject: Partial fixes for Zmodem RX buffering problems. --- apps/include/zmodem.h | 34 ++++++------- apps/system/zmodem/Kconfig | 18 ++++++- apps/system/zmodem/README.txt | 105 ++++++++++++++++++++++++++++++++++++---- apps/system/zmodem/zm_receive.c | 13 ++--- apps/system/zmodem/zm_send.c | 6 --- apps/system/zmodem/zm_state.c | 12 ++++- apps/system/zmodem/zm_utils.c | 15 +++++- 7 files changed, 158 insertions(+), 45 deletions(-) (limited to 'apps') diff --git a/apps/include/zmodem.h b/apps/include/zmodem.h index bc85ba0a1..64951a7b8 100644 --- a/apps/include/zmodem.h +++ b/apps/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) } diff --git a/apps/system/zmodem/Kconfig b/apps/system/zmodem/Kconfig index 7d84d1812..aa6da18e2 100644 --- a/apps/system/zmodem/Kconfig +++ b/apps/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 diff --git a/apps/system/zmodem/README.txt b/apps/system/zmodem/README.txt index ebde16c1d..4217e66aa 100755 --- a/apps/system/zmodem/README.txt +++ b/apps/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 + $ sudo stty -F /dev/ttyS0 9600 # Select 9600 BAUD + $ 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 @@ -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 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 + $ sudo stty -F /dev/ttyS0 9600 # Select 9600 BAUD + $ 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 t /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. + diff --git a/apps/system/zmodem/zm_receive.c b/apps/system/zmodem/zm_receive.c index e172b5740..e8c0968ac 100644 --- a/apps/system/zmodem/zm_receive.c +++ b/apps/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", diff --git a/apps/system/zmodem/zm_send.c b/apps/system/zmodem/zm_send.c index 9bcc23581..8ba31abc1 100644 --- a/apps/system/zmodem/zm_send.c +++ b/apps/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: diff --git a/apps/system/zmodem/zm_state.c b/apps/system/zmodem/zm_state.c index be0285d85..fb12569ea 100644 --- a/apps/system/zmodem/zm_state.c +++ b/apps/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) { diff --git a/apps/system/zmodem/zm_utils.c b/apps/system/zmodem/zm_utils.c index 95eae7e8d..7717f6cbe 100644 --- a/apps/system/zmodem/zm_utils.c +++ b/apps/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) { - /* Get the next gulp of data from the file */ +#if CONFIG_SYSTEM_ZMODEM_WRITESIZE > 0 + if (buflen > CONFIG_SYSTEM_ZMODEM_WRITESIZE) + { + wrsize = CONFIG_SYSTEM_ZMODEM_WRITESIZE; + } + else +#endif + { + wrsize = buflen; + } - nwritten = write(fd, buffer, buflen); + /* Get the next gulp of data from the file */ + nwritten = write(fd, buffer, wrsize); if (nwritten < 0) { int errorcode = errno; -- cgit v1.2.3