fix: prevent cpu watts overflow

[vandabbin]

Fixes: #1478

It seems sometimes the sensor can give a bad value that gets calculated as a large negative number (ex. -64759.826).

Example

This PR prevents the bad value from breaking formatting and overflowing the screen by using the last good value if the new reading is negative or greater than 9999

Additionally the checks for decimal precision for both the CPU and GPU watts had a rounding issue that could cause a one character overflow.

• Values between 9.95 - 9.99... and values between 99.95 - 99.99... would be below 10 and 100 so they would be allowed 2 and 1 decimal places even though when they are rounded they would become 10 and 100 and thus have one more decimal place then can fit in the alloted space.

Old

If the value is 10,000 or greater then the value is abbreviated and ends in KW

If cpu.usage_watts is a negative value then bad is printed to the screen instead of the watts. When watts value becomes good again then it prints it.

I think the cause of the bad value is that get_cpuConsumptionUJoules() can sometimes return 0. If that happens then this line would return a negative number.

auto watts = (float)(current_usage - previous_usage) / (float)(current_timestamp - previous_timestamp);

In fact if this is where the negative number is originating and it is because current_usage is 0 then the reason that the next collection cycle isn't wildly inaccurate is because if previous_usage is 0 then it retrieves it from get_cpuConsumptionUJoules() before getting current_usage again so I'm almost certain that this must be what is happening. I suppose if current_usage value is 0 then this function could just try retrieving it again (along with the new timestamp) however if it did end up failing a second time in a row the screen output would still overflow so that code would still be needed. And I don't think it is a good idea to loop over it until it does get a good value. I guess a middle ground could be trying up to 5 times or something? I have added a second commit that does this. Another possibility is that if it fails 5 times in a row it could set supports_watts to false. screen redraw would be need for that though

This need testing on systems that display cpu watts as mine don't and I had to test using static values. However I do think that I have eliminated any way for the cpu watts value to be larger then 4 characters and overflow the box. And eliminated the most obvious bad wattage values. (negative or 5-digit positive)

I will wait to mark this ready for review after it has been tested by the initial issue reporter

edit: After receiving information from the issue reporter. I have learned that my attempt to fix this issue inside the collection failed. 5 attempts within such a small window was just not enough. It would likely need more time for the reading to become good again and that doesn't seem like a good idea. So instead I have removed that portion and left only the part that prevents an overflow in the Draw code.

[deckstose]

Can you add a comment in code so it's obvious why a retry is needed?

[vandabbin]

Can you add a comment in code so it's obvious why a retry is needed?

absolutely!

[vandabbin]

@deckstose This is ready now. The collection fix didn't work but the overflow prevention in Draw does work.

[deckstose]

Fix is looking good to me! 🏅

[deckstose]

AS K is formatted with cwatts, K and W will be styled differently, is that on purpose?

[vandabbin]

AS K is formatted with cwatts, K and W will be styled differently, is that on purpose?

Oof no that was not intentional. I completely missed that. I'll make it not colored different then W

[vandabbin]

AS K is formatted with cwatts, K and W will be styled differently, is that on purpose?

Oof no that was not intentional. I completely missed that. I'll make it not colored different then W

My quick fix for this was to just add Theme::c("main_fg") into that line. but then Theme::c("main_fg") is potentially printed twice. Should I make it not print the second Theme::c("main_fg") if it is using the K? Or should I move the K to the main print line and add an expression to check for the need for it there too?

[deckstose]

is_kilo ? "K" : ""?

Definitely not print the color twice.

[vandabbin]

is_kilo ? "K" : ""?

Definitely not print the color twice.

Alrighty! Sounds good I've changed it so that KW or W are the variable cpost

[vandabbin]

@deckstose apparently there is still an issue with this. Give me a moment to figure out what is happening

edit: I think I have fixed it though I have to wait to hear back from the reporter. I think maybe it actually is possible for it to report a bad positive number as well. So I made it report bad if the number is 1 million or greater also.
Additionally I had screwed up my math. Now extra large values would be correctly displayed instead of 123,456 becoming 12KW it becomes 123KW haha

This means that CPU watts can only be displayed between a range of 0 - 999,999 (999KW) watts. No 1 million watt cpus allowed 😆

[vandabbin]

Also I figure I should ask other opinions. I left no space between bad and W if the value is bad. I did this intentionally to indicate that they are together. However if you think that doesn't look great. I can make sure there is a space between them.

I could also get rid of the W altogether however then it isn't clear what the bad is referring too

And does me having KW as a unit for 10,000 or greater watts make sense or should those values just show as bad as well? I don't think that kind of power consumption is realistic however for all I know it could happen

[sjclayton]

@vandabbin

I kind of think it looks better with the padding between bad and W... but will have to see what others say.

Also, the logic for having KW start at 10,000 is a bit odd... being as kilowatts start at 1000W, so if there was a system drawing that much or more it could be abbreviated (i.e. 1KW, if exactly 1000 watts is used... and something like 1.55KW in the case of 1550W).

