labstreaminglayer / App-BrainAmpSeries

LSL connector for BrainAmps
MIT License
3 stars 4 forks source link

new BrainAmp fork #5

Open dmedine opened 4 years ago

dmedine commented 4 years ago

This isn't really an issue, or a pull request per se, but I forked the BrainAmp app and made some changes, including a downsampling feature. It still isn't thoroughly tested, because I don't have a signal generator at the moment, but it seems to be doing the right thing afaict.

@tstenner, I have been moving the Brain Product apps to our own github association. Part of this is ditching the CMake stuff so that I can quickly make changes with my own visual studio projects. I keep the VS project separate from the source code in an attempt to keep this choice isolated from any upstream repos (e.g. this one). The fork is here: https://github.com/brain-products/LSL-BrainAmpSeries

In the future, I would like to freeze all the Brain Products apps permanently and point people to the release pages at the BP github page. I have already moved the RDA client, LiveAmp, and actiCHamp to the BP association and separated them all into separate repos. This should make it easier for users to get the software they need, especially since we are basically shutting the door on the old SCCN ftp server.

The only one of the BP apps that receives any attention from developers apart from myself is this one, so please let me know if you have any suggestions or input you would like to include. I realize that in this case I am making 2 of that which there should be only 1 and I don't want to step on any toes. However, it is very confusing for end users who are trying to get hold of LSL connectors for Brain Products amplifiers to find what they are looking for. It is simply with this in mind that I have made these changes.

tstenner commented 4 years ago

I haven't looked through all of it, but before this issue gathers dust I'll add my two cents.

This isn't really an issue, or a pull request per se, but I forked the BrainAmp app and made some changes, including a downsampling feature. It still isn't thoroughly tested, because I don't have a signal generator at the moment, but it seems to be doing the right thing afaict.

