Update FX3 support on fresh libsigrok #1
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Introduce a new tests/conv.c source file which exercises the endianess conversion macros. It's assumed that some use cases may break their operation, fortunately these edge cases were not seen before, or the unreliable operation went unnoticed. This test raises awareness of the implementation's constraints. This is a start, the test sequence will benefit from adding some more cases to increase coverage.
Address style, robustness, and usability nits in the common endianess conversion helpers in the libsigrok-internal.h header file. Rephrase preprocessor macros as static inline C language functions to eliminate side effects, and improve data type safety. Provide macros under the previous names for backwards compatibility, so that call sites can migrate to the routines at their discretion (or not at all). Performance is not affected. Inline routines are identically accessible to compiler optimizers as preprocessor macros with their text expansion are. Resulting machine code should be the same. Introduce variants which also increment the read or write position in the byte stream after data transfer. This reduces more redundancy at call sites.
Add another endianess conversion helper which reads 24bit values in little endian format.
Cover the recently introduced inline routines which back the preprocessor macros for endianess conversion. Add test sequences for read and write routines for different data types of different sizes, different endianess formats and signedness, and include those routines which increment the read/write position.
Introduce a text to number conversion routine which is more general than sr_atol() is. It accepts non-decimal numbers, with optional caller given or automatic base, including 0b for binary. It is not as strict and can return the position after the number, so that callers can optionally support suffix notations (units, or scale factors, or multiple separated numbers in the same text string).
Eliminate an unnecessary magic number for the maximum filename length of SIGMA netlists. Use a more compact source code phrase to "unclutter" the list of filenames and their features/purpose. Move the filesize limit to the list of files to simplify future maintenance.
Stick with the FTDI library for data acquisition, and most of all for firmware upload (bitbang is needed during FPGA configuration). Removing this dependency is more complex, and needs to get addressed later. Re-use common USB support during scan before open, which also allows to select devices if several of them are connected. Either of "conn=vid.pid" or "conn=bus.addr" formats are supported and were tested. This implementation detects and displays SIGMA and SIGMA2 devices. Though their function is identical, users may want to see the respective device name. Optionally detect OMEGA devices, too (compile time option, off by default), though they currently are not supported beyond detection. They just show up during scans for ASIX logic analyzers, and users may want to have them listed, too, for awareness. This implementation also improves robustness when devices get disconnected between scan and use. The open and close routines now always create the FTDI contexts after the code has moved out of the scan phase, where common USB support code is used. This resolves bug #841.
Slightly rephrase and comment on the FPGA configuration of the ASIX SIGMA logic analyzer. Use symbolic pin names to eliminate magic numbers. Concentrate FPGA related comments in a single spot, tell the Xilinx FPGA from FTDI cable (uses bitbang mode for slave serial configuration). This fixes typos in the PROG pulse and INIT check (tests D5 and comments on D6). Also removes the most probably undesired 100s timeout in the worst case (100M us, 10K iterations times 10ms delay). Obsoletes labels for error paths. Drops a few empty lines to keep related instruction blocks together. Includes other style nits.
Rename source code identifiers for FPGA registers to closer match the vendor's documentation.
Eliminate a few magic numbers in FPGA commands, use symbolic identifiers for automatic register address increments, and DRAM access bank selects. Improve grouping of related declarations in the header file.
Move the FPGA commands (which can access registers, and sample memory) declarations before the register layout declaration. Which then no longer separates the registers declarations from their bit fields. Update comments on the register set while we are here.
Add more symbolic identifiers, and rename some of the existing names for access to SIGMA sample memory. This eliminates magic numbers and reduces redundancy and potential for errors during maintenance. This commit also concentrates DRAM layout related declarations in the header file in a single location, which previously were scattered, and separated registers from their respective bit fields. Extend comments on the difference of events versus sample data.
Introduce a 4MiB session feed submission buffer in the device context. This reduces the number of API calls and improves performance of srzip archive creation. This change also eliminates complex logic which manipulates a previously created buffer's length and data position, to split the queued data when a trigger position was involed. The changed implementation results in a data flow from sample memory to the session feed which feels more natural during review, and better lends itself to future trigger support code. Use common SW limits support for the optional sample count limit. Move 'sdi' and 'devc' parameters to the front to match conventions. Reduce indentation in routine signatures while we are here. This implementation is prepared to handle trigger positions, but for now disables the specific logic which checks for trigger condition matches to improve the trigger marker's resolution. This will get re-enabled in a later commit.
Use common support for SW limits, and untangle the formerly convoluted logic for sample count or time limits. Accept user provided samplerate values when the hardware supports them, also those which are not listed. The previous implementation mapped sample count limits to timeout specs which depend on the samplerate. The order of applications' calls into the config set routines is unspecified, the use of one common storage space led to an arbitrary resulting value for the msecs limit, and loss of user specified values for read-back. Separate the input which was specified by applications, from limits which were derived from this input and determine the acquisition phase's duration, from sample count limits which apply to sample data download and session feed submission after the acquisition finished. This allows to configure the values in any order, to read back previously configured values, and to run arbitrary numbers of acquisition and download cycles without losing input specs. This commit also concentrates all the limits related computation in a single location at the start of the acquisition. Moves the submission buffer's count limit container to the device context where the other limits are kept as well. Renames the samplerate variable, and drops an aggressive check for supported rates (now uses hardware constraints as the only condition). Removes an unused variable in the device context.
FPGA configuration (netlist upload) of ASIX SIGMA devices is rather special a phase, and deserves its own state in the device context's "state" tracking. Not only is the logic analyzer not available during this period, the FTDI cable is also put into bitbanging mode instead of regular data communication in FIFO mode, and netlist configuration takes a considerable amount of time (tenths of a second).
Use symbolic identifiers to select firmware images, which eliminates magic 0/1/2 position numbers in the list of files, improves readability and also improves robustness. Move 'devc' to 'ctx' and before other arguments in routine signatures while we are here.
Running several firmware uploads in quick repetition sometimes failed. It's essential to stop the active netlist from preventing the FPGA's getting reconfigured (FTDI to FPGA pins are so few, and shared). Delays in a single iteration of the initiation sequence improves reliability. Retries of the sequence are belt and suspenders on top of that. Before the change, failure to configure was roughly one in ten. After the change, several thousand reconfigurations passed without failure.
The driver got extended in a previous commit to accept any hardware supported samplerate in the setter API, although the list call does suggest a discrete set of rates (a subset of the hardware capabilities). Update a comment to catch up with the implementation. Drop the 250kHz item, it's too close to 200kHz. Add a 2MHz item to achieve a more consistent 1/2/5 sequence in each decade. Unfortunately 50MHz and an integer divider will never result in 20MHz, that's why 25MHz is an exception to this rule (has been before, just "stands out more perceivably" in this adjusted sequence).
Move the 'devc' parameter to the front in routine signatures for the remaining locations which were not adjusted yet. Reduce indentation of continuation lines, especially in long routine signatures. Try to not break string literals in diagnostics messages, rephrase some of the messages. Massage complex formulae for the same reason. Whitespace changes a lot, word positions move on text lines. See a corresponding whitespace ignoring and/or word diff for the essence of the change.
Keep application data in its logical presentation in C language struct fields. Explicitly convert to raw byte streams by means of endianess aware conversion helpers. Don't assume a specific memory layout for C language variables any longer. This improves portability, and reliability of hardware access across compiler versions and build configurations. This change also unobfuscates the "disabled channels" arithmetics in the sample rate dependent logic. Passes read-only pointers to write routines. Improves buffer size checks. Reduces local buffer size for DRAM reads. Rewords comments on "decrement then subtract 64" during trigger/stop position gathering. Unobfuscates access to sample data after download (timestamps, and values). Covers a few more occurances of magic numbers for memory organization. Prefer masks over shift counts for hardware register bit fields, to improve consistency of the declaration block and code instructions. Improve maintenability of the LA mode initiation after FPGA netlist configuration (better match written data and read-back expectation, eliminate magic literals that are hidden in nibbles).
Further "flatten" the DRAM layout's declaration for sample data. Declare timestamps and sample data as uint16_t, keep accessing them via endianess aware conversion routines. Accessing a larger integer in smaller quantities is perfectly fine, the inverse direction would be problematic.
Address minor style nits to improve readability and simplify review. The sizeof() expressions need not duplicate data type details. Concentrate the assignment to, update of, and evaluation of variables in closer proximity to reduce potential for errors during maintenance. Separate the gathering of input data and the check for their availability from each other, to simplify expressions and better reflect the logic's flow.
Detect more error conditions, and unbreak those code paths where wrong data was forwarded. It's essential to tell the USB communication layer, sigrok API error codes, and glib mainloop receive callbacks apart. Since the compiler won't notice, maintainers have to be extra careful. Rephrase diagnostics messages. The debug and spew levels are intended for developers, but the error/warn/info levels will get presented to users, should read more fluently and speak from the application's POV. Allow long text lines in source code, to not break string literals which users will report and developers need to search for (this matches Linux kernel coding style). This commit also combines the retrieval of sample memory fill level, trigger position, and status flags. Since these values span an adjacent set of FPGA registers. Which reduces USB communication overhead, and simplifies error handling. The helper routine considers the retrieval of each of these values as optional from the caller's perspective, to simplify other use cases (mode check during acquisition, before sample download after acquisition has stopped). INIT pin sensing after PROG pin pulsing was reworked, to handle the technicalities of the FTDI chip and its USB communication and the FTDI library which is an external dependency of this device driver. Captures of USB traffic suggest that pin state is communicated at arbitrary times.
Move all of the FTDI connection handling from api.c to protocol.c, and prepare "forced" and "optional" open/close. This allows future driver code to gracefully handle situations where FPGA registers need to get accessed, while the caller may be inside or outside the "opened" period of the session. This is motivated by automatic netlist type and sample rate detection, to avoid the cost of repeated firmware uploads.
Introduce a DMM packet parser in src/dmm/ and register it with the serial-dmm device driver. This adds support for the Meterman 38XR multimeter. [ gsi: style adjustment, raise awareness during maintenance ]
While the raw sample is the actual received data, the expanded sample is the crucial data I as a developer am interested in seeing. At least if I assume that the sample expansion works.
This fixes a bug where the channel group setting would not be overwritten in a new run, but instead the channel group settings of both runs would be merged together.
... and thus selectable in PulseView.
This copies the code verbatim from one place to another in preparation to adding support for complex triggers.
Owner
|
Hi zougloub. Thank you for your contribution, and sorry about belated reply. It's difficult for me to review your pull request because it has zeldin:fx3lafw as the base branch, meaning that every commit from mainline is showing up as part of the PR... Not sure what the best path to proceed would be here. As for your "quick fix", unfortunately there is no guarantee that only the bus number will change. The USB2 bus can have a completely different topology from the USB3 one. For example, on my Talos II, 2-3 (USB3) is the same physical port as 1-4.2 (USB2). The USB2 side of the connectors is connected via a hub, but the USB3 side goes directly to a root port. The two highest sample speeds use a different path in the GPIF code, since they are too fast to work with the delay counter. I don't know why that would affect the readings, but in general the behaviour of sampling unconnected inputs is undefined. |
|
Take a look at this pull-request (sigrokproject/libsigrok#148). This adds support for using FX3 devices as 100MHz 16 channel logic analyzers using sigrok |
Author
|
Hi @ashwinnaircy sorry I didn't have enough free time to take this further. Is your pull request taking care of everything? I could take a few minutes to test it, if you have your firmware in a repo somewhere. |
|
HI @zougloub - sorry for the delayed reply. The firmware source is available at EZ-USB™ FX3 Explorer kit as 16-channel 100 MHz logic analyzer with sigrok PulseView - KBA233652. Do check it out! |
Author
|
@ashwinnaircy I'll check out your merge request but since I think it's interesting to keep this work also (due to the additional tooling/licensing constraints that come with your firmware) I'll take some time to refresh this branch. |
Author
|
@zeldin I submitted sigrokproject/libsigrok#154 with a rebased version of this branch (which I haven't tested yet). I invite you to follow up there and I'll close this PR, since your fork is a tad behind. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
24 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hi @zeldin,
I (imperfectly) rebased your changes on the latest upstream sigrok master and added a "quick fix" which makes the device discoverable after firmware loading (because it is changing speed, it changes USB bus). At this point I just wanted to start a conversation; I can clean-up and try to upstream if you're too busy to do it.
Sorry I haven't tested your original revision, it looks like things are working, except that I noticed that the behavior is inconsistent between <= 64 Msps and >= 96Msps. For a disconnected logic analyzer, the signal was all ones, and it becomes all zeroes. At first sight I don't think any of this code can be the problem.