Unmanageable changes to configuration file WRT algo switching in 5_0

[starlilyth]

While I am very excited to see per algo switching coming to sgminer, I must raise issue with the way that the configuration file is being managed. The current approach is unnecessarily difficult for miners and applications.

In addition to having to rewrite every pool on every change (some of it directly since its not in the API save function yet) the format is unnecessarily verbose. Here is an example of the format with these changes currently: http://pastebin.com/aVR8A2bz These changes were introduced in the following commits from May 28/29: c4f2b78 13cdc33 2ec0915

Here is an example of a format that would be far easier for miners and applications to manage: http://pastebin.com/17KD3X8S (both examples use the exact same data) Instead of duplicating information in each pool (each bringing a chance of introducing errors), the example shown simply adds one field to the pool defining its algo, then a new section would specify the algo specific details for each pool that defines it.

I feel this approach is far clearer and far more manageable for miners and applications, and I hope it will not take a great deal of effort for you to implement it, or something similar.

Thanks!

[mrbrdo]

I like per-algorithm settings (instead of per-pool), it seems like a good idea.

Not including your confusion on the difference between JSON arrays and objects, also:

• due to current limitations in sgminer config parsing code, it is not possible to reuse the same identifiers that are used in global config (e.g. "intensity"), unless a complete rewrite of parsing configuration is made (or with ugly hacks)
• related to above, this change would break command line parameters
• algorithm settings should be nested under a root key such as "algorithms"
• at the moment it is not possible to specify different algorithms per GPU (afaik, also not planned)
• many of the settings you list as per-algo are not even implemented per-pool at the moment

[sling00]

Something else of note not yet pointed out, with the current implementation if you delete your pool via TUI or API you delete your settings. Specific settings for devices are not handled this way in any cgminer variant and never have been until this point. Algorithm is logical and a necessity to accomplish the task, however everything else can be better defined elsewhere. A call can be made to get the current algo based on get_current_pool pool->algorithm and then from there the settings can be read from the specified algorithms section this is the logical method that follows precedence in cgminer itself, as well as bfgminer. This minor change to a short-sighted short cut will prevent many hours of heartache for months/years to come. I beg you please not to make the current implementation the final implementation, as it is a nightmare for all the other developers who have to interface with your miner, as well as for your users whether they realize the impact this will have yet or not.

Thank you for your hard work and consideration. This should be done for the same reason we leave comments in code, it is not only to make our lives easier, but to make better the lives of those who work with us, and also they who come even years on down the line.

Edit: we could still give the parameters a different name though they provide the same function, or an if statement so that if the parameter is defined within the algo specific data, that is used, else pull from global.

Also, not including your confusion in an opencl kernel and an 'algorithm', I don't understand why a different opencl device cannot have its kernel specified per card as it does now, and why you would not plan this as again, it is the next logical step.

[mrbrdo]

Nobody said this is the final implementation. Like I said, I agree that settings should be per-algo instead of per-pool. If anyone would like to work on this, write here so work is not duplicated. Unless someone will undertake rewriting config parsing (which would be awesome), then I propose to add a prefix to settings such as algo-intensity.

[melt7777]

Try not to delete and censor those who contribute to the discussion. We are responsible for thousands of your users and helping them use your software.

[mrbrdo]

Sorry but this is for issues, not for advertisement. Keep to the point of discussion please.

[starlilyth]

I am glad to hear you are open to change in this area.

A root key for algos, then subkeys for each with their definitions that could be referenced from a pool definition would work fine, whatever will be the most manageable going forward for everyone on both sides of the application. The goal would be to prevent unnecessary duplication of data in the config, and prevent placing device specific parameters in pool definitions (pools come and go, but the device specific parameters for an algo should not, in my opinion), while preserving existing CLI and API expectations.

Is the primary blocker for extending the miner in this way the config parser?

I am not qualified to contribute useful code in this area or I certainly would love to. I do know that this feature is highly anticipated, and will likely be well used on both sides of the interface, so extra effort spent now will benefit everyone who comes after. I do appreciate you taking the time.

[veox]

Is the primary blocker for extending the miner in this way the config parser?

Yes. It grew out of the CCAN opt module, which is for command-line option parsing, not JSON. There is actually no dedicated config file parser, or config structure, just calls to opt and some support functions.

[starlilyth]

I believe a solution for this issue has now been submitted: 8ca5bb2

[veox]

The configuration parser has been merged. Pool- are still available.