Merge pull request #22 from tehmaze/bugfix-block-persisting-retry

Bugfix block persisting retry

.gitignore

@@ -6,4 +6,6 @@
 /xmodem.egg-info
 /.coverage
 /.tox
+/.cache
+/.idea
 *.py.swp

README.rst

@@ -44,6 +44,16 @@ For more information, take a look at the documentation_.
 Changes
 =======
 
+0.4.4:
+  * bugfix: Large file transfers in ``send()`` were more likely to fail for
+    small values of ``retry``: This value should be the maximum failures per
+    block transfer as documented, but was improperly implemented as the number
+    of failures allowed for the total duration of the transfer, `PR #21
+    <https://github.com/tehmaze/xmodem/pull/21>`_.
+  * bugfix: ``send(retry=n)`` and ``recv(retry=n)`` would not retry ``n-1``
+    times, rather than ``n`` times as documented, causing 'retry=1' to never
+    retry, for example.
+
 0.4.3:
   * bugfix: ``putc()`` callback was called in series, 3 times for each part of
     xmodem block header, data, and checksum during block transfer.  Now all
@@ -57,7 +67,7 @@ Changes
     `Issue #16 <https://github.com/tehmaze/xmodem/issues/16>`_.
 
 0.4.1
-  * bugfix: re-transmit in send() on NAK or timeout, previously
+  * bugfix: re-transmit in ``send()`` on ``NAK`` or timeout, previously
     re-transmissions (wrongly) occurred only on garbage bytes.
     `PR #12 <https://github.com/tehmaze/xmodem/pull/12>`_.
 

setup.py

