Improvements in StorageOperation command - #396
Conversation
📝 WalkthroughWalkthroughUpdates storage operation wire-protocol payload construction to encode filenames as null-terminated UTF-8 data, append file data, remove the public ChangesStorage operation payload
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
nanoFramework.Tools.DebugLibrary.Shared/WireProtocol/Commands.cs (1)
586-665: 🗄️ Data Integrity & Integration | 🔵 TrivialVerify the firmware compatibility boundary.
Engine.AddStorageFilesends this payload unconditionally, while removingNameLengthshifts subsequent fields for older firmware. Ensure deployment either gates this command to compatible firmware or clearly documents and enforces the required host/firmware pairing.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nanoFramework.Tools.DebugLibrary.Shared/WireProtocol/Commands.cs` around lines 586 - 665, The storage-operation payload prepared by PrepareForSend and sent through Engine.AddStorageFile must not be used with incompatible firmware. Gate AddStorageFile or the related command path on the supported firmware version, or enforce the required host/firmware pairing with clear validation and documentation, while preserving the payload layout for compatible targets.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@nanoFramework.Tools.DebugLibrary.Shared/WireProtocol/Commands.cs`:
- Around line 659-665: Update the filename encoding logic near Data and FileName
so the allocated buffer size and null-terminator index use the UTF-8 byte count
returned for the encoded filename, not FileName.Length. Preserve the complete
UTF-8 byte sequence for multibyte characters and place the terminator
immediately after it.
- Around line 631-643: Restore the DataLength assignment in the
PrepareForSend(byte[] data, int length, int offset) override by setting it to
the requested length before resizing or constructing Data. Ensure chunks sent
through Engine.AddStorageFile expose the correct wire length, including nonzero
lengths.
---
Outside diff comments:
In `@nanoFramework.Tools.DebugLibrary.Shared/WireProtocol/Commands.cs`:
- Around line 586-665: The storage-operation payload prepared by PrepareForSend
and sent through Engine.AddStorageFile must not be used with incompatible
firmware. Gate AddStorageFile or the related command path on the supported
firmware version, or enforce the required host/firmware pairing with clear
validation and documentation, while preserving the payload layout for compatible
targets.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: nanoframework/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f4bda1a2-a41a-4969-a0f2-5f17be552c7f
📒 Files selected for processing (1)
nanoFramework.Tools.DebugLibrary.Shared/WireProtocol/Commands.cs
eb583fb to
0f7f928
Compare
- Dropping name lenght property. - Replacing with null terminated string. - Adjust PrepareForSend methods accordingly. - Adjust parameter name to match declaration.
0f7f928 to
abc6ff4
Compare
(cherry picked from commit 8b42c9c)
Description
>1.17.0.257with support for this new payload formatMotivation and Context
How Has This Been Tested?
Screenshots
Types of changes
Checklist: