Add support for Korad KA3005PS #269
Conversation
|
I think this is the same as the velleman LABPS3005DN needs i'll try and check somewhere this week on mine |
|
That doesn't work. LABPS3005DN.txt |
|
I'm not sure where you applied that patch? For this PR you'd need to add the I also see a change of |
Right, I took your patch, and added which did not detect my velleman. My patch I uploaded lets me detect my velleman, but does not get me to a functioning state. https://cdn.velleman.eu/downloads/80/labps3005dn_communication_protocol.pdf is the doc for my velleman btw. I was just hopeful your patch would work for my device, because you mentioned it needed the newlines, I'm sorry if I am highjacking the thread... |
MartinJM
force-pushed
the
add_device_korad_ka3005ps
branch
from
cf8934c to
c79a973
Compare
|
It does look fairly close - the only real differences seem to be the
No worries! I have changed the code a bit to see if it will allow your device to connect properly as well. If it doesn't take too much work it'd be nice to add support for it as well! You'll need to apply something like the following patch for your device though: diff --git a/src/hardware/korad-kaxxxxp/api.c b/src/hardware/korad-kaxxxxp/api.c
index c6b9b1f6..583252a3 100644
--- a/src/hardware/korad-kaxxxxp/api.c
+++ b/src/hardware/korad-kaxxxxp/api.c
@@ -76,6 +76,8 @@ static const struct korad_kaxxxxp_model models[] = {
{"Tenma", "72-2710", "", 1, volts_30, amps_5, 0},
{"Velleman", "LABPS3005D", "", 1, volts_30, amps_5,
KORAD_QUIRK_LABPS_OVP_EN},
+ {"Velleman", "LABPS3005DN", "", 1, volts_30, amps_5,
+ KORAD_QUIRK_VELLEMAN | KORAD_QUIRK_NEWLINE},
{"Velleman", "PS3005D V1.3", "VELLEMANPS3005DV1.3" , 1, volts_30, amps_5,
KORAD_QUIRK_ID_TRAILING | KORAD_QUIRK_SLOW_PROCESSING},
{"Velleman", "PS3005D", "", 1, volts_30, amps_5, 0},
diff --git a/src/hardware/korad-kaxxxxp/protocol.c b/src/hardware/korad-kaxxxxp/protocol.c
index 0ce7206e..b27d8f9c 100644
--- a/src/hardware/korad-kaxxxxp/protocol.c
+++ b/src/hardware/korad-kaxxxxp/protocol.c
@@ -229,8 +229,13 @@ SR_PRIV int korad_kaxxxxp_set_value(struct sr_serial_dev_inst *serial,
"VSET1:%05.2f", devc->set_voltage_target);
break;
case KAXXXXP_OUTPUT:
- sr_snprintf_ascii(msg, sizeof(msg),
- "OUT%1d", (devc->set_output_enabled) ? 1 : 0);
+ if (devc->model->quirks & KORAD_QUIRK_VELLEMAN) {
+ sr_snprintf_ascii(msg, sizeof(msg),
+ "OUTPUT%1d", (devc->set_output_enabled) ? 1 : 0);
+ } else {
+ sr_snprintf_ascii(msg, sizeof(msg),
+ "OUT%1d", (devc->set_output_enabled) ? 1 : 0);
+ }
/* Set value back to recognize changes */
devc->output_enabled = devc->set_output_enabled;
break;
diff --git a/src/hardware/korad-kaxxxxp/protocol.h b/src/hardware/korad-kaxxxxp/protocol.h
index 43653d14..19aef882 100644
--- a/src/hardware/korad-kaxxxxp/protocol.h
+++ b/src/hardware/korad-kaxxxxp/protocol.h
@@ -40,7 +40,8 @@ enum korad_quirks_flag {
KORAD_QUIRK_ID_OPT_VERSION = 1UL << 3,
KORAD_QUIRK_SLOW_PROCESSING = 1UL << 4,
KORAD_QUIRK_NEWLINE = 1UL << 5,
- KORAD_QUIRK_ALL = (1UL << 6) - 1,
+ KORAD_QUIRK_VELLEMAN = 1UL << 6,
+ KORAD_QUIRK_ALL = (1UL << 7) - 1,
};
/* Information on single model */The |
|
Im probably doing something weird, but I am faling at applying your patch for my velleman ^^; I'll have another poke later on, my brain is a bit foggy atm i'm afraid. |
|
You'll need to apply it on top of this PR. I think you tried to apply it on master right? I also forgot to add the newline quirk for your device - just edited it in, so you might need to grab it again. |
|
i first tried master, then this PR as a patch, then the other one. final: prev:
{
libsigrok = prev.libsigrok.overrideAttrs (prevAttrs: {
buildInputs = (prevAttrs.buildInputs or []) ++ [prev.nettle];
src = prev.fetchgit {
#url = "git://sigrok.org/libsigrok";
url = "https://github.com/sigrokproject/libsigrok/";
rev = "c79a9738f8643fe84998d95d0578ec1035235471";
hash = "sha256-Ogg7/8ZIqKejyp9HWLRa6DMWM7nxGgVMew+EbaNxCKo=";
};
patches = (prevAttrs.patches or []) ++ [
#../patches/269.patch
../patches/VELLEMAN.patch
];
});
}I don;t know if you are familiar with nix, but that is the last one i tried. |
|
Ah I think something went wrong with my patch. I recreated it and double-checked this one, so it should work: diff --git a/src/hardware/korad-kaxxxxp/api.c b/src/hardware/korad-kaxxxxp/api.c
index c6b9b1f6..3967a0fb 100644
--- a/src/hardware/korad-kaxxxxp/api.c
+++ b/src/hardware/korad-kaxxxxp/api.c
@@ -76,6 +76,8 @@ static const struct korad_kaxxxxp_model models[] = {
{"Tenma", "72-2710", "", 1, volts_30, amps_5, 0},
{"Velleman", "LABPS3005D", "", 1, volts_30, amps_5,
KORAD_QUIRK_LABPS_OVP_EN},
+ {"Velleman", "LABPS3005DN", "", 1, volts_30, amps_5,
+ KORAD_QUIRK_NEWLINE | KORAD_QUIRK_VELLEMAN},
{"Velleman", "PS3005D V1.3", "VELLEMANPS3005DV1.3" , 1, volts_30, amps_5,
KORAD_QUIRK_ID_TRAILING | KORAD_QUIRK_SLOW_PROCESSING},
{"Velleman", "PS3005D", "", 1, volts_30, amps_5, 0},
diff --git a/src/hardware/korad-kaxxxxp/protocol.c b/src/hardware/korad-kaxxxxp/protocol.c
index 0ce7206e..b27d8f9c 100644
--- a/src/hardware/korad-kaxxxxp/protocol.c
+++ b/src/hardware/korad-kaxxxxp/protocol.c
@@ -229,8 +229,13 @@ SR_PRIV int korad_kaxxxxp_set_value(struct sr_serial_dev_inst *serial,
"VSET1:%05.2f", devc->set_voltage_target);
break;
case KAXXXXP_OUTPUT:
- sr_snprintf_ascii(msg, sizeof(msg),
- "OUT%1d", (devc->set_output_enabled) ? 1 : 0);
+ if (devc->model->quirks & KORAD_QUIRK_VELLEMAN) {
+ sr_snprintf_ascii(msg, sizeof(msg),
+ "OUTPUT%1d", (devc->set_output_enabled) ? 1 : 0);
+ } else {
+ sr_snprintf_ascii(msg, sizeof(msg),
+ "OUT%1d", (devc->set_output_enabled) ? 1 : 0);
+ }
/* Set value back to recognize changes */
devc->output_enabled = devc->set_output_enabled;
break;
diff --git a/src/hardware/korad-kaxxxxp/protocol.h b/src/hardware/korad-kaxxxxp/protocol.h
index 43653d14..19aef882 100644
--- a/src/hardware/korad-kaxxxxp/protocol.h
+++ b/src/hardware/korad-kaxxxxp/protocol.h
@@ -40,7 +40,8 @@ enum korad_quirks_flag {
KORAD_QUIRK_ID_OPT_VERSION = 1UL << 3,
KORAD_QUIRK_SLOW_PROCESSING = 1UL << 4,
KORAD_QUIRK_NEWLINE = 1UL << 5,
- KORAD_QUIRK_ALL = (1UL << 6) - 1,
+ KORAD_QUIRK_VELLEMAN = 1UL << 6,
+ KORAD_QUIRK_ALL = (1UL << 7) - 1,
};
/* Information on single model */ |
|
hmm, not sure what I am doing wrong: |
|
little bit more info: |
|
are you sending a newline, or the actual characters '\n' |
|
You're doing nothing wrong, but your device is not responding to the Also just read your original diff again: and noticed that you're actually adding a literal backslash and then Can you try the following patch? It will take a while to connect, but might work for your device: diff --git a/src/hardware/korad-kaxxxxp/api.c b/src/hardware/korad-kaxxxxp/api.c
index c6b9b1f6..824303f1 100644
--- a/src/hardware/korad-kaxxxxp/api.c
+++ b/src/hardware/korad-kaxxxxp/api.c
@@ -76,6 +76,8 @@ static const struct korad_kaxxxxp_model models[] = {
{"Tenma", "72-2710", "", 1, volts_30, amps_5, 0},
{"Velleman", "LABPS3005D", "", 1, volts_30, amps_5,
KORAD_QUIRK_LABPS_OVP_EN},
+ {"Velleman", "LABPS3005DN", "", 1, volts_30, amps_5,
+ KORAD_QUIRK_NEWLINE | KORAD_QUIRK_VELLEMAN},
{"Velleman", "PS3005D V1.3", "VELLEMANPS3005DV1.3" , 1, volts_30, amps_5,
KORAD_QUIRK_ID_TRAILING | KORAD_QUIRK_SLOW_PROCESSING},
{"Velleman", "PS3005D", "", 1, volts_30, amps_5, 0},
@@ -290,6 +292,17 @@ static GSList *scan(struct sr_dev_driver *di, GSList *options)
ret = korad_kaxxxxp_read_chars(serial, len, reply, true);
if (ret < 0)
return NULL;
+
+ // Also attempt literal \\n for VELLMAN
+ if (ret == 0) {
+ ret = korad_kaxxxxp_send_cmd(serial, "*IDN?\\n", true);
+ if (ret < 0)
+ return NULL;
+
+ ret = korad_kaxxxxp_read_chars(serial, len, reply, true);
+ if (ret < 0)
+ return NULL;
+ }
sr_dbg("Received: %d, %s", ret, reply);
/*
diff --git a/src/hardware/korad-kaxxxxp/protocol.c b/src/hardware/korad-kaxxxxp/protocol.c
index 0ce7206e..b27d8f9c 100644
--- a/src/hardware/korad-kaxxxxp/protocol.c
+++ b/src/hardware/korad-kaxxxxp/protocol.c
@@ -229,8 +229,13 @@ SR_PRIV int korad_kaxxxxp_set_value(struct sr_serial_dev_inst *serial,
"VSET1:%05.2f", devc->set_voltage_target);
break;
case KAXXXXP_OUTPUT:
- sr_snprintf_ascii(msg, sizeof(msg),
- "OUT%1d", (devc->set_output_enabled) ? 1 : 0);
+ if (devc->model->quirks & KORAD_QUIRK_VELLEMAN) {
+ sr_snprintf_ascii(msg, sizeof(msg),
+ "OUTPUT%1d", (devc->set_output_enabled) ? 1 : 0);
+ } else {
+ sr_snprintf_ascii(msg, sizeof(msg),
+ "OUT%1d", (devc->set_output_enabled) ? 1 : 0);
+ }
/* Set value back to recognize changes */
devc->output_enabled = devc->set_output_enabled;
break;
diff --git a/src/hardware/korad-kaxxxxp/protocol.h b/src/hardware/korad-kaxxxxp/protocol.h
index 43653d14..19aef882 100644
--- a/src/hardware/korad-kaxxxxp/protocol.h
+++ b/src/hardware/korad-kaxxxxp/protocol.h
@@ -40,7 +40,8 @@ enum korad_quirks_flag {
KORAD_QUIRK_ID_OPT_VERSION = 1UL << 3,
KORAD_QUIRK_SLOW_PROCESSING = 1UL << 4,
KORAD_QUIRK_NEWLINE = 1UL << 5,
- KORAD_QUIRK_ALL = (1UL << 6) - 1,
+ KORAD_QUIRK_VELLEMAN = 1UL << 6,
+ KORAD_QUIRK_ALL = (1UL << 7) - 1,
};
/* Information on single model */Might also be necessary for the other commands, not sure yet, we will see. Another option could be to change the |
Newlines. Only noticed that you meant the actual characters just before my previous comment. For my device it's about newlines (or carriage-returns). A literal Edit: This clears up a lot:
Try the following patch: diff --git a/src/hardware/korad-kaxxxxp/api.c b/src/hardware/korad-kaxxxxp/api.c
index c6b9b1f6..30d5d36a 100644
--- a/src/hardware/korad-kaxxxxp/api.c
+++ b/src/hardware/korad-kaxxxxp/api.c
@@ -76,6 +76,8 @@ static const struct korad_kaxxxxp_model models[] = {
{"Tenma", "72-2710", "", 1, volts_30, amps_5, 0},
{"Velleman", "LABPS3005D", "", 1, volts_30, amps_5,
KORAD_QUIRK_LABPS_OVP_EN},
+ {"Velleman", "LABPS3005DN", "", 1, volts_30, amps_5,
+ KORAD_QUIRK_VELLEMAN},
{"Velleman", "PS3005D V1.3", "VELLEMANPS3005DV1.3" , 1, volts_30, amps_5,
KORAD_QUIRK_ID_TRAILING | KORAD_QUIRK_SLOW_PROCESSING},
{"Velleman", "PS3005D", "", 1, volts_30, amps_5, 0},
@@ -282,7 +284,10 @@ static GSList *scan(struct sr_dev_driver *di, GSList *options)
// TODO: check if adding the newline breaks some other devices - I cannot do so
// This is required for the KA3005PS
- ret = korad_kaxxxxp_send_cmd(serial, "*IDN?", true);
+ // TODO: check if adding the \\n breaks some other devices - I cannot do so
+ // This is required for the Velleman LABPS3005DN
+ // It does not break the KA3005PS
+ ret = korad_kaxxxxp_send_cmd(serial, "*IDN?", true, true);
if (ret < 0)
return NULL;
@@ -290,6 +295,7 @@ static GSList *scan(struct sr_dev_driver *di, GSList *options)
ret = korad_kaxxxxp_read_chars(serial, len, reply, true);
if (ret < 0)
return NULL;
+
sr_dbg("Received: %d, %s", ret, reply);
/*
diff --git a/src/hardware/korad-kaxxxxp/protocol.c b/src/hardware/korad-kaxxxxp/protocol.c
index 0ce7206e..76672e4c 100644
--- a/src/hardware/korad-kaxxxxp/protocol.c
+++ b/src/hardware/korad-kaxxxxp/protocol.c
@@ -25,18 +25,22 @@
#define EXTRA_PROCESSING_TIME_MS 450
SR_PRIV int korad_kaxxxxp_send_cmd(struct sr_serial_dev_inst *serial,
- const char *cmd, bool add_newline)
+ const char *cmd, bool add_newline, bool velleman_literal)
{
int ret;
char* addition = "";
- if (add_newline)
+ if (add_newline && velleman_literal)
+ addition = "\\n\n";
+ else if (add_newline)
addition = "\n";
+ else if (velleman_literal)
+ addition = "\\n"; // Very weird one
- // 21 was chosen here because 20 is chosen in korad_kaxxxxp_set_value
- char newcmd[21];
- ret = sr_snprintf_ascii(newcmd, 21, "%s%s", cmd, addition);
- if (ret < 0 || ret >= 21) {
+ // 23 was chosen here because 20 is chosen in korad_kaxxxxp_set_value
+ char newcmd[23];
+ ret = sr_snprintf_ascii(newcmd, sizeof(newcmd), "%s%s", cmd, addition);
+ if (ret < 0 || ret >= (int) sizeof(newcmd)) {
sr_err("Error creating command: %d.", ret);
if (ret > 0)
ret = -ret; // make errors always return negative numbers
@@ -229,8 +233,13 @@ SR_PRIV int korad_kaxxxxp_set_value(struct sr_serial_dev_inst *serial,
"VSET1:%05.2f", devc->set_voltage_target);
break;
case KAXXXXP_OUTPUT:
- sr_snprintf_ascii(msg, sizeof(msg),
- "OUT%1d", (devc->set_output_enabled) ? 1 : 0);
+ if (devc->model->quirks & KORAD_QUIRK_VELLEMAN) {
+ sr_snprintf_ascii(msg, sizeof(msg),
+ "OUTPUT%1d", (devc->set_output_enabled) ? 1 : 0);
+ } else {
+ sr_snprintf_ascii(msg, sizeof(msg),
+ "OUT%1d", (devc->set_output_enabled) ? 1 : 0);
+ }
/* Set value back to recognize changes */
devc->output_enabled = devc->set_output_enabled;
break;
@@ -277,7 +286,8 @@ SR_PRIV int korad_kaxxxxp_set_value(struct sr_serial_dev_inst *serial,
}
if (ret == SR_OK && msg[0]) {
- ret = korad_kaxxxxp_send_cmd(serial, msg, devc->model->quirks & KORAD_QUIRK_NEWLINE);
+ ret = korad_kaxxxxp_send_cmd(serial, msg, devc->model->quirks & KORAD_QUIRK_NEWLINE,
+ devc->model->quirks & KORAD_QUIRK_VELLEMAN);
devc->next_req_time = next_req_time(devc, TRUE, target);
}
@@ -303,26 +313,27 @@ SR_PRIV int korad_kaxxxxp_get_value(struct sr_serial_dev_inst *serial,
count = 6;
bool newline_quirk = devc->model->quirks & KORAD_QUIRK_NEWLINE;
+ bool velleman_quirk = devc->model->quirks & KORAD_QUIRK_VELLEMAN;
switch (target) {
case KAXXXXP_CURRENT:
/* Read current from device. */
- ret = korad_kaxxxxp_send_cmd(serial, "IOUT1?", newline_quirk);
+ ret = korad_kaxxxxp_send_cmd(serial, "IOUT1?", newline_quirk, velleman_quirk);
value = &(devc->current);
break;
case KAXXXXP_CURRENT_LIMIT:
/* Read set current from device. */
- ret = korad_kaxxxxp_send_cmd(serial, "ISET1?", newline_quirk);
+ ret = korad_kaxxxxp_send_cmd(serial, "ISET1?", newline_quirk, velleman_quirk);
value = &(devc->current_limit);
break;
case KAXXXXP_VOLTAGE:
/* Read voltage from device. */
- ret = korad_kaxxxxp_send_cmd(serial, "VOUT1?", newline_quirk);
+ ret = korad_kaxxxxp_send_cmd(serial, "VOUT1?", newline_quirk, velleman_quirk);
value = &(devc->voltage);
break;
case KAXXXXP_VOLTAGE_TARGET:
/* Read set voltage from device. */
- ret = korad_kaxxxxp_send_cmd(serial, "VSET1?", newline_quirk);
+ ret = korad_kaxxxxp_send_cmd(serial, "VSET1?", newline_quirk, velleman_quirk);
value = &(devc->voltage_target);
break;
case KAXXXXP_STATUS:
@@ -330,7 +341,7 @@ SR_PRIV int korad_kaxxxxp_get_value(struct sr_serial_dev_inst *serial,
case KAXXXXP_OCP:
case KAXXXXP_OVP:
/* Read status from device. */
- ret = korad_kaxxxxp_send_cmd(serial, "STATUS?", newline_quirk);
+ ret = korad_kaxxxxp_send_cmd(serial, "STATUS?", newline_quirk, velleman_quirk);
count = 1;
if (newline_quirk)
count = 2;
diff --git a/src/hardware/korad-kaxxxxp/protocol.h b/src/hardware/korad-kaxxxxp/protocol.h
index 43653d14..4540c831 100644
--- a/src/hardware/korad-kaxxxxp/protocol.h
+++ b/src/hardware/korad-kaxxxxp/protocol.h
@@ -40,7 +40,8 @@ enum korad_quirks_flag {
KORAD_QUIRK_ID_OPT_VERSION = 1UL << 3,
KORAD_QUIRK_SLOW_PROCESSING = 1UL << 4,
KORAD_QUIRK_NEWLINE = 1UL << 5,
- KORAD_QUIRK_ALL = (1UL << 6) - 1,
+ KORAD_QUIRK_VELLEMAN = 1UL << 6,
+ KORAD_QUIRK_ALL = (1UL << 7) - 1,
};
/* Information on single model */
@@ -105,7 +106,7 @@ struct dev_context {
};
SR_PRIV int korad_kaxxxxp_send_cmd(struct sr_serial_dev_inst *serial,
- const char *cmd, bool add_newline);
+ const char *cmd, bool add_newline, bool velleman_literal);
SR_PRIV int korad_kaxxxxp_read_chars(struct sr_serial_dev_inst *serial,
size_t count, char *buf, bool strip_newline);
SR_PRIV int korad_kaxxxxp_set_value(struct sr_serial_dev_inst *serial,I doubt it will work at once, you'll probably need something like And maybe you also need the newline quirk? I don't think you do so I've removed it from the quirks now, but if it doesn't work, you can try adding it again as well. |
A literal '\n' is very weird to me, too! idno what the person that wrote that was smoking :P So I changed your last patch a bit, + {"Velleman", "LABPS3005DN", "QJE3005PV1.0", 1, volts_30, amps_5,
+ KORAD_QUIRK_NEWLINE | KORAD_QUIRK_VELLEMAN},And I changed ret = korad_kaxxxxp_send_cmd(serial, "*IDN?\\n", true);to ret = korad_kaxxxxp_send_cmd(serial, "*IDN?\\n", false);It seems it doesn't like ACTUAL newlines, at that 🤦 It now gets detected, but of course the rest doesn't work, because actual |
|
Ah I just edited my previous response. See the patch there, that might still need some of your edits.
Ah that will be an issue for supporting both of our devices then. Mine must have a newline, and yours cannot have one... Maybe one different option: Can you try it with a carriage return ( Edit: Oh yeah your |
|
that last one isn't detecting it anymore at all... I'll poke at it some more tomorrow, thanks for the help! |
|
ok, you last patch works, if I change: Add QJE3005PV1.0+ {"Velleman", "LABPS3005DN", "QJE3005PV1.0", 1, volts_30, amps_5,
+ KORAD_QUIRK_VELLEMAN},set add_newline to false in the discoveryret = korad_kaxxxxp_send_cmd(serial, "*IDN?", false, true);So the patch that works atm isdiff --git a/src/hardware/korad-kaxxxxp/api.c b/src/hardware/korad-kaxxxxp/api.c
index c6b9b1f6..30d5d36a 100644
--- a/src/hardware/korad-kaxxxxp/api.c
+++ b/src/hardware/korad-kaxxxxp/api.c
@@ -76,6 +76,8 @@ static const struct korad_kaxxxxp_model models[] = {
{"Tenma", "72-2710", "", 1, volts_30, amps_5, 0},
{"Velleman", "LABPS3005D", "", 1, volts_30, amps_5,
KORAD_QUIRK_LABPS_OVP_EN},
+ {"Velleman", "LABPS3005DN", "QJE3005PV1.0", 1, volts_30, amps_5,
+ KORAD_QUIRK_VELLEMAN},
{"Velleman", "PS3005D V1.3", "VELLEMANPS3005DV1.3" , 1, volts_30, amps_5,
KORAD_QUIRK_ID_TRAILING | KORAD_QUIRK_SLOW_PROCESSING},
{"Velleman", "PS3005D", "", 1, volts_30, amps_5, 0},
@@ -282,7 +284,10 @@ static GSList *scan(struct sr_dev_driver *di, GSList *options)
// TODO: check if adding the newline breaks some other devices - I cannot do so
// This is required for the KA3005PS
- ret = korad_kaxxxxp_send_cmd(serial, "*IDN?", true);
+ // TODO: check if adding the \\n breaks some other devices - I cannot do so
+ // This is required for the Velleman LABPS3005DN
+ // It does not break the KA3005PS
+ ret = korad_kaxxxxp_send_cmd(serial, "*IDN?", false, true);
if (ret < 0)
return NULL;
@@ -290,6 +295,7 @@ static GSList *scan(struct sr_dev_driver *di, GSList *options)
ret = korad_kaxxxxp_read_chars(serial, len, reply, true);
if (ret < 0)
return NULL;
+
sr_dbg("Received: %d, %s", ret, reply);
/*
diff --git a/src/hardware/korad-kaxxxxp/protocol.c b/src/hardware/korad-kaxxxxp/protocol.c
index 0ce7206e..76672e4c 100644
--- a/src/hardware/korad-kaxxxxp/protocol.c
+++ b/src/hardware/korad-kaxxxxp/protocol.c
@@ -25,18 +25,22 @@
#define EXTRA_PROCESSING_TIME_MS 450
SR_PRIV int korad_kaxxxxp_send_cmd(struct sr_serial_dev_inst *serial,
- const char *cmd, bool add_newline)
+ const char *cmd, bool add_newline, bool velleman_literal)
{
int ret;
char* addition = "";
- if (add_newline)
+ if (add_newline && velleman_literal)
+ addition = "\\n\n";
+ else if (add_newline)
addition = "\n";
+ else if (velleman_literal)
+ addition = "\\n"; // Very weird one
- // 21 was chosen here because 20 is chosen in korad_kaxxxxp_set_value
- char newcmd[21];
- ret = sr_snprintf_ascii(newcmd, 21, "%s%s", cmd, addition);
- if (ret < 0 || ret >= 21) {
+ // 23 was chosen here because 20 is chosen in korad_kaxxxxp_set_value
+ char newcmd[23];
+ ret = sr_snprintf_ascii(newcmd, sizeof(newcmd), "%s%s", cmd, addition);
+ if (ret < 0 || ret >= (int) sizeof(newcmd)) {
sr_err("Error creating command: %d.", ret);
if (ret > 0)
ret = -ret; // make errors always return negative numbers
@@ -229,8 +233,13 @@ SR_PRIV int korad_kaxxxxp_set_value(struct sr_serial_dev_inst *serial,
"VSET1:%05.2f", devc->set_voltage_target);
break;
case KAXXXXP_OUTPUT:
- sr_snprintf_ascii(msg, sizeof(msg),
- "OUT%1d", (devc->set_output_enabled) ? 1 : 0);
+ if (devc->model->quirks & KORAD_QUIRK_VELLEMAN) {
+ sr_snprintf_ascii(msg, sizeof(msg),
+ "OUTPUT%1d", (devc->set_output_enabled) ? 1 : 0);
+ } else {
+ sr_snprintf_ascii(msg, sizeof(msg),
+ "OUT%1d", (devc->set_output_enabled) ? 1 : 0);
+ }
/* Set value back to recognize changes */
devc->output_enabled = devc->set_output_enabled;
break;
@@ -277,7 +286,8 @@ SR_PRIV int korad_kaxxxxp_set_value(struct sr_serial_dev_inst *serial,
}
if (ret == SR_OK && msg[0]) {
- ret = korad_kaxxxxp_send_cmd(serial, msg, devc->model->quirks & KORAD_QUIRK_NEWLINE);
+ ret = korad_kaxxxxp_send_cmd(serial, msg, devc->model->quirks & KORAD_QUIRK_NEWLINE,
+ devc->model->quirks & KORAD_QUIRK_VELLEMAN);
devc->next_req_time = next_req_time(devc, TRUE, target);
}
@@ -303,26 +313,27 @@ SR_PRIV int korad_kaxxxxp_get_value(struct sr_serial_dev_inst *serial,
count = 6;
bool newline_quirk = devc->model->quirks & KORAD_QUIRK_NEWLINE;
+ bool velleman_quirk = devc->model->quirks & KORAD_QUIRK_VELLEMAN;
switch (target) {
case KAXXXXP_CURRENT:
/* Read current from device. */
- ret = korad_kaxxxxp_send_cmd(serial, "IOUT1?", newline_quirk);
+ ret = korad_kaxxxxp_send_cmd(serial, "IOUT1?", newline_quirk, velleman_quirk);
value = &(devc->current);
break;
case KAXXXXP_CURRENT_LIMIT:
/* Read set current from device. */
- ret = korad_kaxxxxp_send_cmd(serial, "ISET1?", newline_quirk);
+ ret = korad_kaxxxxp_send_cmd(serial, "ISET1?", newline_quirk, velleman_quirk);
value = &(devc->current_limit);
break;
case KAXXXXP_VOLTAGE:
/* Read voltage from device. */
- ret = korad_kaxxxxp_send_cmd(serial, "VOUT1?", newline_quirk);
+ ret = korad_kaxxxxp_send_cmd(serial, "VOUT1?", newline_quirk, velleman_quirk);
value = &(devc->voltage);
break;
case KAXXXXP_VOLTAGE_TARGET:
/* Read set voltage from device. */
- ret = korad_kaxxxxp_send_cmd(serial, "VSET1?", newline_quirk);
+ ret = korad_kaxxxxp_send_cmd(serial, "VSET1?", newline_quirk, velleman_quirk);
value = &(devc->voltage_target);
break;
case KAXXXXP_STATUS:
@@ -330,7 +341,7 @@ SR_PRIV int korad_kaxxxxp_get_value(struct sr_serial_dev_inst *serial,
case KAXXXXP_OCP:
case KAXXXXP_OVP:
/* Read status from device. */
- ret = korad_kaxxxxp_send_cmd(serial, "STATUS?", newline_quirk);
+ ret = korad_kaxxxxp_send_cmd(serial, "STATUS?", newline_quirk, velleman_quirk);
count = 1;
if (newline_quirk)
count = 2;
diff --git a/src/hardware/korad-kaxxxxp/protocol.h b/src/hardware/korad-kaxxxxp/protocol.h
index 43653d14..4540c831 100644
--- a/src/hardware/korad-kaxxxxp/protocol.h
+++ b/src/hardware/korad-kaxxxxp/protocol.h
@@ -40,7 +40,8 @@ enum korad_quirks_flag {
KORAD_QUIRK_ID_OPT_VERSION = 1UL << 3,
KORAD_QUIRK_SLOW_PROCESSING = 1UL << 4,
KORAD_QUIRK_NEWLINE = 1UL << 5,
- KORAD_QUIRK_ALL = (1UL << 6) - 1,
+ KORAD_QUIRK_VELLEMAN = 1UL << 6,
+ KORAD_QUIRK_ALL = (1UL << 7) - 1,
};
/* Information on single model */
@@ -105,7 +106,7 @@ struct dev_context {
};
SR_PRIV int korad_kaxxxxp_send_cmd(struct sr_serial_dev_inst *serial,
- const char *cmd, bool add_newline);
+ const char *cmd, bool add_newline, bool velleman_literal);
SR_PRIV int korad_kaxxxxp_read_chars(struct sr_serial_dev_inst *serial,
size_t count, char *buf, bool strip_newline);
SR_PRIV int korad_kaxxxxp_set_value(struct sr_serial_dev_inst *serial,
log: Setting values works, reading them back does not. |
|
bit further poking it, sending Who the hell implemented this thing, and why did I end up with one? 😁 |
Hahaha good questions :P
Hmmm I was hoping that either of these would work for your device, as then there would be overlap with my device as well. I'm wondering if we can just send Edit: So for my device it seems to be that it needs to start with |
|
I don't know if you are still wanting to know @MartinJM ... it seems you just append 'n' no |
I can SET values, and enable/disable the output, but sigrok-cli can't read them back it seems: Set / Read |
|
Yeah, so control works, but reading back doesn't. from smuview i can enable the output, but it doesn't see it is on, and the voltage readings jump all over the place... |
|
Hey, yeah always interested in stuff like this.
And this is giving me a good laugh, so thank you for sharing. Is your device still fine if you send
Hmmm that's too bad. Is this with just the |
Yep, it doesn't seem to care about the \n, you just send the normal commands, and then 1 or 0 characters, and then an
either, if I run sigrok-cli -d korad-kaxxxxp:conn=/dev/tty-CP2102 --continuous |
|
What's your take on this PR, @MartinJM? Should I merge it as-is or do you intend to make any changes to it? That said, please only use /* ... */ for comments, not // |
| return ret; | ||
| } | ||
|
|
||
| sr_dbg("Sending '%s'.", cmd); |
There was a problem hiding this comment.
This logs cmd, but actual command sent is newcmd.
There was a problem hiding this comment.
Yes, it would only show up as an additional newline, which I thought would not be wanted. Though on second thought I agree that it makes more sense to actually log what is being sent (including the newline).
There was a problem hiding this comment.
Hmm, right, but still makes sense to log what is written, or maybe escape it into the string so it's clear.
There was a problem hiding this comment.
It's only for debug logging though, so I'd rather not do any extra processing. I agree that it makes sense to log what is written, will change it.
For the changes, do you prefer multiple smaller commits, one larger commit containing multiple smaller changes, or a change of the original commit (including a force-push)?
EDIT:
The default logger strips the newlines:
Lines 228 to 243 in c79a973
I'll still make the change, as I understand that the logging function can be changed.
| retries = retries_later; | ||
| } | ||
|
|
||
| if (strip_newline && buf[received-1] == 0x0a) |
There was a problem hiding this comment.
I suggest to use '\n' instead of 0x0a, it's more clear for people who don't have ASCII burned in their retinas :-)
|
Hey sorry for the slow answer. I have been busy - including a move that requires me to set up my dev environment for this again. This should still work for the Korad KA3005PS (I'll double-check when I have a dev env for this again), but I cannot verify if it still works on the other devices listed: libsigrok/src/hardware/korad-kaxxxxp/api.c Lines 62 to 81 in c79a973 If we want to err on the safe side, I'd wait with merging this until someone with one of those devices tests the changes as well. On the other hand, that can take forever - but I'm not sure if that warrants testing on production. We do know that the current state does NOT work for the Velleman LABPS3005DN, as reported by @cnf. There might still be common ground, as I believe there may still be options with this:
But for this I can also not verify if it will break any of the other devices. They seem to be a bit more finicky than I'd like. Thank you for review and comments, will fix/reply to them separately :) |
|
@MartinJM No worries, thanks for taking the time to get back to us. I wonder if @cnf could check if the LABPS3005DN works with current master? Based on the conversation (which is pretty long and I admittedly only read the first and last third to see if it got resolved) I'm not clear if it is actually this PR that breaks it. Also, it might be possible to scan in two phases? I suspect some devices will compare the whole "packet" received (I suspect this is USB serial?u), so if there is anything extra, it would screw things up. On the other hand, I think we can just probe the device twice and see if it responds to the other probe if the first one doesn't match. I'll check at work, we have some Korad PSUs, we might get lucky. |
I tested it, and it still works perfectly fine!
It was not supported on master either. It needs a literal
I like that idea! Will switch to that instead - marking the PR as WIP in the meantime. That should then also be a solution to the Velleman LABPS3005DN - as that can then be a third phase.
Though I am still curious if it still works on the other PSUs, if you don't mind checking. I tend to be curious :)
Hahaha, completely fair :) |
MartinJM
changed the title
MartinJM
changed the title
|
Hi! Good news, bad news. Good news is, I got a powersupply we can test with. It' Korad KA3003P. The firmware reports Now, the bad news is the added newline breaks the detection: And I'll let it finish with some good news. I tried retrying the |
|
@MartinJM I also noticed the PR #219 seems to do a similar thing, including the two phase scan. I originally though we could add the PN here too, but it seems to require more stuff there. I suggest we merge this PR first, since it's more up to date, we can then update the other one just to modify the protocol part (if the original author is still interested/somebody has the device for testing). |
Cool, thank you for testing! Yeah a bit sad it didn't work out of the box like this, but good to know the other options.
Oh good find, not sure how I missed that before writing/opening this...
I think they just send EDIT:
This is a good point that I hadn't considered yet. Would it work fine if we just send |
|
@Krakonos I pushed a version that uses |
|
@MartinJM Good attempt, but unfortunately, it doesn't work. The PSU does not reply unless I give at least some delay there. It likely processes a command after some period of inactivity. I have zero idea how somebody thought this is a good idea. In any case, this works: The value 40000 is the minimum I found working, 30000 already fails. I'd suggest using higher delay Not the most elegant, but might be an alternative. This device is not automatically scanned based on vid/pid (it uses a generic converter), so I don't think it is an issue. I wonder if the following patch would work with your PSU? |
Cool, thank you for trying!
Yeah the second patch works perfectly fine. The length of the sleep between the two doesn't seem to matter too much. I tried it with up to 10 seconds ( It also works as the following, which could also enable detecting the Velleman DN - if @cnf is still available for testing: diff --git a/src/hardware/korad-kaxxxxp/api.c b/src/hardware/korad-kaxxxxp/api.c
index 7e4f2eb7..9b26ce7a 100644
--- a/src/hardware/korad-kaxxxxp/api.c
+++ b/src/hardware/korad-kaxxxxp/api.c
@@ -287,7 +287,9 @@ static GSList *scan(struct sr_dev_driver *di, GSList *options)
* But the normal ones need an `*IDN?` without anything at the end.
* So this sends a combination that satisfies these constraints.
*/
- ret = korad_kaxxxxp_send_cmd(serial, "*IDN?\n*IDN?", false);
+ ret = korad_kaxxxxp_send_cmd(serial, "*IDN?", false);
+ g_usleep(500000);
+ ret = korad_kaxxxxp_send_cmd(serial, "n\n", false);
if (ret < 0)
return NULL;The current state of this PR actually doesn't work too well for my device. When closing the connection it seems to restart the serial connection, which is not ideal. With your patch it doesn't do that, so it's even better for my device as well! |
|
Ok, I think we can call that a decent solution then. If @cnf will get in touch to test some of the variants we can tune it a little more. I'm not sure about the On other note, could you please rebase this on top of master and clean up the commits (squash the iterative commits and update commit messages to prefix with "drivername: "? Once this is done, I'd be happy to recommend this for merging. |
|
Also:
I don't see how these changes could affect what happens on the device after the connection closes. But these PSUs are weird. The only thing I see on mine is it beeps after some inactivity (30s? give or take), but does not seem reset any state. |
Should just be a single
Will do! I'll remove the draft status when complete.
Yeah I'm not sure either. But as long as this version works I'm fine with it :) |
MartinJM
force-pushed
the
add_device_korad_ka3005ps
branch
from
6dd6d90 to
6a8f93c
Compare
MartinJM
force-pushed
the
add_device_korad_ka3005ps
branch
from
6a8f93c to
a421746
Compare
|
@MartinJM Great, thanks! I gave it a quick test and all seems good. @cnf In case you're interested, please try the patch posted above to see if it resolves your issue as well. Tag us here/reach out on the mailing list if so, we'll open and merge a new PR. @abraxa Please check this PR out, I believe it is good to go for merging. PS: I wonder if it's possible to flash/cross flash those PSUs to some common decent firmware. These are not the only quirks, I recall some people having more serious functionality related issues... |
Sorry, only just say the messages... let me see what to apply where, and give it a test... edit: i'm struggling a bit figuring out what patches to apply. So i am using master, adding https://patch-diff.githubusercontent.com/raw/sigrokproject/libsigrok/pull/269.patch on top of that, and then Is that right? It is failing to apply for me. |
Yes, a not crappy common firmware would be awesome :P I don't know if these are flashable, or if any code is available for them? |
The patch linked above ( https://github.com/user-attachments/files/23824054/0001-Add-Velleman-DN-support.patch ) on top of this branch.
Could be nice if possible indeed. Though supporting devices out of the box is also good imo. |
i guess it was renamed somewhere, and i'm patching against a wrong version somewhere... |
|
Oh no that's a typo in my patch - my bad. You can change |
yeah, I just figured that out :P |
|
@cnf Hi! Would you mind running this with ... I suggest to try in "\n\n", or maybe we'll need to split it off. It'd be nice if you could play with this code a little bit and figure out what gives you a response and what doesn't. Unfortunately, there are too many combinations that need to be tried out and I'm starting to think we might not be able to auto detect all of them easily. |
|
@Krakonos sure! though it doesn't respond to scan anymore, not sure what changed I have tried various combinations (see #269 (comment)) it seems to need |
|
LABPS3005DN.patch of course, it still won't detect my actually set values... maybe this one just needs its own driver? or the unified firmware :P |
4 participants
This PR adds support for the Korad KA3005PS. Protocol is almost the same as for the KA3005P, except that it needs a newline1 at the end of every request, and sends back a newline at the end of every response. That is added in this PR.
One thing to keep in mind: This PR also sends the newline for the
*IDN?to identify the device. I assume this will also work for the KA3005P, but I cannot check.Footnotes
It also seems to work with a carriage return, but the responses will still end with a newline. I've implemented it with newlines. ↩