Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

updata CH347 #424

Merged
merged 8 commits into from
Feb 28, 2024
Merged

updata CH347 #424

merged 8 commits into from
Feb 28, 2024

Conversation

ZhiyuanYuanNJ
Copy link
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.
src/ch347jtag.cpp Outdated Show resolved Hide resolved
src/ch347jtag.cpp Outdated Show resolved Hide resolved
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
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
Contributor Author

Choose a reason for hiding this comment

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

Changed.

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
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
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
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
Contributor Author

Choose a reason for hiding this comment

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

Changed.

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
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
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
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
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.

src/ch347jtag.cpp Outdated Show resolved Hide resolved
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
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
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
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
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
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