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.

Sign up for GitHub

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

Jump to bottom

support for FX2 cables #71

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

support for FX2 cables #71

wants to merge 8 commits into from

Conversation

phdussud
Copy link
Contributor

@phdussud phdussud commented Jan 7, 2021

Here you go.
Thanks,
Patrick

@trabucayre
Copy link
Owner

Thanks for this PR, but some remark:

  • your class is called fx2 but this class implement the fpgalink protocol, it seems more logical to rename this to fpgalink.{hpp,cpp} (globally to replace all fx2 by fpgalink since it's possible to use this firmware with avr (exactly as DirtyJTAG))
  • ENABLE_FPGALINK is set to ON by default. I prefer to a default OFF and user select ON if he wan't (with that fpgalink is an optional dependency instead of required by default)
  • you update the default list of source/includes, but this list must be updated only if this cable is enabled
  • your subdirectory is really mandatory? (fpgalink repo provides this in install/include)
  • why libfpgalink.dll.a libraries installed in install/bin are .so ?

@phdussud
Copy link
Contributor Author

phdussud commented Jan 9, 2021 via email •
edited
Loading

@phdussud
Copy link
Contributor Author

phdussud commented Jan 9, 2021 via email •
edited
Loading

@trabucayre
Copy link
Owner

  1. VID:PID are specific to fpgalink (by default, fx2 with eeprom bypass, has 04b4:8613), these informations are provided at the firmware level (according to libfpgalink/firmware/avr/Makefile avr firmware version has the same), glasgow board uses an fx2 too and I'm quite sure VID:PID are different again (this is the same thing for stm32: dirtyJTAG, blackmagic, versaloon, stlink have, each, a VID:PID different). This why I think it's better to call this class by the name of the protocol instead of by the name of the device
  2. /3. Ok. Just take care about the case (FX2 and fx2 are 2 differents files for linux (case sensitive))
  3. weird: after a build I have all headers present in install/include/makestuff (but maybe because I use linux...)
  4. again, library naming depends on OS, so it's maybe required to take into account darwin, msys2 and linux (I can't help: I've no macOS or windows :-/)

I need to recheck fpgalink because being unable to install files in a different location (and having libraries in bin directory) grieves me

makestuff
makestuff reviewed Jan 10, 2021
View reviewed changes
CMakeLists.txt Outdated Show resolved Hide resolved
@makestuff
Copy link

makestuff commented Jan 10, 2021 •
edited
Loading

  1. I agree with @trabucayre: this PR will also work for the AVR and the mbed firmware (although the mbed maintainer lost interest), so it should be called FPGALink or FPGALINK or fpgalink rather than FX2/fx2.

  2. I'm relatively new to CMake - when I wrote FPGALink back in 2011 I made the (in retrospect, ridiculous) decision to build it using GNU Make on all three platforms (Linux, Windows, MacOSX), so the CMake-based build is still very much in the experimental stage, and I have not gotten around to thinking about the "binary distribution" problem - I chose to make the assumption that if anyone wants to use FPGALink in an application, they'd just build it alongside their app. This could be achieved by creating git submodules of the various components (libfpgalink, libfx2loader, libbuffer, etc), or if FPGALink support is optional, have CMake clone the relevant repos before building them, like I do for libusb on Windows. Whichever way you go, there should be a way of anchoring the build on a particular commit, so if I change something, it doesn't break your build.

  3. You should be able to build a thing based on FPGALink by just making a subdir with a very simple CMakeLists.txt and doing #include <makestuff/libfpgalink.h>. If that doesn't work, it's a bug and I need to fix it. For example, you can copy the C example and build it with CMake like this:

     wotan$ cat CMakeLists.txt 
 project(myapp)
 add_executable(${PROJECT_NAME} main.c args.c)
 target_link_libraries(${PROJECT_NAME} PRIVATE fpgalink)
 install(TARGETS ${PROJECT_NAME} DESTINATION bin)
 wotan$ grep libfpgalink main.c
 #include <makestuff/libfpgalink.h>
 wotan$

    That works on Linux (gcc) and Windows (msvc). You should not need to make a copy of the headers.

  4. I don't know much about MinGW, so I have no idea about its conventions. The only configurations I test in CI are gcc on Linux and msvc on Windows.

being unable to install files in a different location (and having libraries in bin directory) grieves me

The reason for installing libraries in the bin directory is that Windows lacks a notion of RPATH, so putting everything in a single directory (i.e bin) means that only one directory needs to be added to PATH in order to make it work. So I just follow the same convention on Linux (because it would be extra complexity to do $ORIGIN/../lib on Linux). But I'm open to suggestions about that.

@trabucayre
Copy link
Owner

Thank for your comments.

  1. Great
  2. In my case (openFPGALoader) adding a submodule may complexify this repository. I have tried this morning to build one by one the full content of fpgalink (not finish yet) successfully: when -DCMAKE_INSTALL_PREFIX is fixed on a classic directory (/usr or /usr/local) next repo is able to find includes and libraries (s/bin/lib/g) with alternate install path -DCMAKE_C_FLAGS="-I/somewhere" do the job. So I assume it's possible with a minimum effort to have fpgalink repo able to install everything correctly without needs to a submodule
  3. I'm only linux user so it's difficult for me to have an universal idea but cmake may have automatic way to deal with install path (maybe @edbordin or @umarcor have more advice this point.

@umarcor
Copy link
Contributor

umarcor commented Jan 10, 2021 •
edited
Loading

I rebased and pushed #65. CI runs are not shown in the PR, because CI is not enabled for this repo yet. However, you can see the Actions tab of my fork: https://github.com/umarcor/openFPGALoader/actions. The latest run has an artifact: https://github.com/umarcor/openFPGALoader/actions/runs/475847257. The artifact contains MINGW64 and MINGW32 packages, which you can extract to see which files were included.

The build recipe is rather simple (~10 relevant lines): https://github.com/trabucayre/openFPGALoader/pull/65/files#diff-57c1a8b93f074586ac89e2e3f65f87bc428edc786ae3ca176cba639141dcf95fR18-R40. You can create a temporary branch based on my PR and rebase it on top of this one. Then, push and CI will run.

The relevant lines with regard to the prefix/path on MSYS2 recipes are the following:

  MSYS2_ARG_CONV_EXCL=- cmake \
    -G "MSYS Makefiles" \
    -DCMAKE_INSTALL_PREFIX="${MINGW_PREFIX}" \
    ../

make DESTDIR="${pkgdir}" install

Therefore, if the cmake recipes are properly written, #65 should work as is (that's the point of using cmake).

The reason for installing libraries in the bin directory is that Windows lacks a notion of RPATH, so putting everything in a single directory (i.e bin) means that only one directory needs to be added to PATH in order to make it work. So I just follow the same convention on Linux (because it would be extra complexity to do $ORIGIN/../lib on Linux). But I'm open to suggestions about that.

That is correct. On MSYS2, all DLLs are installed to $PREFIX/bin, not $PREFIX/lib:

# ls -la /mingw64/bin/*.dll | wc -l
446

# ls -la /mingw64/lib/*.dll | wc -l
2

# ls -la /mingw32/bin/*.dll | wc -l
52

# ls -la /mingw32/lib/*.dll | wc -l
ls: cannot access '/mingw32/lib/*.dll': No such file or directory
0

# ls -la /mingw64/lib/*.a | wc -l
934

# ls -la /mingw64/bin/*.a | wc -l
2

You might have noted that two DLLs exist in /mingw64/lib/. Those are coming from GHDL, which uses makefiles mostly written for GNU/Linux. Therefore, we might consider it non standard. GHDL uses the location of the binary in /mingw64/bin for finding the DLLs.

My suggestion would be for both GHDL and openFPGALoader to handle it in the most natural way: use */bin on Windows and */lib on GNU/Linux.

@makestuff
Copy link

My suggestion would be for both GHDL and openFPGALoader to handle it in the most natural way: use */bin on Windows and */lib on GNU/Linux.

Yep, happy to go that way if someone wants to raise a PR for it.

@umarcor
Copy link
Contributor

umarcor commented Jan 10, 2021

I updated #65. Now, contents of packages are shown in CI. No need to download and extract the artifacts. See https://github.com/umarcor/openFPGALoader/runs/1677313883?check_suite_focus=true#step:8:11.

@trabucayre
Copy link
Owner

@umarcor thanks for your comment. In fact it's not really for openFPGALoader, since no library is installed but for fpgalink.
@makestuff I will try to find a clean way to handle both systems.

@umarcor
Copy link
Contributor

umarcor commented Jan 11, 2021 •
edited
Loading

On MSYS2, should https://github.com/makestuff/fpgalink be a different package, which openFPGALoader depends on?

@trabucayre
Copy link
Owner

@umarcor : yes. Topic of this PR is to add fpgalink protocol support (optional) and to communicate with the cable through fpgalink.

@phdussud
Copy link
Contributor Author

My latest commit tries to address some of the issues raised in the PR review:

  • Replaced the defined ENABLE_FX2 with USE_LIBFPGALINK so more cables than just fx2 can be supported with the use of libfpgalink
  • Removed the src/makestuff directory and use the libfpgalink install include directory instead. This is subject to solving "the discovery of the installation of libpgalink" problem

@trabucayre
Copy link
Owner

Thanks.

  1. Looks good for me. Only one last thing: fx2.{c,h} must be renamed (take to care to the case, linux is case sensitive)
  2. I'm less happy. Using hardcoded directory imply constrains for user about structure. I've send a serie of PR to fppgalink submodule to install libraries in lib for linux system and in bin for window system. Next I think to send pkg-config support (okay it's maybe only linux compliant (it's not clear with msys2) but it's possible to provides libraries and include directories in cmake command line to have something more generic (as done for udev and libftdi).

@umarcor
Copy link
Contributor

umarcor commented Jan 13, 2021 •
edited
Loading

@trabucayre, pkg-config is supported on MSYS2 and used by many packages. It's ok and desirable to use that.

Am I correct assuming the upstream is https://github.com/makestuff/libfpgalink/ and https://github.com/makestuff/fpgalink is just a utility repo for building it manually?

@phdussud
Copy link
Contributor Author

phdussud commented Jan 13, 2021 •
edited
Loading

  1. I appreciate your concern. I don't consider this a final solution. That's why I wrote "This is subject to solving "the discovery of the installation of libpgalink" problem. Hopefully someone can build libfpgalink as a package and we can have a much better solution. I did some cleanup work so we don't duplicate the libfpgalink headers in the OpenFPGALoader source tree.

@phdussud
Copy link
Contributor Author

@trabucayre : The files are named fc2.cpp and fx2.hpp. I am sorry it took so long to do this. I got confused because I renamed fx2.xpp earlier, Windows showed the name in lower case but I didn't notice git didn't do anything to rename the file.

@trabucayre
Copy link
Owner

@umarcor thanks for this information. I think it's a good approach to simplify retrieving informations about a library. As soon my first serie of patches has been accepted by @makestuff I'll send a new serie about pkgconfig support. Yes fpgalink is a repository used to build all required libraries required.
@phdussud in fact when I said renaming files it's fx2.cpp -> fpgaLink.cpp and fx2.hpp -> fpgaLink.hpp.

@phdussud
Copy link
Contributor Author

I got you now! Sorry for being dense.
I did the renaming
I created a more generic FpgaLink base class that supports the openFPGALoader methods
FX2Cable is now a sub class of FpgaLink

@trabucayre
Copy link
Owner

Sorry for the delay, I have prefered to wait for a status of pkg-config in `fpgalink.
Now it's seems clear for me this way is dead. So it's required to find alternate solution....
I see two alternative:

  1. adding some black magic (:-) ) to detect all required libraries, lib and include path(fpgalink and all dependencies of this). But this add a big amount of code in CMakeLists.txt (or in cmake/module/FindFpgaLink)
  2. unlike I've said at the beginning, using a custom code instead of using this library.

I have a certain tendency to prefer the second solution for two reasons:

  1. For build system (msys2, buildroot, fpga-toolchain, ...) it's better to limit dependencies
  2. I have read some functions used in your implementation and finally most part are just a serie of function call to, at the end, calling libusb functions...

For example this:

jtagClockFSM(handle, bitPattern, (ulen > 0) ? 32 : 32 + ulen, &error);

is more or less the same than that

uint8_t buffer[4] = {
buffer[0] = (bitPattern >>  0) & 0xff,
buffer[1] = (bitPattern >>  8) & 0xff,
buffer[2] = (bitPattern >> 16) & 0xff,
buffer[3] = (bitPattern >> 24) & 0xff};

int status = libusb_control_transfer(dev_handle,
        LIBUSB_ENDPOINT_OUT | LIBUSB_REQUEST_TYPE_VENDOR | LIBUSB_RECIPIENT_DEVICE,
        CMD_JTAG_CLOCK_FSM, (ulen > 0) ? 32 : 32 + ulen, 0x0000, buffer, 4, 5000);

Ok, it's more verbose, I haven't added verifications and some calls are a bit more complex but it's finally feasible and it's depending of the situation it's more easy to maintain.

What do you think about these options?

@phdussud
Copy link
Contributor Author

I would like @makestuff to opine on this issue. I believe there is value in the fpgalink library beyond being a middle man between openFPGALoader and libusb. It also provides and loads the firmware for the FX2.

@makestuff
Copy link

I don't have much of an opinion. It's true that this could be made to work fairly easily just by replicating the FPGALink wire protocol, and I completely understand the preference for fewer dependencies. I guess the other thing I'm not so clear about is who this work is intended for? FPGALink had its heyday way back in 2013, with an active user group, etc. But despite my best efforts, continually adding support for more and more FPGA boards, more host platforms, more language-bindings (Python, Perl, Java, even Excel VBA), and more microcontrollers, it was abundantly clear back in 2014 that it was never going to take off in the way I intended. If this was a thriving project with thousands of users and a plethora of cheap FPGA boards available on AliExpress all claiming "FPGALink Compatible!" then I could see the point of all this. As it is, the world has kinda moved on. I'm only committing to those repos because they're dependencies of other unrelated projects, or as a template project-set for some unrelated work I'm doing for my employer.

@trabucayre
Copy link
Owner

I, usually, prefer to use an library when it's possible instead of reinventing the wheel. For fpgalink my problem is to limit the complexity to obtain all required informations to be able to build support. This why my first idea was to propose the pkg-config support. The result is not limited to openFPGALoader but to all apps and library using fpgalink.
For intended users, if @phdussud has create this PR this is, I suppose, because it use it. I dont have to accept or not one protocol according his popularity.

@makestuff
Copy link

makestuff commented Jan 24, 2021 •
edited
Loading

OK so how about @phdussud forks the FPGALink top-level repo, leaving all the individual library submodules as they are, and adds the pkg-config stuff you need at the top level. It should be enough to produce pkgconfig stuff only for libfpgalink.so, because that's the only library you need to link with. The top-level repo is tiny, it's literally just this CMakeLists.txt, with all the real work done in the git submodules, so it won't be reinventing the wheel. That will allow @phdussud to use FPGALink with openFPGALoader, and it allows you to use pkg-config to find your dependencies.

@phdussud
Copy link
Contributor Author

I will give it a shot. Just to make sure I understand: The package will need to contain pretty much what is in the install of fpgalink
(bin and include) since libfpgalink depends on all of the libraries contained in bin.
I will probably need help from @umarcor as he is experienced in cmake and packages.

@makestuff
Copy link

No, I think it just needs to add the install location's include and lib dirs. So if someone sets CMAKE_INSTALL_PREFIX=/opt/fpgalink when they build FPGALink, then somehow you want dependent projects to get -I/opt/fpgalink/include added to their compile-lines, and -L/opt/fpgalink/lib -Wl,-rpath,/opt/fpgalink/lib -lfpgalink added to their link-lines (and the equivalents for MinGW and MSVC if you care about those). I think it's reasonable to assume all the other libs (libusbwrap.so, libfx2loader.so, etc) will have been installed alongside libfpgalink.so, so dependent projects don't need to know or care about them.

@phdussud
Copy link
Contributor Author

I am following what you are saying and it is useful. I was talking about the package itself. As I understand it, cmake needs to be told how to make a package out of the binaries and includes of libfpgalink. The package will be installed somehow (I know how to install packages in msys2 if they are on the msys2 repo). Once it is installed, I understand that the lib command and the include command will be fetched from the package definition and I agree with what you said about that.

@trabucayre
Copy link
Owner

The idea to create a fork is, in my mind, the worst solution. if each one with a specific contribution has to fork a repository, user will be unable to determine which to use.
For example, libfpgalink is, for firmware, not standalone (with hardcoded path). If one wish to add required submodule it will create a new fork with the corrected libfpgalink repository. So you have the main repository, an alternate repository with pkg-config support and another without pkg-config but with a standalone libfpgalink submodule. So which to use?
If it's not possible to have a clear structure I prefer bypassing this library and recoding calls.

@phdussud
Copy link
Contributor Author

@trabucayre I see what you are saying. @makestuff I misunderstood what the fork is intended for. I thought I would fork, do the work and then PR back to the official fpgalink repo. I am with @trabucayre if it would mean people have to get my forked repo in order to produce the package.

@makestuff
Copy link

Fair enough.

@trabucayre
Copy link
Owner

I have already created PR for a subset of repository. No for all, in a first time, to have a go/nogo before continuing...
The answer is, to synthesize,: if you want this feature, you have to fork repository, add your stuff and use it... Nor @umarcor post, nor mine, has changed something.
This why the solution to rewrite calls is the only one I think reasonable.

phdussud and others added 8 commits March 6, 2021 10:39
@phdussud
Support for the FX2 cable using libfpgalink
dba31e7
@phdussud
Added documentation
93526cd 
Conditionalization of pin mapping for FX2
@phdussud
Fixed CMakeLists.txt so fx2 support is not enabled by default
64a95fe 
Fixed CMakeLists.txt so fx2 related include files are not part of the default build
@phdussud
Addressed some of the issue raised in the PR review:
c248e50 
replaced the defined ENABLE_FX2 with USE_LIBFPGALINK so more cables than just fx2 can be supported with the use of libfpgalink
removed the src/makestuff directory and use the  libfpgalink install include directory instead. This is subject to solving the discovery of the install directory problem
@phdussud
rename FX2.cpp to fx2.cpp
7718901
@phdussud
Renamed file fx2.XXX to fpgalink.XXX
82df51d 
Created a base class FpgaLink to encapsulate the general fpgalink functionality (could be used to create a avr cable)
Converted the printf calls to printError and printInfo calls.
@phdussud
Fixed compilation error in fpgalink class header
4111fb0
@phdussud
dirtyJtag cable: optimizations to cut down on USB packet traffic
171d51f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@makestuff makestuff makestuff left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@phdussud @trabucayre @makestuff @umarcor