support for FX2 cables

[phdussud]

trafficstars

Here you go. Thanks, Patrick

[trabucayre]

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]

Thank you for your comments. I am copying the author of fpgalink because I am not sure I know the answer to some of your questions.

1. I opted for a simple experience and I am targeting FX2 chips specifically. I am assuming a non programmed FX2 chip or one that has been program to the default fpgalink firmware. As it is, my code passes specific USB vid:pid that correspond to the configuration I described. That was the reason to call my class fx2 it is a specific cable configuration out of other permitted by fpgalink. I can rename fx2 to fpgalink if you still want to after reading this.
2. OFF is fine with me.
3. Sure I will make this change
4. I did include the files after I discovered that the original include directory (install/include) isn't usable as it is. In Makestuff repo, the 2 .h files of interest (common.h and libfpgalink.h) are coming from 2 different directories and it is ok for libfpgalink.h to include <makestuff/common.h>. When the 2 files are in the same directory, libfpgalink.h should include <common.h> otherwise the include fails. So I decided to modify the include in fpgalink.h and include them as part of my code. These include files are very stable so I thought it was ok. @makestuff should voice his opinion and suggest a better solution.... Thank you.
5. I have no idea.... That's not what I am seeing when I use MinGW32 when I compile on Windows. I see the libraries being .dll and their static stubs being .dll.a as expected. Again, @makestuff, if you can field this question, I will be very grateful

I want to thank you both for your kind attention to this PR. Patrick

---

From: Gwenhael Goavec-Merou [email protected] Sent: Saturday, January 9, 2021 5:20 AM To: trabucayre/openFPGALoader [email protected] Cc: phdussud [email protected]; Author [email protected] Subject: Re: [trabucayre/openFPGALoader] support for FX2 cables (#71)

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 ?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Ftrabucayre%2FopenFPGALoader%2Fpull%2F71%23issuecomment-757181647&data=04%7C01%7C%7C786b2f196c4a4b28c53e08d8b4a167e3%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C637457952611070539%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=t%2BhMwnnQv0rNl02sDvZS4x6HdqHalbJCf%2BKVCZxv0EU%3D&reserved=0, or unsubscribehttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FABVQKX2GK4CKVNBDRGG5ZF3SZBJ3XANCNFSM4VZLGU2A&data=04%7C01%7C%7C786b2f196c4a4b28c53e08d8b4a167e3%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C637457952611080495%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=NgW2yoNCrLAZH%2F7XJhgM1kZV7kMX%2BsM4E7U%2Fq1Bn8zk%3D&reserved=0.

[phdussud]

While working on 2. and 3. I realized that the name ENABLE_FPGALINK is contrary to my earlier comment. Based on the final decision on 1. I will adjust the name accordingly. Thanks, Patrick

---

From: Patrick Dussud [email protected] Sent: Saturday, January 9, 2021 10:40 AM To: trabucayre/openFPGALoader [email protected]; trabucayre/openFPGALoader [email protected] Cc: Author [email protected] Subject: Re: [trabucayre/openFPGALoader] support for FX2 cables (#71)

Thank you for your comments. I am copying the author of fpgalink because I am not sure I know the answer to some of your questions.

1. I opted for a simple experience and I am targeting FX2 chips specifically. I am assuming a non programmed FX2 chip or one that has been program to the default fpgalink firmware. As it is, my code passes specific USB vid:pid that correspond to the configuration I described. That was the reason to call my class fx2 it is a specific cable configuration out of other permitted by fpgalink. I can rename fx2 to fpgalink if you still want to after reading this.
2. OFF is fine with me.
3. Sure I will make this change
4. I did include the files after I discovered that the original include directory (install/include) isn't usable as it is. In Makestuff repo, the 2 .h files of interest (common.h and libfpgalink.h) are coming from 2 different directories and it is ok for libfpgalink.h to include <makestuff/common.h>. When the 2 files are in the same directory, libfpgalink.h should include <common.h> otherwise the include fails. So I decided to modify the include in fpgalink.h and include them as part of my code. These include files are very stable so I thought it was ok. @makestuff should voice his opinion and suggest a better solution.... Thank you!
5. I have no idea.... That's not what I am seeing when I use MinGW32 when I compile on Windows. I see the libraries being .dll and their static stubs being .dll.a as expected. Again, @makestuff, if you can field this question, I will be very grateful

I want to thank you both for your kind attention to this PR. Patrick

---

From: Gwenhael Goavec-Merou [email protected] Sent: Saturday, January 9, 2021 5:20 AM To: trabucayre/openFPGALoader [email protected] Cc: phdussud [email protected]; Author [email protected] Subject: Re: [trabucayre/openFPGALoader] support for FX2 cables (#71)

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 ?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Ftrabucayre%2FopenFPGALoader%2Fpull%2F71%23issuecomment-757181647&data=04%7C01%7C%7C786b2f196c4a4b28c53e08d8b4a167e3%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C637457952611070539%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=t%2BhMwnnQv0rNl02sDvZS4x6HdqHalbJCf%2BKVCZxv0EU%3D&reserved=0, or unsubscribehttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FABVQKX2GK4CKVNBDRGG5ZF3SZBJ3XANCNFSM4VZLGU2A&data=04%7C01%7C%7C786b2f196c4a4b28c53e08d8b4a167e3%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C637457952611080495%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=NgW2yoNCrLAZH%2F7XJhgM1kZV7kMX%2BsM4E7U%2Fq1Bn8zk%3D&reserved=0.

[trabucayre]

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]

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]

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]

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]

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]

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]

@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]

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

[trabucayre]

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

[phdussud]

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]

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]

@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]

2. 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]

@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]

@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]

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]

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]

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]

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]

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]

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]

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]

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]

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]

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]

@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]

Fair enough.