brndnmtthws / conky

Light-weight system monitor for X, Wayland (sort of), and other things, too
https://conky.cc
GNU General Public License v3.0
7.26k stars 620 forks source link

'Bad arguments: " unk" and '59"' #650

Closed ghost closed 5 years ago

ghost commented 6 years ago

Sometimes my Conky generates the error given in my title. The line in my conky script that - intermittently - causes the error is:

${if_match ${wireless_link_qual_perc wlp2s0}>59}\

or, with a little context:

${if_up wlp2s0}\
${if_match ${wireless_link_qual_perc wlp2s0}>59}\

Here are some details of my setup.

$  conky --version
conky 1.11.0_pre compiled Sun 19 Aug 22:24:17 BST 2018 for Linux 4.18.1-041801-generic x86_64

Compiled in features:

System config file: /etc/conky/conky.conf
Package library path: /usr/local/lib/conky

 General:
  * math
  * hddtemp
  * portmon
  * IPv6
  * wireless
  * support for IBM/Lenovo notebooks
  * eve-online
  * builtin default configuration
  * old configuration syntax
  * Imlib2
  * OSS mixer support
  * apcupsd
  * iostats
  * ncurses
  * Internationalization support

 Lua bindings:
  * Cairo
  * Imlib2
  * RSVG
 X11:
  * Xdamage extension
  * Xinerama extension (virtual display)
  * Xshape extension (click through)
  * XDBE (double buffer extension)
  * Xft
  * ARGB visual
  * Own window

 Music detection:
  * CMUS
  * MPD
  * MOC

 Default values:
  * Netdevice: eth0
  * Local configfile: $HOME/.conkyrc
  * Localedir: /usr/local/share/locale
  * Maximum netdevices: 64
  * Maximum text size: 16384
  * Size text buffer: 256

Linux Mint 19 x64 Cinnamon.

lasers commented 6 years ago

I don't know C/C++, but this is the code... ./src/net_stat.cc:430.

void print_wireless_link_qual_perc(struct text_object *obj, char *p, unsigned int p_max_size) {
  struct net_stat *ns = (struct net_stat *)obj->data.opaque;

  if (!ns) return;

  if (ns->link_qual_max > 0) {
    spaced_print(p, p_max_size, "%.0f", 5,
                 (double)ns->link_qual / ns->link_qual_max * 100);
  } else {
    spaced_print(p, p_max_size, "unk", 5);
  }
}

It looks like it didn't get a number so it printed the else statement "unk" then you got ${if_match unk>59} which probably gave you 'Bad arguments: " unk" and '59".

@su8 might be able to give you something to print the values so you can see what's causing the else statement in first place. It could be blank lines.

ghost commented 6 years ago

Thanks.

So 'unk' stands for 'unknown'?

lasers commented 6 years ago

Yes.

lasers commented 6 years ago

I can't test this. unk shows up for me here.

