From 88250ce193aa97bc2acd0daf333c97baba9eed4a Mon Sep 17 00:00:00 2001 From: Hayden Roche Date: Thu, 27 Jul 2023 15:31:01 -0700 Subject: [PATCH 1/2] feat: Refactor serial and I2C code. - Get rid of conditionals on transaction manager existence and if serial locking should be used by adding no-op implementations of each. - Handle serial and I2C locking with decorators, serial_lock and i2c_lock, respectively. - Add timeout functionality to I2C locking. Prior to this commit, it would spin forever if the lock couldn't be acquired. - For both serial and I2C, combine common code from Transaction and Command into an internal function, _transact. Locking happens over this function. - Migrate all freestanding serial functions to methods of OpenSerial. - Create a _read_byte abstraction for the three platforms we support (CPython, MicroPython, CircuitPython). - Merge the functionality of _preprocess_req into _prepare_request. These are always used together, so it doesn't really make sense to have them as separate functions. Additionally, have _prepare_request do everything needed to transform the request data for transmission. Namely, encode the request string as UTF-8 bytes. Finally, make _prepare_request a method of Notecard. There's no reason for it to be a free function. - Rename I2C's _send_payload to _transmit. - Move Transaction and Command into the base Notecard class. They are now fully abstracted from the underlying comms layer. --- notecard/notecard.py | 403 +++++++++++++++----------------- notecard/transaction_manager.py | 16 ++ test/test_notecard.py | 3 +- 3 files changed, 203 insertions(+), 219 deletions(-) diff --git a/notecard/notecard.py b/notecard/notecard.py index 3a84df7..7a8f546 100644 --- a/notecard/notecard.py +++ b/notecard/notecard.py @@ -35,18 +35,24 @@ import json import time from .timeout import start_timeout, has_timed_out -from .transaction_manager import TransactionManager +from .transaction_manager import TransactionManager, NoOpTransactionManager use_periphery = False use_serial_lock = False -if sys.implementation.name == 'cpython': - if sys.platform == 'linux' or sys.platform == 'linux2': - use_periphery = True - from periphery import I2C +if sys.implementation.name == 'cpython' and (sys.platform == 'linux' or sys.platform == 'linux2'): - use_serial_lock = True - from filelock import Timeout, FileLock + use_periphery = True + from periphery import I2C + + use_serial_lock = True + from filelock import FileLock + from filelock import Timeout as SerialLockTimeout +else: + class SerialLockTimeout(Exception): + """A null SerialLockTimeout for when use_serial_lock is False.""" + + pass use_i2c_lock = not use_periphery and sys.implementation.name != 'micropython' @@ -65,119 +71,70 @@ I2C_CHUNK_DELAY_MS = 20 -def _prepare_request(req, debug=False): - """Format the request string as a JSON object and add a newline.""" - req_json = json.dumps(req) - if debug: - print(req_json) +class NoOpContextManager: + """A no-op context manager for use with NoOpSerialLock.""" - req_json += "\n" - return req_json + def __enter__(self): + """No-op enter function. Required for context managers.""" + pass + def __exit__(self, exc_type, exc_value, traceback): + """No-op exit function. Required for context managers.""" + pass -def serialReadByte(port): - """Read a single byte from a Notecard.""" - if sys.implementation.name == 'micropython': - if not port.any(): - return None - elif sys.implementation.name == 'cpython': - if port.in_waiting == 0: - return None - return port.read(1) + +class NoOpSerialLock(): + """A no-op serial lock class for when use_serial_lock is False.""" + + def acquire(*args, **kwargs): + """Acquire the no-op lock.""" + return NoOpContextManager() -def serialReset(port): - """Send a reset command to a Notecard.""" - for i in range(10): +def serial_lock(fn): + """Attempt to get a lock on the serial channel used for Notecard comms.""" + + def decorator(self, *args, **kwargs): try: - port.write(b'\n') - except: - continue - time.sleep(0.5) - somethingFound = False - nonControlCharFound = False - while True: - data = serialReadByte(port) - if (data is None) or (data == b''): - break - somethingFound = True - if data[0] >= 0x20: - nonControlCharFound = True - if somethingFound and not nonControlCharFound: - break - else: - raise Exception("Notecard not responding") + with self.lock.acquire(timeout=5): + return fn(self, *args, **kwargs) + except SerialLockTimeout: + raise Exception('Notecard in use') + return decorator -def serialTransaction(port, req, debug, txn_manager=None): - """Perform a single write to and read from a Notecard.""" - req_json = _prepare_request(req, debug) - transaction_timeout_secs = 30 - if txn_manager: - txn_manager.start(transaction_timeout_secs) +def i2c_lock(fn): + """Attempt to get a lock on the I2C bus used for Notecard comms.""" - try: - seg_off = 0 - seg_left = len(req_json) - while True: - seg_len = seg_left - if seg_len > CARD_REQUEST_SEGMENT_MAX_LEN: - seg_len = CARD_REQUEST_SEGMENT_MAX_LEN - - port.write(req_json[seg_off:seg_off + seg_len].encode('utf-8')) - seg_off += seg_len - seg_left -= seg_len - if seg_left == 0: + def decorator(self, *args, **kwargs): + retries = 5 + while use_i2c_lock and retries != 0: + if self.i2c.try_lock(): break - time.sleep(CARD_REQUEST_SEGMENT_DELAY_MS / 1000) - finally: - if txn_manager: - txn_manager.stop() - rsp_json = port.readline() - if debug: - print(rsp_json.rstrip()) + retries -= 1 + # Try again after 100 ms. + time.sleep(.1) - rsp = json.loads(rsp_json) - return rsp + if retries == 0: + raise Exception('Failed to acquire I2C lock.') + try: + ret = fn(self, *args, **kwargs) + finally: + if use_i2c_lock: + self.i2c.unlock() -def serialCommand(port, req, debug, txn_manager=None): - """Perform a single write to and read from a Notecard.""" - req_json = _prepare_request(req, debug) - - transaction_timeout_secs = 30 - if txn_manager: - txn_manager.start(transaction_timeout_secs) + return ret - try: - seg_off = 0 - seg_left = len(req_json) - while True: - seg_len = seg_left - if seg_len > CARD_REQUEST_SEGMENT_MAX_LEN: - seg_len = CARD_REQUEST_SEGMENT_MAX_LEN - - port.write(req_json[seg_off:seg_off + seg_len].encode('utf-8')) - seg_off += seg_len - seg_left -= seg_len - if seg_left == 0: - break - time.sleep(CARD_REQUEST_SEGMENT_DELAY_MS / 1000) - finally: - if txn_manager: - txn_manager.stop() + return decorator class Notecard: - """Base Notecard class. + """Base Notecard class.""" - Primary Notecard Class, which provides a shared __init__ - to reset the Notecard via Serial or I2C. - """ - - def __init__(self): + def __init__(self, debug=False): """Configure user agent.""" self._user_agent_app = None self._user_agent_sent = False @@ -191,17 +148,46 @@ def __init__(self): self._user_agent['os_family'] = os.name else: self._user_agent['os_family'] = os.uname().machine - self._transaction_manager = None + self._transaction_manager = NoOpTransactionManager() + self._debug = debug - def _preprocess_req(self, req): - """Inspect the request for hub.set and add the User Agent.""" + def _prepare_request(self, req): + """Prepare a request for transmission to the Notecard.""" + # Inspect the request for hub.set and add the User Agent. if 'hub.set' in req.values(): # Merge the User Agent to send along with the hub.set request. req = req.copy() req.update({'body': self.GetUserAgent()}) self._user_agent_sent = True - return req + + # Serialize the JSON request to a string. + req_string = json.dumps(req) + if self._debug: + print(req_string) + + req_string += "\n" + + # Encode the request string as UTF-8 bytes. + return req_string.encode('utf-8') + + def Command(self, req): + """Send a command to the Notecard. The Notecard response is ignored.""" + if 'cmd' not in req: + raise Exception("Please use 'cmd' instead of 'req'") + + req_bytes = self._prepare_request(req) + self._transact(req_bytes, False) + + def Transaction(self, req): + """Perform a Notecard transaction and return the result.""" + req_bytes = self._prepare_request(req) + rsp_bytes = self._transact(req_bytes, True) + rsp_json = json.loads(rsp_bytes) + if self._debug: + print(rsp_json) + + return rsp_json def GetUserAgent(self): """Return the User Agent String for the host for debug purposes.""" @@ -225,63 +211,94 @@ def SetTransactionPins(self, rtx_pin, ctx_pin): class OpenSerial(Notecard): """Notecard class for Serial communication.""" - def Command(self, req): - """Perform a Notecard command and exit with no response.""" - req = self._preprocess_req(req) - if 'cmd' not in req: - raise Exception("Please use 'cmd' instead of 'req'") + @serial_lock + def _transact(self, req, rsp_expected): + """Perform a low-level transaction with the Notecard.""" + rsp = None - if use_serial_lock: - try: - self.lock.acquire(timeout=5) - serialCommand(self.uart, req, self._debug, self._transaction_manager) - except Timeout: - raise Exception("Notecard in use") - finally: - self.lock.release() - else: - serialCommand(self.uart, req, self._debug, self._transaction_manager) + try: + transaction_timeout_secs = 30 + self._transaction_manager.start(transaction_timeout_secs) + + seg_off = 0 + seg_left = len(req) + while seg_left > 0: + seg_len = seg_left + if seg_len > CARD_REQUEST_SEGMENT_MAX_LEN: + seg_len = CARD_REQUEST_SEGMENT_MAX_LEN + + self.uart.write(req[seg_off:seg_off + seg_len]) + seg_off += seg_len + seg_left -= seg_len + time.sleep(CARD_REQUEST_SEGMENT_DELAY_MS / 1000) - def Transaction(self, req): - """Perform a Notecard transaction and return the result.""" - req = self._preprocess_req(req) - if use_serial_lock: - try: - self.lock.acquire(timeout=5) - return serialTransaction(self.uart, req, self._debug, - self._transaction_manager) - except Timeout: - raise Exception("Notecard in use") - finally: - self.lock.release() - else: - return serialTransaction(self.uart, req, self._debug, - self._transaction_manager) + if rsp_expected: + rsp = self.uart.readline() + finally: + self._transaction_manager.stop() + + return rsp + + def _read_byte_micropython(self): + """Read a single byte from the Notecard (MicroPython).""" + if not self.uart.any(): + return None + return self.uart.read(1) + + def _read_byte_cpython(self): + """Read a single byte from the Notecard (CPython).""" + if self.uart.in_waiting == 0: + return None + return self.uart.read(1) + + def _read_byte_circuitpython(self): + """Read a single byte from the Notecard (CircuitPython).""" + return self.uart.read(1) + @serial_lock def Reset(self): """Reset the Notecard.""" - if use_serial_lock: + for i in range(10): try: - self.lock.acquire(timeout=5) - serialReset(self.uart) - except Timeout: - raise Exception("Notecard in use") - finally: - self.lock.release() - else: - serialReset(self.uart) + self.uart.write(b'\n') + except: + continue + time.sleep(0.5) + somethingFound = False + nonControlCharFound = False + while True: + data = self._read_byte() + if (data is None) or (data == b''): + break + somethingFound = True + if data[0] >= 0x20: + nonControlCharFound = True + if somethingFound and not nonControlCharFound: + break + else: + raise Exception('Notecard not responding') def __init__(self, uart_id, debug=False): """Initialize the Notecard before a reset.""" - super().__init__() + super().__init__(debug) self._user_agent['req_interface'] = 'serial' self._user_agent['req_port'] = str(uart_id) self.uart = uart_id - self._debug = debug if use_serial_lock: - self.lock = FileLock('serial.lock', timeout=1) + self.lock = FileLock('serial.lock') + else: + self.lock = NoOpSerialLock() + + if sys.implementation.name == 'micropython': + self._read_byte = self._read_byte_micropython + elif sys.implementation.name == 'cpython': + self._read_byte = self._read_byte_cpython + elif sys.implementation.name == 'circuitpython': + self._read_byte = self._read_byte_circuitpython + else: + raise NotImplementedError(f'Unsupported platform: {sys.implementation.name}') self.Reset() @@ -289,17 +306,17 @@ def __init__(self, uart_id, debug=False): class OpenI2C(Notecard): """Notecard class for I2C communication.""" - def _send_payload(self, json): + def _transmit(self, data): chunk_offset = 0 - json_left = len(json) + data_left = len(data) sent_in_seg = 0 write_length = bytearray(1) - while json_left > 0: - chunk_len = min(json_left, self.max) + while data_left > 0: + chunk_len = min(data_left, self.max) write_length[0] = chunk_len - write_data = bytes(json[chunk_offset:chunk_offset + chunk_len], - 'utf-8') + write_data = data[chunk_offset:chunk_offset + chunk_len] + # Send a message with the length of the incoming bytes followed # by the bytes themselves. if use_periphery: @@ -309,7 +326,7 @@ def _send_payload(self, json): self.i2c.writeto(self.addr, write_length + write_data) chunk_offset += chunk_len - json_left -= chunk_len + data_left -= chunk_len sent_in_seg += chunk_len if sent_in_seg > CARD_REQUEST_SEGMENT_MAX_LEN: @@ -374,92 +391,42 @@ def _receive(self, timeout_secs, chunk_delay_secs, wait_for_newline): # behavior of other SDKs (e.g. note-c). time.sleep(chunk_delay_secs) - if (timeout_secs != 0 and has_timed_out(start, timeout_secs)): + if timeout_secs != 0 and has_timed_out(start, timeout_secs): raise Exception("Timed out while reading data from the Notecard.") return read_data - def Command(self, req): - """Perform a Notecard command and return with no response.""" - if 'cmd' not in req: - raise Exception("Please use 'cmd' instead of 'req'") - - req = self._preprocess_req(req) - req_json = _prepare_request(req, self._debug) - - while not self.lock(): - pass + @i2c_lock + def _transact(self, req, rsp_expected): + """Perform a low-level transaction with the Notecard.""" + rsp = None try: transaction_timeout_secs = 30 - if self._transaction_manager: - self._transaction_manager.start(transaction_timeout_secs) + self._transaction_manager.start(transaction_timeout_secs) - self._send_payload(req_json) - finally: - self.unlock() - if self._transaction_manager: - self._transaction_manager.stop() + self._transmit(req) - def Transaction(self, req): - """Perform a Notecard transaction and return the result.""" - req = self._preprocess_req(req) - req_json = _prepare_request(req, self._debug) - rsp_json = "" - - while not self.lock(): - pass - - try: - transaction_timeout_secs = 30 - if self._transaction_manager: - self._transaction_manager.start(transaction_timeout_secs) - - self._send_payload(req_json) - - read_data = self._receive(transaction_timeout_secs, 0.05, True) - rsp_json = "".join(map(chr, read_data)) + if rsp_expected: + rsp = self._receive(30, 0.05, True) finally: - self.unlock() - if self._transaction_manager: - self._transaction_manager.stop() + self._transaction_manager.stop() - if self._debug: - print(rsp_json.rstrip()) - - return json.loads(rsp_json) + return rsp + @i2c_lock def Reset(self): """Reset the Notecard.""" - while not self.lock(): - pass - - try: - # Read from the Notecard until there's nothing left to read. - self._receive(0, .001, False) - finally: - self.unlock() - - def lock(self): - """Lock the I2C port so the host can interact with the Notecard.""" - if use_i2c_lock: - return self.i2c.try_lock() - return True - - def unlock(self): - """Unlock the I2C port.""" - if use_i2c_lock: - return self.i2c.unlock() - return True + # Read from the Notecard until there's nothing left to read. + self._receive(0, .001, False) def __init__(self, i2c, address, max_transfer, debug=False): """Initialize the Notecard before a reset.""" - super().__init__() + super().__init__(debug) self._user_agent['req_interface'] = 'i2c' self._user_agent['req_port'] = address self.i2c = i2c - self._debug = debug if address == 0: self.addr = NOTECARD_I2C_ADDRESS else: diff --git a/notecard/transaction_manager.py b/notecard/transaction_manager.py index 5177797..bbd5351 100644 --- a/notecard/transaction_manager.py +++ b/notecard/transaction_manager.py @@ -58,3 +58,19 @@ def stop(self): """Make RTX an input to conserve power and remove the pull up on CTX.""" self.rtx_pin.direction(GPIO.IN) self.ctx_pin.pull(GPIO.PULL_NONE) + + +class NoOpTransactionManager: + """Class for transaction start/stop when no transaction pins are set. + + If the transaction pins aren't set, the start and stop operations should be + no-ops. + """ + + def start(self, timeout_secs): + """No-op start function.""" + pass + + def stop(self): + """No-op stop function.""" + pass diff --git a/test/test_notecard.py b/test/test_notecard.py index b107907..9c209bf 100644 --- a/test/test_notecard.py +++ b/test/test_notecard.py @@ -3,6 +3,7 @@ import pytest from unittest.mock import Mock, MagicMock, patch import periphery +import json sys.path.insert(0, os.path.abspath(os.path.join(os.path.dirname(__file__), '..'))) @@ -462,7 +463,7 @@ def setUserAgentInfo(self, info=None): nCard = MockNotecard() orgReq = {"req": "hub.set"} nCard.SetAppUserAgent(info) - req = nCard._preprocess_req(orgReq) + req = json.loads(nCard._prepare_request(orgReq)) return req def test_amends_hub_set_request(self): From 6afd1f7dc1b58f21c194021f48f8632e38d989a4 Mon Sep 17 00:00:00 2001 From: Hayden Roche Date: Wed, 6 Sep 2023 16:10:08 -0700 Subject: [PATCH 2/2] Address review feedback. --- notecard/notecard.py | 119 +++++++++++++++++++++++++++++-------------- 1 file changed, 82 insertions(+), 37 deletions(-) diff --git a/notecard/notecard.py b/notecard/notecard.py index 7a8f546..2c6fff0 100644 --- a/notecard/notecard.py +++ b/notecard/notecard.py @@ -54,8 +54,6 @@ class SerialLockTimeout(Exception): pass -use_i2c_lock = not use_periphery and sys.implementation.name != 'micropython' - NOTECARD_I2C_ADDRESS = 0x17 # The notecard is a real-time device that has a fixed size interrupt buffer. @@ -109,8 +107,8 @@ def i2c_lock(fn): def decorator(self, *args, **kwargs): retries = 5 - while use_i2c_lock and retries != 0: - if self.i2c.try_lock(): + while retries != 0: + if self.lock(): break retries -= 1 @@ -123,8 +121,7 @@ def decorator(self, *args, **kwargs): try: ret = fn(self, *args, **kwargs) finally: - if use_i2c_lock: - self.i2c.unlock() + self.unlock() return ret @@ -306,24 +303,24 @@ def __init__(self, uart_id, debug=False): class OpenI2C(Notecard): """Notecard class for I2C communication.""" + def _write(self, data): + write_length = bytearray(1) + write_length[0] = len(data) + + # Send a message with the length of the incoming bytes followed + # by the bytes themselves. + self._platform_write(write_length, data) + def _transmit(self, data): chunk_offset = 0 data_left = len(data) sent_in_seg = 0 - write_length = bytearray(1) while data_left > 0: chunk_len = min(data_left, self.max) - write_length[0] = chunk_len write_data = data[chunk_offset:chunk_offset + chunk_len] - # Send a message with the length of the incoming bytes followed - # by the bytes themselves. - if use_periphery: - msgs = [I2C.Message(write_length + write_data)] - self.i2c.transfer(self.addr, msgs) - else: - self.i2c.writeto(self.addr, write_length + write_data) + self._write(write_data) chunk_offset += chunk_len data_left -= chunk_len @@ -335,6 +332,21 @@ def _transmit(self, data): time.sleep(I2C_CHUNK_DELAY_MS / 1000) + def _read(self, length): + initiate_read = bytearray(2) + # 0 indicates we are reading from the Notecard. + initiate_read[0] = 0 + # This indicates how many bytes we are prepared to read. + initiate_read[1] = length + # read_buf is a buffer to store the data we're reading. + # length accounts for the payload and the +2 is for the header. The + # header sent by the Notecard has one byte to indicate the number of + # bytes still available to read and a second byte to indicate the number + # of bytes coming in the current chunk. + read_buf = bytearray(length + 2) + + return self._platform_read(initiate_read, read_buf) + def _receive(self, timeout_secs, chunk_delay_secs, wait_for_newline): chunk_len = 0 received_newline = False @@ -342,28 +354,7 @@ def _receive(self, timeout_secs, chunk_delay_secs, wait_for_newline): read_data = bytearray() while True: - initiate_read = bytearray(2) - # 0 indicates we are reading from the Notecard. - initiate_read[0] = 0 - # This indicates how many bytes we are prepared to read. - initiate_read[1] = chunk_len - # read_buf is a buffer to store the data we're reading. - # chunk_len accounts for the payload and the +2 is for the - # header. The header sent by the Notecard has one byte to - # indicate the number of bytes still available to read and a - # second byte to indicate the number of bytes coming in the - # current chunk. - read_buf = bytearray(chunk_len + 2) - - if use_periphery: - msgs = [I2C.Message(initiate_read), I2C.Message(read_buf, read=True)] - self.i2c.transfer(self.addr, msgs) - read_buf = msgs[1].data - elif sys.implementation.name == 'micropython': - self.i2c.writeto(self.addr, initiate_read, False) - self.i2c.readfrom_into(self.addr, read_buf) - else: - self.i2c.writeto_then_readfrom(self.addr, initiate_read, read_buf) + read_buf = self._read(chunk_len) # The number of bytes still available to read. num_bytes_available = read_buf[0] @@ -420,6 +411,31 @@ def Reset(self): # Read from the Notecard until there's nothing left to read. self._receive(0, .001, False) + def _linux_write(self, length, data): + msgs = [I2C.Message(length + data)] + self.i2c.transfer(self.addr, msgs) + + def _non_linux_write(self, length, data): + self.i2c.writeto(self.addr, length + data) + + def _linux_read(self, initiate_read_msg, read_buf): + msgs = [I2C.Message(initiate_read_msg), I2C.Message(read_buf, read=True)] + self.i2c.transfer(self.addr, msgs) + read_buf = msgs[1].data + + return read_buf + + def _micropython_read(self, initiate_read_msg, read_buf): + self.i2c.writeto(self.addr, initiate_read_msg, False) + self.i2c.readfrom_into(self.addr, read_buf) + + return read_buf + + def _circuitpython_read(self, initiate_read_msg, read_buf): + self.i2c.writeto_then_readfrom(self.addr, initiate_read_msg, read_buf) + + return read_buf + def __init__(self, i2c, address, max_transfer, debug=False): """Initialize the Notecard before a reset.""" super().__init__(debug) @@ -427,6 +443,23 @@ def __init__(self, i2c, address, max_transfer, debug=False): self._user_agent['req_port'] = address self.i2c = i2c + + def i2c_no_op_try_lock(*args, **kwargs): + """No-op lock function.""" + return True + + def i2c_no_op_unlock(*args, **kwargs): + """No-op unlock function.""" + pass + + use_i2c_lock = not use_periphery and sys.implementation.name != 'micropython' + if use_i2c_lock: + self.lock = self.i2c.try_lock + self.unlock = self.i2c.unlock + else: + self.lock = i2c_no_op_try_lock + self.unlock = i2c_no_op_unlock + if address == 0: self.addr = NOTECARD_I2C_ADDRESS else: @@ -436,4 +469,16 @@ def __init__(self, i2c, address, max_transfer, debug=False): else: self.max = max_transfer + if use_periphery: + self._platform_write = self._linux_write + self._platform_read = self._linux_read + elif sys.implementation.name == 'micropython': + self._platform_write = self._non_linux_write + self._platform_read = self._micropython_read + elif sys.implementation.name == 'circuitpython': + self._platform_write = self._non_linux_write + self._platform_read = self._circuitpython_read + else: + raise NotImplementedError(f'Unsupported platform: {sys.implementation.name}') + self.Reset()