labstreaminglayer / App-LabRecorder

An application for streaming one or more LSL streams to disk in XDF file format.
MIT License
123 stars 45 forks source link

Update clirecorder.cpp #94

Open mark-shovman opened 1 year ago

mark-shovman commented 1 year ago

better CLI:

agricolab commented 1 year ago

Thanks for the PR. How do you envision to stop recording then from the command line, instead? Keeping execution in a while(true) loop makes it hard to gracefully shut down recording.

As a note, some users use a python wrapper for scripting recording, and changing would be a breaking change in LabRecorderCLIs "API". Not saying this can't be addressed, only highlighting that this is a breaking change.

mark-shovman commented 1 year ago

Well, we use kill(or taskkill in windows) - makes us less likely to accidentally stop recording by pressing a key in the wrong window. If that is a breaking change that we want to avoid, it can be made optional, with the current behaviour as the default - or removed altogether, it's not the main change here.

agricolab commented 1 year ago

Thanks.

About the change to add streams that are not found to watchfor (instead of returning 2), i feel it might be better if that becomes optional, too. Maybe an additional command line argument like --watch-missing-streams, that'd trigger the new behavior?

That could be also a solution for the wait-for-return versus kill/signal approach. Maybe add a command line argument like --to-background? At least on unix, it might be the case that adding & already does the trick...

tstenner commented 1 year ago

Not a thorough review, but from a quick glance the "signal handler" approach is better and even works in the console, you'd just need to switch to Ctrl+C when running it manually.

The while (true) std::this_thread::yield(); on the other hand needlessly wastes CPU cycles. Blocking on user input (while (true) std::cin.get();) or even reading in commands would be better, imho.