sccn / liblsl

C++ lsl library for multi-modal time-synched data transmission over the local network
Other
108 stars 63 forks source link

Incorrect docstring for max_buflen #120

Closed cboulay closed 3 years ago

cboulay commented 3 years ago

The docstring for max_buflen is correct at the level of the inlet, where it says it's a number "in seconds" for streams with a nominal rate.

https://github.com/sccn/liblsl/blob/b3c5886b11f7aa3a9025901c371faca271dde948/include/lsl/inlet.h#L20-L21

The inlet almost immediately transforms that number to "samples" and passes it as the second argument to creation of a stream_inlet_impl here: https://github.com/sccn/liblsl/blob/973606e65181d2ab6ffa9418b59c4e6e752b1ad0/src/lsl_inlet_c.cpp#L13-L15

stream_inlet_impl has a very similar docstring and unfortunately also says this argument is "in seconds" even though it is now in samples: https://github.com/sccn/liblsl/blob/973606e65181d2ab6ffa9418b59c4e6e752b1ad0/src/stream_inlet_impl.h#L27-L30

(when fixing this we may want to think about a new default value here)

Let's dig down a bit deeper.

max_buflen (now in samples, though the docstring says seconds) is passed as the 2nd arg to datareceiver, The data_receiver constructor docstring also says this is in seconds! https://github.com/sccn/liblsl/blob/973606e65181d2ab6ffa9418b59c4e6e752b1ad0/src/data_receiver.h#L32-L35

This is probably just a docstring problem because the data receiver implementation uses max_buflen, without any further modification, to initialize sample_queue_ (an instance of consumer_queue), which expects an integer number of samples: https://github.com/sccn/liblsl/blob/973606e65181d2ab6ffa9418b59c4e6e752b1ad0/src/data_receiver.cpp#L28

I discovered this docstring inconsistency while trying to hunt down another bug, wherein setting an inlet's max_buflen to a reasonably large number (e.g. 30) would crash for streams with low-ish sampling rates (about 1 Hz). I think the docstring inconsistency is just that, and not the source of my bug. I'll open another issue for that.

cboulay commented 3 years ago

I updated the docstrings in #120.

There's still the minor issues that the default value of max_buflen in stream_inlet_impl and data_receiver is about 100x lower than the default value when supplied by lsl_inlet. But, in the very unlikely chance that some piece of software was using stream_inlet_impl directly (how?) and it was on a minimum-resource system where allocating 10x as much buffer would break it, I'll leave the defaults as they are.