Active pending - #20
Conversation
…t. Adjusted 'now' default to safer value of 0, disabling timestamps. Users will need to implement now using a counter from their environment.
Co-authored-by: Christina Quast <chrysh.ng+github@gmail.com>
Co-authored-by: Christina Quast <chrysh.ng+github@gmail.com>
Co-authored-by: Christina Quast <chrysh.ng+github@gmail.com>
Co-authored-by: Christina Quast <chrysh.ng+github@gmail.com>
4eb5ad9 to
7632a4f
Compare
|
Review from Claude In fd_context.rs's activate_pending_component_rsp, the response is built with ActivateFirmwareResponse::new(...), not the ActivatePendingComponentResponse type the PR itself defines in activate_pending_component.rs. ActivateFirmwareResponse::new hardcodes the header command byte to FwUpdateCmd::ActivateFirmware (0x1A) — so a response to an 0x1F request goes out with command code 0x1A in the header. Field layout happens to match (completion_code: u8, estimated_time_activation: u16), so tests/CI pass, but a spec-correct UA that checks the response's command code against what it sent would reject this as a mismatched response. The dedicated ActivatePendingComponentResponse struct they wrote is dead code as a result. |
|
Opcode collision risk — pldm-common/src/protocol/firmware_update.rs, lines defining ActivateComponentImageSet = 0x1E / ActivateComponentImage = 0x1F: |
ActivateComponentImage are not Vendor specific and are part of the Type5 standard.
|
7632a4f to
7d1bfdd
Compare
…nent image transfer. UA sends the request and pldm-interface.fd_context responds by calling FdOps.handle_pending_component which will return the estimated time for activation.
7d1bfdd to
284ccc3
Compare
chrysh
left a comment
There was a problem hiding this comment.
Handler is unreachable
process_fw_update_cmd in cmd_interface.rs has no match arm for ActivateComponentImage, so it falls through to the wildcard and returns UnsupportedPldmCmd. Add an arm next to ActivateFirmware (line 174):
FwUpdateCmd::ActivateComponentImage => {
self.fd_ctx.activate_pending_component_rsp(payload)
}PLDM_PROTOCOL_CAPABILITIES in config.rs does not list the command either, so preprocess_request rejects 0x1F before dispatch even reaches the match, and GetPLDMCommands never advertises it. Add it to the supported_commands array (after line 51):
FwUpdateCmd::ActivateComponentImage as u8,Without both, the code this PR adds is dead.
Missing handler tests
No tests for activate_pending_component_rsp. This one works against the current code:
#[test]
fn test_activate_pending_component_invalid_state() {
let mut fd_ctx = new_test_fd_ctx();
let mut buffer = [0u8; 256];
fd_ctx.internal.set_fd_state(FirmwareDeviceState::Download);
let req = ActivatePendingComponentRequest::new(
1,
PldmMsgType::Request,
ComponentClassification::Firmware,
2,
3,
);
req.encode(&mut buffer).unwrap();
let result = fd_ctx.activate_pending_component_rsp(&mut buffer);
assert!(result.is_ok());
assert_eq!(
buffer[3],
FwUpdateCompletionCode::InvalidStateForCommand as u8,
);
}These two assume the handle_pending_component signature fix from the inline comment on fd_ops.rs:
#[test]
fn test_activate_pending_component_success() {
let mut fd_ctx = new_test_fd_ctx();
let mut buffer = [0u8; 256];
let req = ActivatePendingComponentRequest::new(
1,
PldmMsgType::Request,
ComponentClassification::Firmware,
2,
3,
);
req.encode(&mut buffer).unwrap();
let result = fd_ctx.activate_pending_component_rsp(&mut buffer);
assert!(result.is_ok());
assert_eq!(buffer[3], PldmBaseCompletionCode::Success as u8);
}For a rejection test, add a reject_pending: bool field to TestFdOps (set to false in the existing statics), a new static, and update the mock:
static TEST_FD_OPS_REJECT_PENDING: TestFdOps = TestFdOps {
devid_count: 1,
progress_fails: false,
reject_pending: true,
};
// In the FdOps impl (after signature fix):
fn handle_pending_component(
&self,
_component: &FirmwareComponent,
_fw_params: &FirmwareParameters,
estimated_time: &mut u16,
) -> Result<u8, FdOpsError> {
if self.reject_pending {
return Ok(FwUpdateCompletionCode::ActivatePendingImageNotPermitted as u8);
}
*estimated_time = 100;
Ok(PldmBaseCompletionCode::Success as u8)
}
#[test]
fn test_activate_pending_component_rejected() {
let mut fd_ctx = fd_ctx_with(&TEST_FD_OPS_REJECT_PENDING);
let mut buffer = [0u8; 256];
let req = ActivatePendingComponentRequest::new(
1,
PldmMsgType::Request,
ComponentClassification::Firmware,
2,
3,
);
req.encode(&mut buffer).unwrap();
let result = fd_ctx.activate_pending_component_rsp(&mut buffer);
assert!(result.is_ok());
assert_eq!(
buffer[3],
FwUpdateCompletionCode::ActivatePendingImageNotPermitted as u8,
);
}Naming
Side note: Courtney confirmed the spec names are ActivatePendingComponentImage (0x1F) and ActivatePendingComponentImageSet (0x1E), but the FwUpdateCmd enum variants drop "Pending" (ActivateComponentImage, ActivateComponentImageSet). Worth renaming to match the spec so grep finds them.
cb32176 to
167a850
Compare
chrysh
left a comment
There was a problem hiding this comment.
Minor cleanup nits, none blocking, so I already approve:
- Doc comment on handle_pending_component (fd_ops.rs:248) says Result<(u16), FdOpsError> but the signature returns Result<u8, FdOpsError>.
- test_activate_pending_component_success checks buffer[3] (completion code) but not buffer[4..6] (estimated_time). The mock writes 100 and nobody verifies it arrives in the response.
- ActivatePendingComponentImageSet (0x1E) is in the enum but has no message type, handler, or test. Drop it or mark it as a placeholder for later.
- GetMetaData = 0x19 sits between 0x1F and 0x20 in the enum. The TryFrom is in order now but the declaration still isn't.
Added support for ActivatePendingComponent for out-of-transport image transfer.