git clone https://github.com/brndnmtthws/conky
cd conky
* Save `src/net_stat.cc`.
```bash
mkdir -p build
cd build
cmake ..
make -j4  # 4 cores to run in parallel
ghost commented 6 years ago

OK, I'm running that altered version. I'll post any relevant output - which will appear only in the terminal, yes?

Thanks for the 'j4' point - i didn't know about that, and it really sped up the compilation of Conky.

lasers commented 6 years ago

I'll post any relevant output - which will appear only in the terminal, yes?

To clarify... If you're seeing numbers same as ${wireless_link_qual_perc} in the terminal right now, then the code is good... Leave it as-is. If you're seeing blank lines or something else that is not same as ${wireless_link_qual_perc} then I'm a little off somewhere with this untested code.

When you get your bad argument, then look in the terminal to see what was printed between ${wireless_link_qual_perc}. Maybe we should get rid of unk in favor of 0 next.

ghost commented 6 years ago

have no relevant output at all - but only this:

conky: desktop window (3a02bd5) is subwindow of root window (1a0)
conky: window type - desktop
conky: drawing to created window (0x6e00002)
conky: drawing to double buffer

Nor does anything change if I use this code:

printf("test");
if (!ns) return;
printf("test2");
printf("%s\n", p);

However, that code is compiling - for, if I introduce a mistake into it, the compiler complains and the compilation fails.

lasers commented 6 years ago

Hmm. Maye you need this instead of cmake ...

cmake -D CMAKE_BUILD_TYPE=Release -D MAINTAINER_MODE=ON -D BUILD_WLAN=ON ..

Then your printf("test") should work... I assume we'd see this error. Maybe it's because the variable is inside if_match...?

ghost commented 6 years ago

I (we?) were altering the wrong function. The one we need to alter is print_wireless_link_qual_perc.

Having altered the right function, my output is as follows.

conky: desktop window (3a00023) is subwindow of root window (1a0)
conky: window type - desktop
conky: drawing to created window (0x6e00002)
conky: drawing to double buffer
test1test2
test1test2
test1test2
test1test2
test1test2
test1test2
test1test2

So - yes? (I too have hardly any C) - the probematic state is not occuring. It occurs only occasionally anyway. So perhaps - unless the problem masks a deeper difficulty - we should make nothing happen if, at least for one time only, the function returns the string unk.

lasers commented 6 years ago

Sure. Whatever helps you find the root of the problematic state. Find out what is causing unk in first place. The fix could be to cache and return the last percent similar to this PR #646.

ghost commented 6 years ago

Whatever helps you find the root of the problematic state. Find out what is causing unk in first place.

That sounds like a fix.

The fix could be to cache and return the last percent similar to this PR #646.

That sounds like a work-around!

For a few days, I'll try to capture output from the line you inserted. If I do not get any, then something like that PR would keep the console tidy. But (1) I am unsure I am in a position to code it myself, (2) it would achieve nothing except suppressing the admittedly temporary error message.

lasers commented 5 years ago

@CottonEaster Are you still able to reproduce this? I'm wondering about a simple fix... as I don't know how ${if_match would deal with an empty string. It's possibly ideal too.

diff --git a/src/net_stat.cc b/src/net_stat.cc
index 63a6461..f75b39c 100644
--- a/src/net_stat.cc
+++ b/src/net_stat.cc
@@ -445,7 +445,7 @@ void print_wireless_link_qual_perc(struct text_object *obj, char *p,
     spaced_print(p, p_max_size, "%.0f", 5,
                  (double)ns->link_qual / ns->link_qual_max * 100);
   } else {
-    spaced_print(p, p_max_size, "unk", 5);
+    spaced_print(p, p_max_size, "", 5);
   }
 }
 double wireless_link_barval(struct text_object *obj) {
ghost commented 5 years ago

I forgot about this. However, with conky 1.11.0_pre (and, admittedly some changes to my networking setup) I have not seen this problematic output in a long time.

lasers commented 5 years ago

No problem. I just tried this and it doesn't work either. I think it should print 0 or -1 then... on account of percents. Easy fix, @brndnmtthws.

conky.config = {
    out_to_console = true,
    out_to_x = false
}
conky.text = [[
${if_match ${exec echo -n}>59}Hello, World!${endif}
]]
brndnmtthws commented 5 years ago

The problem is that you're comparing a string unk to an integer 59. I guess the sensible behaviour should be to either:

lasers commented 5 years ago

@brndnmtthws What's in... ns->link_qual_max if not over 0?

if (ns->link_qual_max > 0)

If it's a zero or negative value, then it should work okay too? EDIT: Use it as-is?

brndnmtthws commented 5 years ago

Good question, I'm not sure about that. I guess there are 2 problems here: one is handling the bad comparison, and the other is understanding why ns->link_qual_max is not >0.

However, given the fact that it seems to be a qualitative measure of the wireless link, I wouldn't be surprised if the data is occasionally inconsistent.

lasers commented 5 years ago

and the other is understanding why ns->link_qual_max is not >0.

It's likely there to avoid division by zero (i.e. 0.0/0 * 100) on account of unsupported / unk devices.

lasers commented 5 years ago

@Tomunu Easy fix :question:

Tomunu commented 5 years ago

After looking through the code, it looks like the link quality values that get used to calculate the printed values get updated only if the data can get read correctly from iwlib.

When testing on my system, link_qual_max was a steady 70 the entire time, but it sounds like the problem might not be happening as much anymore (or at all) for anyone else either.

I see two options:

  1. We could say that link_qual_max cannot reasonably be less than 1. So if the value read from iwlib is less than that, the value is invalid and link_qual_max will not get updated. (This could also apply to the link_qual value that we also get from iwlib, saying that it must be greater than or equal to zero, but I don't know if that ever happens.)

  2. We could say that any integer (including zero or negative numbers) is valid for link_qual_max and that we store the value exactly as it is read, and apply any logic only when we need to display the value. Then when it's time to print the value, we would need to decide what to do with a value less than 1. Probably just use a value of 1.

As long as the problem actually occurs intermittently, Option 1 works the best, because the blip just gets ignored and we get another chance to read the value at the next update.

Even if, on some system, the link_qual_max value is never greater than zero, then Option 1 still makes sense, because picking a positive default value for it would make the calculated value of wireless_link_qual_perc basically meaningless.

If we want to change the behavior, I think that we should go with Option 1. (I left up Option 2 just to show my reasoning.)

So, if we decide to do that, then yes, I think it's an easy fix.

lasers commented 5 years ago

When testing on my system, link_qual_max was a steady 70 the entire time, but it sounds like the problem might not be happening as much anymore (or at all) for anyone else either.

Does this still happen on disconnected? (Also, can assign it 0 during testing)

So if the value read from iwlib is less than that, the value is invalid and link_qual_max will not get updated

We could print 0 as-is? If link_qual_max aims to be an integer number, then I think it shouldn't change to string later... which messes with if conditions. Same for any other integer numbers that might turn into strings later. Invalid batteries could be printed 0 on computers to keep things simplified.

So, if we decide to do that, then yes, I think it's an easy fix.

@brndnmtthws... Option 1 or Option 2? Thank you. :blush:

brndnmtthws commented 5 years ago

Let's go with option 1, that seems reasonable.

I don't like the idea of finagling the data, but it sounds like this is the most user friendly approach.

Tomunu commented 5 years ago

When not connected to any wireless network or when the wireless interface is turned off, the max quality value is still read correctly and is the same value (70 on my system) as when connected, but the quality value does not get read. So I don't know if the bad value that started this whole problem was 0 or negative.

Either way, I'll implement Option 1, so link_qual and link_qual_max will get updated only with valid data.

Tomunu commented 5 years ago

Update: I've implemented the fix, but I want to wait until the PR pipeline clears up.