Conversation
Support for KKG305P with new quirk. korad_kaxxxxp_send_cmd() changed in a way to support adding new line termination. STATUS parsed according to Korad "documentation". New flags in device structure to support status of remote sensing status and external control interface status.
|
Hi, thanks for your contribution! Would you be so kind and rebase your branch on top of current git head to solve the conflicts? |
|
I will try and let you know. |
fenugrec
left a comment
fenugrec
left a comment
There was a problem hiding this comment.
(note I am neither a main dev or maintainer of sigrok. Just a 'concerned citizen')
I don't see major problems, but I wonder about the choice of changing every call to korad_kaxxxxp_send_cmd() with the extra arg.
I think I would've implemented a new korad_kkg_send_cmd() with the newline fixes (but no additional argument); change the korad_kaxxxxp_send_cmd() call sites to a instead call a *function pointer, and set that function pointer once at device init according to device type. Then after writing that I would then start doubting on whether it was a even a good idea.
Hope we get some input from others about this
| { | ||
| int ret; | ||
| uint32_t cmd_len; | ||
| char *_cmd; |
There was a problem hiding this comment.
not a fan of leading underscores for var names
| return ret; | ||
| } | ||
|
|
||
| sr_dbg("Sending '%s'.", _cmd); |
There was a problem hiding this comment.
suggest moving this before you append the newline, so the debug message doesn't end up with sometimes a stray \n ?
| } | ||
|
|
||
| sr_dbg("Sending '%s'.", _cmd); | ||
| if ((ret = serial_write_blocking(serial, _cmd, cmd_len, 0)) < 0) { |
There was a problem hiding this comment.
move the assignment outside the conditional
| devc->cc_mode_1_changed = devc->cc_mode[0] != prev_status; | ||
|
|
||
| if( devc->model->quirks & KORAD_QUIRK_KKG_FAMILY) { | ||
| /* |
There was a problem hiding this comment.
missing indent inside block, bad whitespace in if(
| * status_byte & ((1 << 2) | (1 << 3)) | ||
| * 00 independent 01 series 11 parallel | ||
| */ | ||
|
|
| (status_byte & (1 << 5)) ? "enabled" : "disabled", | ||
| (status_byte & (1 << 7)) ? "enabled" : "disabled", | ||
| (status_byte & (1 << 4)) ? "beeping" : "silent"); | ||
| if( devc->model->quirks & KORAD_QUIRK_KKG_FAMILY) { |
| gboolean output_enabled_changed; /**< Output enabled state has changed. */ | ||
| gboolean ocp_enabled_changed; /**< OCP enabled state has changed. */ | ||
| gboolean ovp_enabled_changed; /**< OVP enabled state has changed. */ | ||
| gboolean rmt_comp_changed; /**< Is remote compensation enabled. */ |
There was a problem hiding this comment.
wrong copypasted comment here and below, enabled-> changed
|
Also forgot to mention, one of these commits |
3 participants
Support for KKG305P with new quirk.
korad_kaxxxxp_send_cmd() changed in a way to support new line termination.
STATUS parsed according to Korad "documentation".
New flags in device structure to support status of remote sensing status and external control interface status.