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

WIP: Do not compare verbosity level when passing value along. #357

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

Conversation

inkdot7
Copy link
Contributor

@inkdot7 inkdot7 commented Jul 29, 2023

I was not able to see the display() message FTDIpp_MPSSE::open_device (src/ftdipp_mpsse.cpp), even with e.g. --verbose-level 4. As far as I could tell, this is due to the user-given verbosity level being compared with fixed levels several times as it is passed to the FTDIpp_MPSSE constructor.

It is unclear why I had to remove both comparisons to see the display() message. So the PR is more to easily point at the locations in the code, and not for direct use. It is not much tested.

@trabucayre
Copy link
Owner

Thanks to point this error.
In fact for, at least, with ftdi device a more large rework has to be done. After re reading this part I have more or less a clear idea on how to rewrite/fix verbose aspect.

@trabucayre
Copy link
Owner

I have pushed a serie of commit to fix verbose aspect.
It's good to you?

@inkdot7
Copy link
Contributor Author

inkdot7 commented Jul 30, 2023

Thanks! I now see the FTDIpp_MPSSE::open_device display() message with --verbose-level 3. Is it intentional to require level 3 here, or was it supposed to be 2, which is debug according to the --help text? (3 seems not to be defined.) When grepping for verbose\s*> in the code, this (FTDIpp_MPSSE::FTDIpp_MPSSE) is the only place I find which compares to be > 2.

I saw someplace in the code a verbose_lvl variable name. As there are both bool and int8_t verbose variables, perhaps it would make things clearer if the ones holding a level are called verbose_lvl and the boolean kinds just verbose (with prefixes _ as usual).

The level comparisons may be easier to follow if they compare with >= and the level they require instead of > and the level that has to be exceeded.

@trabucayre
Copy link
Owner

In fact I have used level 3 because this piece of code may produces a real flood. I'm agree this is undocumented (maybe have to add something like 3 -> flood).
verbose_lvl is only present at CTOR level for DFU class, and it is used to set _debug and _quiet.
I'm agree <= may be better/clear.

@inkdot7
Copy link
Contributor Author

inkdot7 commented Jul 30, 2023

In fact I have used level 3 because this piece of code may produces a real flood.

It is other messages than the FTDIpp_MPSSE::open_device() display() message that can flood?

Would it make sense if display() takes an additional argument of level and then per message chooses to print if the overall requested level is at least that? Then messages that do not cause flooding can be shown already at lower levels.

@trabucayre
Copy link
Owner

_verbose is also used by subclasses and most of messages are located here.
It's maybe a better idea to have a level for this class and another one for subclass (or simply compare verbose value at each class level (but it's add lot of tests/overhead)).

@inkdot7
Copy link
Contributor Author

inkdot7 commented Jul 30, 2023

Is there a way for a user to set the verbosity of various components separately?

Even with that, if something is not working, a user likely would not know which component may be giving the problem, so would likely just like to try to gradually turn a global verbosity level up in the hope of getting a message that might give a hint. Repeated -v could just act to increase that level.

With a global verbosity level, it need not be passed around to the various components. Each message location can decide if printing the message is desired. And where needed, perhaps protect the message generation. But typically, the CPU overhead of such comparisons should be negligible.

@trabucayre
Copy link
Owner

-v is more or less a shortcut for --verbose-level=1, so it's possible by setting level to increase volume of messages.
I think a complete rework of the verbosity policy has to be done, and yes it's required, for each class, to let this one decide message to display according to level. It's why the verbose information is a int8_t everywhere.

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.

None yet

2 participants