Read programmer serial number from libusb or hidusb #1223
Conversation
There was a problem hiding this comment.
Review of usb_hidapi.c
- Not sure whether the serial number access function should have a 14 character limit (plus
nul); I generally prefer allocated mem:static const char *usbsn = ""; const char *usbhid_get_serno() { return usbsn; } - The return value of
hid_get_serial_number_string()should be checked for error and only on success the wide character string converted, elseusbsnshould remain"" wcstombs((char *) usbsn, sn, 15);has no error checking either; all sorts of unterminated strings could ensue; better replace the code with sth like untested(!) and [edited(!)]wchar_t sn[1234]; if(hid_get_serial_number_string(dev, sn, sizeof sn/sizeof*sn) == 0) { size_t n = wcstombs(NULL, sn, 0) + 1; if(n) { char *cn = cfg_malloc(__func__, n); if(wcstombs(cn, sn, n) != (size_t) -1) usbsn = cache_string(cn); free(cn); } }
I'd suggest similar changes for usb_libusb.c. In particular, the line is problematic, as it could lead to an unterminated string. I suggest replacing it with
usbsn = cache_string(string);
Other than that:
- The comment might read Reference or Get not Store; the same in
stk500v2.c - This condition might be
if(pgm->usbsn && *pgm->usbsn); the same instk500v2.c - Unrelated to this PR but potentially still a problem: What makes us sure
statusin this line is 3 or more? If it isn't thememmove()blows up, so I'd recommend a sanity check with errorif(status<3)the line before
|
Thank you very much @stefanrueger for the feedback and review!
I have no idea where the number 3 comes from, but I'll add a sanity check just to be safe.
Out of curiosity, isn't it problematic to not allocate a fixed amount of memory that usbsn can point to? Isn't there a theoretical chance that writing a "long" string starting from address usbsn points to may cause memory corruption? Thanks for suggesting this! I've tested it, and it works great. However, a USB serial number is limited to 126 bytes by the USB spec, so there shouldn't be any need to create an array with room for 1234 wide characters. |
|
I just pushed a fix for issue #1212, since this is loosely related to this PR. However, the formatting has been and still is a bit "off". The colons don't line up, and it also prints out clock settings that's not relevant to the target: PICkit4 (should really just print the ISP clock): Xplained Mini ATmega328PB: |
|
Yes the ISP mode works now. |
|
| no idea where the number 3 comes from Nor do I, but | isn't it problematic to not allocate a fixed amount of memory that usbsn can point to? Only if the code doesn't allocate the correct amount of memory needed at runtime; |
|
| a USB serial number is limited to 126 bytes by the USB spec Then |
But this means that there are several other places where |
|
| several other places where ... or wrong, indeed. I almost wish I hadn't looked that closely, but there you go. Here my trawl through
Those pesky sanity checks might not be needed but I could not find the piece of code that would guarantee Anyway, if you could cross-check my thinking and carefully see how to mend the problems above that would be great, even though it's nothing to do with your PR (I normally fix stuff en passant when I go about other goals in my PR's lest I forget later on). |
|
Thanks for looking through the code! I've just pushed a commit with your suggestions. They all sound very reasonable and shouldn't break anything. The code in stk500v2.c and jtag3.c isn't all that nice to look at, with "magic numbers" everywhere and little to no comments on what's going on. The formatting in these files is also all over the place, with a messy mix of tabs and spaces.
The last link doesn't point to anything obvious. Could you clarify?
It's true that this is at the wrong place, but I can't figure out how to get it right. The problem is the typical warning, where resp may be initialized. if you have a suggestion on how it can be done properly, I'm all ears! |
|
Excellent, we are converging!
|
|
| messy mix of tabs and spaces I agree; there is a school of thought that these things should be handled as separate PRs so they don't obfuscate the PR at hand. So maybe a good idea to remove the tabs and trainling white space after v7.1 has been released and doing so in a separate PR. @MCUdude You are particularly disadvantaged as your favourite tab expansion is 4 spaces whilst it's almost always 8 spaces everywhere else incl avrdude source. BTW, I found a great trick to review PRs that change white space; you can tick hide whitespace for web reviews (and you can use
|
I think we can standardized on 4 space after the 7.1 release which seems to be the norm nowadays. |
Well, ~95% of the codebase are already indented with two spaces. Why change to 4? |
I see. Yes that is the way to go then. |
and out-of-scope error
|
Whilst it might look good to generate a uniform formatting, there are multiple disadvantages:
| I was thinking about replacing the remaining tabs with two space indents That won't work. Sometimes, the program is 4-spaces indented and the spacing consists of a mixture of 8-space tabs with the remainder being spaces. |
|
To cut a long short: The messy mix of tabs and spaces hinders readability of the code; I support expanding tabs to spaces, eg, using expand -i in a separate PR. Other than this, I'd caution against overzealous policing, enforcing and changing formatting (unless some gratuitous formatting can be changed at the same time the code needs to change anyway). |
|
I have taken to check compiler warnings as last step of PRs that I carry out. In this case there are rather a lot (not caused by you, @MCUdude). OK if I push them on to your PR, or do you prefer I do these at another time, perhaps after v7.1. These pesky signed/unsigned warnings require a detailed look in each case.
|
Is the list of warnings much longer than what's shown in your PR? I'm fine with you pushing a fix for all these working to my branch. |
Thanks for the commit @stefanrueger! The commit(s) appear very straightforward, and just for kicks, I tested it using an Atmel ICE in ISP mode. Everything seems to work as it should, so I think this PR is ready to be merged unless @mcuee finds any irregularities. |
|
The formating of Test results for xplainedmini_tpi (ATtiny104 Xplained Nano) and xplainedmini_updi (ATtiny416 Xplained Nano).
|
|
More test results: pkobn_updi (AVR128DB48 Curiosity Nano) and xplainedmini ISP mode (ATmega328PB Xplained MINI) |
|
More test results for PICKit 4 (UPDI and ISP) and SNAP. |
|
More test results: Xplained Pro UPDI mode (ATTiny817 Xplained Pro) |
|
Other than the minor issue of You can see I purchased quite some Microchip tools this year and they cost a bit of fortune. :-) I still have a few other Curiosity Nano board but they use the same The PICKit 4 and ATtiny817 Xplained Pro are on loan from Microchip Singapore. I will probably return them in Jan 2023. |
Now the target voltage is printed out along with the HW version, FW version and serial number, and they all allign nicely. When in ISP mode, the "SCK period" value is printed _after_ the target voltage readout, and _before_ the additional clocks the programmer holds, such as JTAG, PDI and UPDI clocks.
Now everything (finally) lines up nicely, and only relevant clock info are printed for the connected target
|
@mcuee and @stefanrueger I've reformatted the programmer info part, so now everything should line up nicely, even in terminal mode. This is what it looks like on a Power Debugger, a PICkit4 and an Xplained Nano TPI: Power debugger (JTAG and ISP)PICkit4 (ISP, PDI and UPDI)Xplained Nano ATtiny104 (TPI) |
|
Yes this PR is good to go now. |
This PR makes it possible to read the serial number from JTAG3-compatible programmers using libusb or hidusb.
Without this PR, Avrdude is not able to read the serial number from the AVRISPmkII, PICkit4, or SNAP.