From 2903ede36b84e29f32abd699a38145b28c7d2369 Mon Sep 17 00:00:00 2001 From: William Emfinger Date: Fri, 30 Oct 2026 21:19:05 -0500 Subject: [PATCH 1/2] feat(persistent_data): pure-Python counterpart (espp.persistent_data) Reads and writes the firmware's versioned settings files from Python with no extension module: a 20-byte header (magic, schema version, layout fingerprint, length, CRC32) in front of an alpaca payload encoded with fixed_length_encoding | strict, which is fixed-width little-endian integers and 32-bit length prefixes, so struct + zlib cover it. Schema versions are dataclasses, newest first, mirroring the C++ type list: a VERSION class variable, migrate(older) on every version but the last, width aliases (u8 ... u64, i8 ... i64, f32, f64, char) for numeric fields (a bare int / float is an error, never a guess), array(T, N) for std::array, unordered(dict/set) for the unordered containers, Optional[T], tuple[...], enums with an Annotated underlying width, and nested dataclasses. The layout fingerprint is derived from the annotations by the grammar in the component README, so a file written in Python verifies on the device and vice versa; a mismatch is layout_mismatch, never wrong values. PersistentData(versions, path): load / load_or_create / save / update / erase / encode / decode, atomic temp + rename writes, the same error taxonomy as PersistentDataErrc (PersistentDataError.errc), thread-safe. python/persistent_data_test.py checks describe() bytes, fingerprints and whole encoded files byte-for-byte against vectors generated by the C++ codec (python/persistent_data_vectors.txt), the migration chain, every error path, the other field types and the file operations (76 checks); it runs from the source tree without a built wheel and is added to the wheel test command. Exported as espp.persistent_data; documented in the component README and python/README.md. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01DFjjnq3XCTRAXENSxsCxJU --- components/persistent_data/README.md | 40 + lib/python_bindings/espp/__init__.py | 4 + lib/python_bindings/espp/persistent_data.py | 860 ++++++++++++++++++++ pyproject.toml | 7 +- python/README.md | 7 + python/persistent_data_test.py | 385 +++++++++ python/persistent_data_vectors.txt | 12 + 7 files changed, 1312 insertions(+), 3 deletions(-) create mode 100644 lib/python_bindings/espp/persistent_data.py create mode 100644 python/persistent_data_test.py create mode 100644 python/persistent_data_vectors.txt diff --git a/components/persistent_data/README.md b/components/persistent_data/README.md index 54de4c3511..119c67e9a0 100644 --- a/components/persistent_data/README.md +++ b/components/persistent_data/README.md @@ -209,6 +209,46 @@ settings file, or for host-side tests of migrations. It is not in the Python bindings: `PersistentData` is a class template over the application's own types, which the generated bindings cannot instantiate generically. +## Python + +The espp Python package ships `espp.persistent_data`, a pure-Python (stdlib +only) counterpart that reads and writes the same files: schema versions are +dataclasses, newest first, with width aliases for the numeric fields and a +`migrate()` classmethod per version, exactly mirroring the C++ type list. The +layout fingerprint is derived from the annotations by the grammar above, so a +file written in Python verifies on the device and vice versa. + +```python +from dataclasses import dataclass, field +from typing import ClassVar +from espp.persistent_data import PersistentData, u8, u32, f32 + +@dataclass +class SettingsV1: + VERSION: ClassVar[int] = 1 + brightness: u8 = 50 + device_name: str = "espp" + +@dataclass +class Settings: + VERSION: ClassVar[int] = 2 + device_name: str = "espp" + brightness: f32 = 0.5 + favorites: list[str] = field(default_factory=list) + + @classmethod + def migrate(cls, v1: SettingsV1) -> "Settings": + return cls(device_name=v1.device_name, brightness=v1.brightness / 100) + +settings = PersistentData([Settings, SettingsV1], "settings.bin") +data = settings.load_or_create(Settings()) # migrates a v1 file and rewrites it +settings.update(lambda s: s.favorites.append("red")) +``` + +Errors are `PersistentDataError` with an `errc` matching `PersistentDataErrc`. +The codec is byte-checked against vectors generated by the C++ codec in +`python/persistent_data_test.py`. + ## Host test The file format, version list, migration chain and file helpers live in diff --git a/lib/python_bindings/espp/__init__.py b/lib/python_bindings/espp/__init__.py index 1743a11364..781cb68313 100644 --- a/lib/python_bindings/espp/__init__.py +++ b/lib/python_bindings/espp/__init__.py @@ -12,6 +12,10 @@ # ``espp.rtps.Publisher`` / ``espp.rtps.Subscriber``). from . import rtps # noqa: F401,E402 +# Pure-Python counterpart of the persistent_data component: reads / writes the +# firmware's versioned settings files from dataclasses (``espp.persistent_data``). +from . import persistent_data # noqa: F401,E402 + # The pure-python ODrive legacy-native protocol CLIENT (espp_odrive, sourced # from components/odrive_native/python) ships alongside this package in the # wheel / installed prefix; expose it as ``espp.odrive`` for convenience. The diff --git a/lib/python_bindings/espp/persistent_data.py b/lib/python_bindings/espp/persistent_data.py new file mode 100644 index 0000000000..09d1b2bbac --- /dev/null +++ b/lib/python_bindings/espp/persistent_data.py @@ -0,0 +1,860 @@ +"""Python counterpart of the C++ ``espp::PersistentData`` component. + +Reads and writes the same files the firmware does -- a 20-byte header (magic, +schema version, layout fingerprint, payload length, CRC32) in front of an +alpaca payload (``fixed_length_encoding | strict``) -- so a PC tool can inspect, +edit or generate a device's settings file, and a host test can exercise a +migration chain without hardware. Pure Python (``struct`` + ``zlib``), no +extension module needed: the payload encoding is fixed-width little-endian +integers, 32-bit length prefixes and nothing else. + +Schema versions are plain :mod:`dataclasses`, newest first, exactly like the C++ +type list. Because Python's ``int`` and ``float`` carry no width, numeric fields +use the width aliases below (``u8`` ... ``f64``); a bare ``int``/``float`` field +is an error rather than a silent guess. Every version declares a ``VERSION`` +class variable, and every version except the oldest provides ``migrate(older)`` +(a classmethod or staticmethod) from the version after it in the list:: + + from dataclasses import dataclass, field + from typing import ClassVar + from espp.persistent_data import PersistentData, u8, u32, f32, array + + @dataclass + class SettingsV1: + VERSION: ClassVar[int] = 1 + brightness: u8 = 50 + device_name: str = "espp" + + @dataclass + class Settings: + VERSION: ClassVar[int] = 2 + device_name: str = "espp" + brightness: f32 = 0.5 + favorites: list[str] = field(default_factory=list) + gains: array(f32, 4) = field(default_factory=lambda: [1.0] * 4) + + @classmethod + def migrate(cls, v1: SettingsV1) -> "Settings": + return cls(device_name=v1.device_name, brightness=v1.brightness / 100) + + settings = PersistentData([Settings, SettingsV1], "/mnt/device/settings.bin") + data = settings.load_or_create(Settings()) # migrates a v1 file, rewrites it + settings.update(lambda s: s.favorites.append("red")) + +Type mapping (Python annotation -> alpaca field type, as the firmware sees it): + +====================================== ============================================ +``bool`` ``bool`` +``u8 u16 u32 u64 i8 i16 i32 i64`` the fixed-width integer +``f32`` / ``f64`` ``float`` / ``double`` +``char`` ``char`` (one-character ``str``) +``str`` ``std::string`` (UTF-8) +``list[T]`` ``std::vector`` +``array(T, N)`` ``std::array`` +``dict[K, V]`` / ``unordered(dict[K, V])`` ``std::map`` / ``std::unordered_map`` +``set[T]`` / ``unordered(set[T])`` ``std::set`` / ``std::unordered_set`` +``Optional[T]`` ``std::optional`` +``tuple[A, B, ...]`` ``std::tuple`` +``enum.Enum`` (``Annotated[E, "B"]`` ...) ``enum class E : uint8_t`` (default ``int``) +nested ``@dataclass`` nested aggregate +====================================== ============================================ + +The layout fingerprint in the header is computed from these annotations by the +grammar documented in ``components/persistent_data/README.md``, so a file written +here verifies on the device and a device file verifies here. A mismatch between +the Python dataclass and the C++ struct surfaces as ``layout_mismatch``, never +as silently wrong values. +""" + +from __future__ import annotations + +import dataclasses +import enum +import os +import struct +import threading +import types +import typing +import zlib +from typing import Annotated, Any, Callable, ClassVar, Optional, Sequence + +__all__ = [ + "Errc", + "PersistentDataError", + "PersistentData", + "LoadInfo", + "MAGIC", + "HEADER_SIZE", + "bool_", + "char", + "u8", + "u16", + "u32", + "u64", + "i8", + "i16", + "i32", + "i64", + "f32", + "f64", + "array", + "unordered", + "describe", + "fingerprint", + "encode_payload", + "decode_payload", + "encode", + "decode", + "VersionList", +] + +# --------------------------------------------------------------------------- +# File format constants (must match include/detail/persistent_data_codec.hpp) +# --------------------------------------------------------------------------- + +MAGIC = 0x44505345 # "ESPD" little-endian +HEADER_SIZE = 20 +_HEADER = struct.Struct(" (field type, struct format) +_SCALARS = { + "?": (_FT.bool_, " str: # pragma: no cover - debugging aid + return f"_ArrayLen({self.n})" + + +class _Unordered: + __slots__ = () + + def __repr__(self) -> str: # pragma: no cover + return "_Unordered()" + + +def array(elem: Any, n: int): + """``std::array``: a list of exactly ``n`` elements (no length prefix on the wire).""" + return Annotated[list[elem], _ArrayLen(n)] + + +def unordered(tp: Any): + """Mark a ``dict``/``set`` annotation as ``std::unordered_map`` / ``std::unordered_set``.""" + return Annotated[tp, _Unordered()] + + +# --------------------------------------------------------------------------- +# Annotation -> Spec tree +# --------------------------------------------------------------------------- + + +@dataclasses.dataclass(frozen=True) +class _Spec: + kind: _FT + fmt: str = "" # scalars: struct format + elems: tuple = () # vector/array/set/optional: (elem,); map: (key, value); tuple: elems + n: int = 0 # array length + cls: Any = None # struct / enum class + fields: tuple = () # struct: ((name, spec), ...) + + +_NONE_TYPE = type(None) + + +def _split_annotated(tp: Any): + """Return (base type, [hints]) for Annotated[...] (hints possibly nested), else (tp, []).""" + hints: list = [] + while typing.get_origin(tp) is Annotated: + args = typing.get_args(tp) + tp = args[0] + hints = list(args[1:]) + hints + return tp, hints + + +def _spec_of(tp: Any, where: str) -> _Spec: + base, hints = _split_annotated(tp) + width = next((h for h in hints if isinstance(h, str)), None) + array_len = next((h for h in hints if isinstance(h, _ArrayLen)), None) + is_unordered = any(isinstance(h, _Unordered) for h in hints) + + # enums before int (IntEnum is an int subclass); the underlying type is the + # width hint, default int32 like an un-typed C++ `enum class` + if isinstance(base, type) and issubclass(base, enum.Enum): + code = width or "i" + if code not in _SCALARS or code in ("?", "c", "f", "d"): + raise TypeError(f"{where}: enum underlying type must be an integer width, got {code!r}") + ft, fmt = _SCALARS[code] + return _Spec(_FT.enum_class, elems=(_Spec(ft, fmt),), cls=base) + + if base is bool: + return _Spec(*_SCALARS["?"]) + if base is int: + if width is None: + raise TypeError(f"{where}: bare 'int' has no width; use u8/u16/u32/u64/i8/i16/i32/i64") + if width not in _SCALARS or width in ("?", "c", "f", "d"): + raise TypeError(f"{where}: {width!r} is not an integer width") + return _Spec(*_SCALARS[width]) + if base is float: + if width is None: + raise TypeError(f"{where}: bare 'float' has no width; use f32 or f64") + if width not in ("f", "d"): + raise TypeError(f"{where}: {width!r} is not a float width") + return _Spec(*_SCALARS[width]) + if base is str: + if width == "c": + return _Spec(*_SCALARS["c"]) + return _Spec(_FT.string) + if base is bytes: + raise TypeError(f"{where}: 'bytes' is ambiguous; use list[u8] (std::vector) or str") + + origin = typing.get_origin(base) + args = typing.get_args(base) + if origin is list: + if len(args) != 1: + raise TypeError(f"{where}: list needs one element type") + elem = _spec_of(args[0], where + "[]") + if array_len is not None: + return _Spec(_FT.array, elems=(elem,), n=array_len.n) + return _Spec(_FT.vector, elems=(elem,)) + if origin is dict: + if len(args) != 2: + raise TypeError(f"{where}: dict needs key and value types") + k = _spec_of(args[0], where + ".key") + v = _spec_of(args[1], where + ".value") + return _Spec(_FT.unordered_map if is_unordered else _FT.map, elems=(k, v)) + if origin in (set, frozenset): + if len(args) != 1: + raise TypeError(f"{where}: set needs one element type") + return _Spec(_FT.unordered_set if is_unordered else _FT.set, + elems=(_spec_of(args[0], where + "{}"),)) + if origin is tuple: + if not args or (len(args) == 2 and args[1] is Ellipsis): + raise TypeError(f"{where}: tuple needs a fixed list of element types") + return _Spec(_FT.tuple, elems=tuple(_spec_of(a, f"{where}[{i}]") for i, a in enumerate(args))) + if origin is typing.Union or origin is types.UnionType: + non_none = [a for a in args if a is not _NONE_TYPE] + if len(non_none) == 1 and len(args) == 2: + return _Spec(_FT.optional, elems=(_spec_of(non_none[0], where + "?"),)) + raise TypeError(f"{where}: only Optional[T] unions are supported (std::variant is not)") + if dataclasses.is_dataclass(base) and isinstance(base, type): + return _struct_spec(base) + raise TypeError(f"{where}: unsupported field type {tp!r}") + + +_struct_cache: dict = {} + + +def _struct_spec(cls: type) -> _Spec: + spec = _struct_cache.get(cls) + if spec is not None: + return spec + if not dataclasses.is_dataclass(cls): + raise TypeError(f"{cls.__qualname__} is not a dataclass") + hints = typing.get_type_hints(cls, include_extras=True) + fields = [] + for f in dataclasses.fields(cls): + fields.append((f.name, _spec_of(hints[f.name], f"{cls.__qualname__}.{f.name}"))) + spec = _Spec(_FT.struct_, cls=cls, fields=tuple(fields)) + _struct_cache[cls] = spec + return spec + + +# --------------------------------------------------------------------------- +# Layout description + fingerprint (README "Layout fingerprint" grammar) +# --------------------------------------------------------------------------- + + +def _describe(spec: _Spec, out: bytearray) -> None: + out.append(int(spec.kind)) + if spec.kind in (_FT.enum_class, _FT.vector, _FT.set, _FT.unordered_set, _FT.optional): + _describe(spec.elems[0], out) + elif spec.kind == _FT.array: + out += _U32.pack(spec.n) + _describe(spec.elems[0], out) + elif spec.kind in (_FT.map, _FT.unordered_map): + _describe(spec.elems[0], out) + _describe(spec.elems[1], out) + elif spec.kind == _FT.tuple: + out += _U16.pack(len(spec.elems)) + for e in spec.elems: + _describe(e, out) + elif spec.kind == _FT.struct_: + out += _U16.pack(len(spec.fields)) + for _, f in spec.fields: + _describe(f, out) + + +def describe(cls: type) -> bytes: + """The layout description bytes of a dataclass (what the fingerprint hashes).""" + out = bytearray() + _describe(_struct_spec(cls), out) + return bytes(out) + + +def fingerprint(cls: type) -> int: + """CRC32 of :func:`describe`; the ``layout`` field of the file header.""" + return zlib.crc32(describe(cls)) & 0xFFFFFFFF + + +# --------------------------------------------------------------------------- +# Payload codec (alpaca, fixed_length_encoding | strict) +# --------------------------------------------------------------------------- + + +def _enc(spec: _Spec, value: Any, out: bytearray, where: str) -> None: + k = spec.kind + try: + if spec.fmt: # scalar + if k == _FT.char_: + b = value.encode("latin-1") if isinstance(value, str) else bytes([value]) + if len(b) != 1: + raise ValueError("char must be exactly one byte") + out += b + else: + out += struct.pack(spec.fmt, value) + elif k == _FT.enum_class: + v = value.value if isinstance(value, enum.Enum) else value + out += struct.pack(spec.elems[0].fmt, v) + elif k == _FT.string: + b = value.encode("utf-8") if isinstance(value, str) else bytes(value) + out += _U32.pack(len(b)) + b + elif k == _FT.vector: + out += _U32.pack(len(value)) + for i, v in enumerate(value): + _enc(spec.elems[0], v, out, f"{where}[{i}]") + elif k == _FT.array: + if len(value) != spec.n: + raise ValueError(f"array needs exactly {spec.n} elements, got {len(value)}") + for i, v in enumerate(value): + _enc(spec.elems[0], v, out, f"{where}[{i}]") + elif k in (_FT.map, _FT.unordered_map): + out += _U32.pack(len(value)) + items = value.items() + if k == _FT.map: + items = sorted(items) # std::map iterates in key order + for kk, vv in items: + _enc(spec.elems[0], kk, out, f"{where}.key") + _enc(spec.elems[1], vv, out, f"{where}[{kk!r}]") + elif k in (_FT.set, _FT.unordered_set): + out += _U32.pack(len(value)) + elems = sorted(value) if k == _FT.set else list(value) + for i, v in enumerate(elems): + _enc(spec.elems[0], v, out, f"{where}{{{i}}}") + elif k == _FT.optional: + if value is None: + out.append(0) + else: + out.append(1) + _enc(spec.elems[0], value, out, where + "?") + elif k == _FT.tuple: + if len(value) != len(spec.elems): + raise ValueError(f"tuple needs {len(spec.elems)} elements, got {len(value)}") + for i, (e, v) in enumerate(zip(spec.elems, value)): + _enc(e, v, out, f"{where}[{i}]") + elif k == _FT.struct_: + for name, f in spec.fields: + _enc(f, getattr(value, name), out, f"{where}.{name}") + else: # pragma: no cover + raise ValueError(f"unsupported kind {k}") + except PersistentDataError: + raise + except (struct.error, ValueError, TypeError, AttributeError, UnicodeError) as exc: + raise PersistentDataError(Errc.serialize_failed, f"{where}: {exc}") from None + + +class _Reader: + __slots__ = ("data", "pos") + + def __init__(self, data: bytes): + self.data = data + self.pos = 0 + + def take(self, n: int, where: str) -> bytes: + if n < 0 or self.pos + n > len(self.data): + raise PersistentDataError(Errc.deserialize_failed, + f"{where}: input ended ({self.pos + n} > {len(self.data)} bytes)") + b = self.data[self.pos:self.pos + n] + self.pos += n + return b + + def count(self, where: str) -> int: + n = _U32.unpack(self.take(4, where))[0] + if n > len(self.data) - self.pos: + raise PersistentDataError(Errc.deserialize_failed, + f"{where}: element count {n} exceeds the remaining bytes") + return n + + +def _dec(spec: _Spec, r: _Reader, where: str) -> Any: + k = spec.kind + if spec.fmt: + b = r.take(struct.calcsize(spec.fmt), where) + v = struct.unpack(spec.fmt, b)[0] + return v.decode("latin-1") if k == _FT.char_ else v + if k == _FT.enum_class: + raw = struct.unpack(spec.elems[0].fmt, r.take(struct.calcsize(spec.elems[0].fmt), where))[0] + try: + return spec.cls(raw) + except ValueError: + raise PersistentDataError(Errc.deserialize_failed, + f"{where}: {raw} is not a {spec.cls.__qualname__}") from None + if k == _FT.string: + n = r.count(where) + try: + return r.take(n, where).decode("utf-8") + except UnicodeDecodeError: + raise PersistentDataError(Errc.deserialize_failed, f"{where}: invalid UTF-8") from None + if k == _FT.vector: + n = r.count(where) + return [_dec(spec.elems[0], r, f"{where}[{i}]") for i in range(n)] + if k == _FT.array: + return [_dec(spec.elems[0], r, f"{where}[{i}]") for i in range(spec.n)] + if k in (_FT.map, _FT.unordered_map): + n = r.count(where) + out = {} + for i in range(n): + kk = _dec(spec.elems[0], r, f"{where}.key[{i}]") + out[kk] = _dec(spec.elems[1], r, f"{where}[{kk!r}]") + return out + if k in (_FT.set, _FT.unordered_set): + n = r.count(where) + return {_dec(spec.elems[0], r, f"{where}{{{i}}}") for i in range(n)} + if k == _FT.optional: + has = r.take(1, where)[0] + return _dec(spec.elems[0], r, where + "?") if has else None + if k == _FT.tuple: + return tuple(_dec(e, r, f"{where}[{i}]") for i, e in enumerate(spec.elems)) + if k == _FT.struct_: + kwargs = {name: _dec(f, r, f"{where}.{name}") for name, f in spec.fields} + return spec.cls(**kwargs) + raise PersistentDataError(Errc.deserialize_failed, f"{where}: unsupported kind {k}") # pragma: no cover + + +def encode_payload(value: Any) -> bytes: + """alpaca-encode a dataclass instance (no header).""" + out = bytearray() + _enc(_struct_spec(type(value)), value, out, type(value).__qualname__) + return bytes(out) + + +def decode_payload(cls: type, payload: bytes) -> Any: + """Decode an alpaca payload (no header) as ``cls``. Strict: the whole payload must be consumed.""" + r = _Reader(bytes(payload)) + value = _dec(_struct_spec(cls), r, cls.__qualname__) + if r.pos != len(r.data): + raise PersistentDataError(Errc.deserialize_failed, + f"{cls.__qualname__}: {len(r.data) - r.pos} trailing byte(s)") + return value + + +# --------------------------------------------------------------------------- +# File format: header + payload +# --------------------------------------------------------------------------- + + +def _version_of(cls: type) -> int: + v = getattr(cls, "VERSION", None) + if not isinstance(v, int) or isinstance(v, bool): + raise TypeError(f"{cls.__qualname__} needs an integer VERSION class variable") + return v + + +def encode(value: Any, version: Optional[int] = None) -> bytes: + """Encode a dataclass instance into the file format (header + payload). + + ``version`` defaults to the class's ``VERSION``. + """ + cls = type(value) + version = _version_of(cls) if version is None else int(version) + payload = encode_payload(value) + head = struct.pack("= self.versions[i - 1]: + raise TypeError("VERSION values must strictly decrease along the list (newest first)") + for i in range(len(versions) - 1): + if not callable(getattr(versions[i], "migrate", None)): + raise TypeError(f"{versions[i].__qualname__} needs a migrate() from " + f"{versions[i + 1].__qualname__}") + for c in versions: + _struct_spec(c) # validate annotations up front + self.fingerprints = [fingerprint(c) for c in versions] + + @property + def current(self) -> type: + return self.classes[0] + + @property + def current_version(self) -> int: + return self.versions[0] + + def supports(self, version: int) -> bool: + return version in self.versions + + def decode(self, version: int, layout: int, payload: bytes) -> Any: + """Decode a payload of schema ``version`` and migrate it to the current class.""" + if version not in self.versions: + raise PersistentDataError(Errc.unknown_version, f"version {version}") + idx = self.versions.index(version) + if layout != self.fingerprints[idx]: + raise PersistentDataError( + Errc.layout_mismatch, + f"file 0x{layout:08x} != {self.classes[idx].__qualname__} 0x{self.fingerprints[idx]:08x}") + value = decode_payload(self.classes[idx], payload) + while idx > 0: + idx -= 1 + value = self.classes[idx].migrate(value) + return value + + +def decode(versions: Sequence[type], data: bytes): + """Decode bytes produced by :func:`encode` (any listed version) -> ``(value, LoadInfo)``.""" + vl = versions if isinstance(versions, VersionList) else VersionList(versions) + version, layout, payload = parse(data) + value = vl.decode(version, layout, payload) + return value, LoadInfo(file_version=version, migrated=version != vl.current_version) + + +# --------------------------------------------------------------------------- +# PersistentData: the file-backed object +# --------------------------------------------------------------------------- + + +class PersistentData: + """File-backed storage of one dataclass with schema versioning and forward migration. + + ``versions`` is the newest-first version list (the first class is the current + one). All methods are thread-safe; ``get()`` returns the in-memory object + itself (dataclasses are mutable), so use :meth:`update` for a locked + read-modify-write. + """ + + def __init__(self, versions: Sequence[type], path: os.PathLike | str, *, + max_file_size: int = 64 * 1024, save_after_migration: bool = True, + auto_load: bool = False): + self.versions = versions if isinstance(versions, VersionList) else VersionList(versions) + self.path = os.fspath(path) + self.max_file_size = int(max_file_size) + self.save_after_migration = bool(save_after_migration) + self._lock = threading.RLock() + self._data: Any = None + self._loaded = False + self._info = LoadInfo() + if auto_load: + self.load() + + # --- state ------------------------------------------------------------- + + @property + def current(self) -> type: + """The current (newest) version class.""" + return self.versions.current + + @property + def current_version(self) -> int: + return self.versions.current_version + + @property + def is_loaded(self) -> bool: + """False until a load or save succeeds.""" + with self._lock: + return self._loaded + + @property + def loaded_version(self) -> int: + """Schema version found in the file by the last successful load (0 if none).""" + with self._lock: + return self._info.file_version + + @property + def was_migrated(self) -> bool: + with self._lock: + return self._info.migrated + + def exists(self) -> bool: + return os.path.isfile(self.path) + + def get(self) -> Any: + """The data in memory (``None`` before any load / set / save).""" + with self._lock: + return self._data + + def set(self, data: Any) -> None: + """Replace the data in memory (does not write the file).""" + self._check_type(data) + with self._lock: + self._data = data + + # --- file operations --------------------------------------------------- + + def load(self) -> Any: + """Load the file, migrating an older version if needed; returns the data. + + On failure the data in memory is unchanged and :class:`PersistentDataError` + says why (``file_not_found``, ``io_error``, ``file_too_large``, + ``bad_header``, ``bad_checksum``, ``unknown_version``, ``layout_mismatch``, + ``deserialize_failed``). + """ + with self._lock: + raw = self._read_file() + version, layout, payload = parse(raw) + value = self.versions.decode(version, layout, payload) + self._data = value + self._loaded = True + self._info = LoadInfo(file_version=version, migrated=version != self.current_version) + if self._info.migrated and self.save_after_migration: + try: + self._write_file(encode(value)) + except PersistentDataError: + pass # the data is loaded fine; only the rewrite failed + return value + + def load_or_create(self, defaults: Any) -> Any: + """Load the file, or save ``defaults`` and return it if there is no file. + + Any error other than ``file_not_found`` (a corrupt file, a newer + firmware's file) is raised, never papered over. + """ + self._check_type(defaults) + with self._lock: + try: + return self.load() + except PersistentDataError as exc: + if exc.errc != Errc.file_not_found: + raise + self._data = defaults + self._write_file(encode(defaults)) + self._loaded = True + return defaults + + def save(self, data: Any = None) -> None: + """Save ``data`` (or the data in memory) to the file, atomically.""" + with self._lock: + if data is not None: + self._check_type(data) + self._data = data + if self._data is None: + raise PersistentDataError(Errc.serialize_failed, "nothing to save: no data set") + self._write_file(encode(self._data)) + self._loaded = True + + def update(self, fn: Callable[[Any], Any]) -> bool: + """Modify the data in memory under the lock and save it. + + ``fn`` receives the data object; it may mutate it in place and return + ``None``, return a replacement object, or return ``False`` to skip the + save (the in-memory change is kept). Returns True if the file was written. + """ + with self._lock: + if self._data is None: + raise PersistentDataError(Errc.serialize_failed, "nothing to update: no data loaded") + result = fn(self._data) + if result is False: + return False + if result is not None and result is not True: + self._check_type(result) + self._data = result + self._write_file(encode(self._data)) + self._loaded = True + return True + + def erase(self) -> None: + """Remove the file (and a leftover temporary); succeeds when there is none.""" + with self._lock: + for p in (self.path + ".tmp", self.path): + try: + os.remove(p) + except FileNotFoundError: + pass + except OSError as exc: + raise PersistentDataError(Errc.io_error, f"{p}: {exc}") from None + self._loaded = False + self._info = LoadInfo() + + # --- bytes without the filesystem ------------------------------------- + + def encode(self, data: Any) -> bytes: + """Encode ``data`` as the current version (header + payload).""" + self._check_type(data) + return encode(data) + + def decode(self, data: bytes): + """Decode bytes of any listed version -> ``(value, LoadInfo)``, migrations applied.""" + return decode(self.versions, data) + + # --- internals --------------------------------------------------------- + + def _check_type(self, data: Any) -> None: + if not isinstance(data, self.current): + raise TypeError(f"expected {self.current.__qualname__}, got {type(data).__qualname__}") + + def _read_file(self) -> bytes: + try: + size = os.path.getsize(self.path) + except FileNotFoundError: + raise PersistentDataError(Errc.file_not_found, self.path) from None + except OSError as exc: + raise PersistentDataError(Errc.io_error, f"{self.path}: {exc}") from None + if size > self.max_file_size: + raise PersistentDataError(Errc.file_too_large, f"{size} > {self.max_file_size} bytes") + try: + with open(self.path, "rb") as f: + return f.read() + except OSError as exc: + raise PersistentDataError(Errc.io_error, f"{self.path}: {exc}") from None + + def _write_file(self, data: bytes) -> None: + tmp = self.path + ".tmp" + try: + with open(tmp, "wb") as f: + f.write(data) + f.flush() + os.fsync(f.fileno()) + os.replace(tmp, self.path) + except OSError as exc: + try: + os.remove(tmp) + except OSError: + pass + raise PersistentDataError(Errc.io_error, f"{self.path}: {exc}") from None diff --git a/pyproject.toml b/pyproject.toml index df43820186..7832f1b222 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -127,9 +127,10 @@ skip = ["*-musllinux_*", "*_i686", "*-win32"] # Smoke-test the wheel: import + assert the RTPS RMI/AMI binding surface is # exposed (a functional in-process round-trip needs multicast, so it lives in the # dockerised interop harness / python/rtps_rpc_demo.py, not here), and run the -# self-contained stream_frame / Dispatcher binding test, and the blocking-destructor test -# (deleting / exiting with a running Task / Timer / TrajectoryPlanner must not deadlock). -test-command = "python \"{project}/python/rtps_bindings_smoke.py\" && python \"{project}/python/dispatcher_test.py\" && python \"{project}/python/blocking_destructors_test.py\"" +# self-contained stream_frame / Dispatcher binding test, the blocking-destructor test +# (deleting / exiting with a running Task / Timer / TrajectoryPlanner must not deadlock), +# and the pure-python persistent_data codec test (byte-checked against C++ vectors). +test-command = "python \"{project}/python/rtps_bindings_smoke.py\" && python \"{project}/python/dispatcher_test.py\" && python \"{project}/python/blocking_destructors_test.py\" && python \"{project}/python/persistent_data_test.py\"" [tool.cibuildwheel.linux] # The repo is mounted at /project inside the manylinux container; git must diff --git a/python/README.md b/python/README.md index 4b659cd030..0d96bee910 100644 --- a/python/README.md +++ b/python/README.md @@ -73,6 +73,13 @@ This section gives a brief overview of what the scripts in this folder do. discovery). Note: RTPS discovery is multicast, so these require a multicast-capable network. - `cobs_demo.py`: Demonstration of ESPP COBS functionality with native Python data types. +- `persistent_data_test.py`: Self-checking test for `espp.persistent_data`, the + pure-Python counterpart of the `persistent_data` component (versioned, + migratable settings files shared with the firmware). Checks the dataclass + layout fingerprint and payload encoding byte-for-byte against + `persistent_data_vectors.txt` (generated by the C++ codec), the migration + chain, every error path and the file operations. Runs from the source tree + without a built wheel. Exits 0 on full pass, 1 on any failure. Shows ESPP encoding/decoding, cross-library compatibility with the cobs-python library, and practical usage examples. Includes design differences explanation and validation against the reference implementation. diff --git a/python/persistent_data_test.py b/python/persistent_data_test.py new file mode 100644 index 0000000000..775e41de26 --- /dev/null +++ b/python/persistent_data_test.py @@ -0,0 +1,385 @@ +"""espp.persistent_data test: the Python mirror of components/persistent_data. + +Checks the dataclass-driven layout fingerprint and alpaca payload codec byte for +byte against vectors produced by the C++ codec (python/persistent_data_vectors.txt, +generated from include/detail/persistent_data_codec.hpp), the newest-first +version list with its migration chain, every error path, and the file-backed +PersistentData object (atomic save, load_or_create, update, erase). + +Runs against an installed espp wheel (``import espp``) or, without one, straight +from the source tree. Exit code 0 on full pass, 1 on any failure. +""" + +from __future__ import annotations + +import enum +import os +import sys +import tempfile +from dataclasses import dataclass, field +from typing import ClassVar, Optional + +try: + from espp import persistent_data as pd +except ImportError: # no wheel installed: load the pure module from the source tree + import importlib.util + + _src = os.path.join(os.path.dirname(os.path.abspath(__file__)), "..", "lib", "python_bindings", + "espp", "persistent_data.py") + _spec = importlib.util.spec_from_file_location("persistent_data", _src) + pd = importlib.util.module_from_spec(_spec) + sys.modules["persistent_data"] = pd # dataclasses defined in the module look it up here + _spec.loader.exec_module(pd) + +u8, u16, u32, u64, i8, i16, i32, i64, f32, f64 = (pd.u8, pd.u16, pd.u32, pd.u64, pd.i8, pd.i16, + pd.i32, pd.i64, pd.f32, pd.f64) +Errc = pd.Errc +PersistentDataError = pd.PersistentDataError + +failures = 0 +checks = 0 + + +def check(desc: str, condition: bool) -> None: + global failures, checks + checks += 1 + if condition: + print(f" PASS: {desc}") + else: + print(f" FAIL: {desc}") + failures += 1 + + +def raises(errc: Errc, fn) -> bool: + try: + fn() + except PersistentDataError as exc: + return exc.errc == errc + return False + + +# --------------------------------------------------------------------------- +# The C++ types from persistent_data_vectors.txt, as dataclasses +# --------------------------------------------------------------------------- + + +class Mode(enum.IntEnum): + off = 0 + on = 1 + + +@dataclass +class Limits: + min: f32 = 0.0 + max: f32 = 1.0 + + +@dataclass +class V1: + VERSION: ClassVar[int] = 1 + counter: u32 = 0 + name: str = "" + + +@dataclass +class V2: + VERSION: ClassVar[int] = 2 + counter: u32 = 0 + name: str = "" + gains: list[f32] = field(default_factory=list) + + @classmethod + def migrate(cls, v1: V1) -> "V2": + return cls(counter=v1.counter, name=v1.name, gains=[1.0]) + + +@dataclass +class V3: + VERSION: ClassVar[int] = 3 + name: str = "" + gains: list[f32] = field(default_factory=list) + limits: Limits = field(default_factory=Limits) + enabled: bool = True + mode: pd.Annotated[Mode, "B"] = Mode.off + tag: pd.array(u8, 3) = field(default_factory=lambda: [0, 0, 0]) + offset: i16 = 0 + scale: f64 = 1.0 + big: u64 = 0 + small: i8 = 0 + + @classmethod + def migrate(cls, v2: V2) -> "V3": + return cls(name=v2.name, gains=v2.gains, limits=Limits(-1.0, 1.0), enabled=v2.counter > 0) + + +# the same layout as V3 but claiming version 3 under another name: the fingerprint must agree +@dataclass +class SameAsV3: + VERSION: ClassVar[int] = 3 + n: str = "" + g: list[f32] = field(default_factory=list) + lim: Limits = field(default_factory=Limits) + e: bool = True + m: pd.Annotated[Mode, "B"] = Mode.off + t: pd.array(u8, 3) = field(default_factory=lambda: [0, 0, 0]) + o: i16 = 0 + s: f64 = 1.0 + b: u64 = 0 + sm: i8 = 0 + + +# a different layout that reuses version 3 +@dataclass +class OtherV3: + VERSION: ClassVar[int] = 3 + a: u64 = 0 + b: u64 = 0 + + +# a version this "firmware" does not know +@dataclass +class V4: + VERSION: ClassVar[int] = 4 + name: str = "future" + flags: u8 = 0 + + +VALUES = { + "v1": V1(7, "one"), + "v2": V2(0, "two", [2.0, 0.5]), + "v3": V3("abc", [0.25], Limits(-3.0, 3.0), False, Mode.on, [1, 2, 3], -5, 2.5, 10000000000, -1), + "v3_empty": V3(), +} +CLASSES = {"v1": V1, "v2": V2, "v3": V3, "v3_empty": V3} + + +def load_vectors(): + here = os.path.dirname(os.path.abspath(__file__)) + out = {} + with open(os.path.join(here, "persistent_data_vectors.txt")) as f: + for line in f: + line = line.strip() + if not line or line.startswith("#"): + continue + name, ver, fp, desc, file = line.split() + out[name] = (int(ver), int(fp, 16), bytes.fromhex(desc), bytes.fromhex(file)) + return out + + +# --------------------------------------------------------------------------- +print("--- layout fingerprint vs the C++ codec ---") +vectors = load_vectors() +check("vectors loaded", set(vectors) == set(VALUES)) +for name, (ver, fp, desc, file) in vectors.items(): + cls = CLASSES[name] + check(f"{name}: describe() bytes match C++", pd.describe(cls) == desc) + check(f"{name}: fingerprint matches C++", pd.fingerprint(cls) == fp) +check("same layout under other field names -> same fingerprint", pd.fingerprint(SameAsV3) == pd.fingerprint(V3)) +check("different layout -> different fingerprint", pd.fingerprint(OtherV3) != pd.fingerprint(V3)) +check("V1 / V2 differ", pd.fingerprint(V1) != pd.fingerprint(V2)) + +# the grammar, spelled out for one struct: struct_(25) nfields=2 u32(4) string(13) +check("describe grammar for V1", pd.describe(V1) == bytes([25, 2, 0, 4, 13])) + +# --------------------------------------------------------------------------- +print("--- payload / file encoding vs the C++ codec ---") +for name, (ver, fp, desc, file) in vectors.items(): + value = VALUES[name] + encoded = pd.encode(value) + check(f"{name}: encode() is byte-identical to the C++ file", encoded == file) + version, layout, payload = pd.parse(file) + check(f"{name}: header version / layout", version == ver and layout == fp) + decoded = pd.decode_payload(CLASSES[name], payload) + check(f"{name}: decode_payload() of the C++ payload equals the value", decoded == value) +check("decoded enum is the Enum member", type(pd.decode_payload(V3, pd.parse(vectors["v3"][3])[2]).mode) is Mode) + +# --------------------------------------------------------------------------- +print("--- version list + migration chain ---") +full = pd.VersionList([V3, V2, V1]) +check("current / current_version", full.current is V3 and full.current_version == 3) +check("supports", full.supports(1) and full.supports(3) and not full.supports(4)) + +value, info = pd.decode(full, vectors["v1"][3]) +check("v1 file migrates v1 -> v2 -> v3", isinstance(value, V3) and value.name == "one" + and value.gains == [1.0] and value.limits == Limits(-1.0, 1.0) and value.enabled is True) +check("LoadInfo reports version 1, migrated", info.file_version == 1 and info.migrated) +value, info = pd.decode(full, vectors["v2"][3]) +check("v2 file migrates v2 -> v3", value.gains == [2.0, 0.5] and value.enabled is False and info.migrated) +value, info = pd.decode(full, vectors["v3"][3]) +check("v3 file is not migrated", value == VALUES["v3"] and not info.migrated) + +check("dropped v1 support -> unknown_version", + raises(Errc.unknown_version, lambda: pd.decode([V3, V2], vectors["v1"][3]))) +check("newer firmware's file -> unknown_version", + raises(Errc.unknown_version, lambda: pd.decode(full, pd.encode(V4())))) +check("same version, different layout -> layout_mismatch", + raises(Errc.layout_mismatch, lambda: pd.decode(full, pd.encode(OtherV3(1, 2))))) +check("same layout, other class name -> decodes", + pd.decode(full, pd.encode(SameAsV3(n="x")))[0].name == "x") + + +def bad_list(classes): + try: + pd.VersionList(classes) + except TypeError: + return True + return False + + +check("oldest-first list is rejected", bad_list([V1, V2, V3])) +check("missing migrate is rejected", bad_list([OtherV3, V2])) + + +@dataclass +class NoVersion: + x: u8 = 0 + + +check("missing VERSION is rejected", bad_list([NoVersion])) + + +@dataclass +class BareInt: + VERSION: ClassVar[int] = 1 + x: int = 0 + + +check("bare int field is rejected", bad_list([BareInt])) + +# --------------------------------------------------------------------------- +print("--- corrupt / truncated input ---") +good = vectors["v3"][3] +check("empty -> bad_header", raises(Errc.bad_header, lambda: pd.parse(b""))) +check("short header -> bad_header", raises(Errc.bad_header, lambda: pd.parse(good[:10]))) +check("wrong magic -> bad_header", raises(Errc.bad_header, lambda: pd.parse(b"X" + good[1:]))) +check("truncated payload -> bad_header", raises(Errc.bad_header, lambda: pd.parse(good[:-1]))) +check("trailing junk -> bad_header", raises(Errc.bad_header, lambda: pd.parse(good + b"\0"))) +flipped = bytearray(good) +flipped[pd.HEADER_SIZE + 2] ^= 1 +check("flipped payload bit -> bad_checksum", raises(Errc.bad_checksum, lambda: pd.parse(bytes(flipped)))) +flipped = bytearray(good) +flipped[4] ^= 1 +check("flipped version byte -> bad_checksum", raises(Errc.bad_checksum, lambda: pd.parse(bytes(flipped)))) +# a payload that passes the header but is short for the type (strict) +check("short payload -> deserialize_failed", + raises(Errc.deserialize_failed, lambda: pd.decode_payload(V1, b"\x07\x00\x00\x00"))) +check("payload with trailing bytes -> deserialize_failed", + raises(Errc.deserialize_failed, lambda: pd.decode_payload(V1, pd.encode_payload(V1(7, "one")) + b"\0"))) +check("absurd element count -> deserialize_failed", + raises(Errc.deserialize_failed, lambda: pd.decode_payload(V1, b"\x07\x00\x00\x00\xff\xff\xff\xff"))) +check("array of the wrong length -> serialize_failed", + raises(Errc.serialize_failed, lambda: pd.encode(V3(tag=[1, 2])))) +check("out-of-range integer -> serialize_failed", + raises(Errc.serialize_failed, lambda: pd.encode(V3(small=200)))) + +# --------------------------------------------------------------------------- +print("--- the other field types round-trip ---") + + +@dataclass +class Kitchen: + VERSION: ClassVar[int] = 1 + c: pd.char = "x" + m: dict[str, i32] = field(default_factory=dict) + um: pd.unordered(dict[u8, str]) = field(default_factory=dict) + s: set[u16] = field(default_factory=set) + o: Optional[f64] = None + o2: Optional[str] = None + t: tuple[i8, str, f32] = (0, "", 0.0) + nested: list[Limits] = field(default_factory=list) + + +k = Kitchen(c="z", m={"b": 2, "a": 1}, um={1: "one"}, s={3, 1, 2}, o=2.5, o2=None, t=(-1, "t", 0.5), + nested=[Limits(1, 2), Limits(3, 4)]) +rt = pd.decode_payload(Kitchen, pd.encode_payload(k)) +check("kitchen-sink round trip", rt == k) +check("std::map keys are written sorted", + pd.encode_payload(Kitchen(m={"b": 2, "a": 1})) == pd.encode_payload(Kitchen(m={"a": 1, "b": 2}))) +check("kitchen describe grammar", pd.describe(Kitchen) == bytes( + [25, 8, 0, # struct, 8 fields + 1, # char + 16, 13, 8, # map + 17, 2, 13, # unordered_map + 18, 3, # set + 20, 11, # optional + 20, 13, # optional + 22, 3, 0, 6, 13, 10, # tuple + 15, 25, 2, 0, 10, 10])) # vector + +# --------------------------------------------------------------------------- +print("--- PersistentData file operations ---") +with tempfile.TemporaryDirectory() as tmp: + path = os.path.join(tmp, "settings.bin") + store = pd.PersistentData([V3, V2, V1], path) + check("no file: exists() false, not loaded", not store.exists() and not store.is_loaded) + check("load of a missing file -> file_not_found", raises(Errc.file_not_found, store.load)) + check("erase of a missing file succeeds", (store.erase(), True)[1]) + + created = store.load_or_create(V3(name="fresh")) + check("load_or_create creates the file", store.exists() and created.name == "fresh" and store.is_loaded) + check("no temporary left behind", not os.path.exists(path + ".tmp")) + again = pd.PersistentData([V3, V2, V1], path, auto_load=True) + check("auto_load reads it back", again.get() == V3(name="fresh") and again.loaded_version == 3 + and not again.was_migrated) + + check("update() mutating in place saves", + store.update(lambda s: s.gains.append(0.75)) and pd.PersistentData([V3], path, auto_load=True).get().gains == [0.75]) + check("update() returning a replacement saves it", + store.update(lambda s: V3(name="replaced")) and pd.PersistentData([V3], path, auto_load=True).get().name == "replaced") + check("update() returning False does not save", + not store.update(lambda s: False)) + check("save(data) replaces memory and file", + (store.save(V3(name="saved")), pd.PersistentData([V3], path, auto_load=True).get().name)[1] == "saved") + + # an old-version file on disk is migrated and rewritten + with open(path, "wb") as f: + f.write(vectors["v1"][3]) + loaded = store.load() + check("v1 file on disk loads migrated", loaded.name == "one" and store.loaded_version == 1 and store.was_migrated) + fresh = pd.PersistentData([V3, V2, V1], path, auto_load=True) + check("file was rewritten as the current version", fresh.loaded_version == 3 and not fresh.was_migrated) + + store2 = pd.PersistentData([V3, V2, V1], path, save_after_migration=False) + with open(path, "wb") as f: + f.write(vectors["v2"][3]) + store2.load() + check("save_after_migration=False keeps the old file", + pd.PersistentData([V3, V2, V1], path, auto_load=True).loaded_version == 2) + + # a newer-firmware file is never replaced by load_or_create + with open(path, "wb") as f: + f.write(pd.encode(V4())) + check("load_or_create does not replace a newer file", + raises(Errc.unknown_version, lambda: store.load_or_create(V3()))) + check("memory untouched after a failed load", store.get().name == "one") + + # corruption on disk + with open(path, "r+b") as f: + f.seek(pd.HEADER_SIZE) + f.write(b"\xff") + check("corrupt file -> bad_checksum", raises(Errc.bad_checksum, store.load)) + with open(path, "wb") as f: + f.write(vectors["v3"][3][:pd.HEADER_SIZE + 3]) + check("truncated file -> bad_header", raises(Errc.bad_header, store.load)) + + # size cap + small = pd.PersistentData([V3], path, max_file_size=8) + check("file over max_file_size -> file_too_large", raises(Errc.file_too_large, small.load)) + + # type checks + try: + store.save(V1()) + check("save() of the wrong type is rejected", False) + except TypeError: + check("save() of the wrong type is rejected", True) + + # erase clears + store.erase() + check("erase removes the file", not store.exists() and not store.is_loaded) + + # save into a directory that does not exist -> io_error + bad = pd.PersistentData([V3], os.path.join(tmp, "missing", "x.bin")) + check("save into a missing directory -> io_error", raises(Errc.io_error, lambda: bad.save(V3()))) + +print(f"\n{checks - failures}/{checks} checks passed") +sys.exit(1 if failures else 0) diff --git a/python/persistent_data_vectors.txt b/python/persistent_data_vectors.txt new file mode 100644 index 0000000000..b1cc404a3b --- /dev/null +++ b/python/persistent_data_vectors.txt @@ -0,0 +1,12 @@ +# Reference vectors generated by the C++ persistent_data codec (components/persistent_data) +# for python/persistent_data_test.py. Columns: +# Types (C++): +# enum class Mode : uint8_t { off, on }; struct Limits { float min{0}; float max{1}; }; +# V1 { uint32_t counter; std::string name; } +# V2 { uint32_t counter; std::string name; std::vector gains; } +# V3 { std::string name; std::vector gains; Limits limits; bool enabled{true}; Mode mode; std::array tag; int16_t offset; double scale{1.0}; uint64_t big; int8_t small; } +# Values: v1 = {7, "one"}; v2 = {0, "two", {2.0, 0.5}}; v3 = {"abc", {0.25}, {-3, 3}, false, on, {1,2,3}, -5, 2.5, 10000000000, -1}; v3_empty = V3{} +v1 1 1b0673dc 190200040d 4553504401000000dc73061b0b000000bf68e64307000000030000006f6e65 +v2 2 09498518 190300040d0f0a 45535044020000001885490917000000c3d5b909000000000300000074776f02000000000000400000003f +v3 3 7926c78c 190a000d0f0a1902000a0a000c020e0300000002070b0506 45535044030000008cc726792f0000008616eb9203000000616263010000000000803e000040c0000040400001010203fbff000000000000044000e40b5402000000ff +v3_empty 3 7926c78c 190a000d0f0a1902000a0a000c020e0300000002070b0506 45535044030000008cc72679280000009c97044f0000000000000000000000000000803f01000000000000000000000000f03f000000000000000000 From a7ae878706b550e33901956f9a3222ddaef313ff Mon Sep 17 00:00:00 2001 From: William Emfinger Date: Fri, 30 Oct 2026 22:07:08 -0500 Subject: [PATCH 2/2] refactor(persistent_data): python self-review + code-quality findings - Code-quality bot: the two bare `except: pass` (erase of a missing file, temp-file cleanup after a failed write) are explicit now, and the unused ClassVar import is gone. - load(): a failed post-migration rewrite is logged (logging, module logger) instead of silently swallowed, matching the C++ warning. - A VERSION declared as a dataclass field instead of a ClassVar is rejected with a message saying so (it used to surface as a confusing "bare int" error from the payload spec). - A dataclass constructor that raises during decode (a __post_init__ validation) is reported as deserialize_failed instead of escaping as a raw exception. - parse() is exported; the test imports Annotated from typing rather than through the module; the module docstring's type-mapping table is a valid RST table (column widths). - Two new test checks for the VERSION-as-field and constructor-failure cases (78 total). pyflakes clean. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01DFjjnq3XCTRAXENSxsCxJU --- lib/python_bindings/espp/persistent_data.py | 64 +++++++++++++-------- python/persistent_data_test.py | 29 +++++++++- 2 files changed, 66 insertions(+), 27 deletions(-) diff --git a/lib/python_bindings/espp/persistent_data.py b/lib/python_bindings/espp/persistent_data.py index 09d1b2bbac..680e3f3c7e 100644 --- a/lib/python_bindings/espp/persistent_data.py +++ b/lib/python_bindings/espp/persistent_data.py @@ -43,21 +43,21 @@ def migrate(cls, v1: SettingsV1) -> "Settings": Type mapping (Python annotation -> alpaca field type, as the firmware sees it): -====================================== ============================================ -``bool`` ``bool`` -``u8 u16 u32 u64 i8 i16 i32 i64`` the fixed-width integer -``f32`` / ``f64`` ``float`` / ``double`` -``char`` ``char`` (one-character ``str``) -``str`` ``std::string`` (UTF-8) -``list[T]`` ``std::vector`` -``array(T, N)`` ``std::array`` +========================================== ============================================ +``bool`` ``bool`` +``u8 u16 u32 u64 i8 i16 i32 i64`` the fixed-width integer +``f32`` / ``f64`` ``float`` / ``double`` +``char`` ``char`` (one-character ``str``) +``str`` ``std::string`` (UTF-8) +``list[T]`` ``std::vector`` +``array(T, N)`` ``std::array`` ``dict[K, V]`` / ``unordered(dict[K, V])`` ``std::map`` / ``std::unordered_map`` -``set[T]`` / ``unordered(set[T])`` ``std::set`` / ``std::unordered_set`` -``Optional[T]`` ``std::optional`` -``tuple[A, B, ...]`` ``std::tuple`` -``enum.Enum`` (``Annotated[E, "B"]`` ...) ``enum class E : uint8_t`` (default ``int``) -nested ``@dataclass`` nested aggregate -====================================== ============================================ +``set[T]`` / ``unordered(set[T])`` ``std::set`` / ``std::unordered_set`` +``Optional[T]`` ``std::optional`` +``tuple[A, B, ...]`` ``std::tuple`` +``enum.Enum`` (``Annotated[E, "B"]`` ...) ``enum class E : uint8_t`` (default ``int``) +nested ``@dataclass`` nested aggregate +========================================== ============================================ The layout fingerprint in the header is computed from these annotations by the grammar documented in ``components/persistent_data/README.md``, so a file written @@ -70,13 +70,14 @@ def migrate(cls, v1: SettingsV1) -> "Settings": import dataclasses import enum +import logging import os import struct import threading import types import typing import zlib -from typing import Annotated, Any, Callable, ClassVar, Optional, Sequence +from typing import Annotated, Any, Callable, Optional, Sequence __all__ = [ "Errc", @@ -104,10 +105,13 @@ def migrate(cls, v1: SettingsV1) -> "Settings": "encode_payload", "decode_payload", "encode", + "parse", "decode", "VersionList", ] +_log = logging.getLogger(__name__) + # --------------------------------------------------------------------------- # File format constants (must match include/detail/persistent_data_codec.hpp) # --------------------------------------------------------------------------- @@ -530,7 +534,11 @@ def _dec(spec: _Spec, r: _Reader, where: str) -> Any: return tuple(_dec(e, r, f"{where}[{i}]") for i, e in enumerate(spec.elems)) if k == _FT.struct_: kwargs = {name: _dec(f, r, f"{where}.{name}") for name, f in spec.fields} - return spec.cls(**kwargs) + try: + return spec.cls(**kwargs) + except Exception as exc: # a __post_init__ validation, typically + raise PersistentDataError(Errc.deserialize_failed, + f"{where}: {type(exc).__name__}: {exc}") from exc raise PersistentDataError(Errc.deserialize_failed, f"{where}: unsupported kind {k}") # pragma: no cover @@ -557,6 +565,9 @@ def decode_payload(cls: type, payload: bytes) -> Any: def _version_of(cls: type) -> int: + if dataclasses.is_dataclass(cls) and any(f.name == "VERSION" for f in dataclasses.fields(cls)): + raise TypeError(f"{cls.__qualname__}.VERSION must be a ClassVar[int], not a dataclass field " + "(it is part of the header, not the payload)") v = getattr(cls, "VERSION", None) if not isinstance(v, int) or isinstance(v, bool): raise TypeError(f"{cls.__qualname__} needs an integer VERSION class variable") @@ -667,7 +678,9 @@ class PersistentData: ``versions`` is the newest-first version list (the first class is the current one). All methods are thread-safe; ``get()`` returns the in-memory object itself (dataclasses are mutable), so use :meth:`update` for a locked - read-modify-write. + read-modify-write. ``auto_load=True`` calls :meth:`load` in the constructor, + so the constructor then raises :class:`PersistentDataError` when the file is + missing or unusable. """ def __init__(self, versions: Sequence[type], path: os.PathLike | str, *, @@ -746,8 +759,10 @@ def load(self) -> Any: if self._info.migrated and self.save_after_migration: try: self._write_file(encode(value)) - except PersistentDataError: - pass # the data is loaded fine; only the rewrite failed + except PersistentDataError as exc: + # The data is loaded fine; only the rewrite to the current + # version failed, so report success and leave the old file. + _log.warning("%s: could not rewrite the migrated data: %s", self.path, exc) return value def load_or_create(self, defaults: Any) -> Any: @@ -783,8 +798,9 @@ def update(self, fn: Callable[[Any], Any]) -> bool: """Modify the data in memory under the lock and save it. ``fn`` receives the data object; it may mutate it in place and return - ``None``, return a replacement object, or return ``False`` to skip the - save (the in-memory change is kept). Returns True if the file was written. + ``None`` (or ``True``), return a replacement object, or return ``False`` + to skip the save (the in-memory change is kept). Returns True if the file + was written. """ with self._lock: if self._data is None: @@ -806,7 +822,7 @@ def erase(self) -> None: try: os.remove(p) except FileNotFoundError: - pass + continue # nothing to erase: that is success, not an error except OSError as exc: raise PersistentDataError(Errc.io_error, f"{p}: {exc}") from None self._loaded = False @@ -854,7 +870,7 @@ def _write_file(self, data: bytes) -> None: os.replace(tmp, self.path) except OSError as exc: try: - os.remove(tmp) + os.remove(tmp) # best effort: the write itself is the error to report except OSError: - pass + _log.debug("%s: could not remove the temporary file after a failed write", tmp) raise PersistentDataError(Errc.io_error, f"{self.path}: {exc}") from None diff --git a/python/persistent_data_test.py b/python/persistent_data_test.py index 775e41de26..3eab29193c 100644 --- a/python/persistent_data_test.py +++ b/python/persistent_data_test.py @@ -17,7 +17,7 @@ import sys import tempfile from dataclasses import dataclass, field -from typing import ClassVar, Optional +from typing import Annotated, ClassVar, Optional try: from espp import persistent_data as pd @@ -100,7 +100,7 @@ class V3: gains: list[f32] = field(default_factory=list) limits: Limits = field(default_factory=Limits) enabled: bool = True - mode: pd.Annotated[Mode, "B"] = Mode.off + mode: Annotated[Mode, "B"] = Mode.off tag: pd.array(u8, 3) = field(default_factory=lambda: [0, 0, 0]) offset: i16 = 0 scale: f64 = 1.0 @@ -120,7 +120,7 @@ class SameAsV3: g: list[f32] = field(default_factory=list) lim: Limits = field(default_factory=Limits) e: bool = True - m: pd.Annotated[Mode, "B"] = Mode.off + m: Annotated[Mode, "B"] = Mode.off t: pd.array(u8, 3) = field(default_factory=lambda: [0, 0, 0]) o: i16 = 0 s: f64 = 1.0 @@ -246,6 +246,29 @@ class BareInt: check("bare int field is rejected", bad_list([BareInt])) + +@dataclass +class VersionAsField: + VERSION: int = 1 # not a ClassVar: a mistake worth a clear message + x: u8 = 0 + + +check("VERSION as a dataclass field is rejected", bad_list([VersionAsField])) + + +@dataclass +class Validated: + VERSION: ClassVar[int] = 1 + x: u8 = 0 + + def __post_init__(self): + if self.x > 9: + raise ValueError("x must be <= 9") + + +check("constructor validation failure -> deserialize_failed", + raises(Errc.deserialize_failed, lambda: pd.decode_payload(Validated, b"\x10"))) + # --------------------------------------------------------------------------- print("--- corrupt / truncated input ---") good = vectors["v3"][3]