From 3ef3581d692718a1cd08ac1d2595bc6f357ef740 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc=20Recht=C3=A9?= Date: Wed, 20 Aug 2025 14:20:05 +0200 Subject: [PATCH 1/4] Improves serial communication behaviour in case of error In production mode, if connection could not be established, the system was endlessly looping trying again. The connection is only established at MPF startup, should we later loose the serial port for some reason, the system would not re-establish the connection by itself. Therefore, it is better to exit MPF should the connexion not be established, and manage a restart externally. In case of a problem with a serial port, the warning message clearly indicates the faulty port. In case of MPF shutdown due to comm failure, do not try to close the faulty connection which is already in the closing state (avoid cascading exception). Regarding OPP platform: - in the rare case where there is only one device, do not treat it as special case regarding serial - if device does not report a serial number, default it to #0 - _poll_sender (opp.py): - optimize for speed - send pool before waiting response - logs port name and serial number attached --- mpf/platforms/base_serial_communicator.py | 32 +++++++------------- mpf/platforms/opp/opp.py | 23 ++++++++------ mpf/platforms/opp/opp_serial_communicator.py | 24 ++++++++++++--- 3 files changed, 43 insertions(+), 36 deletions(-) diff --git a/mpf/platforms/base_serial_communicator.py b/mpf/platforms/base_serial_communicator.py index c7eeabc88..119e747f4 100644 --- a/mpf/platforms/base_serial_communicator.py +++ b/mpf/platforms/base_serial_communicator.py @@ -48,22 +48,11 @@ async def connect(self): async def _connect_to_hardware(self, port, baud, xonxoff=False): self.log.info("Connecting to %s at %sbps", port, baud) - while True: - try: - connector = self.machine.clock.open_serial_connection( - url=port, baudrate=baud, limit=0, xonxoff=xonxoff, - bytesize=EIGHTBITS, parity=PARITY_NONE, stopbits=STOPBITS_ONE) - self.reader, self.writer = await connector - except SerialException: - if not self.machine.options["production"]: - raise - - # if we are in production mode retry - await asyncio.sleep(.1) - self.log.debug("Connection to %s failed. Will retry.", port) - else: - # we got a connection - break + connector = self.machine.clock.open_serial_connection( + url=port, baudrate=baud, limit=0, xonxoff=xonxoff, + bytesize=EIGHTBITS, parity=PARITY_NONE, stopbits=STOPBITS_ONE) + + self.reader, self.writer = await connector serial = self.writer.transport.serial if hasattr(serial, "set_low_latency_mode"): @@ -118,14 +107,15 @@ async def read(self, n=-1): except asyncio.CancelledError: # pylint: disable-msg=try-except-raise raise except Exception as e: # pylint: disable-msg=broad-except - self.log.warning("Serial error: {}".format(e)) + self.log.warning("Serial {} error: {}".format(self.port, e)) self.machine.events.post("serial_error") - return None + resp = None # we either got empty response (-> socket closed) or and error if not resp: - self.log.warning("Serial closed.") - self.machine.stop("Serial {} closed.".format(self.port)) + if self.writer.is_closing(): + self.log.error("Serial {} closed.".format(self.port)) + self.machine.stop("Serial {} closed.".format(self.port)) return None if self.debug: @@ -142,7 +132,7 @@ def stop(self): if self.read_task: self.read_task.cancel() self.read_task = None - if self.writer: + if self.writer and not self.writer.is_closing(): self.writer.close() if hasattr(self.writer, "wait_closed"): # Python 3.7+ only diff --git a/mpf/platforms/opp/opp.py b/mpf/platforms/opp/opp.py index 60d0f18c4..4a7df18a4 100644 --- a/mpf/platforms/opp/opp.py +++ b/mpf/platforms/opp/opp.py @@ -33,6 +33,7 @@ from mpf.platforms.opp.opp_switch import OPPSwitch # pylint: disable-msg=cyclic-import,unused-import + # pylint: disable-msg=too-many-instance-attributes class OppHardwarePlatform(LightsPlatform, SwitchPlatform, DriverPlatform, ServoPlatform): @@ -304,8 +305,6 @@ async def _connect_to_hardware(self): for port in self.config['ports']: # overwrite serial if defined for port overwrite_chain_serial = port_chain_serial_map.get(port, None) - if overwrite_chain_serial is None and len(self.config['ports']) == 1: - overwrite_chain_serial = port comm = OPPSerialCommunicator(platform=self, port=port, baud=self.config['baud'], overwrite_serial=overwrite_chain_serial) @@ -348,6 +347,7 @@ def send_to_processor(self, chain_serial, msg): """ self.opp_connection[chain_serial].send(msg) + def update_incand(self): """Update all the incandescents connected to OPP hardware. @@ -955,20 +955,23 @@ async def _poll_sender(self, chain_serial): # there is no point in polling without switches return + poll_timeout = 1 / self.config['poll_hz'] * 25 + pause_time = 1 / self.config['poll_hz'] while True: + # send poll + self.send_to_processor(chain_serial, self.read_input_msg[chain_serial]) + await self.opp_connection[chain_serial].drain_writer() + # wait for previous poll response - timeout = 1 / self.config['poll_hz'] * 25 try: - await asyncio.wait_for(self._poll_response_received[chain_serial].wait(), timeout) + await asyncio.wait_for(self._poll_response_received[chain_serial].wait(), poll_timeout) except asyncio.TimeoutError: - self.log.warning("Poll took more than %sms for %s", timeout * 1000, chain_serial) + self.log.warning("Poll took more than %sms for board #%s", poll_timeout * 1000, chain_serial) else: self._poll_response_received[chain_serial].clear() - # send poll - self.send_to_processor(chain_serial, self.read_input_msg[chain_serial]) - await self.opp_connection[chain_serial].writer.drain() - # the line above saturates the link and seems to overwhelm the hardware. limit it to 100Hz - await asyncio.sleep(1 / self.config['poll_hz']) + + # the line above saturates the link and seems to overwhelm the hardware. + await asyncio.sleep(pause_time) def _verify_coil_and_switch_fit(self, switch, coil): chain_serial, card, solenoid = coil.hw_driver.number.split('-') diff --git a/mpf/platforms/opp/opp_serial_communicator.py b/mpf/platforms/opp/opp_serial_communicator.py index cdd354292..ec1bbb0b8 100644 --- a/mpf/platforms/opp/opp_serial_communicator.py +++ b/mpf/platforms/opp/opp_serial_communicator.py @@ -5,6 +5,8 @@ from mpf.platforms.base_serial_communicator import BaseSerialCommunicator, HEX_FORMAT +from serial import SerialException + MYPY = False if MYPY: # pragma: no cover from mpf.platforms.opp.opp import OppHardwarePlatform # pylint: disable-msg=cyclic-import,unused-import @@ -30,6 +32,7 @@ def __init__(self, platform: "OppHardwarePlatform", port, baud, overwrite_serial super().__init__(platform, port, baud) self.platform = platform # hint the right type + async def _read_id(self): msg = bytearray([0x20, 0x00, 0x00, 0x00, 0x00, 0x00]) msg.extend(OppRs232Intf.calc_crc8_whole_msg(msg)) @@ -74,8 +77,9 @@ async def _identify_connection(self): self.log.debug("Got ID response: %s", "".join(HEX_FORMAT % b for b in resp)) if self.chain_serial is None: - # get ID from hardware if it is not overwritten - self.chain_serial = str(await self._read_id()) + # get ID from hardware if it is not overwritten by config. If not available defaults to "0" + chain_serial = await self._read_id() + self.chain_serial = str(chain_serial) if chain_serial != 0xffffffff else "0" if self.chain_serial in self.platform.opp_connection: raise AssertionError("Duplicate chain serial {} on ports: {} and {}. Each OPP board has to have a " @@ -118,7 +122,7 @@ async def _identify_connection(self): self._create_vers_str(self.platform.min_version[self.chain_serial]))) # get initial value for inputs - self.log.debug("Getting initial inputs states for %s", self.chain_serial) + self.log.debug("Getting initial inputs states for #%s", self.chain_serial) self.send(self.platform.read_input_msg[self.chain_serial]) cards = len([x for x in self.platform.opp_inputs if x.chain_serial == self.chain_serial]) while True: @@ -127,8 +131,7 @@ async def _identify_connection(self): if cards <= 0: break self.log.debug("Waiting for another %s cards", cards) - - self.log.info("Init of OPP board %s done", self.chain_serial) + self.log.info("Init of OPP board #%s done on port %s", self.chain_serial, self.port) self.platform.register_processor_connection(self.chain_serial, self) def send_get_gen2_cfg_cmd(self): @@ -231,3 +234,14 @@ def _parse_msg(self, msg): self._lost_synch = True return message_found + + async def drain_writer(self): + """Drain writer buffer. + """ + try: + await self.writer.drain() + except SerialException as e: + self.log.warning("Serial {} error: {}".format(self.port, e)) + if self.writer.is_closing(): + self.log.error("Serial {} closed.".format(self.port)) + self.machine.stop("Serial {} closed.".format(self.port)) From 1b86b242b3efc7f6827b63c7a5c187f5a8bd7d85 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc=20Recht=C3=A9?= Date: Wed, 20 Aug 2025 16:40:21 +0200 Subject: [PATCH 2/4] Fix test accordingly. --- mpf/tests/test_OPP.py | 44 +++++++++++++++++++++++-------------------- 1 file changed, 24 insertions(+), 20 deletions(-) diff --git a/mpf/tests/test_OPP.py b/mpf/tests/test_OPP.py index 94a651814..5be498ac6 100644 --- a/mpf/tests/test_OPP.py +++ b/mpf/tests/test_OPP.py @@ -349,6 +349,7 @@ def setUp(self): inputs2_message = b"\x21\x08\x00\x00\x00\x00" inputs3a_message = b"\x23\x08\x00\x00\x00\x00" inputs3b_message = b"\x23\x19\x00\x00\x00\x00\x00\x00\x00\x01" + getserial_message = b'\x20\x00\x00\x00\x00\x00' self.serialMock.expected_commands = { b'\xf0': b'\xf0\x20\x21\x22\x23', # boards 20 + 21 + 22 + 23 installed @@ -372,6 +373,7 @@ def setUp(self): } self.serialMock.permanent_commands = { b'\xff': b'\xff', + self._crc_message(getserial_message): self._crc_message(getserial_message), self._crc_message(b'\x20\x08\x00\x00\x00\x00'): self._crc_message(inputs1_message), self._crc_message(b'\x21\x08\x00\x00\x00\x00'): self._crc_message(inputs2_message), self._crc_message(b'\x23\x08\x00\x00\x00\x00'): self._crc_message(inputs3a_message), @@ -382,40 +384,40 @@ def setUp(self): assert isinstance(self.machine.default_platform, OppHardwarePlatform) self._wait_for_processing() - self.assertEqual(0x02000000, self.machine.default_platform.min_version["com1"]) + self.assertEqual(0x02000000, self.machine.default_platform.min_version["0"]) self.assertFalse(self.serialMock.expected_commands) self.maxDiff = 100000 # test hardware scan info_str = """Connected CPUs: - - Port: com1 at 115200 baud. Chain Serial: com1 + - Port: com1 at 115200 baud. Chain Serial: 0 -> Board: 0x20 Firmware: 0x2000000 -> Board: 0x21 Firmware: 0x2000000 -> Board: 0x22 Firmware: 0x2000000 -> Board: 0x23 Firmware: 0x2000000 Incand cards: - - Chain: com1 Board: 0x20 Card: 0 Numbers: [16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31] - - Chain: com1 Board: 0x22 Card: 2 Numbers: [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20,\ + - Chain: 0 Board: 0x20 Card: 0 Numbers: [16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31] + - Chain: 0 Board: 0x22 Card: 2 Numbers: [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20,\ 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31] Input cards: - - Chain: com1 Board: 0x20 Card: 0 Numbers: [0, 1, 2, 3, 8, 9, 10, 11, 12, 13, 14, 15] - - Chain: com1 Board: 0x21 Card: 1 Numbers: [0, 1, 2, 3, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21,\ + - Chain: 0 Board: 0x20 Card: 0 Numbers: [0, 1, 2, 3, 8, 9, 10, 11, 12, 13, 14, 15] + - Chain: 0 Board: 0x21 Card: 1 Numbers: [0, 1, 2, 3, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21,\ 22, 23, 24, 25, 26, 27] - - Chain: com1 Board: 0x23 Card: 3 Numbers: [0, 1, 2, 3, 8, 9, 10, 11] - - Chain: com1 Board: 0x23 Card: 3 Numbers: [32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49,\ + - Chain: 0 Board: 0x23 Card: 3 Numbers: [0, 1, 2, 3, 8, 9, 10, 11] + - Chain: 0 Board: 0x23 Card: 3 Numbers: [32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49,\ 50, 51, 52, 53, 54, 55, 56, 57, 58, 59, 60, 61, 62, 63, 64, 65, 66, 67, 68, 69, 70, 71, 72, 73, 74, 75, 76, 77, 78,\ 79, 80, 81, 82, 83, 84, 85, 86, 87, 88, 89, 90, 91, 92, 93, 94, 95] Solenoid cards: - - Chain: com1 Board: 0x20 Card: 0 Numbers: [0, 1, 2, 3] - - Chain: com1 Board: 0x21 Card: 1 Numbers: [12, 13, 14, 15] - - Chain: com1 Board: 0x23 Card: 3 Numbers: [0, 1, 2, 3, 4, 5, 6, 7] + - Chain: 0 Board: 0x20 Card: 0 Numbers: [0, 1, 2, 3] + - Chain: 0 Board: 0x21 Card: 1 Numbers: [12, 13, 14, 15] + - Chain: 0 Board: 0x23 Card: 3 Numbers: [0, 1, 2, 3, 4, 5, 6, 7] LEDs: - - Chain: com1 Board: 0x21 Card: 1 + - Chain: 0 Board: 0x21 Card: 1 """ self.assertEqual(info_str, self.machine.default_platform.get_info_string()) @@ -552,6 +554,7 @@ def setUp(self): board2_version = b'\x21\x02\x00\x01\x01\x00' # 0.1.1.0 inputs1_message = b'\x20\x08\x00\x00\x00\x0c' # inputs 0+1 off, 2+3 on, 8 on inputs2_message = b'\x21\x08\x00\x00\x00\x00' + getserial_message = b'\x20\x00\x00\x00\x00\x00' self.serialMock.expected_commands = { b'\xf0': b'\xf0\x20\x21', # boards 20 + 21 installed @@ -569,6 +572,7 @@ def setUp(self): } self.serialMock.permanent_commands = { b'\xff': b'\xff', + self._crc_message(getserial_message): self._crc_message(getserial_message), self._crc_message(b'\x20\x08\x00\x00\x00\x00'): self._crc_message(inputs1_message), self._crc_message(b'\x21\x08\x00\x00\x00\x00'): self._crc_message(inputs2_message), # read inputs } @@ -589,23 +593,23 @@ def test_opp(self): # test hardware scan self.maxDiff = 100000 info_str = """Connected CPUs: - - Port: com1 at 115200 baud. Chain Serial: com1 + - Port: com1 at 115200 baud. Chain Serial: 0 -> Board: 0x20 Firmware: 0x10100 -> Board: 0x21 Firmware: 0x10100 Incand cards: - - Chain: com1 Board: 0x20 Card: 0 Numbers: [16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31] + - Chain: 0 Board: 0x20 Card: 0 Numbers: [16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31] Input cards: - - Chain: com1 Board: 0x20 Card: 0 Numbers: [0, 1, 2, 3, 8, 9, 10, 11, 12, 13, 14, 15] - - Chain: com1 Board: 0x21 Card: 1 Numbers: [0, 1, 2, 3, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27] + - Chain: 0 Board: 0x20 Card: 0 Numbers: [0, 1, 2, 3, 8, 9, 10, 11, 12, 13, 14, 15] + - Chain: 0 Board: 0x21 Card: 1 Numbers: [0, 1, 2, 3, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27] Solenoid cards: - - Chain: com1 Board: 0x20 Card: 0 Numbers: [0, 1, 2, 3] - - Chain: com1 Board: 0x21 Card: 1 Numbers: [12, 13, 14, 15] + - Chain: 0 Board: 0x20 Card: 0 Numbers: [0, 1, 2, 3] + - Chain: 0 Board: 0x21 Card: 1 Numbers: [12, 13, 14, 15] LEDs: - - Chain: com1 Board: 0x21 Card: 1 + - Chain: 0 Board: 0x21 Card: 1 """ self.assertEqual(info_str, self.machine.default_platform.get_info_string()) @@ -641,7 +645,7 @@ def _test_switches(self): self.serialMock.permanent_commands = permanent_commands def _test_coils(self): - self.assertEqual("OPP com1 Board 0x20", self.machine.coils["c_test"].hw_driver.get_board_name()) + self.assertEqual("OPP 0 Board 0x20", self.machine.coils["c_test"].hw_driver.get_board_name()) # pulse coil self.serialMock.expected_commands[self._crc_message(b'\x20\x14\x00\x02\x17\x00')] = False # configure coil 0 self.serialMock.expected_commands[self._crc_message(b'\x20\x07\x00\x01\x00\x01')] = False From 0b4e1b1c113fe826a48d45465da1756e52f402d6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc=20Recht=C3=A9?= Date: Wed, 20 Aug 2025 16:55:53 +0200 Subject: [PATCH 3/4] Fix cosmetic to pass tests. --- mpf/platforms/base_serial_communicator.py | 2 +- mpf/platforms/opp/opp_serial_communicator.py | 7 ++----- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/mpf/platforms/base_serial_communicator.py b/mpf/platforms/base_serial_communicator.py index 119e747f4..bcef0475e 100644 --- a/mpf/platforms/base_serial_communicator.py +++ b/mpf/platforms/base_serial_communicator.py @@ -2,7 +2,7 @@ from typing import Optional import asyncio -from serial import SerialException, EIGHTBITS, PARITY_NONE, STOPBITS_ONE +from serial import EIGHTBITS, PARITY_NONE, STOPBITS_ONE from mpf.core.utility_functions import Util diff --git a/mpf/platforms/opp/opp_serial_communicator.py b/mpf/platforms/opp/opp_serial_communicator.py index ec1bbb0b8..1b0c21a8a 100644 --- a/mpf/platforms/opp/opp_serial_communicator.py +++ b/mpf/platforms/opp/opp_serial_communicator.py @@ -1,12 +1,11 @@ """OPP serial communicator.""" import asyncio +from serial import SerialException from mpf.platforms.opp.opp_rs232_intf import OppRs232Intf from mpf.platforms.base_serial_communicator import BaseSerialCommunicator, HEX_FORMAT -from serial import SerialException - MYPY = False if MYPY: # pragma: no cover from mpf.platforms.opp.opp import OppHardwarePlatform # pylint: disable-msg=cyclic-import,unused-import @@ -32,7 +31,6 @@ def __init__(self, platform: "OppHardwarePlatform", port, baud, overwrite_serial super().__init__(platform, port, baud) self.platform = platform # hint the right type - async def _read_id(self): msg = bytearray([0x20, 0x00, 0x00, 0x00, 0x00, 0x00]) msg.extend(OppRs232Intf.calc_crc8_whole_msg(msg)) @@ -236,8 +234,7 @@ def _parse_msg(self, msg): return message_found async def drain_writer(self): - """Drain writer buffer. - """ + """Drain writer buffer.""" try: await self.writer.drain() except SerialException as e: From 2084a5785bb77a6b612a8d543daf398625a80153 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc=20Recht=C3=A9?= Date: Wed, 20 Aug 2025 16:59:08 +0200 Subject: [PATCH 4/4] Fix cosmetic to pass tests (2) --- mpf/platforms/opp/opp.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/mpf/platforms/opp/opp.py b/mpf/platforms/opp/opp.py index 4a7df18a4..233169777 100644 --- a/mpf/platforms/opp/opp.py +++ b/mpf/platforms/opp/opp.py @@ -33,7 +33,6 @@ from mpf.platforms.opp.opp_switch import OPPSwitch # pylint: disable-msg=cyclic-import,unused-import - # pylint: disable-msg=too-many-instance-attributes class OppHardwarePlatform(LightsPlatform, SwitchPlatform, DriverPlatform, ServoPlatform): @@ -347,7 +346,6 @@ def send_to_processor(self, chain_serial, msg): """ self.opp_connection[chain_serial].send(msg) - def update_incand(self): """Update all the incandescents connected to OPP hardware.