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.

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

WIP: bmp: Implement. Requires firmware from https://github.com/UweBonnes/b… #111

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

Conversation

UweBonnes
Copy link
Contributor

…lackmagic/tree/fixes

Hello,

if you find time, please have a look at this PR. It works only for Linux, maybe for Mac, but windows device detection is not implemented. Code from BMP needs porting and testing. C++ Coding style and general coding style need rework. Speed is probably not good, as shiftIR|DR need up to 5 usb round trips. Please comment.

@trabucayre
Copy link
Owner

Thanks for this PR. Will study this one and add feedback

@UweBonnes
Copy link
Contributor Author

Sorry, fixes isnot enough after yesterdays changes. Stand By.

@UweBonnes
Copy link
Contributor Author

UweBonnes commented Sep 7, 2021

Use https://github.com/UweBonnes/blackmagic/tree/fixes1 or better avr.

Copy link
Owner

@trabucayre trabucayre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

toggleClk: LGTM. Just one thing: use tab instead of space

@UweBonnes
Copy link
Contributor Author

BMP has code to work on windows. Somebody with windows needs to port and test.

@trabucayre
Copy link
Owner

Maybe in a first time it make sense to add this driver only for linux devices, and add a note to request for someone with a windows machine to check/add/fix support.

Copy link
Owner

@trabucayre trabucayre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

src/cable.hpp: it's okay. But maybe it's requires it's own PR since it's not related to bmp.
src/part.hpp: I'm not really convinced, some devices are already present in this file, others are not (currently) supported. It's error prone, since openFPGALoader don't really know if there are supported or not (or consider is it).

src/xilinx.cpp Outdated
Comment on lines 467 to 468
printError("FAIL");
return false;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tab instead of space

Copy link
Owner

@trabucayre trabucayre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some white space are presents in bmp.cpp

src/bmp.cpp Outdated
Comment on lines 26 to 49
void Bmp::DEBUG_WARN(const char *format, ...)
{
va_list ap;
va_start(ap, format);
vfprintf(stderr, format, ap);
va_end(ap);
}

void Bmp::DEBUG_WIRE(const char *format, ...)
{
if (!_verbose)
return;
va_list ap;
va_start(ap, format);
vfprintf(stderr, format, ap);
va_end(ap);
}

void Bmp::DEBUG_PROBE(const char *format, ...)
{
if (!_verbose)
return;
va_list ap;
va_start(ap, format);
vfprintf(stderr, format, ap);
va_end(ap);
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is printWarn, printInfo and printError (with color) in display.hpp

src/bmp.cpp Outdated

int Bmp::setClkFreq(uint32_t clkHZ)
{
char construct[REMOTE_MAX_MSG_SIZE];
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

char construct[REMOTE_MAX_MSG_SIZE], is used in most of methods. Maybe a privtate variable in bmp.hpp to reduce that.

src/bmp.cpp Outdated
s = platform_buffer_read(construct, REMOTE_MAX_MSG_SIZE);

if ((!s) || (construct[0] == REMOTE_RESP_ERR)) {
fprintf(stderr,"Update Firmware to allow to set max SWJ frequency\n");
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's an error. It seems logical to stop no ?

@UweBonnes UweBonnes force-pushed the bmp_remote branch 2 times, most recently from c0c5d4a to 6be1e00 Compare October 3, 2021 15:00
CMakeLists.txt Outdated
@@ -6,7 +6,11 @@ project(openFPGALoader VERSION "0.5.0" LANGUAGES CXX)
add_definitions(-DVERSION=\"v${PROJECT_VERSION}\")

option(BUILD_STATIC "Whether or not to build with static libraries" OFF)
option(ENABLE_UDEV "use udev to search JTAG adapter from /dev/xx" ON)
if (${CMAKE_SYSTEM_NAME} MATCHES "Windows")
option(ENABLE_UDEV "use udev to search JTAG adapter from /dev/xx" OFF)
Copy link

@LAK132 LAK132 Sep 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This default configuration makes it impossible to use this on Windows because searching for the device isn't implemented. Might want to change the --device parameter to not depend on USE_UDEV

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this remark
It make sense: original idea with --device was to use it for FTDI devices on Linux (/dev/ttyUSBx) but if this option may be used for another things it make sense to split that between UDEV support (ftdi devices) and search for a device base on a path (like here).

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have updated repository: now --device has specific USE_DEVICE_ARG. This allows to enable/disable this arg at CMakeLists.txt level according to options (udev, libgpiod, ...).

@@ -186,6 +187,7 @@ set(OPENFPGALOADER_HEADERS
src/xilinxMapParser.hpp
src/colognechip.hpp
src/colognechipCfgParser.hpp
src/bmd.hpp
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing bmd_remote.h ?

@trabucayre
Copy link
Owner

LGTM.
The only thing not clear to me is about adding GPL code (bmd_remote.hpp) into a projet using Apache2 license?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants