Skip to content

updata CH347#424

Merged
trabucayre merged 8 commits into
trabucayre:masterfrom
ZhiyuanYuanNJ:master
Feb 28, 2024
Merged

updata CH347#424
trabucayre merged 8 commits into
trabucayre:masterfrom
ZhiyuanYuanNJ:master

Conversation

@ZhiyuanYuanNJ
Copy link
Copy Markdown
Contributor

Added CH347F, which is a new package version of CH347T.
BUG fix: Sometimes writing bit files may fail.
Added functions: Added VID, PID and other settings, the default selected model is CH347T.

Added CH347F, which is a new package version of CH347T.
BUG fix: Sometimes writing bit files may fail.
Added functions: Added VID, PID and other settings, the default selected model is CH347T.
Comment thread src/ch347jtag.cpp Outdated
Comment thread src/ch347jtag.cpp Outdated
Comment thread src/ch347jtag.cpp Outdated
Comment on lines +162 to +180
if (bus_addr != 0 && dev_addr != 0) {
cnt = libusb_get_device_list(NULL, &devs);
if (cnt < 0) goto err_exit;
while ((dev = devs[i++]) != NULL) {
if (libusb_get_device_descriptor(dev, &desc) < 0){
continue;
}
if (desc.idVendor == CH347_VID && desc.idProduct == CH347_PID && libusb_get_bus_number(dev) == bus_addr && \
libusb_get_device_address(dev) == dev_addr){
if (libusb_open(dev, &dev_handle) < 0){
printError("libusb init failed");
goto err_exit;
}
}
}
}else {
dev_handle = libusb_open_device_with_vid_pid(usb_ctx, CH347_VID,
CH347_PID);
}
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code used here to search for a device using bus/dev addr is quite similar to libusb_open_device_with_vid_pid implementation. It's maybe possible to merge both to:

  1. for each device compare vid/pid
  2. if bus_addr and dev_addr are provided do comparison too

Something similar to:

[...]
if (desc.idVendor != CH347_VID || desc.idProduct != CH347_PID)
    continue;
if (bus_addr != 0 && dev_addr != 0 && (libusb_get_bus_number(dev) != bus_addr || libusb_get_device_address(dev) != dev_addr))
    continue;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed.

Comment thread src/ch347jtag.cpp Outdated
Comment on lines +248 to +251
uint32_t speed_clock[8] = {
KHZ(468.75), KHZ(937.5), MHZ(1.875), MHZ(3.75),
MHZ(7.5), MHZ(15), MHZ(30), MHZ(60)
};
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clock speed/index depends on device (STANDARD_PACK and LARGER_PACK). This must be keep in mind/used here

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I don't understand what you mean STANDARD_PACK.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems my previous comment was lost.
https://github.com/YTEC-info/CH347-Softwares/blob/main/Datasheet%20%26%20Manual/WCH-CH347-JTAG-Interface(1.3).pdf
explains according to model and firmware the is two mode.
Each of them as specific frequencies.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed.

Comment thread src/ch347jtag.cpp
Comment thread src/ch347jtag.cpp
Comment on lines +424 to +429
for (unsigned i = 0; i < size; ++i) {
if (ibuf[3 + i] == 0x01) {
*rptr |= (0x01 << i);
}else{
*rptr &= ~(0x01 << i);
}
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here buffer received is in bit mode: one byte received contains the value for on state of TDO.
So each byte must be reconstructed.
Also rptr is never updated so you always write at the same position.
Why replacing original code?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This represents the value of each bit on the last byte of TDO, and the value of the rptr binary bit has been changed.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

*rptralways point to the same adress, you have to increment memory position each 8bits.
Also you shift using i, if size == 256the bit will be lost because*rptr` is a 8bit.
Again I don't see (but maybe I'm wrong) any issue with the original code.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The size here will not be greater than 8. If chip returns 10bit data 0b1100111101, then 347 will be packaged as D4 00 01 CF D2 02 00 00 01, so according the original code size(here is 2) / 8, the last 2 bits will be missed.

Comment thread src/ch347jtag.cpp Outdated
Comment thread src/ch347jtag.cpp Outdated
Comment on lines +159 to +172
if (bus_addr != 0 && dev_addr != 0) {
cnt = libusb_get_device_list(NULL, &devs);
if (cnt < 0) goto err_exit;
while ((dev = devs[i++]) != NULL) {
if (desc.idVendor != vid || desc.idProduct != pid)
continue;
if (bus_addr != 0 && dev_addr != 0 && (libusb_get_bus_number(dev) != bus_addr || libusb_get_device_address(dev) != dev_addr))
continue;
libusb_open(dev, &dev_handle);
break;
}
}else {
dev_handle = libusb_open_device_with_vid_pid(usb_ctx, vid, pid);
}
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact here my idea is to remove libusb_open_device_with_vid_pidand the test in l.159.

Copy link
Copy Markdown
Contributor Author

@ZhiyuanYuanNJ ZhiyuanYuanNJ Feb 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean like this?
image

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes!
maybe a break after libusb_open to only select the first when bus_addr and dev_addr are not provided?
Also libusb_free_device_list(devs, 1) must be used just after the loop and before if (!dev_handle) {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, thanks for pointing out the error.
image

@trabucayre trabucayre merged commit 4af0bf6 into trabucayre:master Feb 28, 2024
@trabucayre
Copy link
Copy Markdown
Owner

Applied. Thanks @ZhiyuanYuanNJ !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants