Fix logic error that loses the remainder chunk

[jerrysxie]

No description provided.

[Copilot]

Pull request overview

This PR fixes a critical logic error in the firmware update chunk processing that prevented the last chunk with remainder bytes from being sent. The old code had a match condition num if (num < num_chunks) that would always be true for all loop iterations (0..num_chunks), causing the remainder handling branch to never execute.

Key changes:

• Fixed the match condition from num < num_chunks to num < num_chunks - 1 to properly reach the final chunk handling
• Refactored byte fetching to occur once per iteration outside the match statement
• Separated remainder chunk handling into a dedicated block after the main loop
• Added proper response status checking for each chunk

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

When processing the last chunk with a remainder (lines 138-162), the data_length field in the FwUpdateContentHeader will be incorrectly set to DEFAULT_DATA_LENGTH instead of the actual remainder size. The process_last_data_block function at line 234 always sets data_length to DEFAULT_DATA_LENGTH, but when called with a partial chunk, it should reflect the actual number of valid bytes (remainder). This could cause the device to process more bytes than actually provided or fail validation.

[jerrysxie]

@madeleyneVaca @Ctru14 @ankurung It seems like in process_last_data_block(), it is always assuming data length to be DEFAULT_DATA_LENGTH:

    async fn process_last_data_block(
        &mut self,
        w: &mut W,
        chunk: DataChunk,
        seq_num: usize,
    ) -> Result<FwUpdateContentResponse, CfuWriterError> {
        let cmd = FwUpdateContentCommand {
            header: FwUpdateContentHeader {
                flags: FW_UPDATE_FLAG_LAST_BLOCK,
                sequence_num: seq_num as u16,
                data_length: DEFAULT_DATA_LENGTH as u8, <----------------------------------------
                firmware_address: 0,
            },
            data: chunk,
        };
        let cmd_bytes: [u8; 60] = (&cmd).into();
        let offset = seq_num * DEFAULT_DATA_LENGTH;
        let mut resp_buf = [0u8; core::mem::size_of::<FwUpdateContentResponse>()];
        w.cfu_write_read(Some(offset), &cmd_bytes, &mut resp_buf)
            .await
            .map_err(|_| CfuWriterError::StorageError)?;

        FwUpdateContentResponse::try_from(resp_buf).map_err(|_| CfuWriterError::ByteConversionError)
    }

So even if there is fewer bytes, write a whole chunk?

[Copilot]

When the total image size is smaller than DEFAULT_DATA_LENGTH (num_chunks = 0, remainder > 0), the remainder chunk is processed as a last block without the first block flag. Since no iterations occur in the loop (0..0), the single chunk is sent only with the LAST_BLOCK flag at line 153, missing the required FIRST_BLOCK flag. For a single-chunk transfer, both flags should likely be set. Consider handling this edge case by checking if num_chunks == 0 and setting both flags accordingly.

Suggested change

	let r = self
	.process_last_data_block(writer, last_chunk, num_chunks)
	.await
	.map_err(CfuProtocolError::WriterError)?;
	let r = if num_chunks == 0 {
	// Single-chunk transfer: treat this remainder as the first (and only) block
	self.process_first_data_block(writer, last_chunk).await
	} else {
	// There were full-sized chunks before; this is the final (last) block
	self.process_last_data_block(writer, last_chunk, num_chunks).await
	}
	.map_err(CfuProtocolError::WriterError)?;

[jerrysxie]

@madeleyneVaca @Ctru14 @ankurung Not sure what the correct expectation is here if an image is <= chunk length, marked it as both first and last? I will add check to enforce that there are at least 2 chunks worth of data right now.