-
-
Notifications
You must be signed in to change notification settings - Fork 340
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
Swift bindings #716
Swift bindings #716
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot @ScottThomasMiller great job!
I wrote some comments, but overall its really good. It seems like theis some copypaste and duplicates, need to cleanup it first. And probably some files which should not be in the repo were added.
I will help you with docs update and will update the website with info about new language supported
swift_package/BrainFlow/Tests/BrainFlowTests/BoardShimTests.swift
Outdated
Show resolved
Hide resolved
swift_package/BrainFlow/Tests/BrainFlowTests/BrainFlowCITests.swift
Outdated
Show resolved
Hide resolved
.../BrainFlowCI/BrainFlowCI.xcodeproj/project.xcworkspace/xcshareddata/IDEWorkspaceChecks.plist
Outdated
Show resolved
Hide resolved
...inFlowCI/BrainFlowCI.xcodeproj/project.xcworkspace/xcshareddata/WorkspaceSettings.xcsettings
Outdated
Show resolved
Hide resolved
.../BrainFlowCI/BrainFlowCI.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved
Outdated
Show resolved
Hide resolved
Cleanup is complete. Build and CI are good. Should I open a new PR? |
no, lets keep in this one, just push your changes to the same brach |
All changes pushed. All checks passed. The PR is ready for your review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe this branch is not properly updated, but there are still a lot of unresolved comments
swift_package/BrainFlow/Tests/BrainFlowTests/BrainFlowTests.swift
Outdated
Show resolved
Hide resolved
Also need to rename examples from main.swift, here is a full list:
|
I also merged master branch to this one and added docs about swift, would be good if you take a look and maybe add details how to build it using xcode. File is docs/BuildBrainFlow.rst |
Due to a strange issue with Xcode, the example test files must be named main.swift, unless the test file contains ArgumentParser, in which case it must be named the same as the top-level struct (for example, eeg_metrics.) I can try to find a workaround. Maybe I can just add ArgumentParser to all of the tests, even the ones that don't actually need it? I figured it out. I used the @main decorator instead of the main.swift filename. |
I added instructions to docs/BuildBrainFlow.rst. |
I made all of the updates except I have not yet changed the main.swift filenames. The build is failing at the following line:
|
I think you are missing hidden conversations, there are more comments if you expand them |
for example - https://github.com/brainflow-dev/brainflow/pull/716/files/a2c526a09dd43e391bd59be5fa4af2407091acf7#r1575460864 if you scroll messages in this PR and expand hidden conversations you will see this |
Indeed I did! I am working on the missed comments now. |
The build is clean except for this error: Deploy Cpp libs / CppUnix (Debug, macos-11.0) I'm not sure what that's from. My swift_package CI looks good; no errors. I think the only thing remaining are the comments for the docs. I will work on that next. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most important remaining fixes are: create a package and do not copypaste files in users apps, and fix code style, there are more details in comments
|
||
You can build the Swift bindings for BrainFlow using Xcode. Before that you need to compile C/C++ code :ref:`compilation-label` and ensure that native libraries are properly placed. Keep in mind that currently it supports only MacOS. | ||
|
||
To compile your Swift app with the BrainFlow bindings, first copy all of the .swift files from swift_package/BrainFlow/Sources/BrainFlowBindings into your Xcode project: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
copypasting files is not good, I actually thought that its smth like package. e.g. https://www.swiftyplace.com/blog/modular-code-with-swift-package-manager
Can also publish it on github packages later on. BrainFlowCI should use it as a package also
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Creating the package is an important feature, but it is also a challenge and will take a while. I've tried several times over the years but failed each time. I will most likely have to open a help ticket with Apple Developer Support. I cannot promise a delivery date for this feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://developer.apple.com/documentation/xcode/creating-a-standalone-swift-package-with-xcode here is a good guide how to create it, I believe the 1st step should be to separate BrainFlowCI and BrainFlowBinding to different projects, create a package like described in the article and after that try to use this package in CI apps
var temperature_channels: [Int32] = [Int32]() | ||
|
||
// decode the input JSON into self: | ||
init(_ descriptionJSON: String?) throws { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you've added a special method fromJSON, why do we still have regulat constructor from json string?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure. I will take a look at removing the constructor.
let bfParams: BrainFlowInputParams | ||
private let jsonBrainFlowInputParams: [CChar] | ||
|
||
/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove commented out code block
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this one is already resolved. I do not see the commented code block in my local repo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
init (_ boardId: BoardIds, _ params: BrainFlowInputParams) throws { | ||
self.boardId = boardId | ||
self.bfParams = params | ||
self.masterBoardId = try BoardShim.getMasterBoardID(boardId: boardId, params: params) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this method is called getBoardID in all other bindings
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have both getBoardID and getMasterBoardID. I can make getMasterBoardID a private func, so that it is not available outside the bindings. Will that work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getBoardId should return master board id for streaming and playback and normal board id for everything else. Having both is confusing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But if its used only inside binding its probably ok
static func eegNames (_ boardId: BoardIds, _ preset: BrainFlowPresets = .DEFAULT_PRESET) throws -> [String] { | ||
var stringLen: Int32 = 0 | ||
var bytes = [CChar](repeating: 0, count: 4096) | ||
let errorCode = get_eeg_names (boardId.rawValue, preset.rawValue, &bytes, &stringLen) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not consistent code style, in some lines there are spaces before ( in some lines not, I am ok with both options in swift binding as long as its uniform. You can take one of the tools from this thread and apply it - https://forums.swift.org/t/swift-source-code-formatting-tool-like-clang-format-for-objective-c/416
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean the space between get_eeg_names and '('? I prefer no space. I can remove the spaces if you prefer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, and its in plenty of places if you take a look carefully. Removing this manually is not good since you will waste more time doing it and more likely will miss smth. For C++ we use clang-format tool which formats code automatically, would be good to run formatter on the entire binding and force consistent code style this way
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
like in the method below there is getBoardDescr (_
and get_board_descr (
and no space in other lines in the same method
} | ||
|
||
/// If the board is streaming or playback, then return the board ID itself. Otherwise return the master board ID. | ||
static func getMasterBoardID(boardId: BoardIds, params: BrainFlowInputParams) throws -> BoardIds { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make this method private or rename to getBoardId and make it non static(other bindings have getBoardId method)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do.
CI for Swift bindings.