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

add option to change debug/log level per invocation #61

Merged
merged 1 commit into from
Sep 18, 2018
Merged

add option to change debug/log level per invocation #61

merged 1 commit into from
Sep 18, 2018

Conversation

v0lker
Copy link
Contributor

@v0lker v0lker commented Sep 11, 2018

proposed implementation of #59

@@ -42,6 +43,7 @@ char help_full[] = \
"through the standard input.\n\n"
"Miscellaneous:\n"
" -v, --version display version information and exit\n"
" -l, --log set log level (0...100 recommended, 50 default)\n"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why recommended?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because other values don't make sense. actually, the lowest value that makes sense is 10 (ERROR and CRITICAL), the highest is 100.

i was trying to keep within the string length, but could elaborate, if you like...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah OK sorry.. I misinterpreted as if 100 was the recommended value.
Maybe we could just remove "recommended" and check that user don't pick something outside the range?

@jlelli
Copy link
Contributor

jlelli commented Sep 13, 2018

Apart from the nitpick, thanks for this LGTM. @vingu-linaro ?
Bonus point would be to add a commit on top implementing "translation" between magic numbers and more meaningful strings, e.g.. instead of

rt-app -l 75

would be nice to be able to write

rt-app -l info

@v0lker
Copy link
Contributor Author

v0lker commented Sep 13, 2018

i agree with the point about meaningful values. that should be done properly (with an array of LOG_LEVELs and their associated strings), though. would it be ok if i just state the levels explicitly in the help text, in the meantime?

@jlelli
Copy link
Contributor

jlelli commented Sep 13, 2018

would it be ok if i just state the levels explicitly in the help text, in the meantime?

Sure. Please add INFO and NOTICE in the help text above.

@v0lker
Copy link
Contributor Author

v0lker commented Sep 13, 2018

done that...

Signed-off-by: Volker Eckert <volker.eckert@arm.com>
@jlelli
Copy link
Contributor

jlelli commented Sep 18, 2018

It looks now good to me! Thanks!
@vingu-linaro you also OK for merging?

@vingu-linaro
Copy link
Member

LGTM too.
I agree that adding meaningful naming for log level in a next step, would be really helpful.

@jlelli jlelli merged commit 4ee1b96 into scheduler-tools:master Sep 18, 2018
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