openFPGALoader
openFPGALoader copied to clipboard
WIP: Do not compare verbosity level when passing value along.
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.
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.
I have pushed a serie of commit to fix verbose aspect. It's good to you?
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.
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.
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.
_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)).
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.
-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.