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

cbmex 'trialdata' returns 'digin' times but NOT values #103

Closed
jsdpag opened this issue Mar 25, 2019 · 32 comments · Fixed by #105
Closed

cbmex 'trialdata' returns 'digin' times but NOT values #103

jsdpag opened this issue Mar 25, 2019 · 32 comments · Fixed by #105

Comments

@jsdpag
Copy link
Contributor

jsdpag commented Mar 25, 2019

Greetings Sirs and Sirsettes,

I'm having issues with v7 of cbmex getting the 'digin' values from cbmex( 'trialconfig' ). The NSP's digin port is set for 16-bits any rising edge. I've then run a test where I flip the digital inputs on and off 1000 times in a row.

The pseudo-code:

cbmex( 'trialconfig' , 1 , 'absolute' , 'nocontinuous' )
for i = 1 : 1000
Flip digin channels on
Flip digin channels off
end
d = cbmex( 'trialdata' , 1 ) ;

d( 151 , : )
ans =
1×7 cell array
{'digin'} {1000×1 uint32} {0×0 double} {0×0 double} {0×0 double} {0×0 double} {0×0 double}

You see that the 1000 time stamps are there. But the values are not. I'm not sure if this is an issue with cbmex.cpp or one of the cb* functions that it calls ... I'm still trying to decipher the code.

If you have any suggestions then I'm happy to give 'em a try!

Cheers,
js

@dashesy
Copy link
Collaborator

dashesy commented Mar 26, 2019

Is the channel sampling set in Central?

@jsdpag
Copy link
Contributor Author

jsdpag commented Mar 26, 2019

I believe so. In the 'Hardware Configuration' panel of Central, when I click on 'Digital In', it gives me one channel called 'digin', set to '16-bit on rising edge'. With the old v4 firmware and cbmex, this setting resulted in digin times and values being returned by cbmex( 'trialdata' , ... ).

@dashesy
Copy link
Collaborator

dashesy commented Mar 26, 2019

Can you explicitly set the buffer params in trialconfig e.g.

trial_config_params = {
    'buffer_parameter' : {
        'double'            : True,
        'continuous_length' : 35000,
        'event_length'      : 35000,
        'absolute'          : True,
    }
}
cbpy.trial_config(**trial_config_params)

I wonder if 'nocontinuous' disables sampling digin

@jsdpag
Copy link
Contributor Author

jsdpag commented Mar 26, 2019

Sorry, Dude. No Python, here. Though I could get that running for testing purposes. Can the same be achieved in Matlab with something like:
cbmex( 'trialconfig' , 1 , 'double' , 'continuous' , 35000 , 'event' , 35000 , 'absolute' )
?
Anyway, I'll try a few different 'trialconfig' settings and see if anything works.

@dashesy
Copy link
Collaborator

dashesy commented Mar 26, 2019

I think you should not use nocontinuous, yes cbmex has similar functionalities as cbpy

@jsdpag
Copy link
Contributor Author

jsdpag commented Mar 27, 2019

Okay, I tried both of these:

cbmex( 'trialconfig' , 1 )
cbmex( 'trialconfig' , 1 , 'double' , 'continuous' , 35000 , 'event' , 35000 , 'absolute' )

But I still have the same problem.
cbmex( 'trialconfig' , 1 ) returns digital input times but not values.

I can confirm that the .nev files created by Central's 'File Storage' application contain the digital input times AND values. So, it would seem that something is being lost by cbmex.

We seem to be getting normal behaviour with the front-end channels.

@cboulay
Copy link
Collaborator

cboulay commented Mar 27, 2019

Thanks for the edit. I was just about to ask that.

