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

What should RecorderLib look like? #11

Open
tstenner opened this issue Dec 13, 2018 · 5 comments
Open

What should RecorderLib look like? #11

tstenner opened this issue Dec 13, 2018 · 5 comments

Comments

@tstenner
Copy link
Contributor

There are two different approaches to a recorderlib:

Tristan's approach:

  • have separate, well-tested libraries for the absolutely necessary parts, namely handling streams (liblsl) and writing xdf files (libxdf, already present in this repo)
  • recorder applications have to
    • resolve a list of streams from liblsl and pick some of them
    • create an xdfwriter with a suitable filename
    • subscribe to the streams and (in a loop) acquire data, save it to the xdf file

Pros

  • clients can use all the languages bells and whistles, e.g. cross-thread signals and slots for C++/Qt, exceptions with the offending part present in the stack trace, properties bound to the UI in Java etc.
  • the API isn't limited by what can be expressed in a C ABI
  • end users / developers can adjust the code without knowing C(++)
  • the common tasks have a much smaller API surface and can be tested better in isolation (e.g. the xdfwriter)
  • different requirements call for different languages and user's requirements correlate with the languages they choose, e.g.:
    • "just record these streams and stop some time later, bonus for easy redistribution" with the C++ labrecorder
    • maximum flexibility (remote control of required streams, changing filenames mid-experiment and notify some presentation software once all streams are present), but a complete Python distribution is required
  • unlike liblsl, recorder apps are mostly standalone and therefore can use a language appropriate for the task, instead of needing to integrate with a larger application / vendor SDK

Cons

  • duplicated code, efforts and bugs for common parts

Chad's approach:

Copied from Chad's comment:

For now, if Lab Recorder functionality is needed on Android, the developer would need to make a Java wrapper around some sort of Lab Recorder library. Is someone currently building such a library?

Tristan and I are not in agreement of what this library should look like.
I hope I'm not misrepresenting him, but I think he's of the opinion that the library-consuming client application should do quite a bit, including handling the data, handling the arrival of new streams, using signals and slots, etc.
I'm of the opinion that the library should do just about everything and the client should do as little as possible, maybe limited to things like get_list_of_stream_identifiers(*stream_ids, *stream_strings), set_stream_recording_status(stream_id, on_or_off), get_likely_filename(some, params), start_recording(filename), stop_recording(), get_recorder_status(*state_struct). The client will never touch the data or know for sure the data are being recorded; it only knows what the state_struct tells it and only when it asks for that info.
Tristan doesn't like my approach because it's not responsive to errors and because it'll make debugging difficult.
The reason I don't like Tristan's approach is because it will require repeating a lot of functionality for each implementation. The list of implementations that we feel are necessary are C++, Python, Java, maybe something for iOS, and I think there was a good reason to have C# but I can't remember right now.

@mgrivich
Copy link
Contributor

I see the primary purpose of this task is to separate the LabRecorder back end from the front end. No more and no less. As such, Chadwick's approach more completely fulfills that goal. It also keeps the back end of the code from crossing language barriers, which is a huge complexity savings.

However, it should be done such that the client can get full status and error information, either polled (probably easier with the C ABI) or event driven (probably more fault tolerant). For the command and status blocks, I recommend JSON strings, which allows easier integration with other systems and future extendability.

Tristan's approach allows people more control. But at the end of the day, if people want that level of control they should probably just pull out the bits and pieces that they want (from the source code), and integrate them however they want. The future LabRecorder library source will give them a good starting point.

@tstenner
Copy link
Contributor Author

I see the primary purpose of this task is to separate the LabRecorder back end from the front end. No more and no less.

The more I work on things like remote control, better handling of existing files and so on the more the separation between the recorderlib and the rest of the C++ labrecorder gets in the way.

To add the original intent from the discussion on slack:

I've been thinking about the LabRecorder situation and I think reviving the old Python LabRecorder would be a good idea
There are two camps: the 'I want to press record and be done with it' and the 'I want all the bells and whistles' people
We will never make catch up with the wishes of the second camp, so a plain unzip-and-run C++ LabRecorder for the former camp and a scriptable Python LabRecorder with BIDS, remote control and what not seems more realistic to me

One of the problems I don't see yet how to properly handle with one big lib:

For example, if the target file can't be written (e.g. due to lacking permissions, or if it already exists), the call to xdfwriter::xdfwriter can throw an exception that can be caught and handled (e.g. present a dialog box "File already exists, a) move old file, b) save as 'suggested name', c) cancel) when it makes sense
With one class for everything, we'd either need to catch and categorize every possible exception in the library and then throw an exception without a stacktrace or the original error in the interface or just say "there's been an error but we don't know what"
And making a function for every step (e.g. rec = recorder(); rec.openFile("foo.xdf"); rec.addStreams(...); rec.startRecording();) is just waiting for someone to call them in the wrong order or forgetting something

@mgrivich
Copy link
Contributor

mgrivich commented Dec 14, 2018

I see the primary purpose of this task is to separate the LabRecorder back end from the front end. No more and no less.

The more I work on things like remote control, better handling of existing files and so on the more the separation between the recorderlib and the rest of the C++ labrecorder gets in the way.

