mpremote: Add smart encoding selection for fs_writefile.#11
mpremote: Add smart encoding selection for fs_writefile.#11andrewleech wants to merge 1 commit intomasterfrom
Conversation
|
Code size report: |
|
/review |
![mpy-reviewer[bot]](https://avatars.githubusercontent.com/in/2934917?s=60&v=4){kind=small}
There was a problem hiding this comment.
There's a critical bug in _detect_encoding_capabilities: the single dict eval includes __import__('deflate') first, so on any device without the deflate module, ImportError is raised mid-expression and the whole eval fails. The except Exception path then sets both deflate and base64 to False, meaning devices without deflate permanently lose base64 encoding and fall back to repr — a regression. The deflate and base64 capabilities need to be probed independently. Also, test_compression_ratio is defined twice in compression_utils.py.
| try: | ||
| caps = self.eval( | ||
| "{" | ||
| "'deflate':hasattr(__import__('deflate'),'DeflateIO')," |
There was a problem hiding this comment.
If deflate is not available on the device, __import__('deflate') raises ImportError, aborting the entire dict construction. The except Exception at the outer level then sets both deflate and base64 to False — even though binascii.a2b_base64 is almost certainly present. On any device without deflate this PR is a regression vs repr.
The capabilities must be probed independently. Either do two separate eval calls, or use a device-side try/except around the deflate import:
caps = self.eval(
"{'base64':hasattr(__import__('binascii'),'a2b_base64'),"
"'bytesio':hasattr(__import__('io'),'BytesIO')}"
)
try:
has_dfl = self.eval("hasattr(__import__('deflate'),'DeflateIO')")
except Exception:
has_dfl = False
caps['deflate'] = has_dfl and caps.get('base64') and caps.get('bytesio')| if not sample: | ||
| return 1.0 | ||
| compressed = compress_chunk(sample) | ||
| return len(compressed) / len(sample) |
There was a problem hiding this comment.
test_compression_ratio is already defined at line 69. Remove the duplicate.
| @@ -0,0 +1,86 @@ | |||
| #!/usr/bin/env python3 | |||
There was a problem hiding this comment.
Please remove the shebang; this is a library module, not a standalone script.
| # Setup imports and file handle on device | ||
| if encoding == "deflate": | ||
| self.exec( | ||
| "from binascii import a2b_base64 as _a\n" |
There was a problem hiding this comment.
The ROMFS path in commands.py imports under the full names (a2b_base64, BytesIO, DeflateIO, RAW). Using single-letter aliases (_a, _B, _D, _R) here is inconsistent. Please use the same names in both paths.
|
/review |
There was a problem hiding this comment.
The .strip() fix in the ROMFS base64 path is correct and the performance gains are well documented. However there is a correctness bug in _detect_encoding_capabilities that will regress base64 support on devices without deflate, and a few smaller issues below.
| try: | ||
| caps = self.eval( | ||
| "{" | ||
| "'deflate':hasattr(__import__('deflate'),'DeflateIO')," |
There was a problem hiding this comment.
This will raise ImportError on devices that don't have the deflate module, causing the entire eval() to fail. The except Exception then sets base64 to False too, which is a regression — devices without deflate but with binascii.a2b_base64 will silently lose base64 support and fall back to repr.
The old code probed each capability with a separate try/exec block precisely to avoid this. Either do the same here, or guard the import in the eval string, e.g. with a helper that catches ImportError on the device side.
| "deflate": caps.get("deflate") and caps.get("bytesio") and caps.get("base64"), | ||
| "base64": caps.get("base64"), | ||
| } | ||
| except Exception: |
There was a problem hiding this comment.
Catching bare Exception here silently swallows errors that are not ImportError / transport errors (e.g. OOM on the device, serialisation bugs). Should be except (Exception,): at minimum with a comment, or ideally just except TransportExecError.
| chunk_size = max(chunk_size, rom_min_write) | ||
|
|
||
| # Detect capabilities of the device to use the fastest method of transfer. | ||
| caps = transport._detect_encoding_capabilities() |
There was a problem hiding this comment.
_detect_encoding_capabilities is a private method (underscore prefix). Calling it directly from commands.py breaks the encapsulation the underscore signals. Either make it public or expose the capabilities through a higher-level API.
| @@ -0,0 +1,86 @@ | |||
| #!/usr/bin/env python3 | |||
There was a problem hiding this comment.
please remove the shebang — this is a library module, not a standalone script.
| def compress_chunk(data, wbits=DEFAULT_WBITS): | ||
| """Compress a single chunk using raw deflate. | ||
|
|
||
| Each chunk is independently compressed/decompressable, which is required |
There was a problem hiding this comment.
"decompressable" → "decompressible"
| Returns: | ||
| Ratio of compressed/original size (0.0-1.0+). Lower = better compression. | ||
| """ | ||
| sample = data[:sample_size] if len(data) > sample_size else data |
There was a problem hiding this comment.
data[:sample_size] if len(data) > sample_size else data can just be data[:sample_size] — Python slicing past the end is safe.
andrewleech
force-pushed
the
feature/smart-encoding-fs-writefile
branch
2 times, most recently
from
a46e4ae to
7f0d6a6
Compare
Automatically detect device capabilities (deflate, base64, bytes.fromhex) and select the best encoding for file transfers. Deflate+base64 is used when the device supports it and data compresses well, base64 alone as a fallback, and repr as the universal fallback. Each capability is probed independently so a missing deflate module does not suppress base64 detection. Signed-off-by: Andrew Leech <andrew.leech@planetinnovation.com.au>
andrewleech
force-pushed
the
feature/smart-encoding-fs-writefile
branch
from
7f0d6a6 to
47d6725
Compare
2 participants
Summary
mpremote fs cpfile transfers to device are slow becausefs_writefile()usesrepr()encoding, expanding each byte to\xNN(~4x wire overhead for binary data).This adds automatic encoding selection with a three-tier fallback:
binascii.a2b_base64but data doesn't compress wellThe ROMFS deploy path is updated to share the new compression utilities and capability detection, replacing its inline
zlib.compressobj(wbits=-9), hardcoded wbits value, and 14-line try/except capability detection. Also fixes a missing.strip()on the ROMFS base64-only encoding path.Testing
64 transfer+readback integrity tests on STM32WB55 over 115200 baud UART with SPI flash. All verified via SHA-256 readback.
Random binary (incompressible, ratio ~1.0 — auto selects base64):
Python source (ratio ~0.4 — auto selects deflate):
Log data (ratio ~0.5 — auto selects deflate):
All zeros (ratio ~0.005 — auto selects deflate):
Auto-selection picks the fastest encoding for each data type in all cases.
Not tested on other ports or boards.
Trade-offs and Alternatives
chunk_sizedefault changes from256toNone(auto-sized per encoding). Callers omittingchunk_sizeget 256 for repr (matching prior behaviour). Explicit values are respected.binascii.a2b_base64fall back torepr()with no behaviour change.hasattr()and cached for the session.