On Sat, 2020-05-30 at 16:30 -0700, Timo Kokkonen wrote: > > This is a new driver for the DC electronic loads made by ITECH. It > would seem ITECH is the OEM manufacturer of BK Precision 8500 series > loads (their programming specs are pretty much identical and units > look identical except logos), so those BK Precision units should work > with that driver "as is"...
How many sigrok drivers have you created before that? This one looks awesome. Instantly operational with all features that you could support for a new device, in a consistent style and with useful abstractions. Nice! Minor style nits remain, but these are easy to address. Haven't dug too deep (reason: see below), but am confident only little adjustment remains to be done before mainline integration. > I've been testing the driver using ITECH IT8511+ which reports itself > as "8511" via the (TTL) serial interface: > > # ./sigrok-cli -d itech-it8500:conn=/dev/ttyUSB1:serialcomm=38400/8n1 --scan > The following devices were found: > itech-it8500 - ITECH 8511 1.13 [S/N: 0717xxxxxx] with 3 channels: V1 I1 P1 Are models using different UART bitrates or frame parameters? If these are default values, the driver could encode them, and users need not specify them. Simplifies the invocation. But you might as well have provided them for demonstration. No issue there. > [ ... ] > > Patches are available at (it's split into two patches): > https://gist.github.com/tjko/492e147f3b84a8f2003da4616711b7f9 This suggests that you have the commits in a git repo, and have an account for a service. Instead of "hiding" the patches in a gist, would it not be easier to push the commits to a public repo? This would let others use either their browser or git or any other tool of choice, on either patches or resulting files. Would provide more options on the consumer's side and reduce the amount of work to get the content, and would help during review. > Any comments most welcome. I tried to follow the recommended > guidelines and used the existing (electronic load) drivers as model > for this driver. Things that come to mind: - Does your driver *depend* on the serial transport in the build instructions? So that it automatically gets deselected when an essential dependency is missing, and won't break the build. - You need not re-invent endianess conversion. Macros like RL16() have been around for ages, recently read_u16le() got added. - Check whitespace, sometimes operands and operators "run into each other", which is harder to read. - Check data types, use of "char" where bytes get processed is unexpected. Prefer C99 uint8_t etc types over "unsigned char" stuff to even better reflect intent? - Check text line length, and indentation depth. There may be different styles preferred by different persons. Let's hear what others say. I personally like to see the structure first and not indent continuation lines _that_ deep. - Check portability. Do you fill in structs and then send _these_ to the wire? Might be convenient but need not work everywhere. It's probably better to accept the tedium and properly stream requests and responses for improved reliability. Notice that the tedium is less with the recently introduced conversion helpers which also advance their position in the stream. This was unsorted, and from the top of my head. Might have another look when the patches are available in a git branch which is easier to handle than downloading patches from web pages. Have also learned that maintainers prefer to fetch from public repos instead of having patches sent when you plan to submit core more often on different subjects, or when you expect several iterations of review and improvement. Dealing with individual files many times quickly gets old. Just send a URL to the repo, that's easier. Are you aware of a wiki page for this load? Want to update or create one? Want "more interactive" discussion than what's possible in a mailing list? Many developers hang out in IRC. virtually yours Gerhard Sittig -- If you don't understand or are scared by any of the above ask your parents or an adult to help you. _______________________________________________ sigrok-devel mailing list sigrok-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/sigrok-devel
