Add driver for GW Instek PSP series #227
Conversation
| { "GW Instek", "PSP-603", volts_60, amps_3_5 }, | ||
| { "GW Instek", "PSP-405", volts_40, amps_5 }, | ||
| { "GW Instek", "PSP-2010", volts_20, amps_10 }, | ||
| ALL_ZERO |
There was a problem hiding this comment.
You don't need to NULL-terminate your array when you use ARRAY_SIZE to iterate over it.
| sr_dbg("Found: [%s] [%s]", check->vendor, check->name); | ||
| return check; | ||
| } | ||
| sr_dbg("Not found"); |
There was a problem hiding this comment.
The three debug messages seem a bit much, considering it's an in-memory lookup with almost hardcoded input (the force_detect value).
| serialcomm = "2400/8n1"; | ||
| if (!force_detect) { | ||
| sr_err("The gwinstek-psp driver requires the force_detect parameter"); | ||
| return NULL; |
There was a problem hiding this comment.
Does this hardware really provide no way at all to detect the model? What does the vendor-provided software do?
| devc->next_req_time = 0; | ||
| devc->set_voltage_target_updated = 0; | ||
| devc->voltage_target_updated = 0; | ||
| devc->last_status_query_time = 0; |
There was a problem hiding this comment.
You specifically used g_malloc0, so no need to initialize devc members to 0.
| if (dval < devc->model->voltage[0] || dval > devc->model->voltage[1]) | ||
| return SR_ERR_ARG; | ||
|
|
||
|
|
There was a problem hiding this comment.
Please don't leave double blank lines anywhere in the source.
|
Sorry about the long delay in getting this reviewed, I hope you still want to get this driver merged. The code looks good, aside from the minor changes. Seems like a challenging device to write a driver for. |
The protocol is described in the manual, available here.
Generally, everything seems to work well. One annoying limitation of the protocol/device is that it uses the same field for reporting both the actual voltage and the target voltage. This means that querying the target voltage is not possible while the output is enabled or the device is in CC mode. I did implement a heuristic to work around this as best as I could, but one corner case remains - when SmuView is launched while the output is enabled and the device is in CC mode, the target voltage will be reported as 0.00V until the output is turned on (at which point the target voltage will be updated according to the one set on the device), or until a new target voltage is set via the UI. I think that this limitation is acceptable if properly documented.