mpremote: Fixbyte length calculation in `wr_bytes` and `read` methods. by Josverl · Pull Request #19113 · micropython/micropython · GitHub
Skip to content

mpremote: Fixbyte length calculation in wr_bytes and read methods.#19113

Open
Josverl wants to merge 2 commits intomicropython:masterfrom
Josverl:mpremote/mount_arrays
Open

mpremote: Fixbyte length calculation in wr_bytes and read methods.#19113
Josverl wants to merge 2 commits intomicropython:masterfrom
Josverl:mpremote/mount_arrays

Conversation

@Josverl
Copy link
Copy Markdown
Contributor

@Josverl Josverl commented Apr 16, 2026 •
edited
Loading

Summary

Refactor the byte length calculation in the wr_bytes and read methods to use a new buffer_nbytes function. This change corrects the accuracy of byte length determination for various buffer types.

I have addedd a helper function buffer_nbytes that :

  • has a fast‑path for str, bytes, bytearray
  • Tries memoryview. itemsize()
  • Tries typecode + struct.calcsize()
  • Finally falls back to len(bytes(obj)) ( which causes a memory allocation )

Before this fix mpremote used len(buf) to determine how many bytes to send or receive.
For buffer‑protocol objects like array('h') or array('H'), len() returns number of items, not number of bytes.
This caused:

  • write(): sends more bytes than declared → junk leaks to console
  • readinto(): reads too few bytes → partially filled arrays

Closes : #17665

Testing

Tested on relevant MicroPython boards to ensure functionality remains intact.

Trade-offs and Alternatives

The cleanest solution would be for moare ports/boards to actually enable MICROPY_PY_BUILTINS_MEMORYVIEW_ITEMSIZE as currenltly memoryview.itemsize is only enabled on the unix coverage variant, leaving this useful feature unused.

Generative AI

I did use generative AI tools when creating this PR, And I personally checked and verified the results.

@Josverl Josverl changed the title Refactor byte length calculation in wr_bytes and read methods mpremote: Fixbyte length calculation in wr_bytes and read methods. Apr 16, 2026
@Josverl Josverl added the tools Relates to tools/ directory in source, or other tooling label Apr 16, 2026
@Josverl Josverl force-pushed the mpremote/mount_arrays branch from 598dcb0 to f7b590c Compare April 16, 2026 10:53
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 16, 2026 •
edited
Loading

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 16, 2026 •
edited
Loading

Code size report:

Reference:  extmod/moductypes: Be more defensive with uctypes_struct_agg_size args. [8a56be6]
Comparison: tools/mpremote: Add tests for array readinto and write. [merge of 2fe5494]
  mpy-cross:    +0 +0.000% 
   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 
   unix x64:    +0 +0.000% standard
      stm32:    +0 +0.000% PYBV10
      esp32:    +0 +0.000% ESP32_GENERIC
     mimxrt:    +0 +0.000% TEENSY40
        rp2:    +0 +0.000% RPI_PICO_W
       samd:    +0 +0.000% ADAFRUIT_ITSYBITSY_M4_EXPRESS
  qemu rv32:    +0 +0.000% VIRT_RV32

dpgeorge
dpgeorge reviewed Apr 17, 2026
View reviewed changes
Comment thread tools/mpremote/mpremote/transport_serial.py Outdated
Comment thread tools/mpremote/mpremote/transport_serial.py Outdated
Comment thread tools/mpremote/mpremote/transport_serial.py Outdated
@Josverl Josverl force-pushed the mpremote/mount_arrays branch from f7b590c to 3a2233b Compare April 17, 2026 13:54
@Josverl
Copy link
Copy Markdown
Contributor Author

Josverl commented Apr 17, 2026 •
edited
Loading

  • Updated as requested.
    And while updating on a different machine - I noticed that I had not committed the test.sh files , so these will come later. ( Done)

dpgeorge
dpgeorge reviewed Apr 21, 2026
View reviewed changes
Comment thread tools/mpremote/mpremote/transport_serial.py Outdated

SEEK_SET = 0

def buffer_nbytes(obj):
Copy link
Copy Markdown
Member

@dpgeorge dpgeorge Apr 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just realised that this function is visible to the user (eg at the REPL, tab completion) when using mpremote mount, which is a bit messy.

So I suggest moving it into RemoteCommand as a method there. That's the logical place for it.

Copy link
Copy Markdown
Contributor Author

@Josverl Josverl Apr 22, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note it is also used from RemoteFile.readinto(), so would need to be a @staticmethod now @classmethod

I have also moved __MPR_HAS_MV_IS into a class variable RemoteCommand.HAS_MV_IS
to avoid

>>> __
__class__       __name__        __dict__        __file__
__MPR_HAS_MV_IS                 __mount
>>> __MPR_HAS_MV_IS
False

@Josverl
tools/mpremote: Fix length calculation in RemoteCommand for arrays.
011b5be 
Signed-off-by: Jos Verlinde <Jos_Verlinde@hotmail.com>
@Josverl Josverl force-pushed the mpremote/mount_arrays branch from 3a2233b to 011b5be Compare April 22, 2026 15:47
@Josverl
tools/mpremote: Add tests for array readinto and write.
2fe5494 
Co-authored-by: Copilot <copilot@github.com>
Signed-off-by: Jos Verlinde <Jos_Verlinde@hotmail.com>
@Josverl Josverl force-pushed the mpremote/mount_arrays branch from a4a584c to 2fe5494 Compare April 22, 2026 21:30
dpgeorge
dpgeorge reviewed Apr 23, 2026
View reviewed changes
Comment thread tools/mpremote/mpremote/transport_serial.py
wr_str = wr_bytes

@classmethod
def buffer_nbytes(cls, obj):
Copy link
Copy Markdown
Member

@dpgeorge dpgeorge Apr 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function somehow reverted to an earlier version. Please change it back 😄

dpgeorge
dpgeorge reviewed Apr 23, 2026
View reviewed changes
Comment thread tools/mpremote/mpremote/transport_serial.py
Copy link
Copy Markdown
Member

@dpgeorge dpgeorge Apr 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be c.buffer_nbytes(buf), and then the buffer_nbytes can be a normal method.

That's slightly more efficient because c is a local variable here (while RemoteCommand is global, which is slower to lookup).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dpgeorge dpgeorge dpgeorge left review comments

Assignees

No one assigned

Labels

tools Relates to tools/ directory in source, or other tooling

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

mpremote mount: file write of non-byte arrays fails due to incorrect length calculation

2 participants

@Josverl @dpgeorge