It's going to be hard either way. Chadwick and I believe that more isolation will be easier overall and in the long run. Also, note that the more isolated design is more naturally portable regardless of the language, UI toolkit, and location of the UI (in the same application or across the network).

One of the problems I don't see yet how to properly handle with one big lib:

For example, if the target file can't be written (e.g. due to lacking permissions, or if it already exists), the call to xdfwriter::xdfwriter can throw an exception that can be caught and handled (e.g. present a dialog box "File already exists, a) move old file, b) save as 'suggested name', c) cancel) when it makes sense

This can be implemented with
json_info_string start_recording(string filename)
The JSON string includes a (function dependent) integer error code and message, which the caller can handle. Caller handling should depend on the code, not the message (which is meant to be human, not machine readable).

During recording
json_info_string query_status()
The JSON string returns the status of all streams, as well as any errors thrown.

With one class for everything, we'd either need to catch and categorize every possible exception in the library and then throw an exception without a stacktrace or the original error in the interface or just say "there's been an error but we don't know what"

Common and expected errors can be categorized. Unexpected exceptions can return exception::what().

And making a function for every step (e.g. rec = recorder(); rec.openFile("foo.xdf"); rec.addStreams(...); rec.startRecording();) is just waiting for someone to call them in the wrong order or forgetting something

There is no solution that will prevent the user from being able to make errors. Appropriate and helpful error messages should be returned when mistakes are made. Also, overall your solution demands significantly more competency on the part of the downstream developer, so this objection is rather strange.

@cboulay
Copy link
Contributor

cboulay commented Dec 14, 2018

For example, if the target file can't be written (e.g. due to lacking permissions, or if it already exists), the call to xdfwriter::xdfwriter can throw an exception that can be caught and handled (e.g. present a dialog box "File already exists, a) move old file, b) save as 'suggested name', c) cancel) when it makes sense.

The way I envisioned this scenario is that the library function would have a bool rename_if_exists arg to rename existing files and use the provided filename anyway (i.e., the current LabRecorder behaviour). If that option is not set and the provided filename exists then the function returns something non-zero. Sure it's possible the user will not pay attention to this return value and then wonder why nothing is being recorded, but when there are only 5-10 functions exposed and they are all straightforward, expecting users to adhere to a simple API is IMO much more realistic than expecting them to use the programming techniques you're advocating or expecting busy researchers to maintain this.

I can propose a compromise:
You can write the fancy library that does all of the things you desire. I'll write a C API around that (we may have to push-and-pull a bit to keep the C++ lib reasonably easy to wrap), and at least a couple of the interfaces. You're welcome to make a LabRecorder application using your lib and you'll be expected to maintain it for the rest of eternity. Anyone else that wants custom functionality not in your application will be referred to the C-API and other language interfaces.

@tstenner
Copy link
Contributor Author

Also, note that the more isolated design is more naturally portable

That's true, but how compared to liblsl, the recorderlib doesn't need to be portable. Most users will use a PC with a graphical user interface to record the data and some might want to record on mobile devices (android/ios?). Also, the integration with other programs everyone wants is mostly limited to "wait for streams, start recording, stop recording".

regardless of the language,

Will there be any language other than C++, Python and maybe (if Qt on Android stays a pipe dream) Java?

UI toolkit

That's a good point against Qt's signals and slots as core part of the recorderlib.

and location of the UI (in the same application or across the network).

What did you have in mind? A browser based UI could be nice, but that should be covered by Python + some library.

This can be implemented with
json_info_string start_recording(string filename)
The JSON string includes a (function dependent) integer error code and message, which the caller can handle. Caller handling should depend on the code, not the message (which is meant to be human, not machine readable).

JSON has it's place, but even in this simple example the pseudocode for that would look like:

char* json_info_string = start_recording("filename.xdf", /*rename_if_exists*/ true);
json* result;
int jsonparseresult = json_parse(json_info_string);
if(!jsonparseresult) {
    // Handle the JSON parsing error, don't forget to free json_info_string
}
recorderlib_free_string(json_info_string);
if(json_has_field(result, "status")) {
    int code = json_get_field(result, "status");
    switch(code) {
        case 1: // Do stuff, don't forget to free result before returning
             break;
        // ......
    }
} else // Error, don't forget to free result before returning
json_free(result);

We're at a minimum of 16 lines (+ a JSON parsing library) for a single function call with all the fun of manual memory management.

During recording
json_info_string query_status()
The JSON string returns the status of all streams, as well as any errors thrown.

The recorderlib needs to keep track of all streams anyway, but we'd need additional code to serialize the stream info into JSON. We could have a lsl_inlet* get_stream(const char* id) to get a stream with a JSON ID, but in case a stream is lost and rediscovered on another computer that pointer wouldn't be valid anymore. It's possible to do this in C, but it's not pretty, there will be a lot more than 5-10 functions, it won't be straightforward anymore and the library developers (i.e. us) will have to solve threading problems that are pretty much solved in C++, Python and Java in C.

You can write the fancy library that does all of the things you desire. I'll write a C API around that (we may have to push-and-pull a bit to keep the C++ lib reasonably easy to wrap)

I'd rather start from the other end: we write code samples for the library users in pseudocode and then evaluate, whether that's something that should be exposed and if it's possible / reasonable with a C / C++ API

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

No branches or pull requests

3 participants