MRG Handle triggers split between buffers

[jasmainak]

Taking over #3696

Closes #3696 and #3695

cc @Odingod and @LauriParkkonen. Let me know if this works for you guys. Tests pass on my box.

[codecov-io]

Current coverage is 87.42% (diff: 100%)

Merging #3738 into master will increase coverage by <.01%

@@             master      #3738   diff @@
==========================================
  Files           343        343          
  Lines         61000      61041    +41   
  Methods           0          0          
  Messages          0          0          
  Branches       9326       9332     +6   
==========================================
+ Hits          53326      53367    +41   
  Misses         4924       4924          
  Partials       2750       2750          

Powered by Codecov. Last update bc323fb...071df35

[larsoner]

LGTM. @Odingod can you take a look?

[Odingod]

Seems to be working, except for one small bug that is not worth fixing in my opinion. If the the first buffer starts with the trigger channel active the onset is missed and the code gets stuck like it used to. However the trigger channel shouldn't be high at the beginning of the measurement (unless using some kind of continuous measure pressure e.g.?). Maybe this is still worth a mention?

[Odingod]

Let's take that back. There was trouble with a test I added that has two triggers at two consecutive buffer lines. For some reason the first one is not found. I'll take a closer what breaks this time. The test can be found from my branch if you'd like to take a look. On closer look everything was fine I just forgot to change the assertion comparison...

[jasmainak]

On closer look everything was fine I just forgot to change the assertion comparison...

Great, so ready to merge?

Seems to be working, except for one small bug that is not worth fixing in my opinion. If the the first buffer starts with the trigger channel active the onset is missed and the code gets stuck like it used to. However the trigger channel shouldn't be high at the beginning of the measurement (unless using some kind of continuous measure pressure e.g.?). Maybe this is still worth a mention?

I would say it is expected that an event is not detected? Does this get detected as an event in the offline version? I think what is perhaps a bit unintuitive to the user is the "getting stuck" part since the isi_max is set to 0.5. I think it is worth changing this behavior so that the rt_client stops if there are no epochs even in the first isi_max seconds. I'd probably keep it for another PR. What do you think @Eric89GXL @Odingod ?

[jasmainak]

Thanks @Odingod for taking a stab at this. Looking forward to more contributions! We should increase the bus factor of the realtime module.

[Odingod]

@jasmainak changing the behavior so that the isi_max is respected in all situations might make things more consistent, but is worth a second PR. I don't see any other problems for merging, so go ahead!

[jasmainak]

Merged!