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

Add swift binding #327

Open
Andrey1994 opened this issue Aug 16, 2021 · 56 comments
Open

Add swift binding #327

Andrey1994 opened this issue Aug 16, 2021 · 56 comments
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@Andrey1994
Copy link
Member

Use other languages(java, c#, python) as an API reference.

Useful link: https://stackoverflow.com/questions/34669958/swift-how-to-call-a-c-function-loaded-from-a-dylib

@Andrey1994 Andrey1994 added good first issue Good for newcomers enhancement New feature or request labels Aug 16, 2021
@ScottThomasMiller
Copy link

I am trying to create a bridging header for Swift 5 and Xcode 12.5.1. Must my Swift code instantiate the BoardShim object, or can it just call prepare_session() and start_stream() directly?

@Andrey1994
Copy link
Member Author

Andrey1994 commented Aug 19, 2021

The idea of bindings is to provide high-level API which is easier to use than plain C methods exposed from low-level library. So, we created BoardShim class for each binding. Users are expected to work with BoardShim methods instead calling low-level prepare_session and other methods directly.

Example from Java:(https://github.com/brainflow-dev/brainflow/blob/master/java-package/brainflow/src/main/java/brainflow/BoardShim.java)

    public void prepare_session () throws BrainFlowError
    {
        int ec = instance.prepare_session (board_id, input_json);
        if (ec != ExitCode.STATUS_OK.get_code ())
        {
            throw new BrainFlowError ("Error in prepare_session", ec);
        }
    }

@Andrey1994
Copy link
Member Author

So I would say let's create BoardShim, DataFilter, and other classes like in existing bindings

@Andrey1994
Copy link
Member Author

I dont know swift but why do we need a bridging header at all?(and what is it?)

I think it can look like this(pseudocode inspired from python binding):

class BoardShimDynLibLoader {
    handler = nil;
    static func get_handler() {
        if (handler == nil)
            handler = dlopen("libBoardContoller.dylib", RTLD_NOW)
        return handler;
}

class BoardShim {
    func prepare_session() {
        typealias prepare_session = @convention(c) (CInt, Cchar *) -> CInt
        handler = BoardShimDynLibLoader.get_handler()
        var prepare_session = dlsym(handle, "prepare_session")
        if (prepare_session != nil) {
            let f = unsafeBitCast(sym, prepare_session .self)
            let result = f(board_id, params)
       }
    }    
}

Idea is to create a singleton that will call dlopen only once and find addresses of required functions manually.
Is it worse or better?(I don't know swift best practices)

@ScottThomasMiller
Copy link

The bridging header tells Swift how to resolve the object names in your Swift code. Without it you get errors such as, "Cannot find 'prepare_stream' in scope". It's typically a .h file which simply includes other .h files. You have to register it in the Xcode build settings, so that Swift knows where to look.

Your example is similar to this: https://stackoverflow.com/questions/35229149/interacting-with-c-classes-from-swift

I should be able to implement something like the that, because I already have all of BrainFlow's C++ headers.

I am still learning Swift, but I think that your idea is basically a wrapper, and wrappers are very "Swifty".

@ScottThomasMiller
Copy link

I managed to call the low-level functions directly, such as prepare_session() and get_eeg_channels(), but so far I am unable to instantiate BoardShim(). I need some help. Everything I've found so far, I can't get working.

@Andrey1994
Copy link
Member Author

You should not call methods from C++ BoardShim class. This class even doesn't exist in low-level API(low-level API is for bindings, high-level API is for users).

Instead, you need to create a new class in Swift and call it BoardShim, inside this class you need to wrap methods from low-level API. Take a look at the cpp-package for reference. https://github.com/brainflow-dev/brainflow/blob/master/cpp-package/src/board_shim.cpp#L104 Here method is_prepared from BoardShim class wraps method is_prepared from low-level API, you need to do the same

@ScottThomasMiller
Copy link

OK I think I finally understand: you want me to write BoardShim.swift modeled after board_shim.cpp. That seems completely doable. I will also have to write swift bindings for all of BoardShim's dependencies, such as BrainFlowException, BrainFlowArray, BrainFlowInputParams, etc. And nowhere in all of those bindings will I need to instantiate any C++ classes, correct?

@Andrey1994
Copy link
Member Author

Correct, but probably you dont need BrainFlowArray(its specific for c++ for better perf, you can do the same as in java for example). Also, there are two more classes: DataFilter and MLModule, its the same there.(I can help and do it for some of them)

@Andrey1994
Copy link
Member Author

Also, you need to duplicate enums like BoardIds, etc. All of that is pretty small and simple except main classes(BoardShim and DataFilter)

@ScottThomasMiller
Copy link

@Andrey1994
Copy link
Member Author

Looks good so far!

@ScottThomasMiller
Copy link

Thanks. Swift doesn't like binding its String type to "char *" parameters in its bridging headers, so I'm making them all "const char *" in board_controller.h instead.

Also I think Swift will automatically make BoardIds iterable if I add the CaseIterable protocol to its type definition. I'll try that shortly.

@Andrey1994
Copy link
Member Author

"I'm making them all "const char *" in board_controller.h instead."

Its not ok, some other bindings don't like const char *. Lets find a way to keep board_controller.h as is(add type casting somewhere on swift side)

@Andrey1994
Copy link
Member Author

I cannot say that I fully understand what exactly they are doing here https://forums.swift.org/t/how-to-pass-swift-string-to-c-function-in-swift-5/28416 but seems like its doable with char * in C header

@ScottThomasMiller
Copy link

Understood. I'll take a look.

@ScottThomasMiller
Copy link

ScottThomasMiller commented Aug 24, 2021

I reverted board_controller.h to its original state. For the board parameter string I am now converting the JSON string inside BoardShim.init() like so:

self.cParams = UnsafeMutablePointer<CChar>(mutating: jsonParams)

And then I pass cParams into the low-level functions. It seems to be working OK.

Swift gives me a warning about creating a dangling pointer, at the preceding line of code. From what I've read, if the low-level functions never modify or destroy the JSON parameters, then we can safely ignore the warning.

@ScottThomasMiller
Copy link

I need some more help, this time with BrainFlowArray. My C++ is weak. I cannot translate BrainFlowArray into Swift all by myself. From what little I do understand of it, BrainFlowArray provides various matrix operations, including reshaping the input buffer in a sort of numpy fashion.

For the app I am building, all I need is a 2D matrix of doubles, so I will focus on that initially. For testing purposes, to make sure I reshape the buffer correctly, what do you suggest? Is there a simulator mode I can use to compare results against?

@Andrey1994
Copy link
Member Author

You dont need to port BrainFlowArray to Swift, its numpy-like optimization specific for C++. Key idea there is memory layout(2d array is 1d array in fact)

You can do the same as in java https://github.com/brainflow-dev/brainflow/blob/master/java-package/brainflow/src/main/java/brainflow/BoardShim.java#L779 there its just standard 2d array

@Andrey1994
Copy link
Member Author

More info if you are interested: https://brainflow.org/2021-03-09-new-major-version/

@Andrey1994
Copy link
Member Author

"For the app I am building, all I need is a 2D matrix of doubles, so I will focus on that initially. For testing purposes, to make sure I reshape the buffer correctly, what do you suggest?"

Run it using synthetic board and check that rows are correct(first row is package num, etc)

@ScottThomasMiller
Copy link

Using the synthetic board, I am comparing my output against the output of your C++ example. It looks like I am reshaping my buffer correctly.

In both cases we are using get_current_board_data(5).

Here is the output from your C++ example:

98.000000 99.000000 100.000000 101.000000 102.000000
3.524325 1.759109 -0.000000 -1.844007 -3.426920
-13.467129 -5.613274 1.408918 8.663252 15.039456
25.759608 10.453843 -4.274830 -20.293409 -36.518251
-37.565274 -15.633888 9.210492 37.694390 57.364029
71.815144 36.607924 -16.633953 -52.961736 -81.124755
-80.163680 -42.131472 16.581405 73.079982 76.795649
103.085663 39.600036 -24.570546 -104.618697 -83.167381
-145.595706 -60.288164 49.365395 92.544312 81.063926
75.094505 62.654041 -34.156802 -145.984865 -55.713966
-154.178495 -120.002921 61.347603 192.979051 25.696258
167.315720 142.702001 -41.541240 -212.566271 19.168347
-103.980679 -190.077269 60.032875 162.163131 -79.682029
138.539331 188.915072 -126.283773 -70.084075 161.071326
-49.621182 -255.658966 42.593981 120.334711 -172.826360
18.479719 280.558184 -107.601329 -134.791289 88.188602
6.314293 -107.612166 111.620795 17.905098 -113.680189
0.962317 0.981579 0.918030 0.800336 0.999687
0.882748 0.990145 0.992720 0.860554 0.829110
0.930930 0.922554 0.806216 0.938207 0.963169
0.919554 0.833327 0.924387 0.995097 0.818384
0.822444 0.968036 0.898841 0.996125 0.838067
0.831681 0.943121 0.962370 0.885325 0.996443
1.080530 1.061077 0.940015 1.095128 1.063627
5432.291190 5411.946023 4528.449774 5425.031334 5443.923847
4680.233026 4931.376847 4703.288257 4516.483963 5402.541694
36.593704 36.602607 36.596204 36.605233 36.601470
1079.024751 982.201564 956.497338 973.923423 1078.052445
990.825113 1073.140646 1009.932247 1022.242693 1039.514976
85.979647 94.859912 84.274026 81.926733 94.004491
1629995103.728148 1629995103.731914 1629995103.736948 1629995103.740714 1629995103.745530
0.000000 0.000000 0.000000 0.000000 0.000000

And here is the output from my Swift code:

100.00000 101.00000 102.00000 103.00000 104.00000
-0.00000 -1.69165 -3.44543 -4.99500 -6.62305
1.44753 8.28995 14.16592 18.66274 24.80209
-4.34256 -20.31284 -28.98679 -38.16113 -37.66824
10.10112 37.05097 56.79074 59.00251 49.33110
-13.89766 -40.02346 -85.40910 -53.10395 -22.44765
15.85307 71.53188 92.72715 47.15099 -12.59683
-36.72619 -107.82567 -85.19615 -23.98563 51.49561
33.87915 152.06859 56.80625 -23.14171 -99.88292
-38.42393 -166.58373 -80.56335 88.75330 134.13172
50.69680 81.78940 35.05108 -97.93932 -73.78425
-73.65167 -203.25009 14.83315 190.58374 19.88039
123.58570 184.52759 -109.25622 -174.56790 28.48647
-58.28310 -147.67590 200.37148 175.14706 -140.58163
42.38645 127.63895 -263.29436 -66.21680 320.38816
-213.30514 -70.51221 116.57182 -25.88802 -137.70446
84.85873 18.95906 -127.68043 151.27837 151.23315
0.91632 0.89045 0.96501 0.85506 0.99279
0.89318 0.95777 0.92006 0.98815 0.93787
0.99624 0.95188 0.87109 0.92150 0.92891
0.80789 0.80934 0.86136 0.90575 0.91562
0.93334 0.91119 0.84393 0.97291 0.84798
0.99401 0.98418 0.89540 0.96639 0.88983
1.02263 1.09288 0.94792 0.92615 1.07662
4576.38763 5264.11655 5197.22065 4898.32205 5393.38484
5318.84261 5102.69790 5163.85087 5157.62249 5268.54776
36.60431 36.59329 36.60232 36.60342 36.60356
931.45280 952.83389 1080.53608 1041.21333 1031.47774
922.83384 1055.93884 953.47350 976.94542 1008.56613
94.12669 89.46505 93.62441 96.79821 93.19209
1629995240.96847 1629995240.97224 1629995240.97725 1629995240.98102 1629995240.98603
0.00000 0.00000 0.00000 0.00000 0.00000

@Andrey1994
Copy link
Member Author

Yes, that's 100% correct!

@ScottThomasMiller
Copy link

Nice. What does the first row signify?

@Andrey1994
Copy link
Member Author

package_num, for real boards it can be used to track package loss

@ScottThomasMiller
Copy link

And the 2nd-to-last line:

1629995240.96847 1629995240.97224 1629995240.97725 1629995240.98102 1629995240.98603

Are those the timestamps?

@Andrey1994
Copy link
Member Author

Yes, you can call method get_board_descr to get info about these rows programmatically, or take a look at https://github.com/brainflow-dev/brainflow/blob/master/src/board_controller/brainflow_boards.cpp

@ScottThomasMiller
Copy link

I am testing my Swift bindings by streaming EEG data from my OpenBCI Cyton+Daisy. All of the package IDs are coming back even-numbered, as if I am missing every other sample. Do I have another bug? The issue does not occur with the synthetic board.

Here's my code:

func streamEEG() {
    do {
        print("preparing session")
        try board.prepareSession()
        print("setting gain to x1")
        try setGain(setting: .x1)
        while try !board.isPrepared() {
            sleep(1)
        }
        try board.startStream()
        print("streaming EEG")
        while true {
            let matrix = try board.getBoardData()
            guard matrix.count > 0 else {
                continue
            }
            print("matrix dim: [\(matrix.count),\(matrix[0].count)]")
            print(matrix)
        }
    }
    catch {
        try? logMessage (logLevel: LogLevels.LEVEL_ERROR.rawValue, message: error.localizedDescription)
        try? board.isPrepared() ? try? board.releaseSession() : print("session already closed")
    }
}

@ScottThomasMiller
Copy link

The headset battery died, so I swapped it out for a freshly recharged battery, re-ran my test, and the issue went away. So: next item on my to-do list is to add code to check the battery level!

Also, when the issue was happening, my calls to set the gains were causing BrainFlow to log warnings about rescaling the data, something about how your code uses x24 to convert int to uV. But with the fresh battery, those warnings are no longer printed.

@ScottThomasMiller
Copy link

ScottThomasMiller commented Aug 27, 2021

Oh: also I had moved the setGain call to after the isPrepared loop. That might have something to do with it.

@Andrey1994
Copy link
Member Author

"Also, when the issue was happening, my calls to set the gains were causing BrainFlow to log warnings about rescaling the data, something about how your code uses x24 to convert int to uV. But with the fresh battery, those warnings are no longer printed."

It cannot be so, these warnings doesn't depend on it and printed everytime. About even numbers, it can be ok for daisy, it sends 2 packages and probably I keep it as is

@Andrey1994
Copy link
Member Author

Do you have plans to push swift binding to brainflow upstream and\or add other methods from BoardController(and probably DataFilter)?

@ScottThomasMiller
Copy link

Yes. I plan to push my bindings when they are 100% coded and tested.

So far I have coded 27 BoardShim functions, plus all the enums, the BrainFlowInputParams, and BrainFlowException. Next up will be DataFilter.

@Andrey1994
Copy link
Member Author

Great, thanks!

@ScottThomasMiller
Copy link

Your "Java Signal Filtering" example contains the following line of code:

                DataFilter.perform_lowpass (data[eeg_channels[i]], BoardShim.get_sampling_rate (board_id), 20.0, 4,
                        FilterTypes.BESSEL.get_code (), 0.0);

The example fetches 30 samples. In the preceding line of code, does data[eeg_channels[i]] represent a 30-element vector of doubles for the ith channel?

@Andrey1994
Copy link
Member Author

yes

@ScottThomasMiller
Copy link

Follow-up question: in the preceding example, data is a 2D array of doubles, which is why you are iterating over each channel, calling the filter once for each channel. Is it OK if I instead use a slice of the 1D buffer, before it's been reshaped, to call the filter only once for all channels?

For example, if I have 10 samples and 16 channels, then the first 10 elements of the 1D buffer are package IDs, and the next 160 elements are EEG channel data. Is it OK to call the filter just once using data[10...169]?

@Andrey1994
Copy link
Member Author

No, it's not ok.

These 2 APIs are independent and BoardShim should return 2d array while existing methods for signal processing operate on a 1d array. Also, from the user's point of view, it allows you to apply different filters for different channels.

@ScottThomasMiller
Copy link

ScottThomasMiller commented Aug 29, 2021

I see what you mean, and indeed I plan to provide matching functionality in my bindings. But what I would like to also do is to overload get_board_data() with a second function which returns the 1D buffer. Users will still be able to call the original function and get the 2D array. The two APIs will remain independent.

My question is more about whether the filter will still return the correct values if I pass in all channels at once.

@Andrey1994
Copy link
Member Author

For correct implementation of 1d array it should be like in C++ binding, we can add it later on. For beginning, I think we should make it like in java.

As is filter function will not understand that its multiple arrays joined together in 1d array, it will treat it as a single 1d array and will generate wrong result. To fix it method signature for filtering should be changed.

I was going to implement smth like that before #108 but have no time and don't fully understand how to make it the same for all bindings

@Andrey1994
Copy link
Member Author

Hi, how is it going? If you need some help you can create a WIP PR

@ScottThomasMiller
Copy link

It's going well. BoardShim is completely coded, but not yet 100% tested. I'm working on building a unit test module for it. DataFilter is only about 25% coded.

@ScottThomasMiller
Copy link

Actually I do have one remaining question about BoardShim: in your BoardShim.java, the get_num_rows binding calls instance.get_num_rows, but the get_board_data and get_current_board_data bindings call BoardShim.get_num_rows. Why is that?

@Andrey1994
Copy link
Member Author

instance.get_num_rows is a method from low-level library(BoardController) and this method is called from BoardShim.get_num_rows. Later on in user-code and\or in other methods from BoardShim class you can call BoardShim.get_num_rows while instance object is private and not for end users

@ScottThomasMiller
Copy link

I'm not yet sure how to do the same in Swift, nor am I certain it's necessary.

I am ready to push BoardShim.swift and its dependencies. Please give me instructions for committing and pushing.

@Andrey1994
Copy link
Member Author

<<I'm not yet sure how to do the same in Swift, nor am I certain it's necessary.

This stuff with instance in java is kinda bridging header in swift. It describes the interface of BoardController.dll and allows you to call C\C++ methods. You don't need that.

<<I am ready to push BoardShim.swift and its dependencies. Please give me instructions for committing and pushing.

Create WIP PR from non-master branch of your fork and if you see a checkbox like "allow edit to maintainers" click on it. So, I will be able to make changes and help you a little. Make sure that swift binding matches the common structure in terms of folder and file names. Would be good to add a few tests to CI pipelines but it can be done later on

@ScottThomasMiller
Copy link

In my fork should I duplicate ./brainflow/settings.xml and ./brainflow/pom.xml?

@Andrey1994
Copy link
Member Author

no, these files are specific for maven packaging(like setup.py in python)

@ScottThomasMiller
Copy link

ScottThomasMiller commented Sep 9, 2021

I created my fork and within it a branch named swift-bindings. I cloned that branch to my laptop, added my .swift files, committed and pushed them, and now am ready to create my pull request. How do I make it WIP?

Here's a screenshot:

image

@Andrey1994
Copy link
Member Author

Green Button "Create pull Request" click on triangle close to it

@ScottThomasMiller
Copy link

"Create draft pull request"?

@Andrey1994
Copy link
Member Author

yes

@ScottThomasMiller
Copy link

Because of the Swift scope rules, I had to move the logger functions of DataFilter inside its type definition. Otherwise it would have caused name collisions with the analogous BoardShim functions. For consistency I should probably do the same for the BoardShim logger functions. What do you think?

@Andrey1994
Copy link
Member Author

I think all methods like get_exg_channels ,set_log_level, etc should be static class methods instead of global functions. This way it will be consistent with other bindings

@Andrey1994
Copy link
Member Author

In fact there should not be any global functions, only class methods and static class methods

@Andrey1994
Copy link
Member Author

And lets move this discussion to PR comments

@Andrey1994 Andrey1994 linked a pull request Sep 26, 2021 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants