[wip] fd commands #1
Draft
Conversation
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
Signed-off-by: leongross <[email protected]>
Signed-off-by: leongross <[email protected]>
Signed-off-by: leongross <[email protected]>
Signed-off-by: leongross <[email protected]>
Signed-off-by: leongross <[email protected]>
Signed-off-by: leongross <[email protected]>
The results for the corresponding pldm requests have an u8 field called CompletionCode. These completion codes are typically a set of PLDM_BASE_CODES and a selection of FW_UPDATE codes. To pass these return codes in a generic and type safe way, we want to define enums that implement the `From` trait for u8, individually for each response. Signed-off-by: leongross <[email protected]>
leongross
force-pushed
the
leongross/fd-commands
branch
from
c42fcbe to
a8594d2
Compare
Signed-off-by: leongross <[email protected]>
leongross
force-pushed
the
leongross/fd-commands
branch
3 times, most recently
from
9c5914d to
ecd1727
Compare
Signed-off-by: leongross <[email protected]>
leongross
force-pushed
the
leongross/fd-commands
branch
5 times, most recently
from
18eb056 to
bcf45fe
Compare
embediver
reviewed
Nov 19, 2025
Comment on lines
+1373
to
+1472
| assert!(dsd_iter.is_some()); | ||
|
|
There was a problem hiding this comment.
Checking that we actually get the correct descriptors would be nice.
A few more asserts to check the first descriptor field for every descriptor should be enough.
leongross
force-pushed
the
leongross/fd-commands
branch
6 times, most recently
from
a652e60 to
b225fbe
Compare
Copilot finished reviewing on behalf of
leongross
November 20, 2025 16:15
There was a problem hiding this comment.
Pull Request Overview
This WIP PR adds support for firmware device (FD) commands as defined in DSP0267, implementing several downstream device management commands for the PLDM firmware update protocol. The implementation includes new command structures, a reusable completion code macro, and comprehensive test coverage.
Key Changes:
- Introduces 7 new FD commands: QueryDownstreamDevices, QueryDownstreamIdentifiers, GetDownstreamFirmwareParameters, RequestDownstreamDeviceUpdate, GetPackageData, GetDeviceMetaData, and GetMetaData
- Adds
pldm_completion_code!macro for type-safe completion code handling with base and custom codes - Implements
PldmCodecWithLifetimetrait for structures that need to borrow from decode buffers
Reviewed Changes
Copilot reviewed 17 out of 18 changed files in this pull request and generated 20 comments.
Show a summary per file
| File | Description |
|---|---|
src/protocol/firmware_update.rs |
Adds new command enums, completion codes, KnownLayout trait to Descriptor, custom PldmCodec for PldmFirmwareString, and updates FirmwareDeviceCapability bitfield |
src/protocol/base.rs |
Introduces pldm_completion_code! macro for creating type-safe completion code enums and adds PartialEq to PldmBaseCompletionCode |
src/message/firmware_update/query_downstream.rs |
New module implementing downstream device query commands with iterators for parsing variable-length response data |
src/message/firmware_update/get_package_data.rs |
New module implementing package data and metadata retrieval commands |
src/message/firmware_update/mod.rs |
Registers new query_downstream and get_package_data modules |
src/message/firmware_update/request_fw_data.rs |
Migrates to PldmCodecWithLifetime trait and implements decode functionality |
src/message/firmware_update/*.rs |
Standardizes test naming convention with _codec suffix across multiple test files |
src/codec.rs |
Adds PldmCodecWithLifetime trait for lifetime-aware codec operations and InvalidData error variant |
Cargo.toml / Cargo.lock |
Adds bytemuck dependency (appears unused) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| self._iter_offset += descriptor_size; | ||
| self._iter_dev_count += 1; | ||
|
|
||
| // println!("{descriptor:?}"); |
There was a problem hiding this comment.
Commented-out code at line 441 (// println!("{descriptor:?}")) should be removed before merging.
Suggested change
| // println!("{descriptor:?}"); |
Signed-off-by: leongross <[email protected]>
leongross
force-pushed
the
leongross/fd-commands
branch
from
b225fbe to
0bd97de
Compare
leongross
force-pushed
the
leongross/fd-commands
branch
from
d0bb060 to
8d1b62f
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Roadmap
67.67% coverage, 1080/1596 lines covered