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

Update clirecorder.cpp #94

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mark-shovman
Copy link

better CLI:

  • accurate help message
  • streams that can't be found on start are added to wachfor
  • runs until exited, not until keypress

better CLI:
* accurate help message
* streams that can't be found on start are added to wachfor
* runs until exited, not until keypress
@agricolab
Copy link
Contributor

agricolab commented Mar 17, 2023

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
Copy link
Author

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
Copy link
Contributor

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
Copy link
Contributor

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.

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 this pull request may close these issues.

3 participants