@@ -7,8 +7,8 @@
 
 setup(
     name='xmodem',
-    version='0.4.3',
-    author='Wijnand Modderman, Jeff Quast',
+    version='0.4.4',
+    author='Wijnand Modderman, Jeff Quast, Kris Hardy',
     author_email='[email protected]',
     description=('XMODEM protocol implementation.'),
     url='https://github.com/tehmaze/xmodem',

test/unit/test_xmodem.py

@@ -7,10 +7,10 @@
     from io import BytesIO
 except ImportError:
     # python 2
-    import StringIO.StringIO as BytesIO
+    from StringIO import StringIO as BytesIO
 
 # local
-import xmodem
+from xmodem import NAK, CRC, ACK, XMODEM
 
 # 3rd-party
 import pytest
@@ -24,24 +24,95 @@ def dummy_putc(data, timeout=1):
     return 0
 
 
+def test_xmodem_bad_mode():
+    # given,
+    mode = 'XXX'
+    modem = XMODEM(getc=dummy_getc, putc=dummy_putc, mode=mode)
+    # exercise
+    with pytest.raises(ValueError):
+        status = modem.send(BytesIO(b'dummy-stream'))
+
+
 @pytest.mark.parametrize('mode', ['xmodem', 'xmodem1k'])
 def test_xmodem_dummy_fails_send(mode):
     # given,
-    modem = xmodem.XMODEM(getc=dummy_getc,
-                          putc=dummy_putc,
-                          mode=mode)
+    modem = XMODEM(getc=dummy_getc, putc=dummy_putc, mode=mode)
     # exercise
     status = modem.send(BytesIO(b'dummy-stream'))
     # verify
     assert not status, ("Expected value of status `False'")
 
 
-def test_xmodem_bad_mode():
+@pytest.mark.parametrize('mode', ['xmodem', 'xmodem1k'])
+@pytest.mark.parametrize('stream_data', [BytesIO(b'dummy-stream ' * 17),
+                                         BytesIO(b'dummy-stream ' * 1000)])
+def test_xmodem_send_exceed_maximum_number_of_resend(mode, stream_data):
+    """ Verify send(retry=n) after 'n' transfer failures of single block. """
+
     # given,
-    mode = 'XXX'
-    modem = xmodem.XMODEM(getc=dummy_getc,
-                          putc=dummy_putc,
-                          mode=mode)
+    max_resend = 3
+
+    def getc_generator():
+        if mode == 'xmodem':
+            yield NAK
+        else:
+            # xmodem1k
+            yield CRC
+
+        if mode == 'xmodem':
+            yield ACK
+
+        for i in range(max_resend + 1):
+            yield None
+
+        while True:
+            yield ACK
+
+    mock = getc_generator()
+
+    def mock_getc(size, timeout=1):
+        return next(mock)
+
+    xmodem = XMODEM(getc=mock_getc, putc=dummy_putc, mode=mode)
+
     # exercise
-    with pytest.raises(ValueError):
-        status = modem.send(BytesIO(b'dummy-stream'))
+    result = xmodem.send(stream=stream_data, retry=max_resend)
+
+    # verify
+    assert not result
+
+
+@pytest.mark.parametrize('mode', ['xmodem', 'xmodem1k'])
+@pytest.mark.parametrize('stream_data', [BytesIO(b'dummy-stream ' * 17),
+                                         BytesIO(b'dummy-stream ' * 1000)])
+def test_xmodem_send_fails_once_each_packet(mode, stream_data):
+    """ Verify send(retry=n) under 'n' transfer failures of single block. """
+    # given,
+    max_resend = 1
+
+    def getc_generator():
+        if mode == 'xmodem':
+            yield NAK
+        else:
+            # xmodem1k
+            yield CRC
+
+        while True:
+            # fail
+            yield None
+
+            # succeed
+            yield ACK
+
+    mock = getc_generator()
+
+    def mock_getc(size, timeout=1):
+        return next(mock)
+
+    xmodem = XMODEM(getc=mock_getc, putc=dummy_putc, mode=mode)
+
+    # exercise
+    result = xmodem.send(stream=stream_data, retry=max_resend)
+
+    # verify
+    assert result

xmodem/__init__.py

@@ -112,7 +112,7 @@
 __copyright__ = ['Copyright (c) 2010 Wijnand Modderman',
                  'Copyright (c) 1981 Chuck Forsberg']
 __license__ = 'MIT'
-__version__ = '0.4.3'
+__version__ = '0.4.4'
 
 import platform
 import logging
@@ -210,7 +210,7 @@ def send(self, stream, retry=16, timeout=60, quiet=False, callback=None):
         '''
         Send a stream via the XMODEM protocol.
 
-            >>> stream = file('/etc/issue', 'rb')
+            >>> stream = open('/etc/issue', 'rb')
             >>> print(modem.send(stream))
             True
 
@@ -276,7 +276,7 @@ def callback(total_packets, success_count, error_count)
                                    'got %r', char)
 
             error_count += 1
-            if error_count >= retry:
+            if error_count > retry:
                 self.log.info('send error: error_count reached %d, '
                               'aborting.', retry)
                 self.abort(timeout=timeout)
@@ -308,14 +308,15 @@ def callback(total_packets, success_count, error_count)
                     success_count += 1
                     if callable(callback):
                         callback(total_packets, success_count, error_count)
+                    error_count = 0
                     break
 
                 self.log.error('send error: expected ACK; got %r for block %d',
                                char, sequence)
                 error_count += 1
                 if callable(callback):
                     callback(total_packets, success_count, error_count)
-                if error_count >= retry:
+                if error_count > retry:
                     # excessive amounts of retransmissions requested,
                     # abort transfer
                     self.log.error('send error: NAK received %d times, '
@@ -338,7 +339,7 @@ def callback(total_packets, success_count, error_count)
             else:
                 self.log.error('send error: expected ACK; got %r', char)
                 error_count += 1
-                if error_count >= retry:
+                if error_count > retry:
                     self.log.warn('EOT was not ACKd, aborting transfer')
                     self.abort(timeout=timeout)
                     return False
@@ -370,7 +371,7 @@ def recv(self, stream, crc_mode=1, retry=16, timeout=60, delay=1, quiet=0):
         '''
         Receive a stream via the XMODEM protocol.
 
-            >>> stream = file('/etc/issue', 'wb')
+            >>> stream = open('/etc/issue', 'wb')
             >>> print(modem.recv(stream))
             2342
 
@@ -466,7 +467,7 @@ def recv(self, stream, crc_mode=1, retry=16, timeout=60, delay=1, quiet=0):
                         print(err_msg, file=sys.stderr)
                     self.log.warn(err_msg)
                     error_count += 1
-                    if error_count >= retry:
+                    if error_count > retry:
                         self.log.info('error_count reached %d, aborting.',
                                       retry)
                         self.abort()