The downsampling feature is a welcome addition. I also have some code to add downsampling but since the project it's for was suspended because of the corona outbreak I don't have the time to polish it (https://github.com/labstreaminglayer/App-BrainAmpSeries/commit/2693d88087cfc7ae8b6f14d733fcf7248c9eb9b5). My approach uses quite a bit of "recent" C++ features, doesn't include any filter generator code and the filter configuration is a bit too flexible at the moment.

I have been moving the Brain Product apps to our own github association.

Sounds good, when it's official we can point the submodule reference in the labstreaminglayer repository to the official repository.

Part of this is ditching the CMake stuff so that I can quickly make changes with my own visual studio projects.

This is certainly faster for you and easier to use than the (documented) CMake config, but I think any user who can deduce that he needs

  1. Qt 5.13.1 for 32bit MSVC 2017 in C:\Qt\Qt5.13.1\5.13.1\msvc2017 (harder than it looks, as the Qt installer only offer 5.13.2)
  2. Boost 1.71.0 for 32bit MSVC 2017 in C:\local\boost_1_71_0
  3. the liblsl binary, compiled with the nonstandard LSL_FORCE_FANCY_LIBNAME=ON option, in C:\Users\David.Medine\LSL\bin
  4. but the library file for the linker in C:\Users\David.Medine\LSL\lib and
  5. the liblsl headers in C:\Users\David.Medine\LSL\include
  6. the Windows 10 SDK (version 18362)

will be able to follow the CMake instructions.

I keep the VS project separate from the source code in an attempt to keep this choice isolated from any upstream repos (e.g. this one).

Wouldn't it be easier to upload your VS project files and add a note "for VS2017 users who don't like CMake, download the project files ->here<-" to the readme? With the CMake (and CI config) you get clean builds and CI checks for any commits and PRs for free, but you wouldn'T have to use them.

In the future, I would like to freeze all the Brain Products apps permanently and point people to the release pages at the BP github page. I have already moved the RDA client, LiveAmp, and actiCHamp to the BP association and separated them all into separate repos. This should make it easier for users to get the software they need, especially since we are basically shutting the door on the old SCCN ftp server.

Fully agreed. But, instead of the separate repositories (one official BP repository and one offical labstreaminglayer repository) it'd be better to have one official repository with the officially blessed version in the main branch and experimental / non-official developments in separate branches.

As for the changes it's a bit messy right now because the first commit effectively reverts the newest commit (https://github.com/labstreaminglayer/App-BrainAmpSeries/commit/92189edfec354088535ad0c2cdcb46cf23441629) and adds some stuff that's removed immediately afterwards. As soon as I've got some spare time, I could rebase it before there's a lot piled on top of it.

dmedine commented 4 years ago

The most important thing for me here is that Brain Products customers who aren't developers can easily find and grab the latest version of the LSL connectors for their amplifiers. This has been a pain point for a long time. This is the reason for the move to the Brain Products organization. It puts the apps more front facing and avoids the overwhelming confusion researchers often feel when hit in the face with a big bad github repo when all they want to do is stream some data from their BrainAmp.

So, I am all for re-adding the cmake and CI gear in the project, but I would prefer not to have my hand made project disturbed.

Also, just a note, this project (and none of the Brain Products LSL apps at this point) depend on boost anymore. All the necessary functionalities from boost in the original versions of the apps are now part of the STL. I believe you were already making an effort to move away from boost. I am all for it, just for simplicity's sake. I think that in LSL, the only thing from boost we really need is ASIO. Once that makes it in to the standard, I would suggest removing all dependencies on boost from the lib as well, but that is another issue.

One more note, the anti-aliasing filters are meant to be in line with the ones used in Recorder. These are simply 2nd order Butterworth filters and the coefficients are hard-coded to be used in the cases of the available downsampling factors (1, 2, 5, 10, 20, 25, and 50) from 5kHz. The implementation is direct form II and all is handled in downsampling.h. This is fully tested and rather clean, so I suggest we keep it.

Finally, as of now, this is not finished. There is a bug with the triggers in the case of EEG stream. I think what is happening is that I am downsampling and filtering the trigger values, which is the wrong way to handle the triggers. I will try to fix this over the next few days so that there will be a tested, 'official' release on the Brain Products repositories. After that, I would prefer the main branch on the BP organization to be the blessed one and fork out from there.

tstenner commented 4 years ago

The most important thing for me here is that Brain Products customers who aren't developers can easily find and grab the latest version of the LSL connectors for their amplifiers. This has been a pain point for a long time. This is the reason for the move to the Brain Products organization. It puts the apps more front facing and avoids the overwhelming confusion researchers often feel when hit in the face with a big bad github repo when all they want to do is stream some data from their BrainAmp.

That's definitely needed. One idea I had was to reorganize the "Supported devices list" in the documentation into a table like this:

Device Support status Code Download
BrainAmp Official Github Download ↓
Some other device untested Github

The move to the BrainProducts organization makes it more official, but it's still a github repository where everyone notices the big green "download the source code" button first.

So, I am all for re-adding the cmake and CI gear in the project, but I would prefer not to have my hand made project disturbed.

The easiest way to keep them as they are is to keep them out of the repository, otherwise Visual Studio will change things as soon as anyone else launches the solution and since the files are already in the repository most git clients will add their local changes to the commits. (And for those who use CMake some parts of Visual Studio still load the solution even when in CMake mode).

I think that in LSL, the only thing from boost we really need is ASIO. Once that makes it in to the standard, I would suggest removing all dependencies on boost from the lib as well, but that is another issue.

Once the boost parts are replaced, we can replace Boost.Asio with standalone asio, but at the moment some parts remain:

One more note, the anti-aliasing filters are meant to be in line with the ones used in Recorder. These are simply 2nd order Butterworth filters and the coefficients are hard-coded to be used in the cases of the available downsampling factors (1, 2, 5, 10, 20, 25, and 50) from 5kHz. The implementation is direct form II and all is handled in downsampling.h. This is fully tested and rather clean, so I suggest we keep it.

Good to know. I had a slightly more complicated implementation because I wanted to downsample to 50Hz, but getting the same results as Recorder is more important than that.

Finally, as of now, this is not finished. There is a bug with the triggers in the case of EEG stream. I think what is happening is that I am downsampling and filtering the trigger values, which is the wrong way to handle the triggers.

That's something my downsampling code already does correctly, but the filters produce a temporal shift that'd need to be corrected for.

dmedine commented 4 years ago

I fixed the triggering bug. It was actually something to do with the organization of the downsampling vectors. Anyway, it is fixed and the latest version is pushed.

I can add 50Hz as an option. This is quite easy to implement now. I just need to add the coefficients to the list of options and Robert is your Father's brother.

I like the proposed organization of the list of supported devices. We should move that discussion over there, or else on Slack.

Lastly, I see your point about the visual studio project. I will fork the BrainAmp repo and re-implement the CMake and CI stuff that is the labstreaminglayer/Apps folder. However, the cmake infrastructure there is quite complicated (in/out of tree etc.). If the 'official' repo is going to be on the Brain Products organization, it might make sense to seal off the cmake code from the rest of LSL. All it would need is pointers to the LSL and Qt libraries. I could do this with the other Brain Products apps as time goes on. The visual studio projects are best kept out of each repo.

tstenner commented 4 years ago

I can add 50Hz as an option. This is quite easy to implement now. I just need to add the coefficients to the list of options and Robert is your Father's brother.

Great, I've improved the code a bit (#6) so it's even easier to add the coefficients.

Lastly, I see your point about the visual studio project. I will fork the BrainAmp repo and re-implement the CMake and CI stuff that is the labstreaminglayer/Apps folder.

I have rebased your code onto the master branch. I had to add the Qt resources file manually and the paths changed, but other than that it includes all your changes.

However, the cmake infrastructure there is quite complicated (in/out of tree etc.). If the 'official' repo is going to be on the Brain Products organization, it might make sense to seal off the cmake code from the rest of LSL.

With the (mostly finished) CMake changes it takes less than 10 lines to find liblsl in a user supplied installation directory or one of the default paths, so it's gotten a lot better.

I could do this with the other Brain Products apps as time goes on.

There's the apptemplate where most changes land the earliest, so you could use that as a… well, template.