openbci-archive / OpenBCI_NodeJS

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

Fragmented and Buffered Samples (#115) #118

Closed baffo32 closed 7 years ago

baffo32 commented 7 years ago

For now this is just a failing test for #115.

So far it looks like there are two issues: the first packet is dropped after the fragmented reset response (unless my understanding of sample numbers is faulty), and only the first two packets of the large buffered clump are processed.

I've also noticed while working with the relevant daisy test, that most of the time the board is not detected as being a daisy board, and the whole test is skipped. I think this is why it usually passes.

andrewjaykeller commented 7 years ago

@baffo32 interesting report on daisy. Anther user talked about that being a problem as well. Daisy is supposed to be parsed on the softrest so there is for sure a problem if that's not picked up.

andrewjaykeller commented 7 years ago

This might be related to #119

baffo32 commented 7 years ago

Yeah. So, I'm still really new to the OpenBCI protocol. Most of the issue is that every other sample is merged for the daisy board. But it does seem that sample #0 is dropped. This is probably because 0 is even, and the code seems to look for sample pairs starting with an odd number, followed by an even number. I would have fixed this so the test passed, but I don't know the actual correct behavior. Does the board itself sample the values simultaneously, and then send them off with an odd-initiated sample pair? Or does it sample them at the same time they are dispatched, in which case either odd or even could start a pair?

andrewjaykeller commented 7 years ago

Oh!! Yes, the firmware downsamples to 125Hz for daisy and sends 8 channels per data packet 250 times a second.

Please read about to protocol here: http://docs.openbci.com/software/02-OpenBCI_Streaming_Data_Format#openbci-v3-data-format-binary-format

And here for daisy http://docs.openbci.com/software/02-OpenBCI_Streaming_Data_Format#openbci-v3-data-format-16-channel-data

And if you can read some C code here is the line in source: https://github.com/OpenBCI/OpenBCI_32bit_Library/blob/master/OpenBCI_32bit_Library.cpp#L2172

andrewjaykeller commented 7 years ago

Can you please change the base branch to development? I'm so sorry, i did not realize deleting that branch would close this PR!

baffo32 commented 7 years ago

Sorry about the base branch; now I know. I'm also incredibly sorry about putting forth so fervently that this was a packet dropping issue when it turns out it is not. @aj-ptw Your fix for stripping the EOT is an important one that should be merged. But most of my work here was incorrect; the first sample is supposed to be ignored for the daisy board, and the rest of the samples are actually being processed correctly.

andrewjaykeller commented 7 years ago

rtfd!! all good Karl! is your test passing now?

andrewjaykeller commented 7 years ago

Can you fix/remove that failing test?

codecov-io commented 7 years ago

Current coverage is 94.45% (diff: 100%)

Merging #118 into development will increase coverage by 0.02%

@@           development       #118   diff @@
=============================================
  Files                4          4          
  Lines             2263       2273    +10   
  Methods            135        136     +1   
  Messages             0          0          
  Branches           484        487     +3   
=============================================
+ Hits              2137       2147    +10   
  Misses             126        126          
  Partials             0          0          

Powered by Codecov. Last update f7f8517...5d989f6

andrewjaykeller commented 7 years ago

Your fix for stripping the EOT is an important one that should be merged.

@baffo32 you are very very right! this should have been in since the beginning. I guess it's just you never receive output from the board while it's streaming (other than streaming data), so it has been a non issue since. We need to fix that test that is trying two sntp syncs.