papampi / nvOC_miners

nvOC easy-to-use Linux Nvidia Mining OS Miners
8 stars 7 forks source link

Conversion to Pluggable Miners #22

Closed LuKePicci closed 5 years ago

LuKePicci commented 6 years ago

This PR is intended to keep an open discussion about conversion of each currently supported miner to the PM format and allow quick testing and review of a work that is expected to take a lot of time. The plan is to convert step-by-step one miner per time. As soon as we get evidence of a limitation in the surrounding PM framework which is preventing a correct conversion of a particular miner I will try working on it to proceed further. At the end, when we will have a clear idea about how each miner has been adapted we could discuss about cleaning up and refactoring the pm framework behind them taking the best from discussed issues.

I hereby invite any nvOC insider to try writing a PM adaptation of existing builtin miners and to submit PRs against this branch. It is an invaluable help since it is quite easy to do following existing examples (see contents of existing JSON manifests), but it is a long and boring work - once you get the idea it is kind of a repetition.

Note: this branch should not require any change to the main nvOC scripts so it is easy for you to test, just switch your nvOC_miners submodule to this branch and - if you give your PM experiment a custom name - change the miner name in 1bash accordingly. If a PM exists with the same name of a builtin one it takes precedence over the latter.

Here is the list of currently converted miners (working ✔ / untested ⚠ / missing ❌):

PM Mining Report Install Compile Foreman
ETHMINER 0.14.0
ETHMINER 0.15.0
ETHMINER 0.16.0
XMR_Stak 2.4.4
XMR_Stak 2.4.7
XMR_Stak 2.5.1
XMR_Stak 2.5.2
KTccminer_cryptonight 2.06
KTccminer_cryptonight 3.05
Z_EWBF 0.6
ZENEMYminer 1.28
CryptoDredge 0.14.0
t-rex 0.8.8
Claymore 11.9
Bminer 10.7.0
ewbf 0.3.4b
papampi commented 6 years ago

Will start writting helper jsons as soon as get some time

papampi commented 5 years ago

@LuKePicci In most ccminers for minerinfo we have 2 solutions for MH/s and KH/s, how to add both to pm json?

papampi commented 5 years ago

@LuKePicci Is it still needed to rename other variants of ccminer executables like z-enemy, t-rex, CryptoDredge and ... to ccminer?

LuKePicci commented 5 years ago

About the first point, I was thinking for a solution to this, maybe the easiest thing is to make this dynamic by grepping it from the output as well. So, where you know a priori you can just use for example "echo MH/s", otherwise you execute a grep like for the other stats.

About the second point, the reason to have miner executables named like they are created when compiling is that the copy command can simply consist in a list of files to be copied. If the copy command would need to rename the compiled binary output to something else, I will need to add code to perform such operation (easy) but also add another filed to manifests which specify which is the name of the output binary to be renamed. Can be done but is is just a complication we can avoid leaving miners executables with their names. For closed-source miner this issue, obviously, does not exist.

Please, check in the manifest you updated for unescaped quotes ", they will break the json file. Just escape them with \ and it should be ok. You can check syntax with an online json validator, if you need it.

papampi commented 5 years ago

Fixed json errors and renamed binary to default, Since we are moving webinfo to foreman I think we can skip the hashrate read out too. Please have a look at miner command, if all is ok I can start making it for all the other ccminers too.

LuKePicci commented 5 years ago

Looks good, I'm now editing the initial post for this PR to include z_ewbf and z-enemy, if you add anything else please keep it updated (github should let you edit it).

papampi commented 5 years ago

@LuKePicci Most of the daily driver miners are covered, do you think we can merge this to 3.1 and let beta testers start testing them?

LuKePicci commented 5 years ago

Yes, definitely, but I would rather suggest to merge this branch directly or via another PR for that same branch, keeping this PR open for tracking further changes. I'm quite busy atm but if you think you can get in troubles by doing so I could also do it for you in a few days - hopefully.

papampi commented 5 years ago

No rush, I am a bit busy a.t.m. too.