From b1c09f6d1c3316a3f725fadf803a8e974df07c19 Mon Sep 17 00:00:00 2001 From: Kyle Altendorf Date: Sat, 16 Jan 2021 09:05:05 -0500 Subject: [PATCH 01/49] minimal gha This is just to confirm it is running before adding lots of config (presumably with errors...). --- .github/workflows/ci.yml | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) create mode 100644 .github/workflows/ci.yml diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml new file mode 100644 index 000000000..01f7f5f5c --- /dev/null +++ b/.github/workflows/ci.yml @@ -0,0 +1,25 @@ +name: CI + +on: + push: + branches: + - dev + - master + tags: + - v* + pull_request: + branches: + - "*" + schedule: + # Daily at 05:14 + - cron: '14 5 * * *' + +jobs: + all: + name: All + runs-on: ubuntu-latest + steps: + - name: This + shell: python + run: | + import this From d923e218b7d44e4e9869ee5c2706e98c2ca4a5a8 Mon Sep 17 00:00:00 2001 From: Kyle Altendorf Date: Sat, 16 Jan 2021 09:14:06 -0500 Subject: [PATCH 02/49] add in actual testing --- .github/workflows/ci.yml | 102 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 102 insertions(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 01f7f5f5c..12be8bc33 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -15,9 +15,111 @@ on: - cron: '14 5 * * *' jobs: + test: + # Should match JOB_NAME below + name: ${{ matrix.task.name }} - ${{ matrix.os.name }} ${{ matrix.qt_library.name }} ${{ matrix.python.name }} ${{ matrix.arch.name }} + runs-on: ${{ matrix.os.runs-on }} + container: ${{ matrix.os.container[matrix.python.docker] }} + strategy: + fail-fast: false + matrix: + task: + - name: Test + tox: test + coverage: true + os: + - name: Linux + runs-on: ubuntu-latest + python_platform: linux + matrix: linux + container: + 2.7: docker://python:2.7-buster + 3.6: docker://python:3.6-buster + 3.7: docker://python:3.7-buster + 3.8: docker://python:3.8-buster + 3.9: docker://python:3.9-buster + pypy2: docker://pypy:2-jessie + pypy3: docker://pypy:3-stretch + - name: Windows + runs-on: windows-latest + python_platform: win32 + matrix: windows + - name: macOS + runs-on: macos-latest + python_platform: darwin + matrix: macos + python: + - name: CPython 3.6 + tox: py36 + action: 3.6 + docker: 3.6 + - name: CPython 3.7 + tox: py37 + action: 3.7 + docker: 3.7 + - name: CPython 3.8 + tox: py38 + action: 3.8 + docker: 3.8 +# - name: CPython 3.9 +# tox: py39 +# action: 3.9 +# docker: 3.9 + arch: + - name: x86 + action: x86 + matrix: x86 + - name: x64 + action: x64 + matrix: x64 + exclude: + - os: + matrix: linux + arch: + matrix: x86 + - os: + matrix: macos + arch: + matrix: x86 + env: + # Should match name above + JOB_NAME: ${{ matrix.task.name }} - ${{ matrix.os.name }} ${{ matrix.qt_library.name }} ${{ matrix.python.name }} ${{ matrix.arch.name }} + steps: + - uses: actions/checkout@v2 + with: + fetch-depth: 0 + - name: Set up ${{ matrix.python.name }} + if: job.container == '' + uses: actions/setup-python@v2 + with: + python-version: '${{ matrix.python.action }}.0-alpha - ${{ matrix.python.action }}.X' + architecture: '${{ matrix.arch.action }}' + - name: Install + run: | + pip install --upgrade pip setuptools wheel + pip install --upgrade tox coveralls + - uses: twisted/python-info-action@v1.0.1 + - name: Test + run: | + tox -vv -e ${{ matrix.python.tox }} + - name: Coverage Processing + if: matrix.task.coverage + run: | + mkdir coverage_reports + cp .coverage "coverage_reports/coverage.${{ env.JOB_NAME }}" + cp coverage.xml "coverage_reports/coverage.${{ env.JOB_NAME }}.xml" + coveralls + - name: Publish Coverage + if: matrix.task.coverage + uses: actions/upload-artifact@v2 + with: + name: coverage + path: coverage_reports/* all: name: All runs-on: ubuntu-latest + needs: + - test steps: - name: This shell: python From 9dddf845826791d2c7d7e45ca9456467eadb94bc Mon Sep 17 00:00:00 2001 From: Kyle Altendorf Date: Sat, 16 Jan 2021 09:25:41 -0500 Subject: [PATCH 03/49] misc --- .github/workflows/ci.yml | 24 ++++++++++++++++++++---- tox.ini | 6 ++++-- 2 files changed, 24 insertions(+), 6 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 12be8bc33..7823e2371 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -49,6 +49,10 @@ jobs: python_platform: darwin matrix: macos python: + - name: CPython 2.7 + tox: py27 + action: 2.7 + docker: 2.7 - name: CPython 3.6 tox: py36 action: 3.6 @@ -61,10 +65,22 @@ jobs: tox: py38 action: 3.8 docker: 3.8 -# - name: CPython 3.9 -# tox: py39 -# action: 3.9 -# docker: 3.9 + - name: CPython 3.9 + tox: py39 + action: 3.9 + docker: 3.9 + - name: PyPy 2.7 + tox: pypy27 + action: pypy-2.7 + docker: pypy2.7 + - name: PyPy 3.6 + tox: pypy36 + action: pypy-3.6 + docker: pypy3.6 + - name: PyPy 3.7 + tox: pypy37 + action: pypy-3.7 + docker: pypy3.7 arch: - name: x86 action: x86 diff --git a/tox.ini b/tox.ini index 909d6a74d..247cde1f5 100644 --- a/tox.ini +++ b/tox.ini @@ -4,11 +4,13 @@ # directory. [tox] -envlist = py27, py35, py36, py37, pypy +envlist = py{27,py27,36,37,38,py36,py37} [testenv] deps = -r requirements-tests.txt -commands = py.test {posargs} +commands = + py{27,py27}: py.test {posargs} + py{36,37,38,py36,py37}: py.test {posargs} test/test_server_asyncio.py test setenv = with_gmp=no [flake8] From a66951091f57af191efb36d7562540ac68aa0cce Mon Sep 17 00:00:00 2001 From: Kyle Altendorf Date: Sat, 16 Jan 2021 09:45:05 -0500 Subject: [PATCH 04/49] actually use coverage in tox --- tox.ini | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/tox.ini b/tox.ini index 247cde1f5..44d52f11e 100644 --- a/tox.ini +++ b/tox.ini @@ -9,9 +9,10 @@ envlist = py{27,py27,36,37,38,py36,py37} [testenv] deps = -r requirements-tests.txt commands = - py{27,py27}: py.test {posargs} - py{36,37,38,py36,py37}: py.test {posargs} test/test_server_asyncio.py test -setenv = with_gmp=no + pytest {posargs:--cov=pymodbus/ --cov-report term-missing {env:PYTEST_PY3_ARGS}} +setenv = + with_gmp=no + py{36,37,38,py36,py37}: PYTEST_PY3_ARGS = test/test_server_asyncio.py test [flake8] exclude = .tox From 6f114d0baf3895e3319b8a4b12f70997d2a8ff4b Mon Sep 17 00:00:00 2001 From: Kyle Altendorf Date: Sat, 16 Jan 2021 09:47:34 -0500 Subject: [PATCH 05/49] coverage xml --- tox.ini | 1 + 1 file changed, 1 insertion(+) diff --git a/tox.ini b/tox.ini index 44d52f11e..9a965648b 100644 --- a/tox.ini +++ b/tox.ini @@ -10,6 +10,7 @@ envlist = py{27,py27,36,37,38,py36,py37} deps = -r requirements-tests.txt commands = pytest {posargs:--cov=pymodbus/ --cov-report term-missing {env:PYTEST_PY3_ARGS}} + coverage xml setenv = with_gmp=no py{36,37,38,py36,py37}: PYTEST_PY3_ARGS = test/test_server_asyncio.py test From e09746ce7e656a745fec0e26622e22d02decb4cb Mon Sep 17 00:00:00 2001 From: Kyle Altendorf Date: Sat, 16 Jan 2021 10:00:11 -0500 Subject: [PATCH 06/49] skip coveralls upload for now --- .github/workflows/ci.yml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 7823e2371..7be0d2c13 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -124,7 +124,8 @@ jobs: mkdir coverage_reports cp .coverage "coverage_reports/coverage.${{ env.JOB_NAME }}" cp coverage.xml "coverage_reports/coverage.${{ env.JOB_NAME }}.xml" - coveralls +# TODO: deal with the classic secrets and forked PRs issue... +# coveralls - name: Publish Coverage if: matrix.task.coverage uses: actions/upload-artifact@v2 From 1117b1b03f6a5cbe353b3ef27bd5f5acc0ebc50b Mon Sep 17 00:00:00 2001 From: Kyle Altendorf Date: Sat, 16 Jan 2021 10:01:09 -0500 Subject: [PATCH 07/49] remove travis to avoid continued spamming with pointless builds --- .travis.yml | 35 ----------------------------------- scripts/travis.sh | 11 ----------- 2 files changed, 46 deletions(-) delete mode 100644 .travis.yml delete mode 100755 scripts/travis.sh diff --git a/.travis.yml b/.travis.yml deleted file mode 100644 index ae4bc08e5..000000000 --- a/.travis.yml +++ /dev/null @@ -1,35 +0,0 @@ -sudo: false -language: python -matrix: - include: - - os: linux - python: "2.7" - - os: linux - python: "3.6" - - os: linux - python: "3.7" - - os: linux - python: "3.8" - - os: osx - osx_image: xcode8.3 - language: generic -before_install: - - if [ $TRAVIS_OS_NAME = osx ]; then brew update; fi - - if [ $TRAVIS_OS_NAME = osx ]; then brew install openssl; fi -# - if [$TRAVIS_OS_NAME = osx ]; then python -c "import fcntl; fcntl.fcntl(1, fcntl.F_SETFL, 0)"; fi - -install: -# - scripts/travis.sh pip install pip-accel - - if [ $TRAVIS_OS_NAME = osx ]; then scripts/travis.sh pip install -U "\"setuptools<45"\"; else pip install -U setuptools --upgrade ; fi - - scripts/travis.sh pip install coveralls - - scripts/travis.sh pip install --requirement=requirements-checks.txt - - scripts/travis.sh pip install --requirement=requirements-tests.txt - - scripts/travis.sh LC_ALL=C pip install . -script: -# - scripts/travis.sh make check - - scripts/travis.sh make test -after_success: - - scripts/travis.sh coveralls -branches: - except: - - /^[0-9]/ diff --git a/scripts/travis.sh b/scripts/travis.sh deleted file mode 100755 index 8f4338270..000000000 --- a/scripts/travis.sh +++ /dev/null @@ -1,11 +0,0 @@ -#!/bin/bash -e -set -x -if [ "$TRAVIS_OS_NAME" = osx ]; then - VIRTUAL_ENV="$HOME/.virtualenvs/python2.7" - if [ ! -x "$VIRTUAL_ENV/bin/python" ]; then - virtualenv "$VIRTUAL_ENV" - fi - source "$VIRTUAL_ENV/bin/activate" -fi - -eval "$@" From d75533255e9bada24caf98e08efc726976e8ccc9 Mon Sep 17 00:00:00 2001 From: Kyle Altendorf Date: Sat, 16 Jan 2021 10:31:44 -0500 Subject: [PATCH 08/49] adjust py2 exclusions --- test/conftest.py | 13 ++++++++++--- tox.ini | 7 ++++--- 2 files changed, 14 insertions(+), 6 deletions(-) diff --git a/test/conftest.py b/test/conftest.py index 748fd4b7a..c8e3a2df7 100644 --- a/test/conftest.py +++ b/test/conftest.py @@ -1,3 +1,10 @@ -from pymodbus.compat import IS_PYTHON3, PYTHON_VERSION -if not IS_PYTHON3 or IS_PYTHON3 and PYTHON_VERSION.minor < 7: - collect_ignore = ["test_server_asyncio.py"] +from pymodbus.compat import PYTHON_VERSION + +if PYTHON_VERSION < (3,): + collect_ignore = [ + # TODO: do these really need to be ignored on py2 or can they just get + # super() etc fixed? + "test_client_async.py", + "test_client_async_tornado.py", + "test_server_asyncio.py", + ] diff --git a/tox.ini b/tox.ini index 9a965648b..c5238f7bd 100644 --- a/tox.ini +++ b/tox.ini @@ -9,11 +9,12 @@ envlist = py{27,py27,36,37,38,py36,py37} [testenv] deps = -r requirements-tests.txt commands = - pytest {posargs:--cov=pymodbus/ --cov-report term-missing {env:PYTEST_PY3_ARGS}} - coverage xml + pytest {posargs:--cov=pymodbus/ --cov-report term-missing} + # TODO: do this unconditionally once i figure out the exclusion of + # async/await files in py2 + py{36,37,38,py36,py37}: coverage xml setenv = with_gmp=no - py{36,37,38,py36,py37}: PYTEST_PY3_ARGS = test/test_server_asyncio.py test [flake8] exclude = .tox From a0fb7f9576292f3bd69766b7439d1bfb415bcc2d Mon Sep 17 00:00:00 2001 From: Kyle Altendorf Date: Sat, 16 Jan 2021 10:38:53 -0500 Subject: [PATCH 09/49] coverage xml --ignore-errors --- tox.ini | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/tox.ini b/tox.ini index c5238f7bd..1ec82ddc1 100644 --- a/tox.ini +++ b/tox.ini @@ -10,9 +10,7 @@ envlist = py{27,py27,36,37,38,py36,py37} deps = -r requirements-tests.txt commands = pytest {posargs:--cov=pymodbus/ --cov-report term-missing} - # TODO: do this unconditionally once i figure out the exclusion of - # async/await files in py2 - py{36,37,38,py36,py37}: coverage xml + coverage xml --ignore-errors setenv = with_gmp=no From e52dfa274e46fc07360280fdf4af9c91aac4c59f Mon Sep 17 00:00:00 2001 From: Kyle Altendorf Date: Sat, 16 Jan 2021 10:43:28 -0500 Subject: [PATCH 10/49] fix python action for pypy --- .github/workflows/ci.yml | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 7be0d2c13..8c3385ddc 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -104,12 +104,18 @@ jobs: - uses: actions/checkout@v2 with: fetch-depth: 0 - - name: Set up ${{ matrix.python.name }} - if: job.container == '' + - name: Set up ${{ matrix.python.name }} (if CPython) + if: ${{ job.container == '' && matrix.python.implementation == 'cpython'}} uses: actions/setup-python@v2 with: python-version: '${{ matrix.python.action }}.0-alpha - ${{ matrix.python.action }}.X' architecture: '${{ matrix.arch.action }}' + - name: Set up ${{ matrix.python.name }} (if PyPy) + if: ${{ job.container == '' && matrix.python.implementation == 'pypy'}} + uses: actions/setup-python@v2 + with: + python-version: '${{ matrix.python.action }}' + architecture: '${{ matrix.arch.action }}' - name: Install run: | pip install --upgrade pip setuptools wheel From 612dbfb8aff6e43ed37f5e7b58a816680a01742f Mon Sep 17 00:00:00 2001 From: Kyle Altendorf Date: Sat, 16 Jan 2021 10:45:54 -0500 Subject: [PATCH 11/49] add matrix.python.implementation --- .github/workflows/ci.yml | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 8c3385ddc..c4cddd8be 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -53,34 +53,42 @@ jobs: tox: py27 action: 2.7 docker: 2.7 + implementation: cpython + - name: PyPy 2.7 + tox: pypy27 + action: pypy-2.7 + docker: pypy2.7 + implementation: pypy - name: CPython 3.6 tox: py36 action: 3.6 docker: 3.6 + implementation: cpython - name: CPython 3.7 tox: py37 action: 3.7 docker: 3.7 + implementation: cpython - name: CPython 3.8 tox: py38 action: 3.8 docker: 3.8 + implementation: cpython - name: CPython 3.9 tox: py39 action: 3.9 docker: 3.9 - - name: PyPy 2.7 - tox: pypy27 - action: pypy-2.7 - docker: pypy2.7 + implementation: cpython - name: PyPy 3.6 tox: pypy36 action: pypy-3.6 docker: pypy3.6 + implementation: pypy - name: PyPy 3.7 tox: pypy37 action: pypy-3.7 docker: pypy3.7 + implementation: pypy arch: - name: x86 action: x86 From f62b12d13c4a5efa07a3062e0929cf28b4427f64 Mon Sep 17 00:00:00 2001 From: Kyle Altendorf Date: Sat, 16 Jan 2021 10:54:19 -0500 Subject: [PATCH 12/49] use twisted's serial extra --- requirements-tests.txt | 2 +- setup.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/requirements-tests.txt b/requirements-tests.txt index eba656a38..b7d4bb23c 100644 --- a/requirements-tests.txt +++ b/requirements-tests.txt @@ -14,6 +14,6 @@ sqlalchemy>=1.1.15 #wsgiref>=0.1.2 verboselogs >= 1.5 tornado==4.5.3 -Twisted>=20.3.0 +Twisted[serial]>=20.3.0 zope.interface>=4.4.0 asynctest>=0.10.0 diff --git a/setup.py b/setup.py index 60e7c4eef..20b210c05 100644 --- a/setup.py +++ b/setup.py @@ -83,7 +83,7 @@ 'sphinx_rtd_theme', 'humanfriendly'], 'twisted': [ - 'twisted >= 20.3.0', + 'twisted[serial] >= 20.3.0', 'pyasn1 >= 0.1.4', ], 'tornado': [ From f599c3b3a7e71cbe2eeca6f2615c8c9f1db3db7e Mon Sep 17 00:00:00 2001 From: Kyle Altendorf Date: Sat, 16 Jan 2021 11:42:24 -0500 Subject: [PATCH 13/49] pytest-cov for xml --- tox.ini | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tox.ini b/tox.ini index 1ec82ddc1..b0fb1fd25 100644 --- a/tox.ini +++ b/tox.ini @@ -9,8 +9,7 @@ envlist = py{27,py27,36,37,38,py36,py37} [testenv] deps = -r requirements-tests.txt commands = - pytest {posargs:--cov=pymodbus/ --cov-report term-missing} - coverage xml --ignore-errors + pytest {posargs:--cov=pymodbus/ --cov-report=term-missing --cov-report=xml} setenv = with_gmp=no From d64ec353be873955f2b21f23e9ae0615fc0e6e6c Mon Sep 17 00:00:00 2001 From: Kyle Altendorf Date: Sat, 16 Jan 2021 14:28:04 -0500 Subject: [PATCH 14/49] exclude windows and macos They can be enabled in separate PRs that fix their issues. --- .github/workflows/ci.yml | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index c4cddd8be..fd0ed6107 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -40,14 +40,14 @@ jobs: 3.9: docker://python:3.9-buster pypy2: docker://pypy:2-jessie pypy3: docker://pypy:3-stretch - - name: Windows - runs-on: windows-latest - python_platform: win32 - matrix: windows - - name: macOS - runs-on: macos-latest - python_platform: darwin - matrix: macos +# - name: Windows +# runs-on: windows-latest +# python_platform: win32 +# matrix: windows +# - name: macOS +# runs-on: macos-latest +# python_platform: darwin +# matrix: macos python: - name: CPython 2.7 tox: py27 From 36dba19452704344907f42ce1ecc2bb3baf522ba Mon Sep 17 00:00:00 2001 From: Kyle Altendorf Date: Sat, 16 Jan 2021 14:42:30 -0500 Subject: [PATCH 15/49] Fixup test_client_async[_tornado].py for py2 testing --- pymodbus/client/asynchronous/tornado/__init__.py | 2 +- test/conftest.py | 6 ++---- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/pymodbus/client/asynchronous/tornado/__init__.py b/pymodbus/client/asynchronous/tornado/__init__.py index 29e0e4db2..bbf9585ed 100644 --- a/pymodbus/client/asynchronous/tornado/__init__.py +++ b/pymodbus/client/asynchronous/tornado/__init__.py @@ -315,7 +315,7 @@ def __init__(self, *args, **kwargs): self.silent_interval = 3.5 * self._t0 self.silent_interval = round(self.silent_interval, 6) self.last_frame_end = 0.0 - super().__init__(*args, **kwargs) + super(AsyncModbusSerialClient, self).__init__(*args, **kwargs) def get_socket(self): """ diff --git a/test/conftest.py b/test/conftest.py index c8e3a2df7..861074ca3 100644 --- a/test/conftest.py +++ b/test/conftest.py @@ -1,10 +1,8 @@ from pymodbus.compat import PYTHON_VERSION if PYTHON_VERSION < (3,): + # These files use syntax introduced in Python 3 (not necessarily 3.0) and + # just won't be run during tests in Python 2. collect_ignore = [ - # TODO: do these really need to be ignored on py2 or can they just get - # super() etc fixed? - "test_client_async.py", - "test_client_async_tornado.py", "test_server_asyncio.py", ] From b12dd6a0b61f01f54ba2c9f8b1ae9e541c1c3d98 Mon Sep 17 00:00:00 2001 From: Kyle Altendorf Date: Sat, 16 Jan 2021 14:44:12 -0500 Subject: [PATCH 16/49] add cpython39 to tox.ini --- tox.ini | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tox.ini b/tox.ini index b0fb1fd25..64a3721fe 100644 --- a/tox.ini +++ b/tox.ini @@ -4,7 +4,7 @@ # directory. [tox] -envlist = py{27,py27,36,37,38,py36,py37} +envlist = py{27,py27,36,37,38,39,py36,py37} [testenv] deps = -r requirements-tests.txt From 9a98534334f8565ac12194392b475cdbaec54d98 Mon Sep 17 00:00:00 2001 From: Kyle Altendorf Date: Sat, 16 Jan 2021 15:50:11 -0500 Subject: [PATCH 17/49] preliminary addition of checks --- .github/workflows/ci.yml | 47 +++++++++++++++++++++++++++++++++++++++- tox.ini | 13 +++++++++++ 2 files changed, 59 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index fd0ed6107..ed4fdab00 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -26,7 +26,6 @@ jobs: task: - name: Test tox: test - coverage: true os: - name: Linux runs-on: ubuntu-latest @@ -146,11 +145,57 @@ jobs: with: name: coverage path: coverage_reports/* + check: + # Should match JOB_NAME below + name: ${{ matrix.task.name }} - ${{ matrix.os.name }} ${{ matrix.qt_library.name }} ${{ matrix.python.name }} ${{ matrix.arch.name }} + runs-on: ${{ matrix.os.runs-on }} + container: ${{ matrix.os.container[matrix.python.docker] }} + strategy: + fail-fast: false + matrix: + task: + - name: flake8 + tox: flake8 + - name: Docs + tox: docs + os: + - name: Linux + runs-on: ubuntu-latest + python_platform: linux + matrix: linux + container: + 3.8: docker://python:3.8-buster + python: + - name: CPython 3.8 + tox: py38 + action: 3.8 + docker: 3.8 + implementation: cpython + arch: + - name: x64 + action: x64 + matrix: x64 + env: + # Should match name above + JOB_NAME: ${{ matrix.task.name }} - ${{ matrix.os.name }} ${{ matrix.qt_library.name }} ${{ matrix.python.name }} ${{ matrix.arch.name }} + steps: + - uses: actions/checkout@v2 + with: + fetch-depth: 0 + - name: Install + run: | + pip install --upgrade pip setuptools wheel + pip install --upgrade tox + - uses: twisted/python-info-action@v1.0.1 + - name: Test + run: | + tox -vv -e ${{ matrix.task.tox }} all: name: All runs-on: ubuntu-latest needs: - test + - check steps: - name: This shell: python diff --git a/tox.ini b/tox.ini index 64a3721fe..c09224bc3 100644 --- a/tox.ini +++ b/tox.ini @@ -13,6 +13,19 @@ commands = setenv = with_gmp=no +[testenv:flake8] +deps = -r requirements-checks.txt +commands = + flake8 + +[testenv:docs] +allowlist_externals = + make +deps = -r requirements-docs.txt +commands = + make -C doc/ clean + make -C doc/ html + [flake8] exclude = .tox ignore = D211,D400,E731 From 9615b68a1ef99c356e3ef94e836eb0e8e759320e Mon Sep 17 00:00:00 2001 From: Kyle Altendorf Date: Sat, 16 Jan 2021 16:07:30 -0500 Subject: [PATCH 18/49] switch to the maintained m2r2 fork --- doc/conf.py | 2 +- requirements-docs.txt | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/doc/conf.py b/doc/conf.py index 294b41b79..4f0dc4122 100644 --- a/doc/conf.py +++ b/doc/conf.py @@ -47,7 +47,7 @@ # ones. #extensions = ['sphinx.ext.autodoc', 'm2r', 'recommonmark'] -extensions = ['sphinx.ext.autodoc', 'm2r'] +extensions = ['sphinx.ext.autodoc', 'm2r2'] # Add any paths that contain templates here, relative to this directory. templates_path = ['_templates'] diff --git a/requirements-docs.txt b/requirements-docs.txt index f1165c9c6..b0a4d2416 100644 --- a/requirements-docs.txt +++ b/requirements-docs.txt @@ -13,5 +13,5 @@ tornado>=4.5.3 # Required to parse some files Twisted>=17.1.0 # Required to parse some files prompt_toolkit>=2.0.4 click>=7.0 -m2r>=0.2.0 +m2r2>=0.2.0 From 5faa8fe3eb346b2e50b4b9d13482bcb899f9ba2b Mon Sep 17 00:00:00 2001 From: Kyle Altendorf Date: Sun, 17 Jan 2021 09:35:01 -0500 Subject: [PATCH 19/49] python3.7 for checks flake8 supposedly works better here --- .github/workflows/ci.yml | 8 ++++---- tox.ini | 1 + 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index ed4fdab00..9cac6ebb6 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -166,10 +166,10 @@ jobs: container: 3.8: docker://python:3.8-buster python: - - name: CPython 3.8 - tox: py38 - action: 3.8 - docker: 3.8 + - name: CPython 3.7 + tox: py37 + action: 3.7 + docker: 3.7 implementation: cpython arch: - name: x64 diff --git a/tox.ini b/tox.ini index c09224bc3..00ee19b88 100644 --- a/tox.ini +++ b/tox.ini @@ -14,6 +14,7 @@ setenv = with_gmp=no [testenv:flake8] +basepython = python3.7 deps = -r requirements-checks.txt commands = flake8 From 5fe4e4a500204d71255615e6ea2b6340ea8733bb Mon Sep 17 00:00:00 2001 From: Kyle Altendorf Date: Sun, 17 Jan 2021 09:42:32 -0500 Subject: [PATCH 20/49] a bit more for flake8 --- .github/workflows/ci.yml | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 9cac6ebb6..c05ce93dc 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -164,13 +164,20 @@ jobs: python_platform: linux matrix: linux container: + 3.7: docker://python:3.7-buster 3.8: docker://python:3.8-buster python: + # Using 3.7 for now since flake8 is super noisy with 3.8 - name: CPython 3.7 tox: py37 action: 3.7 docker: 3.7 implementation: cpython +# - name: CPython 3.8 +# tox: py38 +# action: 3.8 +# docker: 3.8 +# implementation: cpython arch: - name: x64 action: x64 From 26c0f1693a3aa7dc9d5aee4b40de1e1c475cda33 Mon Sep 17 00:00:00 2001 From: Kyle Altendorf Date: Sun, 17 Jan 2021 10:26:18 -0500 Subject: [PATCH 21/49] back to 3.8 for checks and continue on error for flake8 --- .github/workflows/ci.yml | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index c05ce93dc..64f2410a8 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -156,6 +156,7 @@ jobs: task: - name: flake8 tox: flake8 + continue_on_error: true - name: Docs tox: docs os: @@ -164,20 +165,13 @@ jobs: python_platform: linux matrix: linux container: - 3.7: docker://python:3.7-buster 3.8: docker://python:3.8-buster python: - # Using 3.7 for now since flake8 is super noisy with 3.8 - - name: CPython 3.7 - tox: py37 - action: 3.7 - docker: 3.7 + - name: CPython 3.8 + tox: py38 + action: 3.8 + docker: 3.8 implementation: cpython -# - name: CPython 3.8 -# tox: py38 -# action: 3.8 -# docker: 3.8 -# implementation: cpython arch: - name: x64 action: x64 @@ -195,6 +189,7 @@ jobs: pip install --upgrade tox - uses: twisted/python-info-action@v1.0.1 - name: Test + continue-on-error: matrix.task.continue_on_error run: | tox -vv -e ${{ matrix.task.tox }} all: From b490edbacfa3f108e127c0b0da6d4f6a9820d870 Mon Sep 17 00:00:00 2001 From: Kyle Altendorf Date: Sun, 17 Jan 2021 10:29:05 -0500 Subject: [PATCH 22/49] ${{ }} --- .github/workflows/ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 64f2410a8..70646e3fe 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -189,7 +189,7 @@ jobs: pip install --upgrade tox - uses: twisted/python-info-action@v1.0.1 - name: Test - continue-on-error: matrix.task.continue_on_error + continue-on-error: ${{ matrix.task.continue_on_error }} run: | tox -vv -e ${{ matrix.task.tox }} all: From 5fbe39900078f7dacd1a255edf0d0d9555992d7f Mon Sep 17 00:00:00 2001 From: Kyle Altendorf Date: Mon, 18 Jan 2021 07:47:10 -0500 Subject: [PATCH 23/49] Document 2.7, 3.6-3.9 support in classifiers and python_requires --- setup.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/setup.py b/setup.py index 60e7c4eef..d237288fd 100644 --- a/setup.py +++ b/setup.py @@ -47,6 +47,13 @@ """, classifiers=[ 'Development Status :: 4 - Beta', + "Programming Language :: Python :: 2", + "Programming Language :: Python :: 2.7", + "Programming Language :: Python :: 3", + "Programming Language :: Python :: 3.6", + "Programming Language :: Python :: 3.7", + "Programming Language :: Python :: 3.8", + "Programming Language :: Python :: 3.9", 'Environment :: Console', 'Environment :: X11 Applications :: GTK', 'Framework :: Twisted', @@ -71,6 +78,7 @@ platforms=['Linux', 'Mac OS X', 'Win'], include_package_data=True, zip_safe=True, + python_requires='>=2.7,!=3.0.*,!=3.1.*,!=3.2.*,!=3.3.*,!=3.4.*,!=3.5.*', install_requires=install_requires, extras_require={ 'quality': [ From c709c62b6066163fb0ccbbad9703c8b30f8ab432 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thor=20Michael=20St=C3=B8re?= Date: Mon, 21 Dec 2020 15:23:16 +0100 Subject: [PATCH 24/49] Improve sync client unit tests Performance: Patch out time.time() with itertools.count() in both test_client_sync and test_transaction; client.timeout now will be the number of reads being made + 1, this also speeds up execution of test suite (from 10.8s to 4.3s in local testing, with test.test_client_sync going from 2.1s to 0.05s). Coverage, pymodbus/client/sync.py: Add tests of idle_time(), debug_enabled(), trace(...), _dump(...) to testBaseModbusClient. Add test of _recv(None) to testTlsClientRecv. Invoke send/recv instead of _send/_recv, the former of which invoke the latter, increasing coverage. Expand test of serial client instantiation to ModbusSocketFramer; while this code does not make sense it does exist, and it should therefore be tested or removed. Test is_socket_open methods. Add test of connecting to an RTU serial client. Add test of serial client with client.timeout set to None. Fix name of method for testing tcp client repr to testTcpClientRpr. Coverage, pymodbus/client/sync.py: Add test of broadcast and handle_local_echo with wrong response. Missed statements go from 861 to 830. --- test/test_client_sync.py | 68 +++++++++++++++++++++++++++++++++++----- test/test_transaction.py | 25 ++++++++++++++- 2 files changed, 85 insertions(+), 8 deletions(-) mode change 100644 => 100755 test/test_transaction.py diff --git a/test/test_client_sync.py b/test/test_client_sync.py index 1014a9382..5b59dede3 100644 --- a/test/test_client_sync.py +++ b/test/test_client_sync.py @@ -1,5 +1,7 @@ #!/usr/bin/env python import unittest +from itertools import count +from io import StringIO from pymodbus.compat import IS_PYTHON3 if IS_PYTHON3: # Python 3 @@ -17,6 +19,8 @@ from pymodbus.exceptions import ParameterException from pymodbus.transaction import ModbusAsciiFramer, ModbusRtuFramer from pymodbus.transaction import ModbusBinaryFramer +from pymodbus.transaction import ModbusSocketFramer +from pymodbus.utilities import hexlify_packets # ---------------------------------------------------------------------------# @@ -62,8 +66,8 @@ def testBaseModbusClient(self): client = BaseModbusClient(None) client.transaction = None self.assertRaises(NotImplementedException, lambda: client.connect()) - self.assertRaises(NotImplementedException, lambda: client._send(None)) - self.assertRaises(NotImplementedException, lambda: client._recv(None)) + self.assertRaises(NotImplementedException, lambda: client.send(None)) + self.assertRaises(NotImplementedException, lambda: client.recv(None)) self.assertRaises(NotImplementedException, lambda: client.__enter__()) self.assertRaises(NotImplementedException, lambda: client.execute()) self.assertRaises(NotImplementedException, lambda: client.is_socket_open()) @@ -71,6 +75,20 @@ def testBaseModbusClient(self): client.close() client.__exit__(0, 0, 0) + # Test information methods + client.last_frame_end = 2 + client.silent_interval = 2 + self.assertEqual(4, client.idle_time()) + client.last_frame_end = None + self.assertEqual(0, client.idle_time()) + + # Test debug/trace/_dump methods + self.assertEqual(False, client.debug_enabled()) + writable = StringIO() + client.trace(writable) + client._dump([0, 1, 2], None) + self.assertEqual(hexlify_packets([0, 1, 2]), writable.getvalue()) + # a successful execute client.connect = lambda: True client.transaction = Mock(**{'execute.return_value': True}) @@ -133,6 +151,11 @@ def settimeout(self, *a, **kwa): client = ModbusUdpClient() self.assertFalse(client.connect()) + def testUdpClientIsSocketOpen(self): + ''' Test the udp client is_socket_open method''' + client = ModbusUdpClient() + self.assertFalse(client.is_socket_open()) + def testUdpClientSend(self): ''' Test the udp client send method''' client = ModbusUdpClient() @@ -201,6 +224,11 @@ def testTcpClientConnect(self): client = ModbusTcpClient() self.assertFalse(client.connect()) + def testTcpClientIsSocketOpen(self): + ''' Test the tcp client is_socket_open method''' + client = ModbusTcpClient() + self.assertFalse(client.is_socket_open()) + def testTcpClientSend(self): ''' Test the tcp client send method''' client = ModbusTcpClient() @@ -210,11 +238,13 @@ def testTcpClientSend(self): self.assertEqual(0, client._send(None)) self.assertEqual(4, client._send('1234')) + @patch('pymodbus.client.sync.time') @patch('pymodbus.client.sync.select') - def testTcpClientRecv(self, mock_select): + def testTcpClientRecv(self, mock_select, mock_time): ''' Test the tcp client receive method''' mock_select.select.return_value = [True] + mock_time.time.side_effect = count() client = ModbusTcpClient() self.assertRaises(ConnectionException, lambda: client._recv(1024)) @@ -225,7 +255,7 @@ def testTcpClientRecv(self, mock_select): mock_socket = MagicMock() mock_socket.recv.side_effect = iter([b'\x00', b'\x01', b'\x02']) client.socket = mock_socket - client.timeout = 1 + client.timeout = 3 self.assertEqual(b'\x00\x01\x02', client._recv(3)) mock_socket.recv.side_effect = iter([b'\x00', b'\x01', b'\x02']) self.assertEqual(b'\x00\x01', client._recv(2)) @@ -235,7 +265,7 @@ def testTcpClientRecv(self, mock_select): mock_select.select.return_value = [True] self.assertIn(b'\x00', client._recv(None)) - def testSerialClientRpr(self): + def testTcpClientRpr(self): client = ModbusTcpClient() rep = "<{} at {} socket={}, ipaddr={}, port={}, timeout={}>".format( client.__class__.__name__, hex(id(client)), client.socket, @@ -307,19 +337,25 @@ def testTlsClientSend(self): self.assertEqual(0, client._send(None)) self.assertEqual(4, client._send('1234')) - def testTlsClientRecv(self): + @patch('pymodbus.client.sync.time') + def testTlsClientRecv(self, mock_time): ''' Test the tls client receive method''' client = ModbusTlsClient() self.assertRaises(ConnectionException, lambda: client._recv(1024)) + mock_time.time.side_effect = count() + client.socket = mockSocket() self.assertEqual(b'', client._recv(0)) self.assertEqual(b'\x00' * 4, client._recv(4)) + client.timeout = 2 + self.assertIn(b'\x00', client._recv(None)) + mock_socket = MagicMock() mock_socket.recv.side_effect = iter([b'\x00', b'\x01', b'\x02']) client.socket = mock_socket - client.timeout = 1 + client.timeout = 3 self.assertEqual(b'\x00\x01\x02', client._recv(3)) mock_socket.recv.side_effect = iter([b'\x00', b'\x01', b'\x02']) self.assertEqual(b'\x00\x01', client._recv(2)) @@ -354,6 +390,8 @@ def testSyncSerialClientInstantiation(self): ModbusRtuFramer)) self.assertTrue(isinstance(ModbusSerialClient(method='binary').framer, ModbusBinaryFramer)) + self.assertTrue(isinstance(ModbusSerialClient(method='socket').framer, + ModbusSocketFramer)) self.assertRaises(ParameterException, lambda: ModbusSerialClient(method='something')) @@ -384,6 +422,12 @@ def testBasicSyncSerialClient(self, mock_serial): self.assertTrue(client.connect()) client.close() + # rtu connect/disconnect + rtu_client = ModbusSerialClient(method='rtu') + self.assertTrue(rtu_client.connect()) + self.assertEqual(rtu_client.socket.interCharTimeout, rtu_client.inter_char_timeout) + rtu_client.close() + # already closed socket client.socket = False client.close() @@ -402,6 +446,14 @@ def testSerialClientConnect(self): client = ModbusSerialClient() self.assertFalse(client.connect()) + @patch("serial.Serial") + def testSerialClientIsSocketOpen(self, mock_serial): + ''' Test the serial client is_socket_open method''' + client = ModbusSerialClient() + self.assertFalse(client.is_socket_open()) + client.socket = mock_serial + self.assertTrue(client.is_socket_open()) + @patch("serial.Serial") def testSerialClientSend(self, mock_serial): ''' Test the serial client send method''' @@ -444,6 +496,8 @@ def testSerialClientRecv(self): self.assertEqual(b'', client._recv(None)) client.socket.timeout = 0 self.assertEqual(b'', client._recv(0)) + client.timeout = None + self.assertEqual(b'', client._recv(None)) def testSerialClientRepr(self): client = ModbusSerialClient() diff --git a/test/test_transaction.py b/test/test_transaction.py old mode 100644 new mode 100755 index a3c469da1..172b8d796 --- a/test/test_transaction.py +++ b/test/test_transaction.py @@ -1,6 +1,14 @@ #!/usr/bin/env python import pytest import unittest +from itertools import count +from pymodbus.compat import IS_PYTHON3 + +if IS_PYTHON3: # Python 3 + from unittest.mock import patch, Mock, MagicMock +else: # Python 2 + from mock import patch, Mock, MagicMock + from binascii import a2b_hex from pymodbus.pdu import * from pymodbus.transaction import * @@ -82,7 +90,10 @@ def testCalculateExceptionLength(self): self.assertEqual(self._tm._calculate_exception_length(), exception_length) - def testExecute(self): + @patch('pymodbus.transaction.time') + def testExecute(self, mock_time): + mock_time.time.side_effect = count() + client = MagicMock() client.framer = self._ascii client.framer._buffer = b'deadbeef' @@ -123,6 +134,12 @@ def testExecute(self): response = tm.execute(request) self.assertIsInstance(response, ModbusIOException) + # wrong handle_local_echo + tm._recv = MagicMock(side_effect=iter([b'abcdef', b'deadbe', b'123456'])) + client.handle_local_echo = True + self.assertEqual(tm.execute(request).message, '[Input/Output] Wrong local echo') + client.handle_local_echo = False + # retry on invalid response tm.retry_on_invalid = True tm._recv = MagicMock(side_effect=iter([b'', b'abcdef', b'deadbe', b'123456'])) @@ -136,6 +153,12 @@ def testExecute(self): client.framer.processIncomingPacket.side_effect = MagicMock(side_effect=ModbusIOException()) self.assertIsInstance(tm.execute(request), ModbusIOException) + # broadcast + request.unit_id = 0 + client.broadcast_enable = True + self.assertEqual(tm.execute(request), + b'Broadcast write sent - no response expected') + # ----------------------------------------------------------------------- # # Dictionary based transaction manager # ----------------------------------------------------------------------- # From 8693468ceeef3f3917e1d7d21e8e9d3ea1e2c91a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thor=20Michael=20St=C3=B8re?= Date: Thu, 15 Oct 2020 13:01:35 +0200 Subject: [PATCH 25/49] Raise exception when modbus unit unexpectedly closes connection Raise an error when the modbus unit unexpectedly closes the connection during a receive data operation without returning any data, and log a warning if it does return some but not all data before closing the connection. Detecting and handling this here ensures that ModbusTcpClient._recv doesn't needlessly loop until it times out after the stream is closed by the peer, and also makes it certain that a response of b'' from ModbusTcpClient._recv means there was a timeout and cannot mean that the peer closed the stream, as it could mean before. The previous behavior was to not identify the remote end closing the socket, triggering a timeout, in turn causing the transaction manager to treat it as the modbus unit returning a response with errors in it (raising InvalidMessageReceivedException). This will now raise a ConnectionException, which falls through to the client, bypassing the retry mechanisms. Note that https://docs.python.org/3/library/socket.html does not contain a full description on the socket.recv method; see also pydoc for socket.socket.recv. --- pymodbus/client/sync.py | 39 ++++++++++++++++++++++++++++++++++++++- test/test_client_sync.py | 9 +++++++++ 2 files changed, 47 insertions(+), 1 deletion(-) mode change 100644 => 100755 test/test_client_sync.py diff --git a/pymodbus/client/sync.py b/pymodbus/client/sync.py index 8b0b832b6..94a722437 100644 --- a/pymodbus/client/sync.py +++ b/pymodbus/client/sync.py @@ -239,7 +239,12 @@ def _recv(self, size): """ Reads data from the underlying descriptor :param size: The number of bytes to read - :return: The bytes read + :return: The bytes read if the peer sent a response, or a zero-length + response if no data packets were received from the client at + all. + :raises: ConnectionException if the socket is not initialized, or the + peer either has closed the connection before this method is + invoked or closes it before sending any data before timeout. """ if not self.socket: raise ConnectionException(self.__str__()) @@ -270,6 +275,9 @@ def _recv(self, size): ready = select.select([self.socket], [], [], end - time_) if ready[0]: recv_data = self.socket.recv(recv_size) + if recv_data == b'': + return self._handle_abrupt_socket_close( + size, data, time.time() - time_) data.append(recv_data) data_length += len(recv_data) time_ = time.time() @@ -286,6 +294,35 @@ def _recv(self, size): return b"".join(data) + def _handle_abrupt_socket_close(self, size, data, duration): + """ Handle unexpected socket close by remote end + + Intended to be invoked after determining that the remote end + has unexpectedly closed the connection, to clean up and handle + the situation appropriately. + + :param size: The number of bytes that was attempted to read + :param data: The actual data returned + :param duration: Duration from the read was first attempted + until it was determined that the remote closed the + socket + :return: The more than zero bytes read from the remote end + :raises: ConnectionException If the remote end didn't send any + data at all before closing the connection. + """ + self.close() + readsize = ("read of %s bytes" % size if size + else "unbounded read") + msg = ("%s: Connection unexpectedly closed " + "%.6f seconds into %s" % (self, duration, readsize)) + if data: + result = b"".join(data) + msg += " after returning %s bytes" % len(result) + _logger.warning(msg) + return result + msg += " without response from unit before it closed connection" + raise ConnectionException(msg) + def is_socket_open(self): return True if self.socket is not None else False diff --git a/test/test_client_sync.py b/test/test_client_sync.py old mode 100644 new mode 100755 index 5b59dede3..00543f83f --- a/test/test_client_sync.py +++ b/test/test_client_sync.py @@ -265,6 +265,15 @@ def testTcpClientRecv(self, mock_select, mock_time): mock_select.select.return_value = [True] self.assertIn(b'\x00', client._recv(None)) + mock_socket = MagicMock() + mock_socket.recv.return_value = b'' + client.socket = mock_socket + self.assertRaises(ConnectionException, lambda: client._recv(1024)) + + mock_socket.recv.side_effect = iter([b'\x00', b'\x01', b'\x02', b'']) + client.socket = mock_socket + self.assertEqual(b'\x00\x01\x02', client._recv(1024)) + def testTcpClientRpr(self): client = ModbusTcpClient() rep = "<{} at {} socket={}, ipaddr={}, port={}, timeout={}>".format( From 8cbf88fa8532d2c96583d22ddcd1752235a2e2fd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thor=20Michael=20St=C3=B8re?= Date: Thu, 15 Oct 2020 16:08:49 +0200 Subject: [PATCH 26/49] Default to 4K receive buffer size Change the _sync method so that when no specific size is requested it fetches up to 4096 bytes instead of 1 byte at a time from the operating system. It's not clear what the reason was for setting it 1 byte. It was introduced in pymodbus commit: https://github.com/riptideio/pymodbus/commit/9d2c864bcf80f4491621339a8db3ecd42dbba309 Patch: https://github.com/riptideio/pymodbus/pull/292/commits/3b490fee80aa7d06be6e88876a721eaaddd3810d But there's no rationale there for setting it to 1 byte specifically. While it may have been to handle some obscure operating system with a poor network stack implementation this should have been documented, as it is written it comes across as that the implementer didn't understand how the TCP/stream socket networking API works. Note that https://docs.python.org/3/library/socket.html does not contain a full description on the socket.recv method; see also pydoc for socket.socket.recv. --- pymodbus/client/sync.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pymodbus/client/sync.py b/pymodbus/client/sync.py index 94a722437..471626e11 100644 --- a/pymodbus/client/sync.py +++ b/pymodbus/client/sync.py @@ -261,9 +261,9 @@ def _recv(self, size): timeout = self.timeout - # If size isn't specified read 1 byte at a time. + # If size isn't specified read up to 4096 bytes at a time. if size is None: - recv_size = 1 + recv_size = 4096 else: recv_size = size From ec03fdbaf59992a0511d8556c3e3f45e60eaaa27 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thor=20Michael=20St=C3=B8re?= Date: Fri, 23 Oct 2020 16:15:43 +0200 Subject: [PATCH 27/49] Add ModbusTcpDiagClient Synchronized TCP client that performs detail logging of network activity, for diagnosis of network related issues. --- pymodbus/client/sync_diag.py | 167 ++++++++++++++++++++++++++++++++++ test/test_client_sync_diag.py | 116 +++++++++++++++++++++++ 2 files changed, 283 insertions(+) create mode 100644 pymodbus/client/sync_diag.py create mode 100755 test/test_client_sync_diag.py diff --git a/pymodbus/client/sync_diag.py b/pymodbus/client/sync_diag.py new file mode 100644 index 000000000..90521de97 --- /dev/null +++ b/pymodbus/client/sync_diag.py @@ -0,0 +1,167 @@ +import socket +import logging +import time + +from pymodbus.constants import Defaults +from pymodbus.client.sync import ModbusTcpClient +from pymodbus.transaction import ModbusSocketFramer +from pymodbus.exceptions import ConnectionException + +_logger = logging.getLogger(__name__) + +LOG_MSGS = { + 'conn_msg': 'Connecting to modbus device %s', + 'connfail_msg': 'Connection to (%s, %s) failed: %s', + 'discon_msg': 'Disconnecting from modbus device %s', + 'timelimit_read_msg': + 'Modbus device read took %.4f seconds, ' + 'returned %s bytes in timelimit read', + 'timeout_msg': + 'Modbus device timeout after %.4f seconds, ' + 'returned %s bytes %s', + 'delay_msg': + 'Modbus device read took %.4f seconds, ' + 'returned %s bytes of %s expected', + 'read_msg': + 'Modbus device read took %.4f seconds, ' + 'returned %s bytes of %s expected', + 'unexpected_dc_msg': '%s %s'} + + +class ModbusTcpDiagClient(ModbusTcpClient): + """ + Variant of pymodbus.client.sync.ModbusTcpClient with additional + logging to diagnose network issues. + + The following events are logged: + + +---------+-----------------------------------------------------------------+ + | Level | Events | + +=========+=================================================================+ + | ERROR | Failure to connect to modbus unit; unexpected disconnect by | + | | modbus unit | + +---------+-----------------------------------------------------------------+ + | WARNING | Timeout on normal read; read took longer than warn_delay_limit | + +---------+-----------------------------------------------------------------+ + | INFO | Connection attempt to modbus unit; disconnection from modbus | + | | unit; each time limited read | + +---------+-----------------------------------------------------------------+ + | DEBUG | Normal read with timing information | + +---------+-----------------------------------------------------------------+ + + Reads are differentiated between "normal", which reads a specified number of + bytes, and "time limited", which reads all data for a duration equal to the + timeout period configured for this instance. + """ + + # pylint: disable=no-member + + def __init__(self, host='127.0.0.1', port=Defaults.Port, + framer=ModbusSocketFramer, **kwargs): + """ Initialize a client instance + + The keys of LOG_MSGS can be used in kwargs to customize the messages. + + :param host: The host to connect to (default 127.0.0.1) + :param port: The modbus port to connect to (default 502) + :param source_address: The source address tuple to bind to (default ('', 0)) + :param timeout: The timeout to use for this socket (default Defaults.Timeout) + :param warn_delay_limit: Log reads that take longer than this as warning. + Default True sets it to half of "timeout". None never logs these as + warning, 0 logs everything as warning. + :param framer: The modbus framer to use (default ModbusSocketFramer) + + .. note:: The host argument will accept ipv4 and ipv6 hosts + """ + self.warn_delay_limit = kwargs.get('warn_delay_limit', True) + super().__init__(host, port, framer, **kwargs) + if self.warn_delay_limit is True: + self.warn_delay_limit = self.timeout / 2 + + # Set logging messages, defaulting to LOG_MSGS + for (k, v) in LOG_MSGS.items(): + self.__dict__[k] = kwargs.get(k, v) + + def connect(self): + """ Connect to the modbus tcp server + + :returns: True if connection succeeded, False otherwise + """ + if self.socket: + return True + try: + _logger.info(self.conn_msg, self) + self.socket = socket.create_connection( + (self.host, self.port), + timeout=self.timeout, + source_address=self.source_address) + except socket.error as msg: + _logger.error(self.connfail_msg, self.host, self.port, msg) + self.close() + return self.socket is not None + + def close(self): + """ Closes the underlying socket connection + """ + if self.socket: + _logger.info(self.discon_msg, self) + self.socket.close() + self.socket = None + + def _recv(self, size): + try: + start = time.time() + + result = super()._recv(size) + + delay = time.time() - start + if self.warn_delay_limit is not None and delay >= self.warn_delay_limit: + self._log_delayed_response(len(result), size, delay) + elif not size: + _logger.debug(self.timelimit_read_msg, delay, len(result)) + else: + _logger.debug(self.read_msg, delay, len(result), size) + + return result + except ConnectionException as ex: + # Only log actual network errors, "if not self.socket" then it's a internal code issue + if 'Connection unexpectedly closed' in ex.string: + _logger.error(self.unexpected_dc_msg, self, ex) + raise ex + + def _log_delayed_response(self, result_len, size, delay): + if not size and result_len > 0: + _logger.info(self.timelimit_read_msg, delay, result_len) + elif (result_len == 0 or (size and result_len < size)) and delay >= self.timeout: + read_type = ("of %i expected" % size) if size else "in timelimit read" + _logger.warning(self.timeout_msg, delay, result_len, read_type) + else: + _logger.warning(self.delay_msg, delay, result_len, size) + + def __str__(self): + """ Builds a string representation of the connection + + :returns: The string representation + """ + return "ModbusTcpDiagClient(%s:%s)" % (self.host, self.port) + + +def get_client(): + """ Returns an appropriate client based on logging level + + This will be ModbusTcpDiagClient by default, or the parent class + if the log level is such that the diagnostic client will not log + anything. + + :returns: ModbusTcpClient or a child class thereof + """ + return ModbusTcpDiagClient if _logger.isEnabledFor(logging.ERROR) else ModbusTcpClient + + +# --------------------------------------------------------------------------- # +# Exported symbols +# --------------------------------------------------------------------------- # + +__all__ = [ + "ModbusTcpDiagClient", "get_client" +] diff --git a/test/test_client_sync_diag.py b/test/test_client_sync_diag.py new file mode 100755 index 000000000..c32d283e7 --- /dev/null +++ b/test/test_client_sync_diag.py @@ -0,0 +1,116 @@ +#!/usr/bin/env python +import unittest +from itertools import count +from pymodbus.compat import IS_PYTHON3 + +if IS_PYTHON3: # Python 3 + from unittest.mock import patch, Mock, MagicMock +else: # Python 2 + from mock import patch, Mock, MagicMock +import socket + +from pymodbus.client.sync_diag import ModbusTcpDiagClient, get_client +from pymodbus.exceptions import ConnectionException, NotImplementedException +from pymodbus.exceptions import ParameterException +from test.test_client_sync import mockSocket + + +# ---------------------------------------------------------------------------# +# Fixture +# ---------------------------------------------------------------------------# +class SynchronousDiagnosticClientTest(unittest.TestCase): + ''' + This is the unittest for the pymodbus.client.sync_diag module. It is + a copy of parts of the test for the TCP class in the pymodbus.client.sync + module, as it should operate identically and only log some additional + lines. + ''' + + # -----------------------------------------------------------------------# + # Test TCP Diagnostic Client + # -----------------------------------------------------------------------# + + def testSyncTcpDiagClientInstantiation(self): + client = get_client() + self.assertNotEqual(client, None) + + def testBasicSyncTcpDiagClient(self): + ''' Test the basic methods for the tcp sync diag client''' + + # connect/disconnect + client = ModbusTcpDiagClient() + client.socket = mockSocket() + self.assertTrue(client.connect()) + client.close() + + def testTcpDiagClientConnect(self): + ''' Test the tcp sync diag client connection method''' + with patch.object(socket, 'create_connection') as mock_method: + mock_method.return_value = object() + client = ModbusTcpDiagClient() + self.assertTrue(client.connect()) + + with patch.object(socket, 'create_connection') as mock_method: + mock_method.side_effect = socket.error() + client = ModbusTcpDiagClient() + self.assertFalse(client.connect()) + + @patch('pymodbus.client.sync.time') + @patch('pymodbus.client.sync_diag.time') + @patch('pymodbus.client.sync.select') + def testTcpDiagClientRecv(self, mock_select, mock_diag_time, mock_time): + ''' Test the tcp sync diag client receive method''' + + mock_select.select.return_value = [True] + mock_time.time.side_effect = count() + mock_diag_time.time.side_effect = count() + client = ModbusTcpDiagClient() + self.assertRaises(ConnectionException, lambda: client._recv(1024)) + + client.socket = mockSocket() + # Test logging of non-delayed responses + self.assertIn(b'\x00', client._recv(None)) + self.assertEqual(b'\x00', client._recv(1)) + + # Fool diagnostic logger into thinking we're running late, + # test logging of delayed responses + mock_diag_time.time.side_effect = count(step=3) + self.assertEqual(b'', client._recv(0)) + self.assertEqual(b'\x00' * 4, client._recv(4)) + + mock_socket = MagicMock() + mock_socket.recv.side_effect = iter([b'\x00', b'\x01', b'\x02']) + client.socket = mock_socket + client.timeout = 3 + self.assertEqual(b'\x00\x01\x02', client._recv(3)) + mock_socket.recv.side_effect = iter([b'\x00', b'\x01', b'\x02']) + self.assertEqual(b'\x00\x01', client._recv(2)) + mock_select.select.return_value = [False] + self.assertEqual(b'', client._recv(2)) + client.socket = mockSocket() + mock_select.select.return_value = [True] + self.assertIn(b'\x00', client._recv(None)) + + mock_socket = MagicMock() + mock_socket.recv.return_value = b'' + client.socket = mock_socket + self.assertRaises(ConnectionException, lambda: client._recv(1024)) + + mock_socket.recv.side_effect = iter([b'\x00', b'\x01', b'\x02', b'']) + client.socket = mock_socket + self.assertEqual(b'\x00\x01\x02', client._recv(1024)) + + def testTcpDiagClientRpr(self): + client = ModbusTcpDiagClient() + rep = "<{} at {} socket={}, ipaddr={}, port={}, timeout={}>".format( + client.__class__.__name__, hex(id(client)), client.socket, + client.host, client.port, client.timeout + ) + self.assertEqual(repr(client), rep) + + +# ---------------------------------------------------------------------------# +# Main +# ---------------------------------------------------------------------------# +if __name__ == "__main__": + unittest.main() From 7973995b2aa44faae8f4541a824fd3cb579940af Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thor=20Michael=20St=C3=B8re?= Date: Tue, 27 Oct 2020 13:53:51 +0100 Subject: [PATCH 28/49] Improve timeout error message For TCP/IP when no data is received at all before a timeout the phrasing "incomplete message received" in error messages in ModbusTransactionManager._recv is misleading. This wording assertively states that a message has been received from the modbus unit, and when no actual data has received this would imply that a message with an empty payload has been received by the application, which is not possible with TCP/IP. More likely it's just a timeout, without any data at all having been received from the modbus unit. --- pymodbus/transaction.py | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/pymodbus/transaction.py b/pymodbus/transaction.py index 0da18a607..b2bc0e23d 100644 --- a/pymodbus/transaction.py +++ b/pymodbus/transaction.py @@ -275,9 +275,10 @@ def _recv(self, expected_response_length, full): read_min = self.client.framer.recvPacket(min_size) if len(read_min) != min_size: + msg_start = "Incomplete message" if read_min else "No response" raise InvalidMessageReceivedException( - "Incomplete message received, expected at least %d bytes " - "(%d received)" % (min_size, len(read_min)) + "%s received, expected at least %d bytes " + "(%d received)" % (msg_start, min_size, len(read_min)) ) if read_min: if isinstance(self.client.framer, ModbusSocketFramer): @@ -312,9 +313,10 @@ def _recv(self, expected_response_length, full): result = read_min + result actual = len(result) if total is not None and actual != total: - _logger.debug("Incomplete message received, " + msg_start = "Incomplete message" if actual else "No response" + _logger.debug("{} received, " "Expected {} bytes Recieved " - "{} bytes !!!!".format(total, actual)) + "{} bytes !!!!".format(msg_start, total, actual)) if self.client.state != ModbusTransactionState.PROCESSING_REPLY: _logger.debug("Changing transaction state from " "'WAITING FOR REPLY' to 'PROCESSING REPLY'") From 870be4376e1a065a96699baab9772a7ea996c2f6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thor=20Michael=20St=C3=B8re?= Date: Tue, 3 Nov 2020 13:35:04 +0100 Subject: [PATCH 29/49] Log debug on no response to unbounded read It's not just not receiving the right number of bytes when there is a known number of bytes to read that is an issue, but also when there is no response at all to a read of None bytes (_recv isn't invoked unless one expects a response). --- pymodbus/transaction.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/pymodbus/transaction.py b/pymodbus/transaction.py index b2bc0e23d..5b37f4651 100644 --- a/pymodbus/transaction.py +++ b/pymodbus/transaction.py @@ -317,6 +317,10 @@ def _recv(self, expected_response_length, full): _logger.debug("{} received, " "Expected {} bytes Recieved " "{} bytes !!!!".format(msg_start, total, actual)) + elif actual == 0: + # If actual == 0 and total is not None then the above + # should be triggered, so total must be None here + _logger.debug("No response received to unbounded read !!!!") if self.client.state != ModbusTransactionState.PROCESSING_REPLY: _logger.debug("Changing transaction state from " "'WAITING FOR REPLY' to 'PROCESSING REPLY'") From 24c0ffc975c40a9dcab9620847a195f5565eaad3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thor=20Michael=20St=C3=B8re?= Date: Thu, 5 Nov 2020 13:58:20 +0100 Subject: [PATCH 30/49] Close connection on no/erroneous message received Always close connection on no response/error response from modbus unit. This attempts to resolve issue #538, where the response of a previously timed out request over synchronized TCP is being returned as the response to the current request, by leaving the connection open only if there is no unanswered response. This commit only touches the code for synchronized clients, and only seeks to resolve it for TCP based clients, though the situation for UDP clients should improve as well. --- pymodbus/transaction.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/pymodbus/transaction.py b/pymodbus/transaction.py index 5b37f4651..614da2b1d 100644 --- a/pymodbus/transaction.py +++ b/pymodbus/transaction.py @@ -200,6 +200,7 @@ def execute(self, request): "/Unable to decode response") response = ModbusIOException(last_exception, request.function_code) + self.client.close() if hasattr(self.client, "state"): _logger.debug("Changing transaction state from " "'PROCESSING REPLY' to " @@ -209,6 +210,7 @@ def execute(self, request): return response except ModbusIOException as ex: # Handle decode errors in processIncomingPacket method + self.client.close() _logger.exception(ex) self.client.state = ModbusTransactionState.TRANSACTION_COMPLETE return ex From 42a93ccb8d2ec2ef00409a06696bbfe81a275f1e Mon Sep 17 00:00:00 2001 From: Kyle Altendorf Date: Wed, 20 Jan 2021 09:26:25 -0500 Subject: [PATCH 31/49] remove coveralls for now --- .github/workflows/ci.yml | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 70646e3fe..3f1d12d10 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -126,7 +126,7 @@ jobs: - name: Install run: | pip install --upgrade pip setuptools wheel - pip install --upgrade tox coveralls + pip install --upgrade tox - uses: twisted/python-info-action@v1.0.1 - name: Test run: | @@ -137,8 +137,6 @@ jobs: mkdir coverage_reports cp .coverage "coverage_reports/coverage.${{ env.JOB_NAME }}" cp coverage.xml "coverage_reports/coverage.${{ env.JOB_NAME }}.xml" -# TODO: deal with the classic secrets and forked PRs issue... -# coveralls - name: Publish Coverage if: matrix.task.coverage uses: actions/upload-artifact@v2 From 35e1468949e74dad4480ad7847b63723f2136e4d Mon Sep 17 00:00:00 2001 From: Kyle Altendorf Date: Wed, 20 Jan 2021 09:27:27 -0500 Subject: [PATCH 32/49] touchup conftest.py --- test/conftest.py | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/test/conftest.py b/test/conftest.py index 861074ca3..55e688160 100644 --- a/test/conftest.py +++ b/test/conftest.py @@ -1,8 +1,5 @@ from pymodbus.compat import PYTHON_VERSION - if PYTHON_VERSION < (3,): - # These files use syntax introduced in Python 3 (not necessarily 3.0) and - # just won't be run during tests in Python 2. - collect_ignore = [ - "test_server_asyncio.py", - ] + # These files use syntax introduced between Python 2 and our lowest + # supported Python 3 version. We just won't run these tests in Python 2. + collect_ignore = ["test_server_asyncio.py"] From 50c723aace6df92a0adf13d522795881377f75be Mon Sep 17 00:00:00 2001 From: Kyle Altendorf Date: Wed, 20 Jan 2021 09:33:51 -0500 Subject: [PATCH 33/49] drop basepython for flake8 environment --- tox.ini | 1 - 1 file changed, 1 deletion(-) diff --git a/tox.ini b/tox.ini index 00ee19b88..c09224bc3 100644 --- a/tox.ini +++ b/tox.ini @@ -14,7 +14,6 @@ setenv = with_gmp=no [testenv:flake8] -basepython = python3.7 deps = -r requirements-checks.txt commands = flake8 From 4f7413d8c2522b3d4f07e9523b0b72be5582f019 Mon Sep 17 00:00:00 2001 From: Kyle Altendorf Date: Wed, 20 Jan 2021 10:04:06 -0500 Subject: [PATCH 34/49] intra-gha combined coverage --- .github/workflows/ci.yml | 58 +++++++++++++++++++++++++++++++++++++-- requirements-coverage.txt | 1 + requirements-tests.txt | 2 +- tox.ini | 6 ++++ 4 files changed, 64 insertions(+), 3 deletions(-) create mode 100644 requirements-coverage.txt diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 3f1d12d10..f7c010f57 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -137,7 +137,7 @@ jobs: mkdir coverage_reports cp .coverage "coverage_reports/coverage.${{ env.JOB_NAME }}" cp coverage.xml "coverage_reports/coverage.${{ env.JOB_NAME }}.xml" - - name: Publish Coverage + - name: Upload Coverage if: matrix.task.coverage uses: actions/upload-artifact@v2 with: @@ -190,12 +190,66 @@ jobs: continue-on-error: ${{ matrix.task.continue_on_error }} run: | tox -vv -e ${{ matrix.task.tox }} + coverage: + # Should match JOB_NAME below + name: ${{ matrix.task.name }} - ${{ matrix.os.name }} ${{ matrix.qt_library.name }} ${{ matrix.python.name }} ${{ matrix.arch.name }} + runs-on: ${{ matrix.os.runs-on }} + needs: + - test + container: ${{ matrix.os.container[matrix.python.docker] }} + strategy: + fail-fast: false + matrix: + task: + - name: Coverage + tox: combined-coverage + download_coverage: true + os: + - name: Linux + runs-on: ubuntu-latest + python_platform: linux + matrix: linux + container: + 3.8: docker://python:3.8-buster + python: + - name: CPython 3.8 + tox: py38 + action: 3.8 + docker: 3.8 + implementation: cpython + arch: + - name: x64 + action: x64 + matrix: x64 + env: + # Should match name above + JOB_NAME: ${{ matrix.task.name }} - ${{ matrix.os.name }} ${{ matrix.qt_library.name }} ${{ matrix.python.name }} ${{ matrix.arch.name }} + steps: + - uses: actions/checkout@v2 + with: + fetch-depth: 0 + - name: Install + run: | + pip install --upgrade pip setuptools wheel + pip install --upgrade tox + - uses: twisted/python-info-action@v1.0.1 + - name: Download Coverage + if: matrix.task.download_coverage + uses: actions/download-artifact@v2 + with: + name: coverage + path: coverage_reports + - name: Test + continue-on-error: ${{ matrix.task.continue_on_error }} + run: | + tox -vv -e ${{ matrix.task.tox }} all: name: All runs-on: ubuntu-latest needs: - - test - check + - coverage + - test steps: - name: This shell: python diff --git a/requirements-coverage.txt b/requirements-coverage.txt new file mode 100644 index 000000000..9808587c9 --- /dev/null +++ b/requirements-coverage.txt @@ -0,0 +1 @@ +coverage >= 4.2 diff --git a/requirements-tests.txt b/requirements-tests.txt index b7d4bb23c..5eca1922c 100644 --- a/requirements-tests.txt +++ b/requirements-tests.txt @@ -1,6 +1,6 @@ bcrypt>=3.1.6 capturer >= 2.2 -coverage >= 4.2 +-r requirements-coverage.txt cryptography>= 2.3 mock >= 1.0.1 pyserial-asyncio>=0.4.0;python_version>="3.4" diff --git a/tox.ini b/tox.ini index c09224bc3..72b6ff970 100644 --- a/tox.ini +++ b/tox.ini @@ -26,6 +26,12 @@ commands = make -C doc/ clean make -C doc/ html +[testenv:combined-coverage] +deps = -r requirements-coverage.txt +commands = + coverage combine coverage_reports + coverage report --fail-under=90 --ignore-errors + [flake8] exclude = .tox ignore = D211,D400,E731 From 5e03ca744d1df240126105cf0b38a6f89e17b91f Mon Sep 17 00:00:00 2001 From: Kyle Altendorf Date: Wed, 20 Jan 2021 10:11:29 -0500 Subject: [PATCH 35/49] coverage: true --- .github/workflows/ci.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index f7c010f57..49aa4d4fa 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -26,6 +26,7 @@ jobs: task: - name: Test tox: test + coverage: true os: - name: Linux runs-on: ubuntu-latest From 99e24a285ac5864341a633b0d00849c8bf217ad8 Mon Sep 17 00:00:00 2001 From: Kyle Altendorf Date: Wed, 20 Jan 2021 10:30:28 -0500 Subject: [PATCH 36/49] . --- .github/workflows/ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 49aa4d4fa..d045324df 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -136,7 +136,7 @@ jobs: if: matrix.task.coverage run: | mkdir coverage_reports - cp .coverage "coverage_reports/coverage.${{ env.JOB_NAME }}" + cp .coverage "coverage_reports/.coverage.${{ env.JOB_NAME }}" cp coverage.xml "coverage_reports/coverage.${{ env.JOB_NAME }}.xml" - name: Upload Coverage if: matrix.task.coverage From cc5c0d6fd3212f72071ca3fb1efeceb4d95b34ea Mon Sep 17 00:00:00 2001 From: Kyle Altendorf Date: Wed, 20 Jan 2021 13:17:38 -0500 Subject: [PATCH 37/49] ls -la coverage_reports --- tox.ini | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tox.ini b/tox.ini index 72b6ff970..9fc001bce 100644 --- a/tox.ini +++ b/tox.ini @@ -27,8 +27,11 @@ commands = make -C doc/ html [testenv:combined-coverage] +allowlist_externals = + ls deps = -r requirements-coverage.txt commands = + ls -la coverage_reports coverage combine coverage_reports coverage report --fail-under=90 --ignore-errors From 9d11bae3e96e89c43cbe6c5a6830423a487d94bd Mon Sep 17 00:00:00 2001 From: Kyle Altendorf Date: Wed, 20 Jan 2021 13:41:13 -0500 Subject: [PATCH 38/49] remove copy/paste error qt references --- .github/workflows/ci.yml | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index d045324df..e0afe8025 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -17,7 +17,7 @@ on: jobs: test: # Should match JOB_NAME below - name: ${{ matrix.task.name }} - ${{ matrix.os.name }} ${{ matrix.qt_library.name }} ${{ matrix.python.name }} ${{ matrix.arch.name }} + name: ${{ matrix.task.name }} - ${{ matrix.os.name }} ${{ matrix.python.name }} ${{ matrix.arch.name }} runs-on: ${{ matrix.os.runs-on }} container: ${{ matrix.os.container[matrix.python.docker] }} strategy: @@ -107,7 +107,7 @@ jobs: matrix: x86 env: # Should match name above - JOB_NAME: ${{ matrix.task.name }} - ${{ matrix.os.name }} ${{ matrix.qt_library.name }} ${{ matrix.python.name }} ${{ matrix.arch.name }} + JOB_NAME: ${{ matrix.task.name }} - ${{ matrix.os.name }} ${{ matrix.python.name }} ${{ matrix.arch.name }} steps: - uses: actions/checkout@v2 with: @@ -146,7 +146,7 @@ jobs: path: coverage_reports/* check: # Should match JOB_NAME below - name: ${{ matrix.task.name }} - ${{ matrix.os.name }} ${{ matrix.qt_library.name }} ${{ matrix.python.name }} ${{ matrix.arch.name }} + name: ${{ matrix.task.name }} - ${{ matrix.os.name }} ${{ matrix.python.name }} ${{ matrix.arch.name }} runs-on: ${{ matrix.os.runs-on }} container: ${{ matrix.os.container[matrix.python.docker] }} strategy: @@ -177,7 +177,7 @@ jobs: matrix: x64 env: # Should match name above - JOB_NAME: ${{ matrix.task.name }} - ${{ matrix.os.name }} ${{ matrix.qt_library.name }} ${{ matrix.python.name }} ${{ matrix.arch.name }} + JOB_NAME: ${{ matrix.task.name }} - ${{ matrix.os.name }} ${{ matrix.python.name }} ${{ matrix.arch.name }} steps: - uses: actions/checkout@v2 with: @@ -193,7 +193,7 @@ jobs: tox -vv -e ${{ matrix.task.tox }} coverage: # Should match JOB_NAME below - name: ${{ matrix.task.name }} - ${{ matrix.os.name }} ${{ matrix.qt_library.name }} ${{ matrix.python.name }} ${{ matrix.arch.name }} + name: ${{ matrix.task.name }} - ${{ matrix.os.name }} ${{ matrix.python.name }} ${{ matrix.arch.name }} runs-on: ${{ matrix.os.runs-on }} needs: - test @@ -224,7 +224,7 @@ jobs: matrix: x64 env: # Should match name above - JOB_NAME: ${{ matrix.task.name }} - ${{ matrix.os.name }} ${{ matrix.qt_library.name }} ${{ matrix.python.name }} ${{ matrix.arch.name }} + JOB_NAME: ${{ matrix.task.name }} - ${{ matrix.os.name }} ${{ matrix.python.name }} ${{ matrix.arch.name }} steps: - uses: actions/checkout@v2 with: From 4993a38371792cf4454d37c24f74cde19b1c593b Mon Sep 17 00:00:00 2001 From: Kyle Altendorf Date: Wed, 27 Jan 2021 10:22:50 -0500 Subject: [PATCH 39/49] Handle unspecified continue_on_error better --- .github/workflows/ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index e0afe8025..f1b11405e 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -188,7 +188,7 @@ jobs: pip install --upgrade tox - uses: twisted/python-info-action@v1.0.1 - name: Test - continue-on-error: ${{ matrix.task.continue_on_error }} + continue-on-error: ${{ matrix.task.continue_on_error == true }} run: | tox -vv -e ${{ matrix.task.tox }} coverage: From e5157c2dda8137a99a2708d1122c6810b3e374d9 Mon Sep 17 00:00:00 2001 From: Kyle Altendorf Date: Wed, 27 Jan 2021 10:29:32 -0500 Subject: [PATCH 40/49] Handle unspecified continue_on_error better (again) --- .github/workflows/ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index f1b11405e..a0caa141e 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -241,7 +241,7 @@ jobs: name: coverage path: coverage_reports - name: Test - continue-on-error: ${{ matrix.task.continue_on_error }} + continue-on-error: ${{ matrix.task.continue_on_error == true }} run: | tox -vv -e ${{ matrix.task.tox }} all: From 1f2c18e68811c8539622060e65293cb796e52fe3 Mon Sep 17 00:00:00 2001 From: Kyle Altendorf Date: Tue, 9 Feb 2021 11:49:37 -0500 Subject: [PATCH 41/49] --fail-under=85 --- tox.ini | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tox.ini b/tox.ini index 9fc001bce..91a573e7a 100644 --- a/tox.ini +++ b/tox.ini @@ -33,7 +33,7 @@ deps = -r requirements-coverage.txt commands = ls -la coverage_reports coverage combine coverage_reports - coverage report --fail-under=90 --ignore-errors + coverage report --fail-under=85 --ignore-errors [flake8] exclude = .tox From 1cae3315bbf905e56d87cb4404f8cb251394fdf4 Mon Sep 17 00:00:00 2001 From: Kyle Altendorf Date: Tue, 9 Feb 2021 11:59:22 -0500 Subject: [PATCH 42/49] skip test_client_async_asyncio as well in py2 --- test/conftest.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/test/conftest.py b/test/conftest.py index 55e688160..932e8124c 100644 --- a/test/conftest.py +++ b/test/conftest.py @@ -2,4 +2,7 @@ if PYTHON_VERSION < (3,): # These files use syntax introduced between Python 2 and our lowest # supported Python 3 version. We just won't run these tests in Python 2. - collect_ignore = ["test_server_asyncio.py"] + collect_ignore = [ + "test_client_async_asyncio.py", + "test_server_asyncio.py", + ] From e3ef45ee90c1d092a0ba6050710b0ee6375b7ddd Mon Sep 17 00:00:00 2001 From: dhoomakethu Date: Sat, 13 Feb 2021 11:23:45 +0530 Subject: [PATCH 43/49] Update retry strategy, asyncio server and reactive servers. Test fixes --- CHANGELOG.rst | 8 ++++ pymodbus/repl/client/helper.py | 2 +- pymodbus/repl/server/cli.py | 11 +++-- pymodbus/server/async_io.py | 26 ++++++---- pymodbus/server/reactive/main.py | 56 +++++++++++++++++++--- pymodbus/transaction.py | 80 ++++++++++++++----------------- pymodbus/version.py | 2 +- test/test_client_async_asyncio.py | 6 +-- test/test_transaction.py | 16 ++++++- 9 files changed, 137 insertions(+), 70 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 0856cedbd..adbd441ac 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -1,3 +1,11 @@ +version 2.5.0 +---------------------------------------------------------- +* Support response types `stray` and `empty` in repl server. +* Minor updates in asyncio server. +* Update reactive server to send stray response of given length. +* Transaction manager updates on retries for empty and invalid packets. +* Test fixes for asyncio client and transaction manager. + version 2.5.0rc3 ---------------------------------------------------------- * Minor fix in documentations diff --git a/pymodbus/repl/client/helper.py b/pymodbus/repl/client/helper.py index 38a29e9df..eb7ede644 100644 --- a/pymodbus/repl/client/helper.py +++ b/pymodbus/repl/client/helper.py @@ -160,7 +160,7 @@ def get_meta(self, cmd): def __str__(self): if self.doc: - return "Command {0:>50}{:<20}".format(self.name, self.doc) + return "Command {:>50}{:<20}".format(self.name, self.doc) return "Command {}".format(self.name) diff --git a/pymodbus/repl/server/cli.py b/pymodbus/repl/server/cli.py index 6e1c0db17..26a8c5e52 100644 --- a/pymodbus/repl/server/cli.py +++ b/pymodbus/repl/server/cli.py @@ -31,8 +31,9 @@ BOTTOM_TOOLBAR = HTML('(MODBUS SERVER) Type "help" ' 'for list of available commands') -COMMAND_ARGS = ["response_type", "error_code", "delay_by", "clear_after"] -RESPONSE_TYPES = ["normal", "error", "delayed"] +COMMAND_ARGS = ["response_type", "error_code", "delay_by", + "clear_after", "data_len"] +RESPONSE_TYPES = ["normal", "error", "delayed", "empty", "stray"] COMMANDS = { "manipulator": { "response_type": None, @@ -155,7 +156,8 @@ async def interactive_shell(server): "type request - {}".format(value)) warning("Choose from {}".format(RESPONSE_TYPES)) valid = False - elif arg in ["error_code", "delay_by"]: + elif arg in ["error_code", "delay_by", + "clear_after", "data_len"]: try: value = int(value) except ValueError: @@ -166,7 +168,8 @@ async def interactive_shell(server): if valid: val_dict[arg] = value if val_dict: - server.manipulator_config = val_dict + server.update_manipulator_config(val_dict) + # server.manipulator_config = val_dict # result = await run_command(tester, *command) except (EOFError, KeyboardInterrupt): diff --git a/pymodbus/server/async_io.py b/pymodbus/server/async_io.py index c4a7e2836..c6e5df1c8 100755 --- a/pymodbus/server/async_io.py +++ b/pymodbus/server/async_io.py @@ -223,20 +223,28 @@ def execute(self, request, *addr): if not broadcast: response.transaction_id = request.transaction_id response.unit_id = request.unit_id + skip_encoding = False if self.server.response_manipulator: - response = self.server.response_manipulator(response) - self.send(response, *addr) + response, skip_encoding = self.server.response_manipulator(response) + self.send(response, *addr, skip_encoding=skip_encoding) - def send(self, message, *addr): - if message.should_respond: - # self.server.control.Counter.BusMessage += 1 - pdu = self.framer.buildPacket(message) + def send(self, message, *addr, **kwargs): + def __send(msg, *addr): if _logger.isEnabledFor(logging.DEBUG): - _logger.debug('send: [%s]- %s' % (message, b2a_hex(pdu))) + _logger.debug('send: [%s]- %s' % (message, b2a_hex(msg))) if addr == (None,): - self._send_(pdu) + self._send_(msg) else: - self._send_(pdu, *addr) + self._send_(msg, *addr) + skip_encoding = kwargs.get("skip_encoding", False) + if skip_encoding: + __send(message, *addr) + elif message.should_respond: + # self.server.control.Counter.BusMessage += 1 + pdu = self.framer.buildPacket(message) + __send(pdu, *addr) + else: + _logger.debug("Skipping sending response!!") # ----------------------------------------------------------------------- # # Derived class implementations diff --git a/pymodbus/server/reactive/main.py b/pymodbus/server/reactive/main.py index 50ed50447..7e1a30a63 100644 --- a/pymodbus/server/reactive/main.py +++ b/pymodbus/server/reactive/main.py @@ -2,10 +2,12 @@ Copyright (c) 2020 by RiptideIO All rights reserved. """ +import os import asyncio import time import random import logging +from pymodbus.version import version as pymodbus_version from pymodbus.compat import IS_PYTHON3, PYTHON_VERSION from pymodbus.pdu import ExceptionResponse, ModbusExceptions from pymodbus.datastore.store import (ModbusSparseDataBlock, @@ -15,7 +17,7 @@ if not IS_PYTHON3 or PYTHON_VERSION < (3, 6): print(f"You are running {PYTHON_VERSION}." - "Reactive server requires python3.6 or above".PYTHON_VERSION) + "Reactive server requires python3.6 or above") exit() @@ -102,6 +104,7 @@ def __init__(self, host, port, modbus_server, loop=None): self._modbus_server = modbus_server self._loop = loop self._add_routes() + self._counter = 0 self._modbus_server.response_manipulator = self.manipulate_response self._manipulator_config = dict(**DEFAULT_MANIPULATOR) self._web_app.on_startup.append(self.start_modbus_server) @@ -132,9 +135,18 @@ async def start_modbus_server(self, app): """ try: if isinstance(self._modbus_server, ModbusSerialServer): - app["modbus_serial_server"] = asyncio.create_task( - self._modbus_server.start()) - app["modbus_server"] = asyncio.create_task(self._modbus_server.serve_forever()) + if hasattr(asyncio, "create_task"): + app["modbus_serial_server"] = asyncio.create_task( + self._modbus_server.start()) + app["modbus_server"] = asyncio.create_task( + self._modbus_server.serve_forever()) + else: + app["modbus_serial_server"] = asyncio.ensure_future( + self._modbus_server.start() + ) + app["modbus_server"] = asyncio.ensure_future( + self._modbus_server.serve_forever()) + logger.info("Modbus server started") except Exception as e: logger.error("Error starting modbus server") @@ -157,7 +169,7 @@ async def _response_manipulator(self, request): """ POST request Handler for response manipulation end point Payload is a dict with following fields - :response_type : One among (normal, delayed, error, empty) + :response_type : One among (normal, delayed, error, empty, stray) :error_code: Modbus error code for error response :delay_by: Delay sending response by seconds @@ -168,15 +180,31 @@ async def _response_manipulator(self, request): self._manipulator_config.update(data) return web.json_response(data=data) + def update_manipulator_config(self, config): + """ + Updates manipulator config. Resets previous counters + :param config: Manipulator config (dict) + :return: + """ + self._counter = 0 + self._manipulator_config = config + def manipulate_response(self, response): """ Manipulates the actual response according to the required error state. :param response: Modbus response object :return: Modbus response """ + skip_encoding = False if not self._manipulator_config: return response else: + clear_after = self._manipulator_config.get("clear_after") + if clear_after and self._counter > clear_after: + logger.info("Resetting manipulator" + " after {} responses".format(clear_after)) + self.update_manipulator_config(dict(DEFAULT_MANIPULATOR)) + return response response_type = self._manipulator_config.get("response_type") if response_type == "error": error_code = self._manipulator_config.get("error_code") @@ -186,15 +214,28 @@ def manipulate_response(self, response): err_response.transaction_id = response.transaction_id err_response.unit_id = response.unit_id response = err_response + self._counter += 1 elif response_type == "delayed": delay_by = self._manipulator_config.get("delay_by") logger.warning( "Delaying response by {}s for " "all incoming requests".format(delay_by)) time.sleep(delay_by) + self._counter += 1 elif response_type == "empty": logger.warning("Sending empty response") - return response + self._counter += 1 + response.should_respond = False + elif response_type == "stray": + data_len = self._manipulator_config.get("data_len", 10) + if data_len <= 0: + logger.warning(f"Invalid data_len {data_len}. " + f"Using default lenght 10") + data_len = 10 + response = os.urandom(data_len) + self._counter += 1 + skip_encoding = True + return response, skip_encoding def run(self): """ @@ -225,7 +266,7 @@ def create_identity(cls, vendor="Pymodbus", product_code="PM", vendor_url='http://github.com/riptideio/pymodbus/', product_name="Pymodbus Server", model_name="Reactive Server", - version="2.5.0"): + version=pymodbus_version.short()): """ Create modbus identity :param vendor: @@ -324,3 +365,4 @@ def factory(cls, server, framer=None, context=None, unit=1, single=False, **kwargs) return ReactiveServer(host, web_port, server, loop) +# __END__ diff --git a/pymodbus/transaction.py b/pymodbus/transaction.py index 9f09477ba..6fa01792d 100644 --- a/pymodbus/transaction.py +++ b/pymodbus/transaction.py @@ -153,7 +153,6 @@ def execute(self, request): self._transact(request, None, broadcast=True) response = b'Broadcast write sent - no response expected' else: - invalid_response = False expected_response_length = None if not isinstance(self.client.framer, ModbusSocketFramer): if hasattr(request, "get_response_pdu_size"): @@ -171,54 +170,38 @@ def execute(self, request): full = True if not expected_response_length: expected_response_length = Defaults.ReadSize - # retries += 1 + response, last_exception = self._transact( + request, + expected_response_length, + full=full, + broadcast=broadcast + ) while retries > 0: - response, last_exception = self._transact( - request, - expected_response_length, - full=full, - broadcast=broadcast - ) valid_response = self._validate_response( request, response, expected_response_length ) - if not response and request.unit_id \ - not in self._no_response_devices: - self._no_response_devices.append(request.unit_id) - elif request.unit_id in self._no_response_devices and response: - self._no_response_devices.remove(request.unit_id) - if not response and self.retry_on_empty: - _logger.debug("Retry on empty - {}".format(retries)) - retries -= 1 - _logger.debug("Changing transaction state from " - "'WAITING_FOR_REPLY' to 'RETRYING'") - self.client.state = ModbusTransactionState.RETRYING - continue - elif not response: - break - mbap = self.client.framer.decode_data(response) - if mbap.get('unit') == request.unit_id: - break - if ('length' in mbap and expected_response_length and - mbap.get('length') == expected_response_length and - mbap.get('fcode') == request.function_code): + if valid_response: + if request.unit_id in self._no_response_devices and response: + self._no_response_devices.remove(request.unit_id) + _logger.debug("Got response!!!") break else: - invalid_response = True - if invalid_response and not self.retry_on_invalid: - break - _logger.debug("Retry on invalid - {}".format(retries)) - if hasattr(self.client, "state"): - _logger.debug("RESETTING Transaction " - "state to 'RETRY' for retry") - self.client.state = ModbusTransactionState.RETRYING - if self.backoff: - delay = 2 ** (self.retries - retries) * self.backoff - time.sleep(delay) - _logger.debug("Sleeping {}".format(delay)) - full = False - broadcast = False - retries -= 1 + if not response: + if request.unit_id not in self._no_response_devices: + self._no_response_devices.append(request.unit_id) + if self.retry_on_empty: + response, last_exception = self._retry_transaction(retries, "empty", request, expected_response_length, full=full) + retries -= 1 + else: + # No response received and retries not enabled + break + else: + if self.retry_on_invalid: + response, last_exception = self._retry_transaction(retries, "invalid", request, expected_response_length, full=full) + retries -= 1 + else: + break + # full = False addTransaction = partial(self.addTransaction, tid=request.transaction_id) self.client.framer.processIncomingPacket(response, @@ -249,7 +232,16 @@ def execute(self, request): self.client.state = ModbusTransactionState.TRANSACTION_COMPLETE return ex - def _retry(self, packet, response_length, full=False): + def _retry_transaction(self, retries, reason, + packet, response_length, full=False): + _logger.debug("Retry on {} response - {}".format(reason, retries)) + _logger.debug("Changing transaction state from " + "'WAITING_FOR_REPLY' to 'RETRYING'") + self.client.state = ModbusTransactionState.RETRYING + if self.backoff: + delay = 2 ** (self.retries - retries) * self.backoff + time.sleep(delay) + _logger.debug("Sleeping {}".format(delay)) self.client.connect() in_waiting = self.client._in_waiting() if in_waiting: diff --git a/pymodbus/version.py b/pymodbus/version.py index 74b2497c7..59e0c5d40 100644 --- a/pymodbus/version.py +++ b/pymodbus/version.py @@ -41,7 +41,7 @@ def __str__(self): return '[%s, version %s]' % (self.package, self.short()) -version = Version('pymodbus', 2, 5, 0, "rc3") +version = Version('pymodbus', 2, 5, 0) version.__name__ = 'pymodbus' # fix epydoc error diff --git a/test/test_client_async_asyncio.py b/test/test_client_async_asyncio.py index 42455f53c..a666ebbca 100644 --- a/test/test_client_async_asyncio.py +++ b/test/test_client_async_asyncio.py @@ -50,10 +50,10 @@ def test_protocol_connection_state_propagation_to_factory(self): request = mock.MagicMock() protocol.transaction.addTransaction(request, 1) protocol.connection_lost(mock.sentinel.REASON) - if PYTHON_VERSION.major == 3 and PYTHON_VERSION.minor == 6: - call_args = protocol.raise_future.call_args[0] - else: + if PYTHON_VERSION.major == 3 and PYTHON_VERSION.minor >= 8: call_args = protocol.raise_future.call_args.args + else: + call_args = protocol.raise_future.call_args[0] protocol.raise_future.assert_called_once() assert call_args[0] == request assert isinstance(call_args[1], ConnectionException) diff --git a/test/test_transaction.py b/test/test_transaction.py index a3c469da1..91f9776f8 100644 --- a/test/test_transaction.py +++ b/test/test_transaction.py @@ -92,10 +92,16 @@ def testExecute(self): client.framer.buildPacket.return_value = b'deadbeef' client.framer.sendPacket = MagicMock() client.framer.sendPacket.return_value = len(b'deadbeef') - + client.framer.decode_data = MagicMock() + client.framer.decode_data.return_value = { + "unit": 1, + "fcode": 222, + "length": 27 + } request = MagicMock() request.get_response_pdu_size.return_value = 10 request.unit_id = 1 + request.function_code = 222 tm = ModbusTransactionManager(client) tm._recv = MagicMock(return_value=b'abcdef') self.assertEqual(tm.retries, 3) @@ -103,6 +109,7 @@ def testExecute(self): # tm._transact = MagicMock() # some response # tm._transact.return_value = (b'abcdef', None) + tm.getTransaction = MagicMock() tm.getTransaction.return_value = 'response' response = tm.execute(request) @@ -136,6 +143,13 @@ def testExecute(self): client.framer.processIncomingPacket.side_effect = MagicMock(side_effect=ModbusIOException()) self.assertIsInstance(tm.execute(request), ModbusIOException) + # Broadcast + client.broadcast_enable = True + request.unit_id = 0 + response = tm.execute(request) + self.assertEqual(response, b'Broadcast write sent - ' + b'no response expected') + # ----------------------------------------------------------------------- # # Dictionary based transaction manager # ----------------------------------------------------------------------- # From af6ecd8c14a0f25968c6a5c90303e037961ce00a Mon Sep 17 00:00:00 2001 From: dhoomakethu Date: Sat, 13 Feb 2021 11:56:10 +0530 Subject: [PATCH 44/49] Fix Coverage failure --- .github/workflows/ci.yml | 1 + requirements-docs.txt | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index a0caa141e..a08f75b10 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -233,6 +233,7 @@ jobs: run: | pip install --upgrade pip setuptools wheel pip install --upgrade tox + pip install --upgrade six - uses: twisted/python-info-action@v1.0.1 - name: Download Coverage if: matrix.task.download_coverage diff --git a/requirements-docs.txt b/requirements-docs.txt index b0a4d2416..fc6cdda47 100644 --- a/requirements-docs.txt +++ b/requirements-docs.txt @@ -9,7 +9,7 @@ recommonmark>=0.4.0 Sphinx>=1.6.5 sphinx-rtd-theme>=0.2.4 SQLAlchemy>=1.1.15 # Required to parse some files -tornado>=4.5.3 # Required to parse some files +tornado==4.5.3 # Required to parse some files Twisted>=17.1.0 # Required to parse some files prompt_toolkit>=2.0.4 click>=7.0 From 812ae0386a64ac3c9c07dbaf40b1d6accaf256d0 Mon Sep 17 00:00:00 2001 From: dhoomakethu Date: Sat, 13 Feb 2021 20:14:33 +0530 Subject: [PATCH 45/49] Fix coverage failure --- tox.ini | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tox.ini b/tox.ini index 91a573e7a..d84c48458 100644 --- a/tox.ini +++ b/tox.ini @@ -29,7 +29,9 @@ commands = [testenv:combined-coverage] allowlist_externals = ls -deps = -r requirements-coverage.txt +deps = + -r requirements-coverage.txt + -r requirements.txt commands = ls -la coverage_reports coverage combine coverage_reports From 593e1871c0595f444fca0ad2e7b4e53f7c95d3f1 Mon Sep 17 00:00:00 2001 From: dhoomakethu Date: Sat, 13 Feb 2021 21:06:17 +0530 Subject: [PATCH 46/49] Remove travis --- .travis.yml | 36 ------------------------------------ 1 file changed, 36 deletions(-) delete mode 100644 .travis.yml diff --git a/.travis.yml b/.travis.yml deleted file mode 100644 index c83cf395d..000000000 --- a/.travis.yml +++ /dev/null @@ -1,36 +0,0 @@ -sudo: false -language: python -matrix: - include: - - os: linux - python: "2.7" - - os: linux - python: "3.6" - - os: linux - python: "3.7" - - os: linux - python: "3.8" - - os: osx - osx_image: xcode8.3 - language: generic -before_install: - - if [ $TRAVIS_OS_NAME = osx ]; then brew update; fi - - if [ $TRAVIS_OS_NAME = osx ]; then brew install openssl; fi -# - if [$TRAVIS_OS_NAME = osx ]; then python -c "import fcntl; fcntl.fcntl(1, fcntl.F_SETFL, 0)"; fi - -install: -# - scripts/travis.sh pip install pip-accel - - if [ $TRAVIS_OS_NAME = osx ]; then scripts/travis.sh pip install -U pip "\"setuptools<45"\"; else pip install -U pip setuptools --upgrade ; fi - - scripts/travis.sh pip install coveralls - - scripts/travis.sh pip install --requirement=requirements-checks.txt - - scripts/travis.sh pip install --requirement=requirements-tests.txt - - scripts/travis.sh LC_ALL=C pip install --upgrade . -# - scripts/travis.sh pip freeze --all -script: -# - scripts/travis.sh make check - - scripts/travis.sh make test -after_success: - - scripts/travis.sh coveralls -branches: - except: - - /^[0-9]/ From f84d37445e06e722b7c237c3be12d844d6b4f206 Mon Sep 17 00:00:00 2001 From: Siarhei Farbotka Date: Thu, 25 Feb 2021 20:27:09 +0300 Subject: [PATCH 47/49] Properly process incomplete RTU frames, improve test coverage --- pymodbus/framer/rtu_framer.py | 45 +++++++++--------- test/test_framers.py | 89 ++++++++++++++++++++++++++--------- 2 files changed, 89 insertions(+), 45 deletions(-) diff --git a/pymodbus/framer/rtu_framer.py b/pymodbus/framer/rtu_framer.py index b60efb1ea..6c246bae1 100644 --- a/pymodbus/framer/rtu_framer.py +++ b/pymodbus/framer/rtu_framer.py @@ -60,7 +60,7 @@ def __init__(self, decoder, client=None): :param decoder: The decoder factory implementation to use """ self._buffer = b'' - self._header = {'uid': 0x00, 'len': 0, 'crc': '0000'} + self._header = {'uid': 0x00, 'len': 0, 'crc': b'\x00\x00'} self._hsize = 0x01 self._end = b'\x0d\x0a' self._min_frame_size = 4 @@ -89,14 +89,9 @@ def checkFrame(self): self.populateHeader() frame_size = self._header['len'] data = self._buffer[:frame_size - 2] - crc = self._buffer[frame_size - 2:frame_size] + crc = self._header['crc'] crc_val = (byte2int(crc[0]) << 8) + byte2int(crc[1]) - if checkCRC(data, crc_val): - return True - else: - _logger.debug("CRC invalid, discarding header!!") - self.resetFrame() - return False + return checkCRC(data, crc_val) except (IndexError, KeyError, struct.error): return False @@ -107,13 +102,10 @@ def advanceFrame(self): it or determined that it contains an error. It also has to reset the current frame header handle """ - try: - self._buffer = self._buffer[self._header['len']:] - except KeyError: - # Error response, no header len found - self.resetFrame() + + self._buffer = self._buffer[self._header['len']:] _logger.debug("Frame advanced, resetting header!!") - self._header = {} + self._header = {'uid': 0x00, 'len': 0, 'crc': b'\x00\x00'} def resetFrame(self): """ @@ -127,7 +119,7 @@ def resetFrame(self): _logger.debug("Resetting frame - Current Frame in " "buffer - {}".format(hexlify_packets(self._buffer))) self._buffer = b'' - self._header = {} + self._header = {'uid': 0x00, 'len': 0, 'crc': b'\x00\x00'} def isFrameReady(self): """ @@ -137,31 +129,38 @@ def isFrameReady(self): :returns: True if ready, False otherwise """ - if len(self._buffer) > self._hsize: - if not self._header: - self.populateHeader() + if len(self._buffer) <= self._hsize: + return False - return self._header and len(self._buffer) >= self._header['len'] - else: + try: + # Frame is ready only if populateHeader() successfully populates crc field which finishes RTU frame + # Otherwise, if buffer is not yet long enough, populateHeader() raises IndexError + self.populateHeader() + except IndexError: return False + return True + def populateHeader(self, data=None): """ Try to set the headers `uid`, `len` and `crc`. This method examines `self._buffer` and writes meta - information into `self._header`. It calculates only the - values for headers that are not already in the dictionary. + information into `self._header`. Beware that this method will raise an IndexError if `self._buffer` is not yet long enough. """ - data = data if data else self._buffer + data = data if data is not None else self._buffer self._header['uid'] = byte2int(data[0]) func_code = byte2int(data[1]) pdu_class = self.decoder.lookupPduClass(func_code) size = pdu_class.calculateRtuFrameSize(data) self._header['len'] = size + + if len(data) < size: + # crc yet not available + raise IndexError self._header['crc'] = data[size - 2:size] def addToFrame(self, message): diff --git a/test/test_framers.py b/test/test_framers.py index 520409fc6..975271e32 100644 --- a/test/test_framers.py +++ b/test/test_framers.py @@ -8,7 +8,7 @@ from pymodbus.exceptions import ModbusIOException from pymodbus.compat import IS_PYTHON3 if IS_PYTHON3: - from unittest.mock import Mock + from unittest.mock import Mock, patch else: # Python 2 from mock import Mock @@ -44,7 +44,7 @@ def test_framer_initialization(framer): assert framer._start == b':' assert framer._end == b"\r\n" elif isinstance(framer, ModbusRtuFramer): - assert framer._header == {'uid': 0x00, 'len': 0, 'crc': '0000'} + assert framer._header == {'uid': 0x00, 'len': 0, 'crc': b'\x00\x00'} assert framer._hsize == 0x01 assert framer._end == b'\x0d\x0a' assert framer._min_frame_size == 4 @@ -64,47 +64,78 @@ def test_decode_data(rtu_framer, data): assert decoded == expected -@pytest.mark.parametrize("data", [(b'', False), - (b'\x02\x01\x01\x00Q\xcc', True)]) +@pytest.mark.parametrize("data", [ + (b'', False), + (b'\x02\x01\x01\x00Q\xcc', True), + (b'\x11\x03\x06\xAE\x41\x56\x52\x43\x40\x49\xAD', True), # valid frame + (b'\x11\x03\x06\xAE\x41\x56\x52\x43\x40\x49\xAC', False), # invalid frame CRC +]) def test_check_frame(rtu_framer, data): data, expected = data rtu_framer._buffer = data assert expected == rtu_framer.checkFrame() -@pytest.mark.parametrize("data", [b'', b'abcd']) -def test_advance_framer(rtu_framer, data): - rtu_framer._buffer = data +@pytest.mark.parametrize("data", [ + (b'', {'uid': 0x00, 'len': 0, 'crc': b'\x00\x00'}, b''), + (b'abcd', {'uid': 0x00, 'len': 2, 'crc': b'\x00\x00'}, b'cd'), + (b'\x11\x03\x06\xAE\x41\x56\x52\x43\x40\x49\xAD\x12\x03', # real case, frame size is 11 + {'uid': 0x00, 'len': 11, 'crc': b'\x00\x00'}, b'\x12\x03'), +]) +def test_rtu_advance_framer(rtu_framer, data): + before_buf, before_header, after_buf = data + + rtu_framer._buffer = before_buf + rtu_framer._header = before_header rtu_framer.advanceFrame() - assert rtu_framer._header == {} - assert rtu_framer._buffer == data + assert rtu_framer._header == {'uid': 0x00, 'len': 0, 'crc': b'\x00\x00'} + assert rtu_framer._buffer == after_buf @pytest.mark.parametrize("data", [b'', b'abcd']) -def test_reset_framer(rtu_framer, data): +def test_rtu_reset_framer(rtu_framer, data): rtu_framer._buffer = data rtu_framer.resetFrame() - assert rtu_framer._header == {} + assert rtu_framer._header == {'uid': 0x00, 'len': 0, 'crc': b'\x00\x00'} assert rtu_framer._buffer == b'' @pytest.mark.parametrize("data", [ (b'', False), + (b'\x11', False), + (b'\x11\x03', False), (b'\x11\x03\x06', False), (b'\x11\x03\x06\xAE\x41\x56\x52\x43\x40\x49', False), (b'\x11\x03\x06\xAE\x41\x56\x52\x43\x40\x49\xAD', True), - (b'\x11\x03\x06\xAE\x41\x56\x52\x43\x40\x49\xAD\xAB\xCD', True) + (b'\x11\x03\x06\xAE\x41\x56\x52\x43\x40\x49\xAD\xAB\xCD', True), ]) def test_is_frame_ready(rtu_framer, data): data, expected = data rtu_framer._buffer = data - rtu_framer.advanceFrame() + # rtu_framer.advanceFrame() assert rtu_framer.isFrameReady() == expected -def test_populate_header(rtu_framer): - rtu_framer.populateHeader(b'abcd') - assert rtu_framer._header == {'crc': b'd', 'uid': 97, 'len': 5} +@pytest.mark.parametrize("data", [ + b'', + b'\x11', + b'\x11\x03', + b'\x11\x03\x06', + b'\x11\x03\x06\xAE\x41\x56\x52\x43\x40\x43', +]) +def test_rtu_populate_header_fail(rtu_framer, data): + with pytest.raises(IndexError): + rtu_framer.populateHeader(data) + + +@pytest.mark.parametrize("data", [ + (b'\x11\x03\x06\xAE\x41\x56\x52\x43\x40\x49\xAD', {'crc': b'\x49\xAD', 'uid': 17, 'len': 11}), + (b'\x11\x03\x06\xAE\x41\x56\x52\x43\x40\x49\xAD\x11\x03', {'crc': b'\x49\xAD', 'uid': 17, 'len': 11}) +]) +def test_rtu_populate_header(rtu_framer, data): + buffer, expected = data + rtu_framer.populateHeader(buffer) + assert rtu_framer._header == expected def test_add_to_frame(rtu_framer): @@ -126,12 +157,26 @@ def test_populate_result(rtu_framer): assert result.unit_id == 255 -@pytest.mark.parametrize('framer', [ascii_framer, rtu_framer, binary_framer]) -def test_process_incoming_packet(framer): - def cb(res): - return res - # data = b'' - # framer.processIncomingPacket(data, cb, unit=1, single=False) +@pytest.mark.parametrize("data", [ + (b'\x11', 17, False, False), # not complete frame + (b'\x11\x03', 17, False, False), # not complete frame + (b'\x11\x03\x06', 17, False, False), # not complete frame + (b'\x11\x03\x06\xAE\x41\x56\x52\x43', 17, False, False), # not complete frame + (b'\x11\x03\x06\xAE\x41\x56\x52\x43\x40', 17, False, False), # not complete frame + (b'\x11\x03\x06\xAE\x41\x56\x52\x43\x40\x49', 17, False, False), # not complete frame + (b'\x11\x03\x06\xAE\x41\x56\x52\x43\x40\x49\xAC', 17, True, False), # bad crc + (b'\x11\x03\x06\xAE\x41\x56\x52\x43\x40\x49\xAD', 17, False, True), # good frame + (b'\x11\x03\x06\xAE\x41\x56\x52\x43\x40\x49\xAD', 16, True, False), # incorrect unit id + (b'\x11\x03\x06\xAE\x41\x56\x52\x43\x40\x49\xAD\x11\x03', 17, False, True), # good frame + part of next frame +]) +def test_rtu_process_incoming_packet(rtu_framer, data): + buffer, units, reset_called, process_called = data + + with patch.object(rtu_framer, '_process') as mock_process, \ + patch.object(rtu_framer, 'resetFrame') as mock_reset: + rtu_framer.processIncomingPacket(buffer, Mock(), units) + assert mock_process.call_count == (1 if process_called else 0) + assert mock_reset.call_count == (1 if reset_called else 0) def test_build_packet(rtu_framer): From 0ebe3f5ea0b5edce0eeae8757d30202d94426977 Mon Sep 17 00:00:00 2001 From: Siarhei Farbotka Date: Thu, 25 Feb 2021 23:55:01 +0300 Subject: [PATCH 48/49] Fixed test --- test/test_transaction.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/test_transaction.py b/test/test_transaction.py index a3c469da1..0c10770ff 100644 --- a/test/test_transaction.py +++ b/test/test_transaction.py @@ -455,7 +455,7 @@ def testRTUFramerTransactionReady(self): msg_parts = [b"\x00\x01\x00", b"\x00\x00\x01\xfc\x1b"] self._rtu.addToFrame(msg_parts[0]) - self.assertTrue(self._rtu.isFrameReady()) + self.assertFalse(self._rtu.isFrameReady()) self.assertFalse(self._rtu.checkFrame()) self._rtu.addToFrame(msg_parts[1]) From 0cdc39bb911a877c29e88be22fdb0ae1b7bb10b4 Mon Sep 17 00:00:00 2001 From: dhoomakethu Date: Tue, 2 Mar 2021 20:47:28 +0530 Subject: [PATCH 49/49] Fix tests. closes #608. closes #575 --- CHANGELOG.rst | 3 +++ pymodbus/client/sync.py | 4 ++-- pymodbus/client/sync_diag.py | 4 ++-- pymodbus/utilities.py | 5 ++++- test/test_client_sync.py | 8 ++++---- test/test_framers.py | 2 +- test/test_transaction.py | 5 ++++- 7 files changed, 20 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index adbd441ac..c70f4799b 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -5,6 +5,9 @@ version 2.5.0 * Update reactive server to send stray response of given length. * Transaction manager updates on retries for empty and invalid packets. * Test fixes for asyncio client and transaction manager. +* Fix sync client and processing of incomplete frames with rtu framers +* Support synchronous diagnostic client (TCP) +* Server updates (REPL and async) version 2.5.0rc3 ---------------------------------------------------------- diff --git a/pymodbus/client/sync.py b/pymodbus/client/sync.py index 897e83528..aa7fd23cc 100644 --- a/pymodbus/client/sync.py +++ b/pymodbus/client/sync.py @@ -155,8 +155,8 @@ def _dump(self, data, direction): try: fd.write(hexlify_packets(data)) except Exception as e: - self._logger.debug(hexlify_packets(data)) - self._logger.exception(e) + _logger.debug(hexlify_packets(data)) + _logger.exception(e) def register(self, function): """ diff --git a/pymodbus/client/sync_diag.py b/pymodbus/client/sync_diag.py index 90521de97..e2e41291f 100644 --- a/pymodbus/client/sync_diag.py +++ b/pymodbus/client/sync_diag.py @@ -74,7 +74,7 @@ def __init__(self, host='127.0.0.1', port=Defaults.Port, .. note:: The host argument will accept ipv4 and ipv6 hosts """ self.warn_delay_limit = kwargs.get('warn_delay_limit', True) - super().__init__(host, port, framer, **kwargs) + super(ModbusTcpDiagClient, self).__init__(host, port, framer, **kwargs) if self.warn_delay_limit is True: self.warn_delay_limit = self.timeout / 2 @@ -112,7 +112,7 @@ def _recv(self, size): try: start = time.time() - result = super()._recv(size) + result = super(ModbusTcpDiagClient, self)._recv(size) delay = time.time() - start if self.warn_delay_limit is not None and delay >= self.warn_delay_limit: diff --git a/pymodbus/utilities.py b/pymodbus/utilities.py index 1aebb6de4..9e31d0974 100644 --- a/pymodbus/utilities.py +++ b/pymodbus/utilities.py @@ -245,7 +245,10 @@ def hexlify_packets(packet): """ if not packet: return '' - return " ".join([hex(byte2int(x)) for x in packet]) + if IS_PYTHON3: + return " ".join([hex(byte2int(x)) for x in packet]) + else: + return u" ".join([hex(byte2int(x)) for x in packet]) # --------------------------------------------------------------------------- # # Exported symbols # --------------------------------------------------------------------------- # diff --git a/test/test_client_sync.py b/test/test_client_sync.py index cfdee6b46..1b679ed85 100755 --- a/test/test_client_sync.py +++ b/test/test_client_sync.py @@ -86,8 +86,8 @@ def testBaseModbusClient(self): self.assertEqual(False, client.debug_enabled()) writable = StringIO() client.trace(writable) - client._dump([0, 1, 2], None) - self.assertEqual(hexlify_packets([0, 1, 2]), writable.getvalue()) + client._dump(b'\x00\x01\x02', None) + self.assertEqual(hexlify_packets(b'\x00\x01\x02'), writable.getvalue()) # a successful execute client.connect = lambda: True @@ -154,7 +154,7 @@ def settimeout(self, *a, **kwa): def testUdpClientIsSocketOpen(self): ''' Test the udp client is_socket_open method''' client = ModbusUdpClient() - self.assertFalse(client.is_socket_open()) + self.assertTrue(client.is_socket_open()) def testUdpClientSend(self): ''' Test the udp client send method''' @@ -434,7 +434,7 @@ def testBasicSyncSerialClient(self, mock_serial): client.close() # rtu connect/disconnect - rtu_client = ModbusSerialClient(method='rtu') + rtu_client = ModbusSerialClient(method='rtu', strict=True) self.assertTrue(rtu_client.connect()) self.assertEqual(rtu_client.socket.interCharTimeout, rtu_client.inter_char_timeout) rtu_client.close() diff --git a/test/test_framers.py b/test/test_framers.py index 975271e32..d70dac3b9 100644 --- a/test/test_framers.py +++ b/test/test_framers.py @@ -10,7 +10,7 @@ if IS_PYTHON3: from unittest.mock import Mock, patch else: # Python 2 - from mock import Mock + from mock import Mock, patch @pytest.fixture diff --git a/test/test_transaction.py b/test/test_transaction.py index f345f8354..1b34ca638 100755 --- a/test/test_transaction.py +++ b/test/test_transaction.py @@ -144,7 +144,10 @@ def testExecute(self, mock_time): # wrong handle_local_echo tm._recv = MagicMock(side_effect=iter([b'abcdef', b'deadbe', b'123456'])) client.handle_local_echo = True - self.assertEqual(tm.execute(request).message, '[Input/Output] Wrong local echo') + tm.retry_on_empty = False + tm.retry_on_invalid = False + self.assertEqual(tm.execute(request).message, + '[Input/Output] Wrong local echo') client.handle_local_echo = False # retry on invalid response