On Sat, 2022-04-30 at 16:07 -0700, Tom Hebb wrote:
>
> I submitted a pull request to libsigrok on GitHub last year that
> rewrites a significant portion of the FTDI-LA driver to make it more
> reliable, support new features on some chips, and drop support for other
> chips that never worked properly:
>
> https://github.com/sigrokproject/libsigrok/pull/145
I'd love to see this go mainline. The cleanup of build
dependencies, more reliable operation, extended support for more
device features, and performance improvements are impressive.
Apart from minor style nits that are easy to fix, more important
concerns of mine are these:
Don't mix the change of session feed submission with the change
of the physical transport (commit fdc2f4d7a0d0). If something in
the layout of sample memory changes or if there is another reason
to touch the feed logic, see if you can adjust the feed logic in
a first commit (and just the feed, nothing else), before changing
the physical transport and doing a little bit of feed logic
adjustment which _is_ motivated by that physical transport
change. This assumes that my reading of this change is correct.
I might have missed something, the change is quite complex.
Keep the traditional api.c and protocol.c responsibilities.
Current reviewers and future developers expect to see these,
especially during maintenance of a larger code base with multiple
drivers. By convention api.c cares about "the sigrok side", which
is device presence detection and identification during scan,
configuration, acquisition control, etc. While protocol.c cares
about "the device side" of things, the device firmware's protocol
and its dialects, the physical transport, the mapping of
application requests to device communication, the interpretation
of response data. Keep the acquisition start and stop in api.c,
especially when you reintroduce and extend them after you deleted
them. Of course the api.c routine can use protocol.c routines to
do its thing.
Can you see if the math helpers (rounding) should move to common
code, so that other drivers will benefit from having them, and
knowing that they are operational? No need to reinvent them. The
implementation as preprocessor macros is unfortunate (not type
safe, can suffer from side effects, callers need to remain
aware), but their name strongly suggests that these are not
regular functions. So this could be acceptable.
Use SR_MHZ etc phrases where they improve readability of large
numbers like samplerates. Most of them are hard to decipher in
the current implementation.
Consider whether to really _drop_ FT232R support, or whether to
keep it and maybe warn users about known limitations. I read the
information that I found in the 'Net less strong than you do. Am
to understand that an internal clock of these chips is broken
beyond recovery, but that some clones as well as externally
clocked devices do work as expected. So I'd suggest to keep the
FT232R entry, detect when it matches, and emit a warning message
in that case. Which avoids the necessity to check for VID/PID
values that are not in the list of supported devices.
Separate the FT231X related content from FT232R content (in
general: split unrelated changes into several commits). Pass
samplerates as 64bit entities.
I could see if FT231X can be made to work. I think Adafruit FTDI
friends or Watterott adapters employ these. The data that I got
so far is "interesting". USB 2.0 Full Speed, aha. Up to 3Mbps in
UART mode. 48MHz base clock for bitrate divider, 14bit prescaler,
3bit fractional (results in 183bps to 3Mbps). Integer div must be
between 2..16384, cannot divide by 1..2, fractional only takes
effect for higher divider values. 512B RX and TX buffer. The
hardware that I got here only makes some of the channel's signals
available (I think 0, 1, 3, 4). And testers need to take care,
default pin direction starts at O I O I O I I I per port (some
are output!).
Minor style nits after the above major concerns are: Move
assignments out of the declaration block. Concentrate
declarations at the top of routine bodies. End comments in
punctuation. Adjust the opening/closing of multi-line comments.
Break long lines in sensible locations, keep related parameters
together for improved readability. Personal preference whether to
add spaces around initializers for structs and arrays, I'd
suggest them. And certainly put the last comma there, it's not
funny if you have to search for missing ones after their omission
has piled up. Check for trailing whitespace and extra/missing
empty lines, check the end of the file (generally applicable, may
or may not apply to your series specifically). Use size_t for
things that are in-memory sizes or non-negative indices. Avoid
duplicated data types in memset() or malloc() calls (again:
generally applicable, need not be an issue in your series).
When you update documentation, make sure that example commands
and other important information for users "stands out". Don't
hide it in the text paragraph. This relates to the A-D ports.
See existing README content.
Maybe tone down commit messages (and comments?), these remain on
record for decades to come. It's plausible that you are emotional
during commit creation, but take a moment to consider what to
"leave behind for future generations" and how to communicate
that. The incredible situation that you found before your change
will be a thing of the past. (Yes, I said this, and I'm aware of
how funny that may sound to you. But then, check my rants which
are for the moment, and compare these to my commit messages, or
the commit messages that I suggested other contributors to use
instead. Most thanked me for making them look good.)
Something that I'm personally interested in: Could FTDI FIFO
support get factored out to become "even more common"? When it
could disguise as "yet another serial communication channel",
then ols and p-ols finally could get merged. Which reduces
redundancy, results in identical behaviour and the greatest
feature set of them both. Of course that would be a second step
after the FTDI-LA overhaul, and only if possible and useful.
Suggest to ping here if you got an update on that PR. As I'm
not scanning github, and others may be interested and are not
subscribed either. Thank you for working on that subject!
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
