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

Refactor NVML + Health bits #256

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

Conversation

Spudz76
Copy link
Contributor

@Spudz76 Spudz76 commented Mar 21, 2019

Allow unavailable items to disappear

  • various GPUs are missing features; detect via checking the return value for nonsuccess, then tarnish the health bucket by setting it to literal maximum as a token for unsupported/broken (and don't continue to query them) Tested on a laptop which does not report fans due to integrated unified cooling system, and does not have PowerUsage reporting because it's Fermi, etc. All it shows is temperature, now, perfect.
  • refactor/simplify worker health output to build the string incrementally (more easily handle all the possibly missing features) and dump at the end

Make thermal display color thresholds configurable

  • compile time via new src/defaults.h
  • runtime via temp_low and temp_high within advanced thread config

and random patches to anything that stuck out while I was in the area(s) half of which my editor just does for me anyways (actually harder to NOT do them)

@xmrig
Copy link
Owner

xmrig commented Mar 21, 2019

I will merge it today evening or tomorrow, need some time for it, because I support fully functional command line interface, adding new options not that simple.
Thank you.

@Spudz76
Copy link
Contributor Author

Spudz76 commented Mar 21, 2019

I can add that too, I just didn't. Thought it would be safer to have it harder to edit?
Eventually adding high temp exit trigger etc and that's like a thermostat it should be in a plastic box so the kiddies can't tweak it by accident.

@Spudz76
Copy link
Contributor Author

Spudz76 commented Mar 21, 2019

and mainly 45-65 is not my favorite range so I made it compile-time feature first

really just remove the configuration part, I don't want it there

hardcoded only.

@Spudz76
Copy link
Contributor Author

Spudz76 commented Mar 21, 2019

I only added it into the threads tree since that's the only place to store them per-GPU
and if you have mixed GPUs
some old crap likes to be 88c but I would certainly get tired of red letters quick since they are probably over 65 at the first job assignment
Heck my GTX970 is 72c and it's underclocked

I just never saw those colors change and wondered if the range was right, if they would... :)

@Spudz76
Copy link
Contributor Author

Spudz76 commented Mar 21, 2019

The best part is I run everything through meta-miner so it strips off any color anyways so I literally don't see it

@xmrig
Copy link
Owner

xmrig commented Mar 22, 2019

I get multiple issues:

  1. MAXUINT32 is Windows specific, better use std::numeric_limits<uint32_t>::max() or external flags for available features, values like 4294967295 leaks to API.
  2. unsigned __int64 -> uint64_t.
  3. Some formatting issues, like 94W54C.
  4. dynamic_cast -> static_cast.

I decided reject this PR in current state, but I agree with you, temperatures should be configurable, probably better names for options is temp-warning and temp-danger or something like that.
This feature will be added to v2.15.
Thank you.

@xmrig
Copy link
Owner

xmrig commented Mar 22, 2019

Also:

/home/.../xmrig-nvidia/src/workers/Workers.cpp: In function ‘const string _spf(const char*, ...)’:
/home/.../xmrig-nvidia/src/workers/Workers.cpp:135:20: error: array must be initialized with a brace-enclosed initializer
     va_list args = nullptr, copy = nullptr;
                    ^~~~~~~

@Spudz76 Spudz76 force-pushed the dev-fixNVML branch 2 times, most recently from 630c485 to 3554f92 Compare March 23, 2019 01:58
@Spudz76
Copy link
Contributor Author

Spudz76 commented Mar 23, 2019

Applied much of the cool stuff onto #257 and then some
with none of the complainy bits (I mostly agree)
probably will fixup some of those bits

definitely need to test on Linux too or I'm going to keep bashing into silly problems
but I have no Linux with nvidia handy at the moment

Could have sworn MAXUINT32 was standard back to like regular C even (also swore I've used it on Linux). Or at least as soon as standard libraries had limits.h? idk, agree though the C++ standard method for referring to it is better (idk C++ rly)

…play color thresholds configurable both compile-time and runtime
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants