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

Rust package #355

Merged
merged 21 commits into from
Oct 9, 2021
Merged

Rust package #355

merged 21 commits into from
Oct 9, 2021

Conversation

trobanga
Copy link
Contributor

@trobanga trobanga commented Sep 29, 2021

Todo:

  • find a way to avoid name clash of log functions
  • write
    • documentation
    • tests
    • examples
    • README.md
  • use brainflows build system
  • publish to crates.io
    • add meta info to Cargo.toml

@trobanga
Copy link
Contributor Author

Ok, I've managed to link the libraries individually to their module

@trobanga
Copy link
Contributor Author

trobanga commented Oct 1, 2021

The package root of a Rust crate is definitely defined by the location of its Cargo.toml.
Regarding the publishing to crates.io I'm now 99.9999% sure that it's impossible to include the parent directory (unless you want to add a Cargo.toml to the root directory of the repo).

So, my recommendation is to add two repos: (maybe) brainflow-rs and brainflow-rs-sys to brainflow-dev.
Or we do it with the submodule but that would be a weird circular dep.

@Andrey1994
Copy link
Member

"Regarding the publishing to crates.io I'm now 99.9999% sure that it's impossible to include the parent directory (unless you want to add a Cargo.toml to the root directory of the repo)."

There is no need to add a parent directory, we need to create smth like lib folder under rust-package\brainflow and copy binaries there from CMake

@trobanga
Copy link
Contributor Author

trobanga commented Oct 1, 2021

ah, so you want to add precompiled binaries? That would work but limits of course the available target platforms

@Andrey1994
Copy link
Member

<<ah, so you want to add precompiled binaries? That would work but limits of course the available target platforms

Exactly, and its how it works for other bindings. Python/Java/etc users don't need to compile C\C++ source code. And as of right now it covers Win32\x64, Linux x64(all distros) and MacOS starting from 10.13(ARM and x64)

For other platforms(raspberry for example or windows + arm) users still can run it but need to compile it manually by themselves first.

I also have plans to add packages for ARM(#344) but since there were just a few requests for it, its not high priority task

@Andrey1994
Copy link
Member

Its what I meant by brainflow's build system)

@trobanga
Copy link
Contributor Author

trobanga commented Oct 1, 2021

I slowly begin to understand^^

- Put -sys related code directly into brainflow
- Remove submodule
- Add rust-package/brainflow-sys/{lib,inc} to gitignore
@trobanga
Copy link
Contributor Author

trobanga commented Oct 3, 2021

Ok, from my side this looks ready for merging and publishing...

btw I just noticed that the links to the Brainflow bindings in the subfolder readmes do not work...

@trobanga
Copy link
Contributor Author

trobanga commented Oct 3, 2021

Tests and examples can follow later

@trobanga
Copy link
Contributor Author

trobanga commented Oct 3, 2021

There's one more thing where I'd like to get your opinion: in Rust it's common to name getter function just like their variable name i.e. num_rows() instead of get_num_rows(). At the moment I have both versions included but if it's okay with you I'd take out the get_ ones. Alternatively, we could use a feature to enable them.

@Andrey1994
Copy link
Member

I value consistency between languages more than language-specific guidelines, so let's keep only get_* versions

@Andrey1994
Copy link
Member

<<btw I just noticed that the links to the Brainflow bindings in the subfolder readmes do not work...

thanks, I will fix them in another commit

Copy link
Member

@Andrey1994 Andrey1994 left a comment

Choose a reason for hiding this comment

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

Looks great, thanks for implementing it! Let's keep only get_* version of methods to match other languages and it will be good to go

/// You need to call it before any other BoardShim object methods.
pub fn prepare_session(&self) -> Result<()> {
let res = unsafe {
BOARD_CONTROLLER.lock().unwrap().prepare_session(
Copy link
Member

Choose a reason for hiding this comment

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

One question about lock and mutexes. Inside C\C++ code there is a mutex for such methods and they are thread-safe. So, do we really need it here? If its to ensure that the library is loaded only in one thread then it's ok to have it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Due to the dynamic loading of the libraries I need to create a global object, otherwise it wouldn't be possible to use it in the static functions. And since it has to be mutable the mutex is necessary here.

I must say this whole dynamic loading at runtime is only because of the 4 logging functions that share the same name...it would be much easier and nicer if there wouldn't be the name clash i.e. if the log functions would already have a distinctive name - the one they get in the language bindings anyway e.g. enable_logger -> enable_ml_logger

@Andrey1994
Copy link
Member

Andrey1994 commented Oct 3, 2021

also, in addition to tests and CI pipelines we still have some tasks about docs(check this link https://brainflow.readthedocs.io/en/stable/BrainFlowDev.html#instructions-to-build-docs-locally):

Also, need to update website, I will take care of it(and about readmes)

One more task: update Dockerfile to install rust binding https://github.com/brainflow-dev/brainflow/blob/master/Docker/Dockerfile

@trobanga
Copy link
Contributor Author

trobanga commented Oct 8, 2021

also, in addition to tests and CI pipelines we still have some tasks about docs(check this link https://brainflow.readthedocs.io/en/stable/BrainFlowDev.html#instructions-to-build-docs-locally):

* [ ]  add instructions to compile it to https://github.com/brainflow-dev/brainflow/blob/master/docs/BuildBrainFlow.rst

* [ ]  add code samples to https://github.com/brainflow-dev/brainflow/blob/master/docs/Examples.rst

* [ ]  optional: if there is a way to use rust docs with sphinx or with doxygen we can add it to https://github.com/brainflow-dev/brainflow/blob/master/docs/UserAPI.rst

Also, need to update website, I will take care of it(and about readmes)

One more task: update Dockerfile to install rust binding https://github.com/brainflow-dev/brainflow/blob/master/Docker/Dockerfile

I think there is nothing to be done in the Dockerfile for now. Maybe if we publish to crates.io.
I'll add a few examples once I started working with it. Atm there is one but I haven't added a command line parser yet.

And there is indeed the possibility to build the docs for sphinx but I'd postpone that to some distant future.

So, from my side it's good to go :)

@Andrey1994 Andrey1994 merged commit c5b09f3 into brainflow-dev:master Oct 9, 2021
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.

2 participants