Skip to content
This repository has been archived by the owner on May 27, 2020. It is now read-only.

API refactoring and updating #172

Closed
wants to merge 8 commits into from
Closed

Conversation

luke-jr
Copy link

@luke-jr luke-jr commented Apr 22, 2012

This patchset combines gpustatus, pgastatus, and cpustatus (which were mostly doing the same thing) into a single function, and adds the GPU-specific information via a new get_extra_device_info function on the driver API. It also exposes new device information (recently added) to API users when applicable: Driver, Kernel, Model, and Device Path

@kanoi
Copy link

kanoi commented Apr 22, 2012

No idea what the reason is behind most of this except to cause problems by changing things around
You haven't added anything with most of this and even moved some of the code out of api.c
I've never had the 'devs' command error with only fpga (I just tested my current code on my fgpa only rig) though any problems with api.c due to 'total_devices' are caused by you when you changed that in the main code and didn't change anything in api.c related to it ages ago
Most of this change seems to only increase the amount of data it sends but nothing is gained by most of it
As it stands at the moment I don't give a shit if you screw up the API with this
I've no idea what problems these unnecessary changes will cause (nor if you have tested them properly)

@luke-jr
Copy link
Author

luke-jr commented Apr 22, 2012

The reason I looked at api.c at all, is that I wanted to make a GUI aggregating multiple cgminer servers in one display. For that, I wanted to add newly-available device info to the device info requests. I discovered the current api.c code was duplicating the same code for the 3 different device types, so I figured I'd help clean it up by refactoring it to use a single function for all devices. By moving protocol-specific information into the driver, api.c depends less on internal OpenCL-driver code, and other drivers can provide more info without messing with api.c.

The 'devs' command, prior to my fixing it, aborted early if (nDevs == 0 && opt_n_threads == 0); nDevs is a legacy variable containing the number of OpenCL devices, and opt_n_threads is a configuration variable for the number of CPU devices. If there are neither OpenCL device nor CPU devices, this would therefore abort. This was the first problem I encountered trying to interface with the API, since I only have PGAs. This change should have been part of adding PGA support to the JSON API, which wasn't something I did.

I have checked the output of the JSON API that I changed, comparing the before and after, and it looks correct. If there are more extensive tests you would like me to run, please let me know.

@kanoi
Copy link

kanoi commented Apr 25, 2012

The refactor moves data around (and isn't necessary)
It shouldn't (but does) affect the output of the reporting of the API other than to add new data on the end
It should not move code out of the API into other modules (as I've discussed why)
The line change for (nDevs == 0 && opt_n_threads == 0) is needed however it should check the count of supported devices (not total devices) to fix that legacy you created
Of interest is it appears to not work for you due to including CPU mining
If you want to make a pull do:

  1. Fix the MSG_NODEVS as is already needed - but it must check the count of devices that the api.c code supports
  2. Add 'devstatus' (single answer version only) with code like the rest already in api.c
    Please close/delete this pull and start again

@ckolivas
Copy link
Owner

Will not merge based on API maintainer not agreeing with changes.

@ckolivas ckolivas closed this Apr 27, 2012
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants