From ba4b29950652daa8e59b95edc61e724a1690ef8a Mon Sep 17 00:00:00 2001 From: William Poetra Yoga Date: Thu, 29 Jun 2023 11:22:17 +0700 Subject: [PATCH 1/7] Fix index-out-of-bounds issue when reading slave response --- umodbus/serial.py | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/umodbus/serial.py b/umodbus/serial.py index d24b981..07f0a62 100644 --- a/umodbus/serial.py +++ b/umodbus/serial.py @@ -143,17 +143,19 @@ def _exit_read(self, response: bytearray) -> bool: :param response: The response :type response: bytearray - :returns: State of basic read response evaluation + :returns: State of basic read response evaluation, + True if entire response has been read :rtype: bool """ - if response[1] >= Const.ERROR_BIAS: - if len(response) < Const.ERROR_RESP_LEN: + response_len = len(response) + if response_len >= 2 and response[1] >= Const.ERROR_BIAS: + if response_len < Const.ERROR_RESP_LEN: return False - elif (Const.READ_COILS <= response[1] <= Const.READ_INPUT_REGISTER): + elif response_len >= 3 and (Const.READ_COILS <= response[1] <= Const.READ_INPUT_REGISTER): expected_len = Const.RESPONSE_HDR_LENGTH + 1 + response[2] + Const.CRC_LENGTH - if len(response) < expected_len: + if response_len < expected_len: return False - elif len(response) < Const.FIXED_RESP_LEN: + elif response_len < Const.FIXED_RESP_LEN: return False return True From 547d0a8f4039fc78e9e3146d59bb1d3bdcbb9d72 Mon Sep 17 00:00:00 2001 From: William Poetra Yoga Date: Thu, 29 Jun 2023 11:27:18 +0700 Subject: [PATCH 2/7] Simplify calculation of inter-frame delay Also rename the variable to make it more descriptive --- umodbus/serial.py | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/umodbus/serial.py b/umodbus/serial.py index 07f0a62..c68c6f0 100644 --- a/umodbus/serial.py +++ b/umodbus/serial.py @@ -112,12 +112,16 @@ def __init__(self, else: self._ctrlPin = None + # timing of 1 character in microseconds (us) self._t1char = (1000000 * (data_bits + stop_bits + 2)) // baudrate + + # inter-frame delay in microseconds (us) + # - <= 19200 bps: 3.5x timing of 1 character + # - > 19200 bps: 1750 us if baudrate <= 19200: - # 4010us (approx. 4ms) @ 9600 baud - self._t35chars = (3500000 * (data_bits + stop_bits + 2)) // baudrate + self._inter_frame_delay = (self._t1char * 3500) // 1000 else: - self._t35chars = 1750 # 1750us (approx. 1.75ms) + self._inter_frame_delay = 1750 def _calculate_crc16(self, data: bytearray) -> bytes: """ @@ -180,7 +184,7 @@ def _uart_read(self) -> bytearray: break # wait for the maximum time between two frames - time.sleep_us(self._t35chars) + time.sleep_us(self._inter_frame_delay) return response @@ -196,10 +200,9 @@ def _uart_read_frame(self, timeout: Optional[int] = None) -> bytearray: """ received_bytes = bytearray() - # set timeout to at least twice the time between two frames in case the - # timeout was set to zero or None + # set default timeout to at twice the inter-frame delay if timeout == 0 or timeout is None: - timeout = 2 * self._t35chars # in milliseconds + timeout = 2 * self._inter_frame_delay # in microseconds start_us = time.ticks_us() @@ -212,13 +215,13 @@ def _uart_read_frame(self, timeout: Optional[int] = None) -> bytearray: # do not stop reading and appending the result to the buffer # until the time between two frames elapsed - while time.ticks_diff(time.ticks_us(), last_byte_ts) <= self._t35chars: + while time.ticks_diff(time.ticks_us(), last_byte_ts) <= self._inter_frame_delay: # WiPy only # r = self._uart.readall() r = self._uart.read() # if something has been read after the first iteration of - # this inner while loop (during self._t35chars time) + # this inner while loop (within self._inter_frame_delay) if r is not None: # append the new read stuff to the buffer received_bytes.extend(r) From 06aeda05099558899f70353bf060d2d19c8d77fa Mon Sep 17 00:00:00 2001 From: William Poetra Yoga Date: Thu, 29 Jun 2023 11:46:16 +0700 Subject: [PATCH 3/7] Fix receive long slave response by waiting longer --- umodbus/serial.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/umodbus/serial.py b/umodbus/serial.py index c68c6f0..4108e3a 100644 --- a/umodbus/serial.py +++ b/umodbus/serial.py @@ -166,14 +166,16 @@ def _exit_read(self, response: bytearray) -> bool: def _uart_read(self) -> bytearray: """ - Read up to 40 bytes from UART + Read incoming slave response from UART :returns: Read content :rtype: bytearray """ response = bytearray() - for x in range(1, 40): + # TODO: use some kind of hint or user-configurable delay + # to determine this loop counter + for x in range(1, 120): if self._uart.any(): # WiPy only # response.extend(self._uart.readall()) From c7cd786b279838c773887ed6cc631c905dcc157c Mon Sep 17 00:00:00 2001 From: William Poetra Yoga Date: Thu, 29 Jun 2023 12:02:30 +0700 Subject: [PATCH 4/7] Fix receive missing initial bytes by blocking instead of polling --- umodbus/serial.py | 34 +++++++++++++++++++--------------- 1 file changed, 19 insertions(+), 15 deletions(-) diff --git a/umodbus/serial.py b/umodbus/serial.py index 4108e3a..04e9c56 100644 --- a/umodbus/serial.py +++ b/umodbus/serial.py @@ -242,32 +242,36 @@ def _send(self, modbus_pdu: bytes, slave_addr: int) -> None: """ Send Modbus frame via UART - If a flow control pin has been setup, it will be controller accordingly + If a flow control pin has been setup, it will be controlled accordingly :param modbus_pdu: The modbus Protocol Data Unit :type modbus_pdu: bytes :param slave_addr: The slave address :type slave_addr: int """ - serial_pdu = bytearray() - serial_pdu.append(slave_addr) - serial_pdu.extend(modbus_pdu) - - crc = self._calculate_crc16(serial_pdu) - serial_pdu.extend(crc) + # modbus_adu: Modbus Application Data Unit + # consists of the Modbus PDU, with slave address prepended and checksum appended + modbus_adu = bytearray() + modbus_adu.append(slave_addr) + modbus_adu.extend(modbus_pdu) + modbus_adu.extend(self._calculate_crc16(modbus_adu)) if self._ctrlPin: - self._ctrlPin(1) + self._ctrlPin.on() time.sleep_us(1000) # wait until the control pin really changed - send_start_time = time.ticks_us() - self._uart.write(serial_pdu) + # the timing of this part is critical: + # - if we disable output too early, + # the command will not be received in full + # - if we disable output too late, + # the incoming response will lose some data at the beginning + # easiest to just wait for the bytes to be sent out on the wire + + self._uart.write(modbus_adu) + self._uart.flush() if self._ctrlPin: - total_frame_time_us = self._t1char * len(serial_pdu) - while time.ticks_us() <= send_start_time + total_frame_time_us: - machine.idle() - self._ctrlPin(0) + self._ctrlPin.off() def _send_receive(self, modbus_pdu: bytes, @@ -286,7 +290,7 @@ def _send_receive(self, :returns: Validated response content :rtype: bytes """ - # flush the Rx FIFO + # flush the Rx FIFO buffer self._uart.read() self._send(modbus_pdu=modbus_pdu, slave_addr=slave_addr) From e4bdeea19411eccf5c387be57044ecd163cfd7ac Mon Sep 17 00:00:00 2001 From: Jonas Scharpf Date: Sat, 1 Jul 2023 11:30:03 +0200 Subject: [PATCH 5/7] replace machine idle with time sleep_us for MicroPython below v1.20.0 without UART flush function --- umodbus/serial.py | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/umodbus/serial.py b/umodbus/serial.py index 04e9c56..5f60896 100644 --- a/umodbus/serial.py +++ b/umodbus/serial.py @@ -13,7 +13,6 @@ from machine import Pin import struct import time -import machine # custom packages from . import const as Const @@ -96,6 +95,8 @@ def __init__(self, :param ctrl_pin: The control pin :type ctrl_pin: int """ + # UART flush function is introduced in Micropython v1.20.0 + self._has_uart_flush = callable(getattr(UART, "flush", None)) self._uart = UART(uart_id, baudrate=baudrate, bits=data_bits, @@ -258,7 +259,9 @@ def _send(self, modbus_pdu: bytes, slave_addr: int) -> None: if self._ctrlPin: self._ctrlPin.on() - time.sleep_us(1000) # wait until the control pin really changed + # wait until the control pin really changed + # 85-95us (ESP32 @ 160/240MHz) + time.sleep_us(200) # the timing of this part is critical: # - if we disable output too early, @@ -267,8 +270,19 @@ def _send(self, modbus_pdu: bytes, slave_addr: int) -> None: # the incoming response will lose some data at the beginning # easiest to just wait for the bytes to be sent out on the wire + send_start_time = time.ticks_us() + # 360-400us @ 9600-115200 baud (measured) (ESP32 @ 160/240MHz) self._uart.write(modbus_adu) - self._uart.flush() + send_finish_time = time.ticks_us() + if self._has_uart_flush: + self._uart.flush() + else: + sleep_time_us = ( + self._t1char * len(modbus_adu) - # total frame time in us + time.ticks_diff(send_finish_time, send_start_time) + + 100 # only required at baudrates above 57600, but hey 100us + ) + time.sleep_us(sleep_time_us) if self._ctrlPin: self._ctrlPin.off() From c0de0bd4ba17f29fa1ae7679c6cf9a363d1b80a1 Mon Sep 17 00:00:00 2001 From: Jonas Scharpf Date: Sat, 1 Jul 2023 11:30:09 +0200 Subject: [PATCH 6/7] update changelog --- changelog.md | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/changelog.md b/changelog.md index 7ac479b..a8c8b62 100644 --- a/changelog.md +++ b/changelog.md @@ -15,6 +15,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## Released +## [2.3.5] - 2023-07-01 +### Fixed +- Time between RS485 control pin raise and UART transmission reduced by 80% from 1000us to 200us +- The RS485 control pin is lowered as fast as possible by using `time.sleep_us()` instead of `machine.idle()` which uses an IRQ on the order of milliseconds. This kept the control pin active longer than necessary, causing the response message to be missed at higher baud rates. This applies only to MicroPython firmwares below v1.20.0 +- The following fixes were provided by @wpyoga +- RS485 control pin handling fixed by using UART `flush` function, see #68 +- Invalid CRC while reading multiple coils and fixed, see #50 and #52 + ## [2.3.4] - 2023-03-20 ### Added - `package.json` for `mip` installation with MicroPython v1.19.1 or newer @@ -282,8 +290,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - PEP8 style issues on all files of [`lib/uModbus`](lib/uModbus) -[Unreleased]: https://github.com/brainelectronics/micropython-modbus/compare/2.3.4...develop +[Unreleased]: https://github.com/brainelectronics/micropython-modbus/compare/2.3.5...develop +[2.3.5]: https://github.com/brainelectronics/micropython-modbus/tree/2.3.5 [2.3.4]: https://github.com/brainelectronics/micropython-modbus/tree/2.3.4 [2.3.3]: https://github.com/brainelectronics/micropython-modbus/tree/2.3.3 [2.3.2]: https://github.com/brainelectronics/micropython-modbus/tree/2.3.2 From b5416a9307023588a8314bcc42016bfd88f78ea5 Mon Sep 17 00:00:00 2001 From: Jonas Scharpf Date: Sat, 1 Jul 2023 11:37:24 +0200 Subject: [PATCH 7/7] remove flush function from machine UART fake --- fakes/machine.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/fakes/machine.py b/fakes/machine.py index 1053f66..46fc358 100755 --- a/fakes/machine.py +++ b/fakes/machine.py @@ -425,6 +425,9 @@ def sendbreak(self) -> None: """Send a break condition on the bus""" raise MachineError('Not yet implemented') + ''' + # flush introduced in MicroPython v1.20.0 + # use manual timing calculation for testing def flush(self) -> None: """ Waits until all data has been sent @@ -434,6 +437,7 @@ def flush(self) -> None: Only available with newer versions than 1.19 """ raise MachineError('Not yet implemented') + ''' def txdone(self) -> bool: """