Can you confirm that the latest nPlayServer plays back digital channels (known bug: it doesn't play back front panel analog input channels)? If it does then can you please attach a small dataset that has the digital data? If that works then I can try to debug from here without going through the hassle of connecting to my NSP and setting up a digital input.

@jsdpag
Copy link
Contributor Author

jsdpag commented Mar 27, 2019

Er ... nPlayServer dies on me. That will take some fiddling. In the meantime, here is the test file I made with digital input. It steps through uint16 values 1 to 255 on the first 8 bits, then it steps through 1 to 255 again, but bitshifted up to the last 8 bits. You should get something like this:

d = openNEV( 'digin_test.nev' )
>> d.Data.SerialDigitalIO.UnparsedData
ans =
510×1 uint16 column vector
1
2
3
4
5
...
254
255
256
512
768
1024
...
64768
65024
65280

In the meantime, if there's any other tests that I can run on our NSP then please let me know.

digin_test.zip

@cboulay
Copy link
Collaborator

cboulay commented Mar 28, 2019

I'm running the data through nPlayServer. Using cbpy, I see spikes on channels 1-128 and a set of 1021 events on channel 151, but all that's there is the timestamps. I'll spend a few minutes seeing if I can track down where the digital values should go, but @jsdpag I will wait to hear from you about whether or not you have the latest firmware before I do much more digging.

@jsdpag
Copy link
Contributor Author

jsdpag commented Mar 28, 2019

I forgot to say that I had the Neural Signal Stimulator plugged in. So, Front End channels 1 to 128 will have spikes. Interesting that there are 1021 digital events. I guess your setup is counting any bit change as an event ; I got 510 but with "16-bit on rising edge" as the setting for the digin channel.

Anyway, we're all up to date with firmware v7.0.4, the latest one available from the Blackrock Micro. website. Older versions v7.0.0 and v7.0.2 of the firmware had bugs with digital events, but Blackrock support says that 7.0.4 should be fine. I'm afraid that it looks like CereLink is the culprit.

If there's any code testing I can do to help then please let me know.

@cboulay
Copy link
Collaborator

cboulay commented Mar 28, 2019

I'm looking at the Python code because I don't have Matlab on this machine...

Here is where the 'events' field should get filled in. In the data returned by nPlayServer, ch is 151. However, since firmware 7.0, the number of possible analog channels went from 128 to 256, so the digital channel is expected to be found at 256+16+4+2+1=279.

Here is the bit of code inside cbsdk.cpp that assigns the channel number. I don't know what the if (ch > cbNUM_ANALOG_CHANS) ch = (ch - cbNUM_ANALOG_CHANS + 150); does. It's from 6 years ago. I assume the +150 is incorrect.

Before I replace it with +278 (actually a slightly different solution), I would like to confirm that this is correct. My guess is that the channel number is meant only to iterate over channels with digital events which includes all of the front end and analog input channels (cbNUM_ANALOG_CHANS) + digital in + serial, skipping over cbNUM_ANALOGOUT_CHANS.

How about if (ch > cbNUM_ANALOG_CHANS) ch += cbNUM_ANALOGOUT_CHANS;?

@jsdpag
Copy link
Contributor Author

jsdpag commented Mar 28, 2019

Just to verify, shall I test that edit on lines 2215/2216 of cbsdk.cpp?
Replacing:
if (ch > cbNUM_ANALOG_CHANS) ch = (ch - cbNUM_ANALOG_CHANS + 150);
with
if (ch > cbNUM_ANALOG_CHANS) ch += cbNUM_ANALOGOUT_CHANS;
?

@cboulay
Copy link
Collaborator

cboulay commented Mar 28, 2019

Please go ahead. I'm doing it here too but I'm also doing 3 other things and I have to leave in 30 minutes.

@jsdpag
Copy link
Contributor Author

jsdpag commented Mar 28, 2019

Well, I made that edit to my local git project and rebuilt cbmex. But I still don't get digital input values. Would you recommend keeping that edit anyway? It appears to be more portable than the old version.

@cboulay
Copy link
Collaborator

cboulay commented Mar 28, 2019

Yes, it's probably correct.
Even after I made the change and installed new cbpy, I'm still getting channel 151.
I'll push this small change anyway because it's almost definitely correct.

@jsdpag
Copy link
Contributor Author

jsdpag commented Mar 28, 2019

Okay, thanks for checking. If there's anything else I can do then let me know ... though I'll have to head out now, it's late on this side of the sea.

@dashesy
Copy link
Collaborator

dashesy commented Mar 28, 2019

I suggest you contact Blackrock, unfortunately I can only test with nPlayServer which seems to be not helpful. In the weekend I will look at the nPlayServer code to see if it plays back digin

@jsdpag
Copy link
Contributor Author

jsdpag commented Mar 29, 2019

Blackrock said that the firmware had a bug that disrupted digital value transfer in versions v7.0.0 and v7.0.2. But they also said that the bug was fixed in v7.0.3 and the current v7.0.4. I guess I can ask them if this applies to our model of NSP. If there's any code changes you want tested with an NSP then I can try them out for you.

@cboulay
Copy link
Collaborator

cboulay commented Mar 29, 2019

I wrote a little test utility to help me to debug and pushed to master. Using NPlayServer and the (clinical) NSP both gave the same results: the channel number in the packet is wrong for both the digital and serial inputs. The channel number is wrong as early as ProcessIncomingPacket.

This doesn't seem to be a CereLink problem.

@jsdpag
Copy link
Contributor Author

jsdpag commented Mar 30, 2019

I commend your tidy piece of detective work, there. So, what is the culprit – the firmware? Does Blackrock know?

@cboulay
Copy link
Collaborator

cboulay commented Apr 1, 2019

On March 29 2019, I replied with the above information to the support ticket you opened with so I guess they know now.

@jsdpag
Copy link
Contributor Author

jsdpag commented Apr 2, 2019

Thanks! Now let's hope that they take notice ...

@cboulay
Copy link
Collaborator

cboulay commented Apr 23, 2019

Blackrock answered with some example code for identifying digital or serial channels based on channel capabilities. I'll put together a PR.

@cboulay
Copy link
Collaborator

cboulay commented Apr 24, 2019

I fell down a bit of a hole while working on this. It looks like we have to abandon the idea of checking channel number and instead use (some proxy of) channel type. The channel type can be learned from first using cbGetChanCaps and bitmasking the result with one of these cbCHAN_ capabilities. Further discretization can be achieved with type-specific capabilities using functions like cbGetDinpCaps.

The problem is that this is a bit expensive to do for every single packet. It would have to be done when the packet is directed along different code paths. It would have to be done again within certain code paths. It would have to be done yet again within the language wrappers. Instead of inferring channel type 3+ times per packet, it would be nice to calculate each channel's type ONCE, and then just use that stored info. I'm thinking of adding an int16_t m_ChannelType[cbMAXCHANS] to SdkApp somewhere around here, and then filling it during startup.

@dashesy , where would be a good place to fill this channel-type array? As far as I can tell, the only data structure that I need initialized is cb_cfg_buffer_ptr and that seems to happen in cbOpen, called during InstNetwork::run(). So I'm thinking of putting it here.

I'll have to define some new consts for channel types, write a new function in cbsdk that returns this array, and then the python wrapper for that function. Then we should be able to do something like if cbSdkGetChannelTypes(nInstance)[channel] == cbCHTYPE_DIGIN: do something.

Does that all sound reasonable?

@cboulay
Copy link
Collaborator

cboulay commented Apr 24, 2019

@jsdpag I'm almost there, I think. My test util is now printing out the following digital values, each row is an update iteration, and each value is the digital in value. Does this look correct to you?

There are some missing values (e.g. 74). I don't know what happened there.

 0
 1 2
 3 4
 5
 6 7
 8
 9
 10
 11
 12
 13 14
 15 16
 17 18
 19 20
 21
 22 23
 24 25
 26 27
 28 29
 30
 31 32
 33 34
 35 36
 37 38
 39
 40 41
 42 43
 44
 45
 46 47
 48
 49 50
 51 52
 53 54
 55 56
 57
 58 59
 60
 61
 62 63
 64 65
 66
 67 68
 69 70
 71 72
 73
 75
 76 77
 78 79
 80 81
 82 83
 84 85
 86
 87 88
 89 90
 91 92
 93 94
 95
 96 97
 98 99
 100 101
 102
 103 104
 105 106
 107 108
 109 110
 111 112
 113
 114 115
 116 117
 118 119
 120 121
 122
 123 124
 125 126
 127 128
 129 130
 131
 132 133
 134 135
 136 137
 138
 139 140
 141 142
 143 144
 145 146
 147
 148 149
 150 151
 152 153
 154 155
 156
 157 158
 159 160
 161 162
 163 164
 165 166
 167
 168 169
 170 171
 172 173
 174
 175 176
 177 178
 179 180
 181 182
 183
 184 185
 186 187
 188 189
 190 191
 192
 193 194
 195 196
 197 198
 199 200
 201 202
 203
 204 205
 206 207
 208 209
 210 211
 212
 213 214
 215 216
 217 218
 219
 220 221
 222 223
 224 225
 226 227
 228 229
 230 231
 232
 233 234
 235 236
 237 238
 239
 240 241
 242 243
 244 245
 246 247
 248
 249 250
 251 252
 253 254
 255 256
 512 768
 1024 1280
 1536 1792
 2048 2304
 2560
 2816 3072
 3328 3584
 3840 4096
 4352 4608
 4864 5120
 5376
 5632 5888
 6144 6400
 6656 6912
 7168 7424
 7680
 7936 8192
 8448 8704
 8960 9216
 9472 9728
 9984 10240
 10496
 10752 11008
 11264 11520
 11776 12032
 12288 12544
 12800
 13056 13312
 13568 13824
 14080 14336
 14592 14848
 15104
 15360 15616
 15872 16128
 16384 16640
 16896 17152
 17408 17664
 17920
 18176 18432
 18688 18944
 19200 19456
 19712 19968
 20224
 20480 20736
 20992 21248
 21504 21760
 22016 22272
 22528
 22784 23040
 23296 23552
 23808 24064
 24320
 24832
 25088 25344
 25600 25856
 26112 26368
 26624 26880
 27136 27392
 27648
 27904 28160
 28416 28672
 28928 29184
 29440 29696
 29952
 30208 30464
 30720 30976
 31232 31488
 31744 32000
 32256 32512
 32768
 33024 33280
 33536 33792
 34048 34304
 34560 34816
 35072
 35328 35584
 35840 36096
 36352 36608
 36864 37120
 37376 37632
 37888
 38144 38400
 38656 38912
 39168 39424
 39680 39936
 40192 40448
 40704
 40960 41216
 41472 41728
 41984 42240
 42496 42752
 43008 43264
 43520
 43776 44032
 44288 44544
 44800 45056
 45312 45568
 45824
 46080 46336
 46592 46848
 47104 47360
 47616 47872
 48128 48384
 48640
 48896 49152
 49408 49664
 49920 50176
 50432 50688
 50944
 51200 51456
 51712 51968
 52224 52480
 52736 52992
 53248 53504
 53760
 54016 54272
 54528 54784
 55040 55296
 55552 55808
 56064
 56320 56576
 56832 57088
 57344 57600
 57856 58112
 58368 58624
 58880
 59136 59392
 59648 59904
 60160 60416
 60672 60928
 61184
 61440 61696
 61952
 62208
 62464 62720
 62976 63232
 63488 63744
 64000
 64256 64512
 64768 65024
 65280

@cboulay
Copy link
Collaborator

cboulay commented Apr 24, 2019

Work-in-progress: https://github.com/dashesy/CereLink/tree/parse_by_caps
I still need to fixup the Python wrappers.

@dashesy
Copy link
Collaborator

dashesy commented Apr 24, 2019

I am curious how Blackrock handles digin channels.

@cboulay
Copy link
Collaborator

cboulay commented Apr 25, 2019

I am curious how Blackrock handles digin channels.

The new modifications to IsChanSerial and IsChanDigin are effectively identical to what Blackrock gave me, which means that they are using channel caps as well.

@cboulay
Copy link
Collaborator

cboulay commented Apr 25, 2019

In #104 I describe the problem with dropping event samples. I've fixed it with one approach, but I think I'd prefer the other approach I mention.

Here's some Python code I used to test:

from cerebus import cbpy
TESTCHAN = 151
if __name__ == '__main__':
    con_params = cbpy.defaultConParams()
    result, connect_info = cbpy.open(instance=0, connection='default', parameter=con_params)
    cbpy.trial_config(instance=0, reset=True, buffer_parameter={'event_length': 10000},
                      nocontinuous=True, nocomment=True)
    try:
        n_events = 0
        while True:
            result, data = cbpy.trial_event(instance=0, reset=True)
            for d in data:
                if d[0] == TESTCHAN:
                    timestamps = d[1]['timestamps'][0]
                    events = d[1]['events']
                    n_events += len(events)
                    print(n_events, len(events), timestamps, events)
    except KeyboardInterrupt:
        print('Shutting Down!')

    cbpy.close(instance=0)

Edit: Removed wheel. See pull request instead.

@cboulay
Copy link
Collaborator

cboulay commented Apr 25, 2019

Here is a printout from the above script, with each row formatted as
number_of_events_total, number_of_events_this_line, [timestamps], [data]:

1 1 [0] [0]
3 2 [73244 73421] [1 2]
5 2 [73598 73777] [3 4]
6 1 [73983] [5]
8 2 [74181 74351] [6 7]
9 1 [74521] [8]
10 1 [74696] [9]
11 1 [74873] [10]
12 1 [0] [11]
14 2 [172 345] [12 13]
15 1 [75568] [14]
17 2 [75742     0] [15 16]
18 1 [76086] [17]
19 1 [0] [18]
21 2 [76434     0] [19 20]
23 2 [76782     0] [21 22]
24 1 [0] [23]
26 2 [77304     0] [24 25]
28 2 [77652     0] [26 27]
30 2 [77999     0] [28 29]
32 2 [78339     0] [30 31]
33 1 [0] [32]

Notice how the timestamps jump around between something that seems to make sense and 0.
If I go into cbpy.pyx and turn off bActive during init, I get the following:

1 1 [0] [0]
3 2 [73244 73421] [1 2]
4 1 [73598] [3]
5 1 [73777] [4]
6 1 [73983] [5]
8 2 [74181 74351] [6 7]
9 1 [74521] [8]
10 1 [74696] [9]
11 1 [74873] [10]
12 1 [75049] [11]
13 1 [75221] [12]
14 1 [75394] [13]
15 1 [75568] [14]
17 2 [75742 75914] [15 16]
18 1 [76086] [17]
19 1 [76260] [18]
21 2 [76434 76609] [19 20]
22 1 [76782] [21]
23 1 [76956] [22]
24 1 [77129] [23]
25 1 [77304] [24]
26 1 [77477] [25]
28 2 [77652 77827] [26 27]
30 2 [77999 78169] [28 29]
32 2 [78339 78511] [30 31]
33 1 [78683] [32]

This looks better. So I don't really understand what resetting the clock during InitTrial is doing or why it might be desirable. At best it's inconsistent. I'll modify the Python wrapper so resetting the clock during InitTrial is a separate option and is False by default.

@jsdpag
Copy link
Contributor Author

jsdpag commented Apr 26, 2019

Hi Chad,
Sorry for the radio silence. The list of values is mostly right. 74 was missing but should be there. There's a 24576 missing, too. The zero is not in the .nev file, so I guess that it's specific to your test program. In any case, it looks like you've progressed impressively from this.

@cboulay
Copy link
Collaborator

cboulay commented Apr 26, 2019

I put python wheels for windows and Linux on the release page. Let me know how it goes for you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants