mpremote: Add smart encoding selection for fs_writefile.#11
Open
andrewleech wants to merge 25 commits intomasterfrom
Open
mpremote: Add smart encoding selection for fs_writefile.#11andrewleech wants to merge 25 commits intomasterfrom
andrewleech wants to merge 25 commits intomasterfrom
Conversation
Fixes a build issue on newer Zephyr versions. Signed-off-by: Antonio Galea <antonio.galea@gmail.com>
The Pololu Zumo 2040 Robot is supported in pico-sdk now so we should not include the header file here anymore, similarly to other boards. This is necessary for future changes from the SDK to be reflected in MicroPython builds. Signed-off-by: Paul Grayson <paul@pololu.com>
Replace the custom rosc_random_u8()/rosc_random_u32() implementation with the pico_rand API from the Pico SDK. The RP2040 datasheet notes that ROSC "does not meet the requirements of randomness for security systems because it can be compromised", and the current 8-bit LFSR conditioning is not a vetted algorithm under NIST SP 800-90B. pico_rand uses various hardware RNG sources depending on the available platform (including the RP2350 hardware TRNG) and is officially supported and maintained as part of the Pico SDK. This changes os.urandom(), the mbedTLS entropy source, the PRNG seed, and the lwIP random function to all use pico_rand, and removes the custom ROSC random functions from main.c. Signed-off-by: Michel Le Bihan <michel@lebihan.pl>
When the timeout parameter of `esp32.RMT.wait_done()` is set to a non-zero value, the underlying `rmt_tx_wait_all_done` blocks (it passes the timeout to `xQueueReceive`). Thus we should release the GIL so that other MicroPython threads are not blocked from running. Signed-off-by: Daniël van de Giessen <daniel@dvdgiessen.nl>
This commit lets the native emitter preserve the value of the index register when performing register-indexed loads or stores of halfword or word values on Thumb. The original code was optimised too aggressively for a register-starved architecture like Thumb, and the index value in the sequence to generate was assumed to be allocated somewhere safe. This is valid on other architectures, but not on Thumb. To solve this, load operations do clobber a temporary register that should be safe to use, REG_TEMP2, to store the scaled register offset. REG_TEMP2's value is only used within the scope of a single ASM API instruction. Save operations unfortunately use a register that is aliased to REG_TEMP2, since they need to have three values in registers to perform the operation. This means the index register needs to be pushed to the stack before performing the scale + store operation, and then popped from the stack. That's a 4 bytes penalty on each store and a minor speed hit on generated code (plus a minor footprint increase of the firmware image). Signed-off-by: Alessandro Gatti <a.gatti@frob.it>
This commit lets the native emitter preserve the value of the index register when performing register-indexed loads or stores for halfword or word values on RV32. The original code was optimised too aggressively to reduce the generated code's size, using compressed opcodes that alias the target register to one of the operands. In register-indexed load/store operations, the index register was assumed to be allocated somewhere safe, but it was not always the case. To solve this, now all halfword and word register-indexed operations will use REG_TEMP2 to store the scaled index register. The size penalty on generated code varies across operation sizes and enabled extensions: - byte operations stay the same size with or without Zba - halfword operations will be 2 bytes larger without Zba, and will stay the same size with Zba - word operations will be 4 bytes larger without Zba, and 2 bytes larger with Zba There is also a minor firmware footprint increase to hold the extra logic needed for conditional register clobbering, but it shouldn't be that large anyway. Signed-off-by: Alessandro Gatti <a.gatti@frob.it>
This commit introduces a test that should check whether viper load or store operations won't clobber either the buffer address or the index value being used. Signed-off-by: Alessandro Gatti <a.gatti@frob.it>
Call `mp_event_handle_nowait()` in the VFS reader buffer refill path so
that pending scheduled events (USB task, network poll, etc.) get processed
during long-running import/parse/compile operations.
Without this, importing a large Python module from the filesystem blocks
for too long causing TinyUSB event queue to overflow. For example, on
renesas-ra, running a script that imports iperf3 via mpremote run, asserts,
most likely due to SOF interrupts not getting processing:
queue_event at lib/tinyusb/src/device/usbd.c:382
dcd_event_handler at lib/tinyusb/src/device/usbd.c:1318
dcd_event_sof at lib/tinyusb/src/device/dcd.h:237
dcd_int_handler at tinyusb/src/portable/renesas/rusb2/dcd_rusb2.c:964
<signal handler called>
disk_ioctl at extmod/vfs_fat_diskio.c:125
validate at lib/oofatfs/ff.c:3359
f_read at lib/oofatfs/ff.c:3625
file_obj_read at extmod/vfs_fat_file.c:75
mp_stream_rw at py/stream.c:60
mp_reader_vfs_readbyte at extmod/vfs_reader.c:59
next_char at py/lexer.c:174
mp_lexer_to_next at py/lexer.c:713
mp_parse at py/parse.c:1167
Signed-off-by: iabdalkader <i.abdalkader@gmail.com>
The aim of this commit is to clarify the command line options available. While they are available as well as in the CLI with --help, it's useful to document them and provide a few examples. Signed-off-by: Jos Verlinde <jos_verlinde@hotmail.com>
Factor out mp_os_urandom() of each port into extmod/modos.c, which then calls the port-specific function mp_hal_get_random(). Move mp_hal_get_random() to mphalport where suitable. At the MIMXRT and SAMD it is left in modos.c, since there are different implementation depending on the MCU family. At the ALIF, ESP32, CC3200 and RP2 port the file modos.c was removed, since it was empty after moving mp_hal_get_random(). Tested for the cc3200, esp32, esp8266, mimxrt, nrf, rp2, samd, stm32 and unix ports. Compiled for the alif and the renesas port. Signed-off-by: robert-hh <robert@hammelrath.com>
Fixes issue micropython#18825. Signed-off-by: robert-hh <robert@hammelrath.com>
Signed-off-by: Damien George <damien@micropython.org>
|
Code size report: |
Owner
Author
|
/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.
tools/mpremote/mpremote/transport.py
Outdated
| 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.
tools/mpremote/mpremote/transport.py
Outdated
| # 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.
This is a reproducer for micropython#17117. Signed-off-by: Jeff Epler <jepler@unpythonic.net>
Signed-off-by: Jeff Epler <jepler@unpythonic.net>
This turns the reproducer into a sensible-ish crash:
TypeError: exceptions must derive from BaseException
Closes: micropython#17117
Signed-off-by: Jeff Epler <jepler@unpythonic.net>
Signed-off-by: Jeff Epler <jepler@unpythonic.net>
Signed-off-by: stijn <stijn@ignitron.net>
This is convenient when trying to figure out the correct values for --include/--exclude/--test-dirs/... arguments. Signed-off-by: stijn <stijn@ignitron.net>
Test file paths which get passed to the run_tests function can be absolute or relative and with or without leading slash in the latter case, depending on the arguments to run-tests.py, but the skip_tests list with tests to skip only contains relative paths so using simple string equality comparison easily leads to false negatives. Compare the full absolute path instead such that it doesn't matter anymore in which form the tests are passed. Note: - use realpath to resolve symlinks plus make the comparison case insensitive on windows - the test_file passed to run_one_test is not altered by this commit, such that when the user inputs relative paths the tests are also still displayed with relative paths - likewise the test_file_abspath is not modified because functions like run_micropython rely on it having forward slashes In practice this means that it used to be so that the only forms of running tests for which the skip_tests lists actually worked were: >python ./run-tests.py >python ./run-tests.py -d extmod whereas it now works consistently so also for these invocations, which in the end all point to the exact same path: >python ./run-tests.py -d ./extmod >python ./run-tests.py -d ../tests/extmod >python ./run-tests.py -d /full/path/to/tests/extmod These examples used to not skip any of the tests in the extmod/ directory thereby leading to test failures. Signed-off-by: stijn <stijn@ignitron.net>
Scan the --test-dirs argument for the main tests directory being passed and if so do the same thing as if running from within that main test directory. In practice this makes the following (which used to counterintuitively try and fail to run the .py files in the tests/ directory itself) >python micropython/tests/run-tests.py -d micropython/tests do the same thing as >cd micropython/tests >python ./run-tests.py which is logical and convenient. Signed-off-by: stijn <stijn@ignitron.net>
Signed-off-by: stijn <stijn@ignitron.net>
Test file paths which get passed to the run_tests function can be absolute or relative and with or without leading slash in the latter case, depending on the arguments to run-tests.py, but since that path is used to: - display which tests run - record which tests ran in the results.json - craft the filename for the .exp/.out file for failed tests it is desirable to always use the same file path irregardless of how the user passed the path. In practice this means that all forms of running our own tests like: >python ./run-tests.py -i extmod >python ./run-tests.py -d extmod >python ./run-tests.py -d ./extmod >python ./run-tests.py -d ../tests/extmod >python ./run-tests.py -d /full/path/to/tests/extmod will now consistently all display the tests like pass extmod/time_time_ns.py FAIL extmod/some_failing_test.py and produce output files like results/extmod_some_failing_test.py.exp results/extmod_some_failing_test.py.out instead of displaying/using the exact path as passed. For external tests, meaning not in the tests/ directory, we also want to be consistent so there the choice was made to always use absolute paths. Signed-off-by: stijn <stijn@ignitron.net>
Fixes fatal crash if serial port access returns an error (for example: port is native USB-CDC and the host hard faults during the test run). Instead of crashing, have the runner mark this as a test run error and continue. It's not certain the next test will run successfully, but this provides the context of output showing what was happening when the communication error occurred. Without this change, that output is lost when the fatal exception terminates the runner process. This work was funded through GitHub Sponsors. Signed-off-by: Angus Gratton <angus@redyak.com.au>
This gives a more user-friendly name when the Python object (eg Pin) is printed. If the nodelabel is unavailable then it uses `dev->name` as a fallback. Signed-off-by: Fin Maaß <f.maass@vogl-electronic.com>
Owner
Author
|
/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.
tools/mpremote/mpremote/transport.py
Outdated
| 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.
tools/mpremote/mpremote/transport.py
Outdated
| "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.
tools/mpremote/mpremote/commands.py
Outdated
| 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
from
5599a34 to
a46e4ae
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
a46e4ae to
7f0d6a6
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
14 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.