intercreate / smpclient

Simple Management Protocol (SMP) Client for remotely managing MCU firmware
Apache License 2.0
9 stars 6 forks source link

ImageUploadWrite can receive multiple types of responses #31

Closed bjandre closed 4 months ago

bjandre commented 6 months ago

Running example/ble/upload.py throws a pydantic type error when an upload completes. The root cause appears to be that smpclient/requests/image_management.py:ImageUploadWrite is expected to have a response of type smpimg.ImageUploadProgressWriteResponse, but can also receive responses of type smpimg.ImageUploadFinalWriteResponse. smpclient.request() can only handle a single response type. When the ImageUploadFinalWriteResponse comes in, it is not recognized as a valid response by smpclient.request() and is processed as an error, which the data doesn't match.

I'm happy to submit a PR, but I don't see a simple path forward. Any suggestions?

Uploaded 14,647 / 14,670 Bytes | 3.54 KB/s Traceback (most recent call last): File "/Users//projects/research/smpclient-examples/smpclient/smpclient/init.py", line 71, in request return request._ErrorV1.loads(frame) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/Users//projects/research/smpclient-examples/venv/lib/python3.12/site-packages/smp/message.py", line 49, in loads message = cls( ^^^^ File "/Users//projects/research/smpclient-examples/venv/lib/python3.12/site-packages/pydantic/main.py", line 176, in init self.__pydantic_validator__.validate_python(data, self_instance=self) pydantic_core._pydantic_core.ValidationError: 3 validation errors for ImageManagementErrorV1 err Field required [type=missing, input_value={'header': Header(op=<OP....': 14670, 'match': True}, input_type=dict] For further information visit https://errors.pydantic.dev/2.7/v/missing off Extra inputs are not permitted [type=extra_forbidden, input_value=14670, input_type=int] For further information visit https://errors.pydantic.dev/2.7/v/extra_forbidden match Extra inputs are not permitted [type=extra_forbidden, input_value=True, input_type=bool] For further information visit https://errors.pydantic.dev/2.7/v/extra_forbidden

During handling of the above exception, another exception occurred:

Traceback (most recent call last): File "", line 198, in _run_module_as_main File "", line 88, in _run_code File "/Users//projects/research/smpclient-examples/smpclient/examples/ble/upload.py", line 68, in asyncio.run(main()) File "/opt/homebrew/Cellar/python@3.12/3.12.3/Frameworks/Python.framework/Versions/3.12/lib/python3.12/asyncio/runners.py", line 194, in run return runner.run(main) ^^^^^^^^^^^^^^^^ File "/opt/homebrew/Cellar/python@3.12/3.12.3/Frameworks/Python.framework/Versions/3.12/lib/python3.12/asyncio/runners.py", line 118, in run return self._loop.run_until_complete(task) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/opt/homebrew/Cellar/python@3.12/3.12.3/Frameworks/Python.framework/Versions/3.12/lib/python3.12/asyncio/base_events.py", line 687, in run_until_complete return future.result() ^^^^^^^^^^^^^^^ File "/Users//projects/research/smpclient-examples/smpclient/examples/ble/upload.py", line 46, in main async for offset in client.upload(image=fw_file, slot=2, subsequent_timeout_s=40.0): File "/Users//projects/research/smpclient-examples/smpclient/smpclient/init.py", line 135, in upload response = await self.request( ^^^^^^^^^^^^^^^^^^^ File "/Users//projects/research/smpclient-examples/smpclient/smpclient/init.py", line 78, in request raise ValidationError(error_message) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ TypeError: No constructor defined

JPHutchins commented 6 months ago

This is a good thing! Up until now I have not seen an SMP server that correctly sends the upload write response.

There is no such thing as an ImageUploadProgress/FinalWriteResponse in the spec, there is only the ImageUploadWriteResponse:

https://docs.zephyrproject.org/latest/services/device_mgmt/smp_groups/smp_group_1.html#image-upload-response

But Zephyr SMP servers, especially MCUBoot, added an "rc" field and then never send the "match" that should be expected on the last offset. Getting that match would be nice to check from smpclient.

In smpclient, you can test a resolution by changing "Progress" to "Final" here: https://github.com/intercreate/smpclient/blob/26a3a2d266439dc5260ed7b0b53d68f88463a207/smpclient/requests/image_management.py#L18

This should work for all SMP server's that follow the spec.

To support legacy unspecified behavior, the correct solution, I think, will be to get rid of Progress and Final and simply add:

class ImageUploadWriteResponse(message.WriteResponse):
    _GROUP_ID = header.GroupId.IMAGE_MANAGEMENT
    _COMMAND_ID = header.CommandId.ImageManagement.UPLOAD

    off: int | None = None
    match: bool | None = None
    rc: int | None = None

upstream at https://github.com/JPHutchins/smp/blob/main/smp/image_management.py

The optional rc field should handle Zephyr's use of the unspecified field.