The highest TDP of any single CPU in production today is 500W (AMD EPYC 9965), so if a system had more than one of those, only then you'd end up in the KW territory.... but as far as reaching 10KW, I don't think it's likely to happen anytime soon... and certainly no common desktop class CPU is ever likely to get into the KW range.

[vandabbin]

@sjclayton

I kind of think it looks better with the padding between bad and W... but will have to see what others say.

Thanks for your thoughts on this!

Also, the logic for having KW start at 10,000 is a bit odd... being as kilowatts start at 1000W, so if there was a system drawing that much or more it could be abbreviated (i.e. 1 KW, if exactly 1000 watts is used... and something like 1.55KW in the case of 1550W).

Ahh yes, the reason I chose to start KW at 10,000 was because the space allows for 4 digit numbers (or 3 digits and a decimal). With KW that drops to 3 digits (or 2 and a decimal). When for example 1553W was converted to KW it would be 1.5KW and would lose the precision of the last 53 watts even though there was space to display it. That was my thoughts on it anyway.

The highest TDP of any single CPU in production today is 500W (AMD EPYC 9965), so if a system had more than one of those, only then you'd end up in the KW territory.... but as far as reaching 10KW, I don't think it's likely to happen anytime soon... no common desktop class CPU is ever likely to get into the KW range.

Yeah this is a very good point and I think I agree with you. I think the reason I had decided to display values that high was because technically it could already (but would overflow) and while I couldn't think of any CPU that would use that could handle so much power, I thought maybe there would be some extreme overclocker that gets the power up that high even if very unrealistic.

I appreciate your feedback! I think you have convinced me to drop KW and use bad for values that high but I'd like to hear @deckstose thoughts on it as well.

[deckstose]

Agree, with padding looks good (maybe we can explore the string 'err' and/or if dimmed text looks better for the error.

Also, the logic for having KW start at 10,000 is a bit odd... being as kilowatts start at 1000W, so if there was a system drawing that much or more it could be abbreviated (i.e. 1KW, if exactly 1000 watts is used... and something like 1.55KW in the case of 1550W).

Agreed, but I don't mind it either way. If it fits on the screen and the value is right I don't have a string opinion about it.

[...] I thought maybe there would be some extreme overclocker that gets the power up that high even if very unrealistic.

Even then you'd need a battery (laptop) that reports power statistics. I don't know of any mainstream platform with a power supply that does this.

---

If I understand the issue correctly, this is a temporary bad value reported by the system, right? What about not changing the shown value until the next good value is read? This might work and be more comprehensible for a user?

[vandabbin]

Agree, with padding looks good (maybe we can explore the string 'err' and/or if dimmed text looks better for the error.

Oh good to know. I will add padding to it. I will also test with the string err and with dimmed text. You might be right that dimmed text could look better.

Agreed, but I don't mind it either way. If it fits on the screen and the value is right I don't have a string opinion about it.

Cool. I will be considering dropping KW in favor of bad or err while experimenting with the color and string value.

[...] I thought maybe there would be some extreme overclocker that gets the power up that high even if very unrealistic.

Even then you'd need a battery (laptop) that reports power statistics. I don't know of any mainstream platform with a power supply that does this.

Hmm.. I thought that this wattage reading was separate from the battery one. I have a laptop that shows the battery wattage but it doesn't show a value for cpu watts.

If I understand the issue correctly, this is a temporary bad value reported by the system, right? What about not changing the shown value until the next good value is read? This might work and be more comprehensible for a user?

That is correct. I could make it not change until a good value is read. In fact that is what I initially did but then I changed it to this because I thought (maybe falsely) that it would be better to show an accurate representation of the current collection even if it contained a bad value vs showing an old value that may or may not represent the current state. You may be right that the user wouldn't care about that though and that using bad or err might just be more confusing to them. In that case I will change to using the last good value again.

[deckstose]

We need a design decision. I'm not sure what fits best, maybe somebody else can voice their opinion.

[vandabbin]

I have changed it to show the last valid value and set the maximum at 9,999 watts. It really is unrealistic to expect any higher value to be a good reading.

I can still change this back to showing bad or err if others voice opinions saying this is the better way.

However I also removed the usage of the KW unit and the conversion of values 10,000 or larger. I discovered that with the check for a watts error to prevent values of 1,000,000 or greater I forgot that if the value was 999,500 or more then after the division by 1,000 the number gets rounded up from 999 to 1000 and would be capable of causing a 1 character overflow! so If we were to leave the ability for values that high to be shown 999,500 watts would have to be the maximum.

[vandabbin]

Ok I am almost completely certain that I have solved the final watts overflow issue. The existing part of the code that was deciding on how many decimal places to use could be off if the value was between 9.995 - 9.999... and 99.95 - 99.999...

This would cause one more decimal place then should be present because after the number of decimal places was chosen the number is then rounded and can become 10.00 or 100.0 when it should be 10.0 or 100

[vandabbin]

After this realization I have applied the one character overflow fix change to the gpu wattage readings as well. They had the potential for an overflow as well if the value was within those ranges.

I am waiting to hear back from the initial reporter about testing for this hopefully last modification before marking this as ready for review

[vandabbin]

This should be good to go now. It seems to have solved the both the bad sensor data overflow and the bad rounding overflow