Can negotiated MTU w/ MCUMgr params work for serial (uart/usb/shell transport)

[JPHutchins]

This is not covered by the present integration tests for serial (uart/usb/shell) transport. The optimized parameters are given to the transport on initialization by parsing the FW file name.

smpclient/examples/usb/upgrade.py

Line 57 in dcace1a

line_length, line_buffers, max_smp_encoded_frame_size = map(int, match.groups())

smpclient/examples/usb/upgrade.py

Lines 109 to 114 in dcace1a

	async with SMPClient(
	SMPSerialTransport(
	max_smp_encoded_frame_size=max_smp_encoded_frame_size,
	line_length=line_length,
	line_buffers=line_buffers,
	),

See the DUT FW files themselves: https://github.com/intercreate/smpclient/tree/main/examples/duts/nrf52840dk_nrf52840/usb

Likewise, the USB transport upload file test is using default values:

smpclient/examples/usb/upload_file.py

Line 67 in dcace1a

async with SMPClient(SMPSerialTransport(), port) as client:

Defaults:

smpclient/smpclient/transport/serial.py

Lines 74 to 76 in dcace1a

	max_smp_encoded_frame_size: int = 256,
	line_length: int = 128,
	line_buffers: int = 2,

On the other hand, the BLE tests do use the MCUMgr parameters. That works because MTU is given by the BLE MTU, the the mcumgr params give the max frame size, allowing for efficient fragmentation.

What's missing with the serial transport is knowledge of the "line_length", AKA MTU. mcumgr params do not provide the MTU.

This is mostly relevant for USB transports where uploads and downloads can be very slow if the buffer sizes are smaller than what the FW provides. Like 9KBps vs 50KBps with correct MTUs.

CC @aldenhaase

[JPHutchins]

Sorta related, since this work will enable users of smpclient (e.g. smpmgr) to provide easy access to transport-specific configuration: #58

[joerchan]

I've done some investigation into this issue and this is what I have concluded.

MTU for uart/usb/shell should be assumed to be 127 as this is a limit that is not always imposed
by the implementation, but is there in the specification.
In zephyr this is only enforced by the SHELL transport.
See this PR for failed attempt to remove this limitation:
zephyrproject-rtos/zephyr#95891.

That means that the following configuration is invalid: dutfirmware/usb_smp_dut_1024_1_1024.conf:

# ~31.61 KB/s
CONFIG_UART_MCUMGR_RX_BUF_SIZE=1024
CONFIG_UART_MCUMGR_RX_BUF_COUNT=1
CONFIG_MCUMGR_TRANSPORT_NETBUF_SIZE=1024.

It is however possible to get the same throughput with a different configuration.

Serial transport initialized with:

max_smp_encoded_frame_size = 2048
line_length = 128
line_buffers = 16

config for SHELL transport

# ~31.3 KB/s
CONFIG_MCUMGR_TRANSPORT_NETBUF_COUNT=2
CONFIG_MCUMGR_TRANSPORT_NETBUF_SIZE=2048
# Non-functional config, included for demonstration, MTU is always 127
CONFIG_MCUMGR_TRANSPORT_SHELL_MTU=127
CONFIG_MCUMGR_TRANSPORT_SHELL_RX_BUF_COUNT=16

config for UART transport

# Not tested, but assume speed would be the same, as it is sent the same from smpmgr
# The actual MTU of the UART transport
CONFIG_UART_MCUMGR_RX_BUF_SIZE=127
CONFIG_UART_MCUMGR_RX_BUF_COUNT=16
CONFIG_MCUMGR_TRANSPORT_NETBUF_COUNT=2
CONFIG_MCUMGR_TRANSPORT_NETBUF_SIZE=2048
# Unused config, but should match the MTU.
CONFIG_MCUMGR_TRANSPORT_UART_MTU=127

I think the best thing for smpmgr would be to:

1. Assume an MTU of 127 by default.
2. Read the MCUMGR_PARAM page for line_length.
3. Calculate line_buffers as line_length / MTU.
4. Allow giving line_buffers / line_length to override the default behavior, together with MTU.
   That way all assumptions can be overridden.

Without changing the line_buffers setting, and using the enforced shell MTU of 127 I can only
get around 7.2 KB/s using the default smpclient, and only 2.3 UB/s when setting --mtu 127.

The reason for line_buffers being so important for serial / UART is that smpmgr sends all
frames back to back, while there is an around 15 ms delay between each SMP-packet. This delay
I don't know where comes from but seems fixed, and is not affected by baudrate.
That is why both 1M and 230.4K will have the same throughput.

Happy to contribute, or aid in testing.
Let me know if I should provide any more information.

[JPHutchins]

This is great! Discovering that small line length can still allow for fast transfers could resolve the issues with mcumgr param negotiation for the serial transports.

I think the best thing for smpmgr would be to:

1. Assume an MTU of 127 by default.
2. Read the MCUMGR_PARAM page for line_length.
3. Calculate line_buffers as line_length / MTU.
4. Allow giving line_buffers / line_length to override the default behavior, together with MTU.
   That way all assumptions can be overridden.

1. For serial, the SMP "MTU" is called "line length". This is the maximum transmission unit of encoded smp frames. It's important to agree on the terminology since much of the confusion stems from the vocabulary and dead or poorly named configs in Zephyr.
2. The MCUMGR_PARAM does not give line length. It gives the netbuf size and netbuf count, where netbuf size can be "max unencoded smp frame size". It is never necessary or useful to have more than one netbuf. Zephyr is not making compile time guarantees that the netbuf size matches the line length. I will need to look at the Zephyr impl, but I'm assuming that it needs

• 127 byte buffer to hold the encoded packet - a "line" - I think it's called this because the \n is used as delimiter 🙄
• netbuf size buffer to hold the incrementally decoded smp packet
  I do agree with the approach: line_buffers = netbuf_size // 127

3. I like where this is going
4. Agreed!

My only concern is that it seems that the 16 buffer config uses 2x the RAM allocation for the same speed. I may be wrong, since the 1024 config might allocate 2048 as well - I need to investigate Zephyr impl to understand what it's doing with these.

The primary fix, for a default of 127, would be upstream: https://github.com/JPHutchins/smp/blob/edb8a012d8e9fa5b85e524296b73f8e44c0ecdae/smp/packet.py#L30-L31

Finally, this also needs to be tested with MCUBoot's SMP server implementation: https://github.com/mcu-tools/mcuboot/blob/main/boot/boot_serial/src/boot_serial.c

[joerchan]

It has been quite confusing to understand and to figure everything out. The documentation was not very helpful and i basically had to dive into the implementation to understand the full picture.
I did actually mean the MCUMGR_PARAM buf_size and buf_count.

So to rewrite my suggestion with correct terminology:

1. Assume an line_length of 127 by default.
2. Read the MCUMGR_PARAM page for buf_size.
3. Calculate line_buffers as buf_size / line_length.
4. Allow giving line_buffers / line_length to override the default behavior, along with buf_size (max_unencoded_packet_size).
   That way all assumptions can be overridden.

My only concern is that it seems that the 16 buffer config uses 2x the RAM allocation for the same speed. I may be wrong, since the 1024 config might allocate 2048 as well - I need to investigate Zephyr impl to understand what it's doing with these.

There is still the possebility to do the other config that is the fastest for UART, for the shell however there is no choice unless you patch zephyr to remove this limit yourself.
Unfortunately it seems that you would need the 2048 config, as using 1024 only gives you around 20 KB/s.

You actually don't need the full 16 RX buffers in the MCU though, as you can just have 4 and then rely on the buffers being processed fast enough to always have available buffers, but if you run out the transfer could fail. So 16 is needed to have guaranteed numbers of buffers available.

[JPHutchins]

So to rewrite my suggestion with correct terminology:

1. Assume an line_length of 127 by default.
2. Read the MCUMGR_PARAM page for buf_size.
3. Calculate line_buffers as buf_size / line_length.
4. Allow giving line_buffers / line_length to override the default behavior, along with buf_size (max_unencoded_packet_size).
   That way all assumptions can be overridden.

I will implement in this manner, thank you!