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] Update Brainflow #141

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

[WIP] Update Brainflow #141

wants to merge 4 commits into from

Conversation

Andrey1994
Copy link

Changes:

  • updated brainflow manually and updated vs solution(tested, works). It adds support for more devices
  • blacklisted some of devices which are currently in dev state
  • tried to update makefiles(had to replace it to tabs, make had some problems with spaces, sorry it makes it harder to review, but its in its own commit, so its more or less isolated)

@dioptre @cyberjunk could you take a look at makefiles? It does smth weird and I am still trying to understand whats wrong

P.S. Update process is extremely complicated, all files should be copypasted manually and one after another, update in build systems also should be done manually and it takes a lot of time, especially in old makefiles.

@Andrey1994
Copy link
Author

fixed, some folders for build files should be created manually

Tested it only on windows and not for all new boards

@Andrey1994 Andrey1994 marked this pull request as ready for review March 8, 2022 13:26
@Andrey1994 Andrey1994 requested review from dioptre and cyberjunk and removed request for dioptre March 8, 2022 13:26
Copy link
Contributor

@cyberjunk cyberjunk left a comment

Choose a reason for hiding this comment

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

There are lot of build artifacts in the PR.
Things like .obj files and others.
Please double check everything is correclty on .gitignore and those are gone.

PS:

P.S. Update process is extremely complicated, all files should be copypasted manually and one after another, update in build systems also should be done manually and it takes a lot of time, especially in old makefiles.

Yes, indeed. The Next Generation Makefiles including VSC Cross-Platform Support are like 50% done. I hope to finish them soon, which will make dealing with the Make build system magnitudes easier and more familiar. Also providing debuggable build and also likely ARM64/M1 builds.

@Andrey1994
Copy link
Author

@cyberjunk should be fixed now, I removed some files

@Andrey1994 Andrey1994 requested a review from cyberjunk March 22, 2022 14:59
Copy link
Contributor

@cyberjunk cyberjunk left a comment

Choose a reason for hiding this comment

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

@Andrey1994
It looks like you integrated a new third party library: SimpleBLE
About that:

(1) README
Please add some information (version, link etc.) about it to the list of dependencies (README.md) here:
https://github.com/neuromore/studio/blob/master/deps/README.md

(2) LICENSE
License seems MIT which is totally fine.

(3) PREBUILT
There are some prebuilt shared libraries added, like:
https://github.com/neuromore/studio/blob/brainflow_update/deps/prebuilt/win/x64/simpleble-c.dll
(a) Are these proprietary parts of the library? [OR]
(b) Are these the build outputs of the library itself?
If (a), everything is OK.
If (b), please remove them, make sure they are built and builds are used like all the others (pref. as static lib, not dynamic)

(4) QT/OTHER BLUETOOTH
We also got QtBluetooth. I always wondered if and how much these different BTLE implementations may conflict with each other. Any idea about that? I feel like that things like starting BTLE scans or others in one library/framework may at least have an effect on other libs/framework if they use the same HW.

Signed-off-by: Andrey Parfenov <[email protected]>
@Andrey1994
Copy link
Author

(1) README
done, added

(2) LICENSE
its mit

(3) PREBUILT
SimpleBLE has its own build system(cmake) and CMakeLists there is pretty complex, also its in early stage of development and is updated often. Droping its own build system and trying to replicate it will lead to errors and each update will take a lot of time and efforts. I think we should keep it as prebuilt library.

Also, linking simpleble statically or trying to compile it will limit OSes supported, because it requires newer windows\macos. Adding special flag to enable\disable it will make update process even more complex.

(4) QT/OTHER BLUETOOTH
SimpleBLE is used only by brainflow and there is no QT Bluetooth. This code is loaded manually in runtime wo linking and only for selected boards. So, there should not be any conflicts here

@Andrey1994 Andrey1994 requested a review from cyberjunk March 27, 2022 14:43
@Andrey1994
Copy link
Author

and I still think that it would be better to add brainflow as a precompiled libs or at least compile it by invoking cmake command
It should not take so much time to update dependency

@JuliusCosmoRomeo
Copy link
Contributor

@Andrey1994
I'm getting an error when building on Mac. Any idea what the issue could be?
I've run make clean and make -j 4 in the deps/build/make folder and then in the build/make folder.

Here's the output:

