Add serial transport

[Thom-de-Jong]

Added a serial transport layer.
This required a little refactoring of the Flashing struct but nothing to serious.

Tested on a CH32V203C8T6 dev-board with FTDI FT232RL usb-to-serial adapter on windows and linux.

[basilhussain]

I would love to see this functionality added. It would be very useful to be able to use wchisp with the popular CH32V003 line, which only support UART with the factory bootloader. It'll probably also facilitate support for the recently-announced '002, '004, and '006 chips,

This is actually a real coincidence, as I had been looking at the code myself recently to see what it would take to add serial support (and also to see if the serial protocol matches the USB), but concluded I don't have enough Rust experience to do it myself. And then this is posted today! :)

By the way, a note of caution about the CH32V003 factory bootloader: it ignores the 'sectors' parameter of erase commands, because it only supports doing a full erase. (I guess they ran out of space in the BOOT flash to properly support 1K sector erase. Perhaps the newer '00x chips with their 3K bootloader will properly support it.) What this means is that when wchisp is doing a flash operation and attempts to erase only enough sectors to accommodate the given binary, instead every sector will always be erased. This should probably be documented somewhere if this serial support is to be added.

[Thom-de-Jong]

The serial transport layer has been improved to no longer wait until all bytes are received. (The response contains the data length)
And I added the "-b" option to set the baud rate used by the transport layer.
At this point everything seems to work properly on the CH32V203.

@basilhussain I have not updated any documentation yet, but that is something important to mention.

[basilhussain]

@Thom-de-Jong I think something got messed up with your last commit. It shows every line of each file as having changed. I'd guess something to do with line endings, as ignoring whitespace when viewing diffs shows something more sensible.

[basilhussain]

I attempted to build and test this on my Windows system. Everything appeared to build successfully, but am getting crashes when I try and do anything significant (such as get chip info).

I can successfully probe for serial ports:

C:\Users\Basil\Documents\GitHub\wchisp>target\debug\wchisp.exe -t serial probe
21:14:15 [INFO] Found 2 ports:
21:14:15 [INFO]         COM1
21:14:15 [INFO]         COM4
21:14:15 [INFO] hint: use `wchisp info` to check chip info

But getting chip info, for example, crashes:

C:\Users\Basil\Documents\GitHub\wchisp>target\debug\wchisp.exe -v -t serial -p COM4 info
20:48:50 [DEBUG] (1) wchisp::transport: => c50400   00c20100
thread 'main' panicked at /rustc/3f5fd8dd41153bc5fdca9427e9e05be2c767ba23\library\core\src\iter\traits\accum.rs:149:1:
attempt to add with overflow
stack backtrace:
   0: std::panicking::begin_panic_handler
             at /rustc/3f5fd8dd41153bc5fdca9427e9e05be2c767ba23/library\std\src\panicking.rs:652
   1: core::panicking::panic_fmt
             at /rustc/3f5fd8dd41153bc5fdca9427e9e05be2c767ba23/library\core\src\panicking.rs:72
   2: core::panicking::panic_const::panic_const_add_overflow
             at /rustc/3f5fd8dd41153bc5fdca9427e9e05be2c767ba23/library\core\src\panicking.rs:179
   3: core::ops::arith::impl$1::add
             at /rustc/3f5fd8dd41153bc5fdca9427e9e05be2c767ba23\library\core\src\ops\arith.rs:105
   4: core::ops::arith::impl$20::add
             at /rustc/3f5fd8dd41153bc5fdca9427e9e05be2c767ba23\library\core\src\internal_macros.rs:47
   5: core::iter::traits::accum::impl$30::sum::closure$0<core::slice::iter::Iter<u8> >
             at /rustc/3f5fd8dd41153bc5fdca9427e9e05be2c767ba23\library\core\src\iter\traits\accum.rs:75
   6: core::slice::iter::impl$182::fold<u8,u8,core::iter::traits::accum::impl$30::sum::closure_env$0<core::slice::iter::Iter<u8> > >
             at /rustc/3f5fd8dd41153bc5fdca9427e9e05be2c767ba23\library\core\src\slice\iter\macros.rs:230
   7: core::iter::traits::accum::impl$30::sum<core::slice::iter::Iter<u8> >
             at /rustc/3f5fd8dd41153bc5fdca9427e9e05be2c767ba23\library\core\src\iter\traits\accum.rs:72
   8: core::iter::traits::iterator::Iterator::sum<core::slice::iter::Iter<u8>,u8>
             at /rustc/3f5fd8dd41153bc5fdca9427e9e05be2c767ba23\library\core\src\iter\traits\iterator.rs:3581
   9: wchisp::transport::serial::impl$1::send_raw
             at .\src\transport\serial.rs:53
  10: wchisp::transport::Transport::transfer_with_wait<wchisp::transport::serial::SerialTransport>
             at .\src\transport\mod.rs:29
  11: wchisp::transport::Transport::transfer<wchisp::transport::serial::SerialTransport>
             at .\src\transport\mod.rs:23
  12: wchisp::flashing::Flashing::new_from_serial
             at .\src\flashing.rs:79
  13: wchisp::main
             at .\src\main.rs:203
  14: core::ops::function::FnOnce::call_once<enum2$<core::result::Result<tuple$<>,anyhow::Error> > (*)(),tuple$<> >
             at /rustc/3f5fd8dd41153bc5fdca9427e9e05be2c767ba23\library\core\src\ops\function.rs:250
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

By the way, I have a suggestion: is it possible to change the permitted baud rate option values so that instead of "baud115200", "baud1m", "baud2m" they are just "115200", "1m", "2m"?

Also - and forgive me if this is a stupid question - what's the actual need for a baud rate option? Surely a given device's serial bootloader only works at a specific, fixed baud rate? I'm not familiar with other CH32V chips, but for example the CH32V003 only works at 115200. Wouldn't it be more appropriate to have the relevant baud rate picked from the device info?

[Thom-de-Jong]

@basilhussain Thank you for trying it out!
The full file change are indeed white-spaces or newlines... Do you know how to fix that?

By the way, I have a suggestion: is it possible to change the permitted baud rate option values so that instead of "baud115200", "baud1m", "baud2m" they are just "115200", "1m", "2m"?

Unfortunately enums in Rust must start with an alphabetic or _ character, so "baud" is in my opinion a reasonably short option.

Surely a given device's serial bootloader only works at a specific, fixed baud rate?

I'm using an enum because the WCHISP_Tool gives the option between 115200, 1m and 2m for my chip (CH32V203). We could allow the user to put in a numeric value but I don't know if the bootloader would accept anything other than these baud rates. (I don't check the response at the moment)
Checking the response to the "set_baud" command could be possible, I will look into that.
2m does not seem to speed up the upload anymore than the 1m option.

The build-fail is fixed by not relying on libudev-sys.
The panic in the debug build is fixed by explicitly specifying that we want a wrapping addition. (which is the default in release builds)

[andelf]

The full file change are indeed white-spaces or newlines... Do you know how to fix that?

If you do not have a "rustfmt.toml" in your home dir, cargo fmt will fix all that.

Unfortunately enums in Rust must start with an alphabetic or _ character, so "baud" is in my opinion a reasonably short option.

clap's ValueEnum can be customized, see https://docs.rs/clap/latest/clap/trait.ValueEnum.html

[basilhussain]

I rebuilt with the latest changes and it appears to work fine for me now. Tested with a WCHLink-E as USB-UART adapter and CH32V003F4P6.

C:\Users\Basil\Documents\GitHub\wchisp>target\debug\wchisp.exe -t serial -p COM4 info
16:10:21 [WARN] Find chip via alternative id: 0x33
16:10:21 [INFO] Chip: CH32V003*4*6[0x3021] (Code Flash: 16KiB)
16:10:21 [INFO] Chip UID: CD-AB-35-23-48-BC-4A-8B
16:10:21 [INFO] BTVER(bootloader ver): 02.30
16:10:21 [INFO] Current config registers: a55af708ff00ff00ffffffff00020300cdab352348bc4a8b

C:\Users\Basil\Documents\GitHub\wchisp>target\debug\wchisp.exe -t serial -p COM4 reset
16:11:07 [WARN] Find chip via alternative id: 0x33
16:11:07 [INFO] Device reset

C:\Users\Basil\Documents\GitHub\wchisp>target\debug\wchisp.exe -t serial -p COM4 flash "C:\Users\Basil\Desktop\******.hex"
16:12:17 [WARN] Find chip via alternative id: 0x33
16:12:17 [INFO] Chip: CH32V003*4*6[0x3021] (Code Flash: 16KiB)
16:12:17 [INFO] Chip UID: CD-AB-35-23-48-BC-4A-8B
16:12:17 [INFO] BTVER(bootloader ver): 02.30
16:12:17 [INFO] Current config registers: a55af708ff00ff00ffffffff00020300cdab352348bc4a8b
16:12:17 [INFO] Read C:\Users\Basil\Desktop\******.hex as IntelHex format
16:12:17 [INFO] Firmware size: 1024
16:12:17 [INFO] Erasing...
16:12:17 [WARN] erase_code: set min number of erased sectors to 8
16:12:17 [INFO] Erased 8 code flash sectors
16:12:18 [INFO] Erase done
16:12:18 [INFO] Writing to code flash...
██████████████████████████████████████████████████████████████████████████████████████████████████████████████ 1024/1024
16:12:18 [INFO] Code flash 1024 bytes written
16:12:18 [INFO] Verifying...
██████████████████████████████████████████████████████████████████████████████████████████████████████████████ 1024/1024
16:12:18 [INFO] Verify OK
16:12:18 [INFO] Now reset device and skip any communication errors
16:12:18 [INFO] Device reset

C:\Users\Basil\Documents\GitHub\wchisp>target\debug\wchisp.exe -t serial -p COM4 verify "C:\Users\Basil\Desktop\******.hex"
16:12:42 [WARN] Find chip via alternative id: 0x33
16:12:42 [INFO] Read C:\Users\Basil\Desktop\******.hex as IntelHex format
16:12:42 [INFO] Firmware size: 1024
16:12:42 [INFO] Verifying...
██████████████████████████████████████████████████████████████████████████████████████████████████████████████ 1024/1024
16:12:42 [INFO] Verify OK

I note the 'info' command doesn't output any breakdown of the option bytes like for other chips. I assume this is because the devices/0x21-CH32V00x.yaml file doesn't have anything in the config_registers section? I might have a go at fixing that. Also maybe separate out the '00x variants rather than grouping them all into a single 'CH32V003*4*6' entry.

[basilhussain]

@Thom-de-Jong I have made improvements to the device info YAML for CH32V00x and submitted PR against your fork, as I think these changes should go together.

[Thom-de-Jong]

I have checked if it is possible to use other baud rates and it seems to be possible to use any baud rate, as long as it is a reasonable rate (see Reference Manual 18.3) less than 2m. (On the ch32v203 at least)
Working examples are: 460800, 571428 and even 1945945.
Notable exceptions are that common baudrates like 921600 do not work...
I don't know what clock devider settings the bootloader uses.

The response is always 55 aa c5 00 02 00 00 00 c7 so the bootloader does not reject requests for non-working baudrates.

Should we let users specify the u32 they want to use or keep the enum with baudrates?
What do you think? @basilhussain @andelf

[basilhussain]

I suspect the need for users to specify custom baud rates would be very minor, for two reasons. Firstly, not all serial bootloaders on all chips support the 'set baud' command, so the capability is pointless for those that don't. Secondly, I would think the primary motivation to use a different baud other than the default 115200 is to facilitate faster downloads, which is served by choosing one of the faster presets; in that scenario, why wouldn't you just go for the fastest preset?

Further, if you've found that arbitrary bauds are not universally functional, then it's probably just inviting people to blame wchisp if their chosen rate doesn't work. 😛

So my opinion is that I don't think it's warranted.

[basilhussain]

I thought I would just try and see what happens if you attempt to talk to a CH32V003 with 1M or 2M baud rate, and the result is that I think something is not working as expected.

The initial 0xC5 set baud command gets sent at the expected 115200, and the bootloader as expected responds with a failure result (0xFE), but I'm not getting the expected "set_baud failed" error message. Instead I'm getting:

C:\Users\Basil\Documents\GitHub\wchisp>target\debug\wchisp.exe -v -t serial -p COM4 -b 1M info
19:52:51 [DEBUG] (1) wchisp::transport: => c50400   40420f00
19:52:51 [DEBUG] (1) wchisp::transport: <= c5d70200 fe00
Error: A device attached to the system is not functioning.

As I've said before, I'm a Rust noob, so I may be wrong, but it seems like it's failing to recognise that the command responded with failure (flashing.rs line 80), and then also failed somehow to set the baud rate on the serial port (line 81)?

I'm not sure whether the latter isn't actually in my case fault of the WCH-LinkE adapter, because in separate testing trying to transmit at 1M through Tera Term, the max available preset listed in Tera Term for that port is 921600 (although I'm not sure where TT gets these listed presets). Manually forcing 1M gets me a TX bit rate that's inaccurate. The CH32V305 on the WCH-LinkE doesn't use an external crystal oscillator for clock (HSI only I presume), so it's probably unsurprising it's inaccurate at higher bauds, and why the COM port may not support that.

In which case, perhaps I should take back my previous comment about support for arbitrary baud rates, because it appears even WCH's own programming tools don't support the current fixed presets! 😅

[Thom-de-Jong]

Based on WCHISPStudio I think the current implementation is fine, 115200 is supported by every device with serial programming support so I made that the default on fail. Others support 1m and 2m. Lets keep it this way for now, as choosing the baudrate just makes things a little faster, as you said.

[basilhussain]

Sorry to be the bearer of bad news, but the baud setting code is still not working quite right.

It's not failing anymore when the bootloader doesn't successfully acknowledge setting a different baud rate, but I'm not getting the expected "Custom baudrate not supported by the current chip" message:

C:\Users\Basil\Documents\GitHub\wchisp>target\debug\wchisp.exe -v -t serial -p COM4 -b 1M info
01:41:07 [DEBUG] (1) wchisp::transport: => c50400   40420f00
01:41:07 [DEBUG] (1) wchisp::transport: <= c5d70200 fe00
01:41:07 [DEBUG] (1) wchisp::transport: => a11200   00004d4355204953502026205743482e434e
01:41:07 [DEBUG] (1) wchisp::transport: <= a1d70200 3021
01:41:07 [DEBUG] (1) wchisp::transport: => a11200   00004d4355204953502026205743482e434e
01:41:07 [DEBUG] (1) wchisp::transport: <= a1d70200 3021
01:41:07 [DEBUG] (1) wchisp::flashing: found chip: CH32V003F4P6[0x3021]
01:41:07 [DEBUG] (1) wchisp::transport: => a70200   1f00
01:41:07 [DEBUG] (1) wchisp::transport: <= a7d71a00 1f00a55af708ff00ff00ffffffff00020300cdab352348bc4a8b
01:41:07 [DEBUG] (1) wchisp::flashing: read_config: a55af708ff00ff00ffffffff00020300cdab352348bc4a8b
01:41:07 [INFO] Chip: CH32V003F4P6[0x3021] (Code Flash: 16KiB)
01:41:07 [INFO] Chip UID: CD-AB-35-23-48-BC-4A-8B
01:41:07 [INFO] BTVER(bootloader ver): 02.30
01:41:07 [DEBUG] (1) wchisp::transport: => a70200   1f00
01:41:07 [DEBUG] (1) wchisp::transport: <= a7d71a00 1f00a55af708ff00ff00ffffffff00020300cdab352348bc4a8b
01:41:07 [INFO] Current config registers: a55af708ff00ff00ffffffff00020300cdab352348bc4a8b
...

I think the problem may be that resp.is_ok() doesn't do what you think it does. Unless I'm wrong, I can't see anywhere Response actually checks the values in a response payload. All it seems to do is check that the given length matches the amount of data received. So, the is_ok() method is just checking whether the response is well formed (which an error response to the set baud command is), rather than the value of response.

Also, isn't the logic for calling transport.set_baudrate() backwards? It only gets called if the bootloader fails to accept change of baud rate. I'm guessing it should be in an else clause instead.

[basilhussain]

I have an idea. Would it be possible to have the chunk size specified on a per-transport level?

Perhaps make it an abstract property/method of Transport trait to be implemented by USB and Serial transports, and have that value be used by all methods in Flashing instead of hard-coded constant.

If so, I propose to make it 56 bytes for USB, and 64 bytes for serial, for reasons I'll explain below.

---

I note that both WCHISPTool and wchisp use a 56-byte chunk size when transferring data to be written or verified. I believe (correct me if I'm wrong) that this stems from the need to fit such data, minus overheads, into 64-byte USB packets. However, the serial UART transport does not suffer from such an inherent limitation.

Using a chunk size of 56 bytes is actually fairly inefficient due to it not being an exact division of typical flash page sizes (e.g. 64 bytes for CH32V003, 256 bytes for CH32V30x). I know from studying the CH32V003 bootloader that it uses a buffering mechanism so that it is (wherever possible) able to write a full flash page at a time, and I expect bootloaders of other chips will be similar. And so because this chunk size is less than page size, it always takes at least two commands to send the data for every full page write.

From study, and through experimentation (I changed const CHUNK: usize = 56 to 64 in flashing.rs), I have found that the CH32V003 bootloader is perfectly capable of handling 64-byte chunk sizes, and I wouldn't be surprised if bootloaders on other chips are similar in capability.

In the case of the CH32V003, making the chunk size 64 bytes can make significant reductions in the number of write commands needed to download a firmware image, because essentially the buffering mechanism is side-stepped; each write command results in the data immediately being written, with no waiting for a subsequent command to fill up data to a full page.

In the case of a CH32V30x with 256KB flash, even though the page size is larger (and thus would still require multiple commands to fill a page for writing), it could be up to a massive 586 fewer write (and verify!) commands (4682 vs 4096)! Assuming the '30x bootloader supports 64-byte chunks, that is.

---

To finish, while I think this could be a worthwhile change, I do think there are some questions that need definite answers before such a modification is considered:

• Do other chips' bootloaders actually support 64-byte chunks over serial transport? (I would look, but I have no samples myself.)
• Some other chips (e.g. CH58x) feature 'EEPROM', a.k.a. 'DataFlash'. Can/should the increased chunk size also apply to those commands?
• What's the deal with the dump_eeprom command currently using a chunk size of 58 instead of 56? Could this happily be made to conform to a common chunk size of 56?

[Thom-de-Jong]

That would probably be a reasonable thing to do. I don't know much about the workings of the bootloader.
Is there any documentation on the bootloader?
And should this be in this PR or in its own PR?

[basilhussain]

I can answer one of my own questions now:

Do other chips' bootloaders actually support 64-byte chunks over serial transport?

I managed to get hold of a dump of the bootloader from a CH32V30x and after some investigation it appears that it only allocates 64-byte buffers in RAM for storing both command and response payloads (i.e. all data not including serial header bytes and checksum). This is independent of transport in use (USB or UART).

This means that command chunk size can only be a maximum of 61 bytes (64 minus 3 bytes overhead) on the CH32V30x, so my idea will unfortunately not be possible. 😞

Unless... we can make chunk size specified per device... 🤔

@Thom-de-Jong I am not aware of any documentation from WCH about how the bootloaders on CH32V chips work or the communications protocol. Not even in Chinese. The only things I've seen are guides on how to use their ISP software. I assume all the work for wchisp was done based on reverse-engineering WCHISPTool's behaviour.

Yes, perhaps leave it for now and make a future PR if it turns out to be feasible to implement.