Add commands for firmware upgrade & control IAP mode#84
Conversation
|
It would be great to add firmware upgrade support. |
Yes. I'll implement firmware upgrade option later. Should I integrate IAP control function and firmware upgrade function? |
Yes, please integrate them together. Switching IAP mode separately would leave wch-link in an intermediate state, |
…ices (#6) * Initial plan * Refactor library selection to support dynamic DLL loading for IAP and normal modes Co-authored-by: 21km43 <48169975+21km43@users.noreply.github.com> * Don't refresh device_index * Remove dll fallback --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: 21km43 <48169975+21km43@users.noreply.github.com> Co-authored-by: Koki Mizumoto <21km43@gmail.com>
Removed unreleased changes for IAP mode switch and WCH-LinkE firmware upgrade.
There was a problem hiding this comment.
Pull request overview
This pull request adds experimental firmware upgrade capabilities and IAP (In-Application Programming) mode control for WCH-Link/E devices. The implementation draws from community references and adds two new CLI commands: iap for entering/exiting IAP mode, and upgrade for flashing WCH-Link firmware.
Changes:
- Added IAP mode control commands (enter/quit) and firmware upgrade functionality
- Enhanced CH375 driver support to handle both WCHLinkDLL.dll and CH375DLL.dll for IAP mode devices on Windows
- Added new USB constants and endpoints for IAP mode device communication
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main.rs | Added CLI commands for IAP mode switching and firmware upgrade with appropriate warnings |
| src/probe.rs | Implemented IAP mode functions (enter, quit, erase, flash) with USB communication logic |
| src/usb_device.rs | Enhanced CH375 driver to support dual DLL loading and IAP mode device handling |
| src/commands/mod.rs | Added IapProgram enum for write/verify operations and updated command documentation |
| README.md | Updated feature list to include IAP mode switching and firmware upgrade capabilities |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| txbuf.fill(0); | ||
|
|
||
| // First 4 bytes: cmd, size, addr low, addr high | ||
| txbuf[0] = cmd; |
There was a problem hiding this comment.
The copy_size is cast to u8 which limits it to a maximum value of 255. However, the code initializes copy_size to 60 and only reduces it when near the end of the data. If the protocol is ever changed to support larger chunk sizes, this cast could silently truncate the value. While not currently a bug, consider adding an assertion or debug check to ensure copy_size never exceeds 255.
| txbuf[0] = cmd; | |
| txbuf[0] = cmd; | |
| debug_assert!( | |
| copy_size <= u8::MAX as usize, | |
| "copy_size ({}) exceeds u8::MAX ({})", | |
| copy_size, | |
| u8::MAX | |
| ); |
| Ok(()) | ||
| } | ||
|
|
||
| /// Switch IAP mode |
There was a problem hiding this comment.
The comment says "Switch IAP mode" but the function name is "iap_enter". The comment should be more specific, such as "Enter IAP mode" to match the function's purpose.
| /// Switch IAP mode | |
| /// Enter IAP mode |
| /// Switch IAP mode | ||
| // ref: https://github.com/cjacker/wlink-iap/blob/main/src/main.c | ||
| pub fn iap_enter(nth: usize) -> Result<()> { | ||
|
|
There was a problem hiding this comment.
There's an unnecessary blank line at the start of the function body. Remove it for consistency with the codebase style.
| log::info!("Quit IAP mode"); | ||
| let buf = [0x83, 0x02, 0x00, 0x00]; | ||
| log::trace!("send {} {}", hex::encode(&buf[..3]), hex::encode(&buf[3..])); | ||
| let _ = dev.write_endpoint(ENDPOINT_OUT_IAP, &buf); |
There was a problem hiding this comment.
The write_endpoint result is being ignored when sending the IAP quit command. If the write fails, the function will return Ok despite the mode switch command not being sent, potentially leaving the device in IAP mode. Consider propagating the error instead of silently ignoring it.
| let _ = dev.write_endpoint(ENDPOINT_OUT_IAP, &buf); | |
| dev.write_endpoint(ENDPOINT_OUT_IAP, &buf)?; |
| } else { | ||
| return Err(Error::ProbeNotFound); | ||
| }; | ||
|
|
There was a problem hiding this comment.
There's trailing whitespace at the end of this line. Remove it for consistency with the codebase style.
| WchLink::iap_quit(device_index)?; | ||
| } | ||
| Firmware::Sections(_sections) => { | ||
| log::error!("HEX firmware format not supported for WCH-Link upgrade"); |
There was a problem hiding this comment.
When the firmware format is HEX (Sections), the function only logs an error but doesn't return an error. This means the upgrade process will continue silently and quit IAP mode without actually upgrading the firmware, potentially leaving the device in an inconsistent state. The function should return an error to prevent the code from continuing to line 282 (quit IAP mode).
| log::error!("HEX firmware format not supported for WCH-Link upgrade"); | |
| log::error!("HEX firmware format not supported for WCH-Link upgrade"); | |
| return Err( | |
| wlink::Error::Custom( | |
| "HEX firmware format not supported for WCH-Link upgrade".into(), | |
| ) | |
| .into(), | |
| ); |
| .copy_from_slice(&data[offset..offset + copy_size]); | ||
|
|
||
| // Write or verify binary | ||
| let _ = dev.write_endpoint(ENDPOINT_OUT_IAP, &txbuf[0..4 + copy_size]); |
There was a problem hiding this comment.
The write_endpoint result is being ignored, which could hide critical errors during the IAP flash/verify operation. Since the subsequent read_endpoint is expected to return a response confirming the write was successful, a failure to write could lead to misleading error messages. The write_endpoint call should propagate errors.
| let _ = dev.write_endpoint(ENDPOINT_OUT_IAP, &txbuf[0..4 + copy_size]); | |
| dev.write_endpoint(ENDPOINT_OUT_IAP, &txbuf[0..4 + copy_size])?; |
| log::info!("Enter IAP mode"); | ||
| let buf = [0x81, 0x0f, 0x01, 0x01]; | ||
| log::trace!("send {} {}", hex::encode(&buf[..3]), hex::encode(&buf[3..])); | ||
| let _ = dev.write_endpoint(endp_out, &buf); |
There was a problem hiding this comment.
The write_endpoint result is being ignored when sending the IAP enter command. If the write fails but the device is not already in IAP mode, the function will return Ok despite the mode switch command not being sent. Consider propagating the error instead of silently ignoring it.
| let _ = dev.write_endpoint(endp_out, &buf); | |
| if let Err(e) = dev.write_endpoint(endp_out, &buf) { | |
| log::error!("Failed to send IAP enter command: {:?}", e); | |
| return Err(e); | |
| } |
| log::info!("Erase flash"); | ||
| let buf = [0x81, 0x02, 0x00, 0x00]; | ||
| log::trace!("send {} {}", hex::encode(&buf[..3]), hex::encode(&buf[3..])); | ||
| let _ = dev.write_endpoint(ENDPOINT_OUT_IAP, &buf); |
There was a problem hiding this comment.
The write_endpoint result is being ignored when sending the IAP erase command. If the write fails, the function will return Ok despite the erase command not being sent, potentially causing issues in the firmware upgrade process that expects the flash to be erased. Consider propagating the error instead of silently ignoring it.
| // First 4 bytes: cmd, size, addr low, addr high | ||
| txbuf[0] = cmd; | ||
| txbuf[1] = copy_size as u8; | ||
| txbuf[2] = (offset & 0xFF) as u8; | ||
| txbuf[3] = ((offset >> 8) & 0xFF) as u8; |
There was a problem hiding this comment.
The offset is encoded as a 16-bit value (using only 2 bytes at positions 2 and 3), which limits the maximum addressable offset to 65535 bytes. However, the function accepts data of any length without validating this constraint. If data.len() exceeds 65535 bytes, the offset will silently overflow and wrap around, causing firmware to be written to incorrect addresses. Add validation to ensure data.len() does not exceed 65535 bytes, or use a larger address field if the protocol supports it.
|
Leave the rebase work to me. I'll resolve them later. |
I read this issue and implemented commands which controls IAP mode.
Example (Controling iap mode):
Example (Upgrade firmware)