/board_controller -I../../deps/include/brainflow/utils  -c ../../src/Engine/Devices/Emotiv/EmotivEPOCDevice.cpp -o obj/x64/Engine/Devices/Emotiv/EmotivEPOCDevice.o
clang++ -m64 -target x86_64-apple-darwin19.2.0 -mtune=intel -msse -msse2 -msse3 -mssse3 -Xclang -flto-visibility-public-std -std=c++17 -O3 -fpic -mmacosx-version-min=10.12 -D_SILENCE_CXX17_OLD_ALLOCATOR_MEMBERS_DEPRECATION_WARNING -Wno-deprecated-declarations -DUNICODE -DNDEBUG -Wno-unknown-warning-option -DNEUROMORE_PLATFORM_OSX -I../../include/Engine -I../../src/Engine -I../../deps/include -I../../deps/include/brainflow/board_controller -I../../deps/include/brainflow/utils  -c ../../src/Engine/Devices/Emotiv/EmotivInsightDevice.cpp -o obj/x64/Engine/Devices/Emotiv/EmotivInsightDevice.o
In file included from ../../src/Engine/Devices/BrainFlow/BrainFlowDevices.cpp:25:
In file included from ../../src/Engine/Devices/BrainFlow/BrainFlowDevices.h:33:
../../deps/include/brainflow/cpp-package/board_shim.h:15:10: fatal error: 'json.hpp' file not found
#include "json.hpp"
         ^~~~~~~~~~
clang++ -m64 -target x86_64-apple-darwin19.2.0 -mtune=intel -msse -msse2 -msse3 -mssse3 -Xclang -flto-visibility-public-std -std=c++17 -O3 -fpic -mmacosx-version-min=10.12 -D_SILENCE_CXX17_OLD_ALLOCATOR_MEMBERS_DEPRECATION_WARNING -Wno-deprecated-declarations -DUNICODE -DNDEBUG -Wno-unknown-warning-option -DNEUROMORE_PLATFORM_OSX -I../../include/Engine -I../../src/Engine -I../../deps/include -I../../deps/include/brainflow/board_controller -I../../deps/include/brainflow/utils  -c ../../src/Engine/Devices/eSense/eSenseSkinResponseDevice.cpp -o obj/x64/Engine/Devices/eSense/eSenseSkinResponseDevice.o
In file included from ../../src/Engine/Devices/BrainFlow/BrainFlowNodes.cpp:1:
In file included from ../../src/Engine/Devices/BrainFlow/BrainFlowNodes.h:31:
In file included from ../../src/Engine/Devices/BrainFlow/BrainFlowDevices.h:33:
../../deps/include/brainflow/cpp-package/board_shim.h:15:10: fatal error: 'json.hpp' file not found
#include "json.hpp"
         ^~~~~~~~~~
clang++ -m64 -target x86_64-apple-darwin19.2.0 -mtune=intel -msse -msse2 -msse3 -mssse3 -Xclang -flto-visibility-public-std -std=c++17 -O3 -fpic -mmacosx-version-min=10.12 -D_SILENCE_CXX17_OLD_ALLOCATOR_MEMBERS_DEPRECATION_WARNING -Wno-deprecated-declarations -DUNICODE -DNDEBUG -Wno-unknown-warning-option -DNEUROMORE_PLATFORM_OSX -I../../include/Engine -I../../src/Engine -I../../deps/include -I../../deps/include/brainflow/board_controller -I../../deps/include/brainflow/utils  -c ../../src/Engine/Devices/eemagine/eemagineDevices.cpp -o obj/x64/Engine/Devices/eemagine/eemagineDevices.o
1 error generated.
make[1]: *** [obj/x64/Engine/Devices/BrainFlow/BrainFlowDevices.o] Error 1
make[1]: *** Waiting for unfinished jobs....
1 error generated.
make[1]: *** [obj/x64/Engine/Devices/BrainFlow/BrainFlowNodes.o] Error 1
make: *** [all] Error 2

Signed-off-by: Andrey Parfenov <[email protected]>
@Andrey1994
Copy link
Author

should be fixed now

@JuliusCosmoRomeo
Copy link
Contributor

Hey @Andrey1994, before you wonder why the Brainflow update is not merged yet: Clint and I talked about it yesterday and found that it would make more sense to first merge the VS code makefile integration before merging Brainflow. Afterwards he would adjust the Brainflow update slightly to fit the new build system right away.

@cyberjunk cyberjunk marked this pull request as draft May 10, 2023 08:39
@cyberjunk cyberjunk removed request for dioptre and cyberjunk May 10, 2023 08:39
@cyberjunk cyberjunk changed the title update brainflow [WIP] Update Brainflow May 10, 2023
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.

3 participants