openbci-archive / OpenBCI_NodeJS

Node.js SDK for the all OpenBCI Biosensor Boards
https://www.npmjs.com/package/openbci
138 stars 50 forks source link

Return an Observable instead of firing events #114

Closed jspenc72 closed 7 years ago

jspenc72 commented 8 years ago

Anyone interested in an RxJS Observable variant of this project?

https://github.com/Reactive-Extensions/RxJS/blob/master/doc/api/core/operators/start.md

andrewjaykeller commented 8 years ago

Would that simplify the interface?

jspenc72 commented 8 years ago

Yes

jspenc72 commented 8 years ago

See this package as an example where an Observable is returned. https://www.npmjs.com/package/robinhood-observer

jspenc72 commented 8 years ago

If there is a lot of interest from the community I would be happy to create this.

jspenc72 commented 8 years ago
var OpenBCI = require('openbci-observer')     
var updateFrequency = 200 // Frequency with which to pull updates from the device in milliseconds.
var observer = OpenBCI().observeAll(updateFrequency)  

var dataSubscription = observer
                    .map(tick => tick.results)                            
                    .subscribe(x => {
                        //This block of code is executed each time an event would have been called.
                        console.log(x);  
                        // x would be the json representing the data received from the headset.

                    }, e => {
                        console.error(e)
                    }, () => console.log('data subscription disposed'));

//Unsubscribe to updates after 5 seconds. 

setTimeout(function(){
 dataSubscription.dispose();  
}, 5000);
jspenc72 commented 8 years ago

I would envision the interface might look something like this.

jspenc72 commented 8 years ago

Thoughts @aj-ptw ???

andrewjaykeller commented 8 years ago

Would we have to have a different repo or could we make this default?

There is a lot of interest from the community to simplify the number of functions needs to stream to one.

andrewjaykeller commented 8 years ago

@tashoecraft you work with RxJS, good application for it? @alexcastillo thoughts? @baffo32 you have been helping a lot, what are your thoughts?

andrewjaykeller commented 8 years ago

@jspenc72 I think this is super cool and would love it for you to contribute it, just checked out the Robin Hood wrapper, still I don't understand why it's in another repo.

baffo32 commented 8 years ago

@jspenc72 were you considering going 'under the hood' and changing how the library functions to use rxjs instead of promises (if that even makes sense) internally? Perhaps it could be helpful to have another set of eyes reviewing and organizing the internal code.

I am working detecting runaway promises in tests. So, I would be curious if this will still be reasonable to do with the addition of rxjs: to assert at particular points in the program that, globally, no tasks are still running.

Looks good to me, at first glance.

andrewjaykeller commented 8 years ago

@baffo32 I think this flows really well after merge addressing #95 and all the clean up work you're doing. If we are able to connect and well disconnect without resetting, this could play really well with the wrapper @jspenc72 wants to build. The more I think about it, seems like it would almost be a requirement. Or the wrapper doesn't disconnect once connected.

jspenc72 commented 8 years ago

@aj-ptw , the robinhood-observer is in a different repo because the owner of the robinhood repo felt that observers were not needed and would be better built as a standalone project.

jspenc72 commented 8 years ago

You don't have to have a different repo. Yes I would likely "go under the hood"

jspenc72 commented 8 years ago

Ideally the observable would be tied directly to the incoming data stream from the device at the ealiest place possible. So if you have a stream coming in then the observable would be generated at when the stream is created and will be updated each time you receive a new packet from the stream.

andrewjaykeller commented 8 years ago

Ok love it @jspenc72

The current roadmap is:

1.4.0 - Connection - October/Early November

1.4.1 - Connection - Mid November

1.4.2 - CLI - End of November

2.0.0 - Ganglion/Observables - December 2016

From what I hear, the Ganglion is shipping at the end of November. #80 #81

Potential

3.0.0 - Wifi - February 2017

I have really tried to make auto testing a priority so please feel free to ask any questions about writing or running tests on the code you contribute. Given that road map, where do you think observables could go?

tashoecraft commented 8 years ago

@aj-ptw I think making it Observable based is very logically and fitting.

jspenc72 commented 8 years ago

I reviewed the code and see that there are come cases where a promise is probably "ok".

Do you see any advantage in keeping any of the existing promise based methods?

jspenc72 commented 8 years ago

ie

.connect()

.disconnect()

Etc...

jspenc72 commented 8 years ago

To create an observable to handle these might make that portion less friendly than a Simple

.then().catch()

Interface

jspenc72 commented 8 years ago

Thoughts? @aj-ptw

andrewjaykeller commented 8 years ago

@jspenc72 anything is on the table. We are going for simplification so if creating an observable would only make it worse... i would say that's probably not best. Is there a circumstance where users would not want to use observables or even not be able to use observables?

var dataSubscription = observer
                    .map(tick => tick.results)                            
                    .subscribe(x => {
                        //This block of code is executed each time an event would have been called.
                        console.log(x);  
                        // x would be the json representing the data received from the headset.

                    }, e => {
                        console.error(e)
                    }, () => console.log('data subscription disposed'));

This would be cool to get it down to just one line. Where you pass in the port name and boom, now you get events.

jspenc72 commented 8 years ago

Ill write a few lines of code tonight and see what I can do

baffo32 commented 8 years ago

I asked regarding replacement not knowing the scope of behaviors Observable covered. It sounds like Promises should stick around.

jspenc72 commented 8 years ago

Yes, Well definitely keep promises around. Right tool for the right job you know.. 👌

baffo32 commented 8 years ago

One note: be sure to do new work on the 1.4.0 branch so merging is reasonable. All the whitespace changed.

jspenc72 commented 8 years ago

I think it will help to abstract away the tedious stuff required to start streaming data by default with a one liner observable created as shown above. But allow the user to manually call the methods by passing a config object, possibly when they import the library.

jspenc72 commented 8 years ago

Will do @baffo32

alexcastillo commented 8 years ago

+1 for Observables support using RxJS. Let me know if you would to get some input from the RxJS team.

andrewjaykeller commented 7 years ago

@jspenc72 any progress on this? Branch 2.0.0 is looking about good on my end.

jspenc72 commented 7 years ago

@aj-ptw sorry for the delayed response i have been playing with an RxJS implementation... and I think the best solution is to make a new repository as you previously mentioned... Although the raw data processing will remain similar the interface has become extremely different, doesn't resemble this project at all and is a complete rewrite.

I have created an sdk which would be a replacement that is written in TypeScript and will happily share it so we can get feedback from the community soon.

Cheers!

jspenc72 commented 7 years ago

FYI its ES6 and fully compatible with the latest version of node.

alexcastillo commented 7 years ago

Alright, after meeting with @teonbrooks we decided that the best way to move forward with our project is to create a lib that abstracts all the common operations (filtering, buffering, windowing, FFT, etc). So, I took a first pass at implementing Reactive Extensions for OpenBCI.

Here's the project repo and package: https://github.com/NeuroJS/openbci-rx https://www.npmjs.com/package/openbci-rx

Do you guys mind helping by testing? Please feel free to log any issues in the issues page of the repo.

andrewjaykeller commented 7 years ago

Wow awesome @alexcastillo closing this issue for now