openFPGALoader icon indicating copy to clipboard operation
openFPGALoader copied to clipboard

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

Open inkdot7 opened this issue 11 months ago • 8 comments

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.

inkdot7 avatar Jul 29 '23 07:07 inkdot7

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 avatar Jul 30 '23 05:07 trabucayre

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

trabucayre avatar Jul 30 '23 06:07 trabucayre

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.

inkdot7 avatar Jul 30 '23 07:07 inkdot7

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.

trabucayre avatar Jul 30 '23 07:07 trabucayre

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.

inkdot7 avatar Jul 30 '23 07:07 inkdot7

_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)).

trabucayre avatar Jul 30 '23 10:07 trabucayre

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.

inkdot7 avatar Jul 30 '23 15:07 inkdot7

-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.

trabucayre avatar Jul 31 '23 14:07 trabucayre