OLS Improvements #51
OLS Improvements #51
Conversation
|
Also: Please view in gitk, Github doesn't properly order commit, and that's been the case for years now. -.- |
|
|
|
@uwehermann: Is there anything I can help with? |
|
Very interesting branch you have there! I will try to free some time so I can assist with additional review and testing. |
|
@ninjadrm: Thank you! (: If there's anything I can help with, just ask! I can also squash this into 2–3 commits, if it helps. |
|
@v1ne I appreciate you asking, @wsakernel is our resident OLS master, so as soon as he gives his thumbs up, the PR will be merged. I apologize for the delay but we're not that big of a team as you can imagine :) |
|
Time is scarce, no surprise here. Didn't make it before my holidays. I have not forgotten this and will schedule it. The number of patches is fine. Smaller patches are easier to review, and the granularity looks great. @abraxa: "resident OLS master" Wel, thanks, but it is simply the device I have most interest in. |
|
Okay, these were my comments to the first 6 patches. I'll try to review the next 2 ones later. My suggestion is that you check my comments on the first 8 patches then, acknowledge or discard them, and make a seperate pull request for them. Divide & conquer seems to be the way forward here. Makes sense? |
|
Okay, I think I am done with reviewing the first 8 patches. I'll try to test them with my OLS and Pepino the next days, but I don't expect problems. |
|
@wsakernel: I created #84 with the first 8 commits. Interdiff: diff --git a/src/hardware/openbench-logic-sniffer/api.c b/src/hardware/openbench-logic-sniffer/api.c
index a4230fbb..315a56d3 100644
--- a/src/hardware/openbench-logic-sniffer/api.c
+++ b/src/hardware/openbench-logic-sniffer/api.c
@@ -412,7 +412,7 @@ static int dev_acquisition_start(const struct sr_dev_inst *sdi)
/* Reset all operational states. */
devc->rle_count = 0;
- devc->trigger_rle_at_smpl_from_end = CTX_NO_TRIGGER;
+ devc->trigger_rle_at_smpl_from_end = OLS_NO_TRIGGER;
devc->cnt_samples = devc->raw_sample_size = 0;
devc->cnt_rx_bytes = devc->cnt_rx_raw_samples = 0;
memset(devc->raw_sample, 0, 4);
diff --git a/src/hardware/openbench-logic-sniffer/protocol.c b/src/hardware/openbench-logic-sniffer/protocol.c
index dba62469..1a21b2b6 100644
--- a/src/hardware/openbench-logic-sniffer/protocol.c
+++ b/src/hardware/openbench-logic-sniffer/protocol.c
@@ -185,8 +185,8 @@ SR_PRIV struct dev_context *ols_dev_new(void)
struct dev_context *devc;
devc = g_malloc0(sizeof(struct dev_context));
- devc->trigger_at_smpl = CTX_NO_TRIGGER;
- devc->trigger_rle_at_smpl_from_end = CTX_NO_TRIGGER;
+ devc->trigger_at_smpl = OLS_NO_TRIGGER;
+ devc->trigger_rle_at_smpl_from_end = OLS_NO_TRIGGER;
return devc;
}
@@ -473,8 +473,8 @@ SR_PRIV int ols_receive_data(int fd, int revents, void *cb_data)
sr_dbg("RLE count: %" PRIu64, devc->rle_count);
devc->raw_sample_size = 0;
- if (devc->trigger_at_smpl != CTX_NO_TRIGGER
- && devc->trigger_rle_at_smpl_from_end == CTX_NO_TRIGGER
+ if (devc->trigger_at_smpl != OLS_NO_TRIGGER
+ && devc->trigger_rle_at_smpl_from_end == OLS_NO_TRIGGER
&& (unsigned int)devc->trigger_at_smpl == devc->cnt_rx_raw_samples)
devc->trigger_rle_at_smpl_from_end = devc->cnt_samples;
@@ -531,8 +531,8 @@ SR_PRIV int ols_receive_data(int fd, int revents, void *cb_data)
}
if (devc->capture_flags & CAPTURE_FLAG_RLE
- && devc->trigger_at_smpl != CTX_NO_TRIGGER
- && devc->trigger_rle_at_smpl_from_end == CTX_NO_TRIGGER
+ && devc->trigger_at_smpl != OLS_NO_TRIGGER
+ && devc->trigger_rle_at_smpl_from_end == OLS_NO_TRIGGER
&& (unsigned int)devc->trigger_at_smpl == devc->cnt_rx_raw_samples)
devc->trigger_rle_at_smpl_from_end = devc->cnt_samples;
@@ -564,12 +564,12 @@ process_and_forward:
devc->cnt_samples);
if (devc->capture_flags & CAPTURE_FLAG_RLE) {
- if (devc->trigger_rle_at_smpl_from_end != CTX_NO_TRIGGER)
+ if (devc->trigger_rle_at_smpl_from_end != OLS_NO_TRIGGER)
devc->trigger_at_smpl = devc->cnt_samples - devc->trigger_rle_at_smpl_from_end;
else {
- if (devc->trigger_at_smpl != CTX_NO_TRIGGER)
+ if (devc->trigger_at_smpl != OLS_NO_TRIGGER)
sr_warn("No trigger point found. Short read?");
- devc->trigger_at_smpl = CTX_NO_TRIGGER;
+ devc->trigger_at_smpl = OLS_NO_TRIGGER;
}
}
@@ -584,7 +584,7 @@ process_and_forward:
memcpy(&devc->sample_buf[4*(devc->cnt_samples-i-1)], temp, 4);
}
- if (devc->trigger_at_smpl != CTX_NO_TRIGGER) {
+ if (devc->trigger_at_smpl != OLS_NO_TRIGGER) {
/*
* A trigger was set up, so we need to tell the frontend
* about it.
@@ -605,7 +605,7 @@ process_and_forward:
}
/* Send post-trigger / all captured samples. */
- unsigned int num_pre_trigger_samples = devc->trigger_at_smpl == CTX_NO_TRIGGER
+ unsigned int num_pre_trigger_samples = devc->trigger_at_smpl == OLS_NO_TRIGGER
? 0 : MIN((unsigned int)devc->trigger_at_smpl, devc->cnt_samples);
if (devc->cnt_samples > num_pre_trigger_samples) {
packet.type = SR_DF_LOGIC;
@@ -677,7 +677,7 @@ SR_PRIV int ols_prepare_acquisition(const struct sr_dev_inst *sdi) {
devc->limit_samples = (MIN(devc->max_samples / num_changroups, devc->limit_samples) + 3) / 4 * 4;
uint32_t readcount = devc->limit_samples / 4;
uint32_t delaycount;
- int trigger_point = CTX_NO_TRIGGER;
+ int trigger_point = OLS_NO_TRIGGER;
if (!(devc->device_flags & DEVICE_FLAG_IS_DEMON_CORE)) { /* basic trigger only */
struct ols_basic_trigger_desc basic_trigger_desc;
@@ -719,7 +719,7 @@ SR_PRIV int ols_prepare_acquisition(const struct sr_dev_inst *sdi) {
* lookup is needed while reading the samples. Set up the right trigger
* point in that case or the normal trigger point for non-RLE acquisitions.
*/
- devc->trigger_at_smpl = trigger_point == CTX_NO_TRIGGER ? CTX_NO_TRIGGER
+ devc->trigger_at_smpl = trigger_point == OLS_NO_TRIGGER ? OLS_NO_TRIGGER
: devc->capture_flags & CAPTURE_FLAG_RLE ?
(int)devc->limit_samples - trigger_point : trigger_point;
diff --git a/src/hardware/openbench-logic-sniffer/protocol.h b/src/hardware/openbench-logic-sniffer/protocol.h
index 3bb7d412..5ac9a0fd 100644
--- a/src/hardware/openbench-logic-sniffer/protocol.h
+++ b/src/hardware/openbench-logic-sniffer/protocol.h
@@ -99,8 +99,7 @@
#define CAPTURE_FLAG_DEMUX (1 << 0)
/* Capture context magic numbers */
-#define CTX_NO_TRIGGER (-1)
-
+#define OLS_NO_TRIGGER (-1)
struct dev_context {
/* constant device properties: */ |
|
Just to let you know, I hope to fisnish more testing and reviewing the second batch in about 2 weeks. |
Due to the nature of RLE, the number of samples from the start to the trigger point is unknown. What is known is that the hardware triggers still records a given number of raw samples to its sample memory. When reading and expanding these raw samples from the back (remember, OLS sends the recording back-to-front), remember the expanded sample position from the back and then map it back to the right position from the front once the total number of expanded samples is known.
This mode can double the space available for RLE messages because unchanged values don't have to be repeated.
This adds code from http://web.archive.org/web/20190317154112/ http://mygizmos.org/ols/Logic-Sniffer-FPGA-Spec.pdf (GPL2 with the option to relicense it to any later version of that license) with reformatting and without typos to set up the LUT bits. The trigger setup starts with a delay to collect the required number of pre-trigger samples. Afterwards, the remaining samples are captured or further trigger stages follow. Each of these extra stages mirrors what the user has defined as trigger pattern: Level and edge triggers are combined and the state machine only advances to the next stage if all levels and at least one edge meets the conditions. Contrary to level triggers, edge triggers are ORed together. This is an undocumented property of the Demon Core.
v1ne
force-pushed
the
ols-improvements
branch
from
5579c62 to
5696a4d
Compare
|
OK, so I:
@gsigh: I'm sorry if my name caused any unease, but I do not want my contributions to be associated with my commercial activities. @wsakernel: With this push, "ols: Clean up: Obey code style" has changed, plus the attached diff, which mostly concerns advanced triggers (see function names in the diff). Should I pull the commits we talked about into a new PR or how do we want to continue? In order to obey the code style, I had to run diff --git a/src/hardware/openbench-logic-sniffer/protocol.c b/src/hardware/openbench-logic-sniffer/protocol.c
index a7e662c1..df34c690 100644
--- a/src/hardware/openbench-logic-sniffer/protocol.c
+++ b/src/hardware/openbench-logic-sniffer/protocol.c
@@ -38,11 +38,11 @@ SR_PRIV int send_shortcommand(struct sr_serial_dev_inst *serial,
sr_dbg("Sending cmd 0x%.2x.", command);
buf[0] = command;
- if (serial_write_blocking(serial, buf, 1, serial_timeout(serial, 1)) !=
- 1)
+ if (serial_write_blocking(serial, buf, 1, serial_timeout(serial, 1)) != 1)
return SR_ERR;
- RETURN_ON_ERROR(serial_drain(serial));
+ if (serial_drain(serial) != SR_OK)
+ return SR_ERR;
return SR_OK;
}
@@ -59,11 +59,11 @@ SR_PRIV int send_longcommand(struct sr_serial_dev_inst *serial, uint8_t command,
buf[2] = data[1];
buf[3] = data[2];
buf[4] = data[3];
- if (serial_write_blocking(serial, buf, 5, serial_timeout(serial, 1)) !=
- 5)
+ if (serial_write_blocking(serial, buf, 5, serial_timeout(serial, 1)) != 5)
return SR_ERR;
- RETURN_ON_ERROR(serial_drain(serial));
+ if (serial_drain(serial) != SR_OK)
+ return SR_ERR;
return SR_OK;
}
@@ -78,7 +78,7 @@ static int ols_send_longdata(struct sr_serial_dev_inst *serial, uint8_t command,
SR_PRIV int ols_send_reset(struct sr_serial_dev_inst *serial)
{
- int i, ret;
+ int i, ret, delay_ms;
char dummy[16];
/* Drain all data so that the remote side is surely listening. */
@@ -86,15 +86,17 @@ SR_PRIV int ols_send_reset(struct sr_serial_dev_inst *serial)
if (ret != SR_OK)
return ret;
- for (i = 0; i < 5; i++)
- RETURN_ON_ERROR(send_shortcommand(serial, CMD_RESET));
+ for (i = 0; i < 5; i++) {
+ if ((ret = send_shortcommand(serial, CMD_RESET)) != SR_OK)
+ return ret;
+ }
/*
* Remove all stray output that arrived in between.
* This is likely to happen when RLE is being used because
* the device seems to return a bit more data than we request.
*/
- int delay_ms = serial_timeout(serial, 16);
+ delay_ms = serial_timeout(serial, 16);
while ((ret = serial_read_blocking(serial, &dummy, 16, delay_ms)) > 0);
return ret;
@@ -319,8 +321,7 @@ SR_PRIV struct sr_dev_inst *get_metadata(struct sr_serial_dev_inst *serial)
case 2:
/* 8-bit unsigned integer */
delay_ms = serial_timeout(serial, 1);
- if (serial_read_blocking(serial, &tmp_c, 1, delay_ms) !=
- 1)
+ if (serial_read_blocking(serial, &tmp_c, 1, delay_ms) != 1)
break;
sr_dbg("Got metadata token 0x%.2x value 0x%.2x.", key,
tmp_c);
@@ -394,11 +395,12 @@ SR_PRIV int ols_set_samplerate(const struct sr_dev_inst *sdi,
SR_PRIV void abort_acquisition(const struct sr_dev_inst *sdi)
{
+ int ret;
struct sr_serial_dev_inst *serial = sdi->conn;
ols_send_reset(serial);
- int ret = serial_source_remove(sdi->session, serial);
+ ret = serial_source_remove(sdi->session, serial);
if (ret != SR_OK)
sr_warn("Couldn't close serial port: %i", ret);
@@ -452,6 +454,9 @@ SR_PRIV int ols_receive_data(int fd, int revents, void *cb_data)
devc->raw_sample_size);
if (devc->raw_sample_size == num_changroups) {
+ unsigned int samples_to_write;
+ uint64_t new_sample_buf_size;
+
devc->cnt_rx_raw_samples++;
/*
* Got a full sample. Convert from the OLS's little-endian
@@ -529,10 +534,11 @@ SR_PRIV int ols_receive_data(int fd, int revents, void *cb_data)
devc->raw_sample[0]);
}
- uint64_t samples_to_write = devc->rle_count + 1;
- uint64_t new_sample_buf_size =
+ samples_to_write = devc->rle_count + 1;
+ new_sample_buf_size =
4 * MAX(devc->limit_samples,
devc->cnt_samples + samples_to_write);
+
if (devc->sample_buf_size < new_sample_buf_size) {
uint64_t old_size = devc->sample_buf_size;
new_sample_buf_size *= 2;
@@ -576,6 +582,8 @@ SR_PRIV int ols_receive_data(int fd, int revents, void *cb_data)
process_and_forward:
if (revents != G_IO_IN ||
devc->cnt_rx_raw_samples == devc->limit_samples) {
+ unsigned int num_pre_trigger_samples;
+
if (devc->cnt_rx_raw_samples != devc->limit_samples)
sr_warn("Finished with unexpected sample count. Timeout?");
@@ -640,7 +648,7 @@ process_and_forward:
}
/* Send post-trigger / all captured samples. */
- unsigned int num_pre_trigger_samples =
+ num_pre_trigger_samples =
devc->trigger_at_smpl == OLS_NO_TRIGGER ?
0 :
MIN((unsigned int)devc->trigger_at_smpl,
@@ -675,12 +683,12 @@ ols_set_basic_trigger_stage(const struct ols_basic_trigger_desc *trigger_desc,
uint8_t cmd, arg[4];
cmd = CMD_SET_BASIC_TRIGGER_MASK0 + stage * 4;
- RETURN_ON_ERROR(ols_send_longdata(serial, cmd,
- trigger_desc->trigger_mask[stage]));
+ if (ols_send_longdata(serial, cmd, trigger_desc->trigger_mask[stage]) != SR_OK)
+ return SR_ERR;
cmd = CMD_SET_BASIC_TRIGGER_VALUE0 + stage * 4;
- RETURN_ON_ERROR(ols_send_longdata(serial, cmd,
- trigger_desc->trigger_value[stage]));
+ if (ols_send_longdata(serial, cmd, trigger_desc->trigger_value[stage]) != SR_OK)
+ return SR_ERR;
cmd = CMD_SET_BASIC_TRIGGER_CONFIG0 + stage * 4;
arg[0] = arg[1] = arg[3] = 0x00;
@@ -688,13 +696,17 @@ ols_set_basic_trigger_stage(const struct ols_basic_trigger_desc *trigger_desc,
if (stage == trigger_desc->num_stages - 1)
/* Last stage, fire when this one matches. */
arg[3] |= TRIGGER_START;
- RETURN_ON_ERROR(send_longcommand(serial, cmd, arg));
+ if (send_longcommand(serial, cmd, arg) != SR_OK)
+ return SR_ERR;
return SR_OK;
}
SR_PRIV int ols_prepare_acquisition(const struct sr_dev_inst *sdi)
{
+ int ret, trigger_point;
+ uint32_t readcount, delaycount;
+
struct dev_context *devc = sdi->priv;
struct sr_serial_dev_inst *serial = sdi->conn;
@@ -703,7 +715,8 @@ SR_PRIV int ols_prepare_acquisition(const struct sr_dev_inst *sdi)
* reset command must be send prior each arm command
*/
sr_dbg("Send reset command before trigger configure");
- RETURN_ON_ERROR(ols_send_reset(serial));
+ if ((ret = ols_send_reset(serial)) != SR_OK)
+ return ret;
int num_changroups = 0;
uint8_t changroup_mask = 0;
@@ -722,9 +735,8 @@ SR_PRIV int ols_prepare_acquisition(const struct sr_dev_inst *sdi)
devc->limit_samples =
(MIN(devc->max_samples / num_changroups, devc->limit_samples) + 3)
/ 4 * 4;
- uint32_t readcount = devc->limit_samples / 4;
- uint32_t delaycount;
- int trigger_point = OLS_NO_TRIGGER;
+ readcount = devc->limit_samples / 4;
+ trigger_point = OLS_NO_TRIGGER;
if (!(devc->device_flags & DEVICE_FLAG_IS_DEMON_CORE)) {
/* basic trigger only */
@@ -741,15 +753,19 @@ SR_PRIV int ols_prepare_acquisition(const struct sr_dev_inst *sdi)
for (int i = 0; i < basic_trigger_desc.num_stages;
i++) {
sr_dbg("Setting OLS stage %d trigger.", i);
- RETURN_ON_ERROR(ols_set_basic_trigger_stage(
- &basic_trigger_desc, serial, i));
+ ret = ols_set_basic_trigger_stage(
+ &basic_trigger_desc, serial, i);
+ if (ret != SR_OK)
+ return ret;
}
} else {
/* No triggers configured, force trigger on first stage. */
sr_dbg("Forcing trigger at stage 0.");
basic_trigger_desc.num_stages = 1;
- RETURN_ON_ERROR(ols_set_basic_trigger_stage(
- &basic_trigger_desc, serial, 0));
+ if ((ret = ols_set_basic_trigger_stage(
+ &basic_trigger_desc, serial, 0)) != SR_OK)
+ return ret;
+
delaycount = readcount;
}
} else {
@@ -784,25 +800,27 @@ SR_PRIV int ols_prepare_acquisition(const struct sr_dev_inst *sdi)
/* Samplerate. */
sr_dbg("Setting samplerate to %" PRIu64 "Hz (divider %u)",
devc->cur_samplerate, devc->cur_samplerate_divider);
- RETURN_ON_ERROR(
- ols_send_longdata(serial, CMD_SET_DIVIDER,
- devc->cur_samplerate_divider & 0x00FFFFFF));
+ if (ols_send_longdata(serial, CMD_SET_DIVIDER,
+ devc->cur_samplerate_divider & 0x00FFFFFF) != SR_OK)
+ return SR_ERR;
/* Send sample limit and pre/post-trigger capture ratio. */
sr_dbg("Setting sample limit %d, trigger point at %d",
(readcount - 1) * 4, (delaycount - 1) * 4);
if (devc->max_samples > 256 * 1024) {
- RETURN_ON_ERROR(ols_send_longdata(serial, CMD_CAPTURE_READCOUNT,
- readcount - 1));
- RETURN_ON_ERROR(ols_send_longdata(
- serial, CMD_CAPTURE_DELAYCOUNT, delaycount - 1));
+ if (ols_send_longdata(serial, CMD_CAPTURE_READCOUNT,
+ readcount - 1) != SR_OK)
+ return SR_ERR;
+ if (ols_send_longdata(serial, CMD_CAPTURE_DELAYCOUNT,
+ delaycount - 1) != SR_OK)
+ return SR_ERR;
} else {
uint8_t arg[4];
WL16(&arg[0], readcount - 1);
WL16(&arg[2], delaycount - 1);
- RETURN_ON_ERROR(
- send_longcommand(serial, CMD_CAPTURE_SIZE, arg));
+ if (send_longcommand(serial, CMD_CAPTURE_SIZE, arg) != SR_OK)
+ return SR_ERR;
}
/* Flag register. */
@@ -843,8 +861,8 @@ SR_PRIV int ols_prepare_acquisition(const struct sr_dev_inst *sdi)
devc->capture_flags &=
~(CAPTURE_FLAG_RLEMODE0 | CAPTURE_FLAG_RLEMODE1);
- RETURN_ON_ERROR(
- ols_send_longdata(serial, CMD_SET_FLAGS, devc->capture_flags));
+ if (ols_send_longdata(serial, CMD_SET_FLAGS, devc->capture_flags) != SR_OK)
+ return SR_ERR;
return SR_OK;
}
@@ -855,15 +873,16 @@ static int ols_set_advanced_level_trigger(
uint8_t num_trigger_term, /* 0-9 for trigger terms a-j */
uint32_t target, uint32_t mask)
{
+ int ret;
uint32_t lutmask = 1;
uint32_t lutbits[4] = { 0, 0, 0, 0 };
for (uint32_t i = 0; i < 16; ++i) {
- if (((i ^ ((target >> 0) & 0xF)) & ((mask >> 0) & 0xF)) == 0)
+ if (((i ^ ((target >> 0) & 0xF)) & ((mask >> 0) & 0xF)) == 0)
lutbits[0] |= lutmask;
- if (((i ^ ((target >> 4) & 0xF)) & ((mask >> 4) & 0xF)) == 0)
+ if (((i ^ ((target >> 4) & 0xF)) & ((mask >> 4) & 0xF)) == 0)
lutbits[0] |= (lutmask << 16);
- if (((i ^ ((target >> 8) & 0xF)) & ((mask >> 8) & 0xF)) == 0)
+ if (((i ^ ((target >> 8) & 0xF)) & ((mask >> 8) & 0xF)) == 0)
lutbits[1] |= lutmask;
if (((i ^ ((target >> 12) & 0xF)) & ((mask >> 12) & 0xF)) == 0)
lutbits[1] |= (lutmask << 16);
@@ -878,16 +897,26 @@ static int ols_set_advanced_level_trigger(
lutmask <<= 1;
}
- RETURN_ON_ERROR(ols_send_longdata(serial, CMD_SET_ADVANCED_TRIG_SEL,
- 0x20 + (num_trigger_term % 10)));
- RETURN_ON_ERROR(ols_send_longdata(serial, CMD_SET_ADVANCED_TRIG_WRITE,
- lutbits[3]));
- RETURN_ON_ERROR(ols_send_longdata(serial, CMD_SET_ADVANCED_TRIG_WRITE,
- lutbits[2]));
- RETURN_ON_ERROR(ols_send_longdata(serial, CMD_SET_ADVANCED_TRIG_WRITE,
- lutbits[1]));
- RETURN_ON_ERROR(ols_send_longdata(serial, CMD_SET_ADVANCED_TRIG_WRITE,
- lutbits[0]));
+ ret = ols_send_longdata(serial, CMD_SET_ADVANCED_TRIG_SEL, 0x20 + (num_trigger_term % 10));
+ if (ret != SR_OK)
+ return ret;
+
+ ret = ols_send_longdata(serial, CMD_SET_ADVANCED_TRIG_WRITE, lutbits[3]);
+ if (ret != SR_OK)
+ return ret;
+
+ ret = ols_send_longdata(serial, CMD_SET_ADVANCED_TRIG_WRITE, lutbits[2]);
+ if (ret != SR_OK)
+ return ret;
+
+ ret = ols_send_longdata(serial, CMD_SET_ADVANCED_TRIG_WRITE, lutbits[1]);
+ if (ret != SR_OK)
+ return ret;
+
+ ret = ols_send_longdata(serial, CMD_SET_ADVANCED_TRIG_WRITE, lutbits[0]);
+ if (ret != SR_OK)
+ return ret;
+
return SR_OK;
}
@@ -913,11 +942,15 @@ ols_set_advanced_edge_trigger(struct sr_serial_dev_inst *serial,
uint32_t rising_edge, uint32_t falling_edge,
uint32_t neither_edge)
{
- RETURN_ON_ERROR(ols_send_longdata(serial, CMD_SET_ADVANCED_TRIG_SEL,
- 0x34 + (edgesel & 1)));
-
+ int ret;
uint32_t lutbits = 0;
uint32_t bitmask = 0x80000000;
+
+ ret = ols_send_longdata(serial, CMD_SET_ADVANCED_TRIG_SEL,
+ 0x34 + (edgesel & 1));
+ if (ret != SR_OK)
+ return ret;
+
for (unsigned int i = 0; i < 16; i = i + 1) {
/* Evaluate indata bit1... */
if (neither_edge & bitmask)
@@ -945,8 +978,10 @@ ols_set_advanced_edge_trigger(struct sr_serial_dev_inst *serial,
lutbits <<= 16;
else {
/* write total of 256 bits */
- RETURN_ON_ERROR(ols_send_longdata(
- serial, CMD_SET_ADVANCED_TRIG_WRITE, lutbits));
+ ret = ols_send_longdata(
+ serial, CMD_SET_ADVANCED_TRIG_WRITE, lutbits);
+ if (ret != SR_OK)
+ return ret;
lutbits = 0;
}
}
@@ -958,14 +993,28 @@ static int ols_set_advanced_trigger_timer(struct sr_serial_dev_inst *serial,
int timersel, /* 0 or 1 */
uint64_t count_10ns)
{
- RETURN_ON_ERROR(ols_send_longdata(serial, CMD_SET_ADVANCED_TRIG_SEL,
- 0x38 + (timersel & 1) * 2));
- RETURN_ON_ERROR(ols_send_longdata(serial, CMD_SET_ADVANCED_TRIG_WRITE,
- count_10ns & 0xFFFFFFFF));
- RETURN_ON_ERROR(ols_send_longdata(serial, CMD_SET_ADVANCED_TRIG_SEL,
- 0x39 + (timersel & 1) * 2));
- RETURN_ON_ERROR(ols_send_longdata(serial, CMD_SET_ADVANCED_TRIG_WRITE,
- count_10ns >> 32));
+ int ret;
+
+ ret = ols_send_longdata(
+ serial, CMD_SET_ADVANCED_TRIG_SEL, 0x38 + (timersel & 1) * 2);
+ if (ret != SR_OK)
+ return ret;
+
+ ret = ols_send_longdata(
+ serial, CMD_SET_ADVANCED_TRIG_WRITE, count_10ns & 0xFFFFFFFF);
+ if (ret != SR_OK)
+ return ret;
+
+ ret = ols_send_longdata(
+ serial, CMD_SET_ADVANCED_TRIG_SEL, 0x39 + (timersel & 1) * 2);
+ if (ret != SR_OK)
+ return ret;
+
+ ret = ols_send_longdata(
+ serial, CMD_SET_ADVANCED_TRIG_WRITE, count_10ns >> 32);
+ if (ret != SR_OK)
+ return ret;
+
return SR_OK;
}
@@ -1010,25 +1059,43 @@ static int ols_set_advanced_trigger_sum(
int op_mid2, /* sums up f, g, h, range2, i, edge2, j, timer2 */
int op_final) /* sums up mid1, mid2 */
{
- RETURN_ON_ERROR(ols_send_longdata(serial, CMD_SET_ADVANCED_TRIG_SEL,
- 0x40 + (statenum * 4) + stateterm));
- RETURN_ON_ERROR(ols_send_longdata(serial, CMD_SET_ADVANCED_TRIG_WRITE,
- finalvalue[op_final]));
- RETURN_ON_ERROR(ols_send_longdata(serial, CMD_SET_ADVANCED_TRIG_WRITE,
- (midvalue[op_mid2] << 16) |
- midvalue[op_mid1]));
- RETURN_ON_ERROR(ols_send_longdata(serial, CMD_SET_ADVANCED_TRIG_WRITE,
- (pairvalue[op_j_timer2] << 16) |
- pairvalue[op_i_edge2]));
- RETURN_ON_ERROR(ols_send_longdata(serial, CMD_SET_ADVANCED_TRIG_WRITE,
- (pairvalue[op_h_range2] << 16) |
- pairvalue[op_fg]));
- RETURN_ON_ERROR(ols_send_longdata(serial, CMD_SET_ADVANCED_TRIG_WRITE,
- (pairvalue[op_e_timer1] << 16) |
- pairvalue[op_d_edge1]));
- RETURN_ON_ERROR(ols_send_longdata(serial, CMD_SET_ADVANCED_TRIG_WRITE,
- (pairvalue[op_c_range1] << 16) |
- pairvalue[op_ab]));
+ int ret;
+
+ ret = ols_send_longdata(serial, CMD_SET_ADVANCED_TRIG_SEL,
+ 0x40 + (statenum * 4) + stateterm);
+ if (ret != SR_OK)
+ return ret;
+
+ ret = ols_send_longdata(serial, CMD_SET_ADVANCED_TRIG_WRITE,
+ finalvalue[op_final]);
+ if (ret != SR_OK)
+ return ret;
+
+ ret = ols_send_longdata(serial, CMD_SET_ADVANCED_TRIG_WRITE,
+ (midvalue[op_mid2] << 16) | midvalue[op_mid1]);
+ if (ret != SR_OK)
+ return ret;
+
+ ret = ols_send_longdata(serial, CMD_SET_ADVANCED_TRIG_WRITE,
+ (pairvalue[op_j_timer2] << 16) | pairvalue[op_i_edge2]);
+ if (ret != SR_OK)
+ return ret;
+
+ ret = ols_send_longdata(serial, CMD_SET_ADVANCED_TRIG_WRITE,
+ (pairvalue[op_h_range2] << 16) | pairvalue[op_fg]);
+ if (ret != SR_OK)
+ return ret;
+
+ ret = ols_send_longdata(serial, CMD_SET_ADVANCED_TRIG_WRITE,
+ (pairvalue[op_e_timer1] << 16) | pairvalue[op_d_edge1]);
+ if (ret != SR_OK)
+ return ret;
+
+ ret = ols_send_longdata(serial, CMD_SET_ADVANCED_TRIG_WRITE,
+ (pairvalue[op_c_range1] << 16) | pairvalue[op_ab]);
+ if (ret != SR_OK)
+ return ret;
+
return SR_OK;
}
@@ -1055,30 +1122,29 @@ static int ols_set_advanced_trigger_state(
uint8_t else_state, /* 0 to 15 */
uint32_t obtain_count)
{
+ int ret;
+
uint32_t value = ((else_state & TRIGSTATE_STATENUM_MASK)
<< TRIGSTATE_ELSE_BITOFS) |
(obtain_count & TRIGSTATE_OBTAIN_MASK);
- if (last_state)
- value |= TRIGSTATE_LASTSTATE;
- if (set_trigger)
- value |= TRIGSTATE_TRIGGER_FLAG;
- if (start_timer & 1)
- value |= TRIGSTATE_START_TIMER0;
- if (start_timer & 2)
- value |= TRIGSTATE_START_TIMER1;
- if (stop_timer & 1)
- value |= TRIGSTATE_STOLS_ADV_TRIG_OP_TIMER0;
- if (stop_timer & 2)
- value |= TRIGSTATE_STOLS_ADV_TRIG_OP_TIMER1;
- if (clear_timer & 1)
- value |= TRIGSTATE_CLEAR_TIMER0;
- if (clear_timer & 2)
- value |= TRIGSTATE_CLEAR_TIMER1;
-
- RETURN_ON_ERROR(ols_send_longdata(serial, CMD_SET_ADVANCED_TRIG_SEL,
- statenum & TRIGSTATE_STATENUM_MASK));
- RETURN_ON_ERROR(
- ols_send_longdata(serial, CMD_SET_ADVANCED_TRIG_WRITE, value));
+ if (last_state) value |= TRIGSTATE_LASTSTATE;
+ if (set_trigger) value |= TRIGSTATE_TRIGGER_FLAG;
+ if (start_timer & 1) value |= TRIGSTATE_START_TIMER0;
+ if (start_timer & 2) value |= TRIGSTATE_START_TIMER1;
+ if (stop_timer & 1) value |= TRIGSTATE_STOLS_ADV_TRIG_OP_TIMER0;
+ if (stop_timer & 2) value |= TRIGSTATE_STOLS_ADV_TRIG_OP_TIMER1;
+ if (clear_timer & 1) value |= TRIGSTATE_CLEAR_TIMER0;
+ if (clear_timer & 2) value |= TRIGSTATE_CLEAR_TIMER1;
+
+ ret = ols_send_longdata(serial, CMD_SET_ADVANCED_TRIG_SEL,
+ statenum & TRIGSTATE_STATENUM_MASK);
+ if (ret != SR_OK)
+ return ret;
+
+ ret = ols_send_longdata(serial, CMD_SET_ADVANCED_TRIG_WRITE, value);
+ if (ret != SR_OK)
+ return ret;
+
return SR_OK;
}
@@ -1086,34 +1152,46 @@ static int ols_set_advanced_trigger_sums_and_stages(
struct sr_serial_dev_inst *serial, int ols_stage, int sum_inputs[8],
gboolean is_last_stage, gboolean start_timer0)
{
+ int ret;
+
/*
* Hit only when all inputs are true. Always capture for pre-trigger and
* acquisition. Never execute the "Else" action, since we advance trigger
* stages implicity via hits.
*/
- RETURN_ON_ERROR(ols_set_advanced_trigger_sum(serial, ols_stage, OLS_ADV_TRIG_STATETERM_HIT,
- sum_inputs[0], sum_inputs[1], sum_inputs[2], sum_inputs[3],
- sum_inputs[4], sum_inputs[5], sum_inputs[6], sum_inputs[7],
- OLS_ADV_TRIG_OP_AND, OLS_ADV_TRIG_OP_AND,
- OLS_ADV_TRIG_OP_AND));
- RETURN_ON_ERROR(ols_set_advanced_trigger_sum(serial, ols_stage, OLS_ADV_TRIG_STATETERM_CAPTURE,
- OLS_ADV_TRIG_OP_ANY, OLS_ADV_TRIG_OP_ANY, OLS_ADV_TRIG_OP_ANY, OLS_ADV_TRIG_OP_ANY,
- OLS_ADV_TRIG_OP_ANY, OLS_ADV_TRIG_OP_ANY, OLS_ADV_TRIG_OP_ANY, OLS_ADV_TRIG_OP_ANY,
- OLS_ADV_TRIG_OP_AND, OLS_ADV_TRIG_OP_AND,
- OLS_ADV_TRIG_OP_ANY));
- RETURN_ON_ERROR(ols_set_advanced_trigger_sum(serial, ols_stage, OLS_ADV_TRIG_STATETERM_ELSE,
- OLS_ADV_TRIG_OP_ANY, OLS_ADV_TRIG_OP_ANY, OLS_ADV_TRIG_OP_ANY, OLS_ADV_TRIG_OP_ANY,
- OLS_ADV_TRIG_OP_ANY, OLS_ADV_TRIG_OP_ANY, OLS_ADV_TRIG_OP_ANY, OLS_ADV_TRIG_OP_ANY,
- OLS_ADV_TRIG_OP_AND, OLS_ADV_TRIG_OP_AND,
- OLS_ADV_TRIG_OP_NOP));
+ ret = ols_set_advanced_trigger_sum(serial, ols_stage, OLS_ADV_TRIG_STATETERM_HIT,
+ sum_inputs[0], sum_inputs[1], sum_inputs[2], sum_inputs[3],
+ sum_inputs[4], sum_inputs[5], sum_inputs[6], sum_inputs[7],
+ OLS_ADV_TRIG_OP_AND, OLS_ADV_TRIG_OP_AND,
+ OLS_ADV_TRIG_OP_AND);
+ if (ret != SR_OK)
+ return ret;
+
+ ret = ols_set_advanced_trigger_sum(serial, ols_stage, OLS_ADV_TRIG_STATETERM_CAPTURE,
+ OLS_ADV_TRIG_OP_ANY, OLS_ADV_TRIG_OP_ANY, OLS_ADV_TRIG_OP_ANY, OLS_ADV_TRIG_OP_ANY,
+ OLS_ADV_TRIG_OP_ANY, OLS_ADV_TRIG_OP_ANY, OLS_ADV_TRIG_OP_ANY, OLS_ADV_TRIG_OP_ANY,
+ OLS_ADV_TRIG_OP_AND, OLS_ADV_TRIG_OP_AND,
+ OLS_ADV_TRIG_OP_ANY);
+ if (ret != SR_OK)
+ return ret;
+
+ ret = ols_set_advanced_trigger_sum(serial, ols_stage, OLS_ADV_TRIG_STATETERM_ELSE,
+ OLS_ADV_TRIG_OP_ANY, OLS_ADV_TRIG_OP_ANY, OLS_ADV_TRIG_OP_ANY, OLS_ADV_TRIG_OP_ANY,
+ OLS_ADV_TRIG_OP_ANY, OLS_ADV_TRIG_OP_ANY, OLS_ADV_TRIG_OP_ANY, OLS_ADV_TRIG_OP_ANY,
+ OLS_ADV_TRIG_OP_AND, OLS_ADV_TRIG_OP_AND,
+ OLS_ADV_TRIG_OP_NOP);
+ if (ret != SR_OK)
+ return ret;
/*
* Tell the state machine to move to the next stage on a hit by not
* setting the trigger flag. The last stage executes the trigger.
*/
- RETURN_ON_ERROR(ols_set_advanced_trigger_state(
- serial, ols_stage, is_last_stage, is_last_stage,
- start_timer0 ? 1 : 0, 0, 0, 0, 0));
+ ret = ols_set_advanced_trigger_state(serial, ols_stage, is_last_stage,
+ is_last_stage,
+ start_timer0 ? 1 : 0, 0, 0, 0, 0);
+ if (ret != SR_OK)
+ return ret;
return SR_OK;
}
@@ -1122,6 +1200,8 @@ static int
ols_convert_and_set_up_advanced_trigger(const struct sr_dev_inst *sdi,
gboolean *will_trigger)
{
+ int ret;
+
struct sr_serial_dev_inst *serial = sdi->conn;
struct dev_context *devc = sdi->priv;
@@ -1146,8 +1226,9 @@ ols_convert_and_set_up_advanced_trigger(const struct sr_dev_inst *sdi,
sr_dbg("Inserting pre-trigger delay of %" PRIu64 "0 ns",
pretrigger_10ns_ticks);
- RETURN_ON_ERROR(ols_set_advanced_trigger_timer(
- serial, 0, pretrigger_10ns_ticks));
+ if ((ret = ols_set_advanced_trigger_timer(
+ serial, 0, pretrigger_10ns_ticks)) != SR_OK)
+ return ret;
int sum_inputs[8] = {
OLS_ADV_TRIG_OP_ANY, OLS_ADV_TRIG_OP_ANY, OLS_ADV_TRIG_OP_ANY, OLS_ADV_TRIG_OP_ANY,
@@ -1155,13 +1236,15 @@ ols_convert_and_set_up_advanced_trigger(const struct sr_dev_inst *sdi,
};
/* first stage: start timer, advance immediately to second stage */
- RETURN_ON_ERROR(ols_set_advanced_trigger_sums_and_stages(
- serial, ols_stage++, sum_inputs, FALSE, TRUE));
+ if ((ret = ols_set_advanced_trigger_sums_and_stages(
+ serial, ols_stage++, sum_inputs, FALSE, TRUE)) != SR_OK)
+ return ret;
/* second stage: wait until timer expires */
sum_inputs[3] = OLS_ADV_TRIG_OP_B;
- RETURN_ON_ERROR(ols_set_advanced_trigger_sums_and_stages(
- serial, ols_stage++, sum_inputs, FALSE, TRUE));
+ if ((ret = ols_set_advanced_trigger_sums_and_stages(
+ serial, ols_stage++, sum_inputs, FALSE, TRUE)) != SR_OK)
+ return ret;
}
struct sr_trigger *trigger;
@@ -1173,9 +1256,8 @@ ols_convert_and_set_up_advanced_trigger(const struct sr_dev_inst *sdi,
OLS_ADV_TRIG_OP_ANY, OLS_ADV_TRIG_OP_ANY, OLS_ADV_TRIG_OP_ANY, OLS_ADV_TRIG_OP_ANY,
OLS_ADV_TRIG_OP_ANY, OLS_ADV_TRIG_OP_ANY, OLS_ADV_TRIG_OP_ANY, OLS_ADV_TRIG_OP_ANY
};
- RETURN_ON_ERROR(ols_set_advanced_trigger_sums_and_stages(
- serial, ols_stage, sum_inputs, TRUE, FALSE));
- return SR_OK;
+ return ols_set_advanced_trigger_sums_and_stages(
+ serial, ols_stage, sum_inputs, TRUE, FALSE);
}
int num_req_trigger_stages = g_slist_length(trigger->stages);
@@ -1306,8 +1388,10 @@ ols_convert_and_set_up_advanced_trigger(const struct sr_dev_inst *sdi,
sum_inputs[4], sum_inputs[5], sum_inputs[6],
sum_inputs[7]);
- RETURN_ON_ERROR(ols_set_advanced_trigger_sums_and_stages(
- serial, ols_stage, sum_inputs, is_last_stage, 0));
+ ret = ols_set_advanced_trigger_sums_and_stages(
+ serial, ols_stage, sum_inputs, is_last_stage, 0);
+ if (ret != SR_OK)
+ return ret;
++ols_stage;
}
diff --git a/src/libsigrok-internal.h b/src/libsigrok-internal.h
index c3563215..9111bc37 100644
--- a/src/libsigrok-internal.h
+++ b/src/libsigrok-internal.h
@@ -63,15 +63,6 @@ struct zip_stat;
#define SR_RECEIVE_DATA_CALLBACK(f) \
((sr_receive_data_callback) (void (*)(void)) (f))
-
-/* Shorthand to bail out early if an error occurs. Does no clean-up. */
-#define RETURN_ON_ERROR(statement) \
- do{ \
- int retval; \
- if ((retval=(statement)) != SR_OK)\
- return retval; \
- } while(0)
-
/**
* Read a 8 bits unsigned integer out of memory.
* @param x a pointer to the input memory |
|
First of all sorry for the delay. I missed the latest update from v1ne and then this task slipped through the cracks. I hope you guys are still interested? |
|
@wsakernel: I created #155 with the commit range in question. I also tested this branch (#51) again on the hardware to ensure that capture and triggering still works as promised. |
|
@wsakernel: Could you continue by reviewing 6621a22–5696a4db? |
|
I have not forgotten this and will do as best as I can. |
| &devc->raw_sample_buf[4 * (devc->num_samples - | ||
| i - 1)], | ||
| 4); | ||
| memcpy(&devc->raw_sample_buf[4 * (devc->num_samples - | ||
| i - 1)], |
There was a problem hiding this comment.
Minor nit: Putting (i - 1) in a seperate line hurts readability IMHO. I'd rather keep it on one line.
The code looks okay. I think the commit message is a bit vague. We mainly want to introduce 'samples_to_write' here, so we can use it for the dynamic buffer later, or did I miss something?
There was a problem hiding this comment.
OK, I put it on the same line.
I introduce samples_to_write here for clarity, as it's the number of samples that the current sample is expanded to. I think it's easier to understand than "rle_count + 1". The name is debatable, though.
| /* fill with 1010... for debugging */ | ||
| memset(devc->raw_sample_buf + old_size, 0x82, | ||
| new_sample_buf_size - old_size); |
There was a problem hiding this comment.
I wonder if we still need that? Also 0x82 is not 1010...
From my point of view, the benefits of having a dynamic buffer and, thus, supporting RLE fully is bigger than the disadvantage of having problems with high-latency network connections. I didn't dive in fully, but the code looks reasonable.
Brainstorming: would it be possible to add a driver-option 'use static buffers' to support high latencies? Not requesting it for this series, maybe it can be added on top. From a glimpse, I'd think this is possible.
There was a problem hiding this comment.
Ah, indeed, the pattern is wrong. I still consider it useful, though, so I changed it to 0xAA. Especially the most recent version of this file contains changes for a different number of channel groups than 4, which I didn't consider when doing these changes in the first place.
I'm happy that you also find RLE support more worthwhile. Ideally, we would know when to finish reading, but at least 2021 me didn't know how.
About having both buffer options: At this point, I would refrain from adding more options to the code, as this means adding more ways in which things could break. I'd think that looking again into whether there is a way to figure out the buffer size in the first place could be worthwhile instead. I agree that I won't change it as part of this series.
| logic.data = devc->raw_sample_buf + | ||
| num_pre_trigger_samples * 4; | ||
| sr_session_send(sdi, &packet); | ||
| } |
There was a problem hiding this comment.
Looks good. Maybe commit message could state what non-desired behaviour happened with the old code?
There was a problem hiding this comment.
I honestly don't remember what happened when we send garbage via sr_session_send (trigger in the future). I guess it is some unrecoverable state, like a crash, otherwise I think I wouldn't have fixed it.
| char dummy[16]; | ||
|
|
||
| /* Drain all data so that the remote side is surely listening. */ | ||
| while ((ret = serial_read_nonblocking(serial, &dummy, 16)) > 0); |
There was a problem hiding this comment.
Hmm, an endless loop... First, I suggest to put the ';' to a seperate line to make it clear it is an endless loop. Other than that, won't this take a while dumping all the data? I have an OLS clone with 1MB of RAM... Do we really need it?
There was a problem hiding this comment.
I changed the formatting, on both instances to make it more obvious.
Yes, it might take a while to dump the data. But we need it because we need the device to be in the state we think it is: If it's still dumping data from before we reset it, we're not expecting this after the reset and the whole communication afterwards is messed up. Thus, I introduce this period of quiescence to re-sync the device and the driver.
| if (ret != SR_OK) | ||
| return ret; |
There was a problem hiding this comment.
I wonder if we should really bail out of the reset routine? I mean, what should the upper layers do then? It might make sense to try as hard as possible and only present the return value after we tried everything.
There was a problem hiding this comment.
Re: while (!quiet) {read serial input}
Assuming that the device is operational, I only see that this could fail due to a failure of the serial link, which I consider not recoverable. If there is a failure from the device that we could recover from via more reset cycles, I'd first like to know about it before adding support for this scenario.
Re: if ((ret = send_shortcommand(serial, CMD_RESET)) != SR_OK)
What case do you have in mind where the condition fails and trying harder would make it work again?
I agree that a false positive is annoying, but I don't see how this homing sequence could fail, except for a failure on the serial link.
| if ((ret = send_shortcommand(serial, CMD_RESET)) != SR_OK) | ||
| return ret; |
| * the device seems to return a bit more data than we request. | ||
| */ | ||
| delay_ms = serial_timeout(serial, 16); | ||
| while ((ret = serial_read_blocking(serial, &dummy, 16, delay_ms)) > 0); |
| { | ||
| struct sr_serial_dev_inst *serial; | ||
| struct sr_serial_dev_inst *serial = sdi->conn; | ||
|
|
||
| serial = sdi->conn; | ||
| ols_send_reset(serial); | ||
|
|
||
| serial_source_remove(sdi->session, serial); | ||
|
|
There was a problem hiding this comment.
I like that the new code has less lines. But is has not added reset when acquisition ends like commit message says? Maybe I overlooked something or the commit message needs updating.
There was a problem hiding this comment.
Oh, indeed! I removed that part from the commit message – there is no resetting after capture.
| int cnt_samples; | ||
| int cnt_samples_rle; | ||
| unsigned int cnt_rx_bytes; /* number of bytes received */ | ||
| unsigned int raw_sample_size; /* valid bytes in sample[4] */ |
There was a problem hiding this comment.
comment should be 'raw_sample[4]'?
I didn't verify the code and trust the author and the compiler for that :) From the changes in the header, it looks much more readable with this change. Good.
| devc->rle_count = devc->sample_buf_size = 0; | ||
| devc->rle_count = 0; |
There was a problem hiding this comment.
Probably correct, but maybe one sentence to the commit message explaining why it is now always up to date?
There was a problem hiding this comment.
Good catch! I amended the commit message (it's because the buffer and its size were reset in different functions, at different times).
| devc->limit_samples = | ||
| (MIN(devc->max_samples / num_changroups, devc->limit_samples) + 3) |
There was a problem hiding this comment.
TBH, I liked the intermediate step with 'samplecount'. It made the calculation easier to follow. The multiplication with 4 itself makes sense, of course.
There was a problem hiding this comment.
No problem, I brought back the intermediate step, and kept the arithmetic.
| * Even on the rare occasion that the sampling ends with an RLE message, | ||
| * the acquisition should end immediately, without any timeout. | ||
| */ | ||
| goto process_and_forward; |
There was a problem hiding this comment.
I think this is an OK case for 'goto'. So, patch looks good to me.
| return FALSE; | ||
| received_a_byte = FALSE; | ||
| while (revents == G_IO_IN && | ||
| serial_read_nonblocking(serial, &byte, 1) == 1) { |
There was a problem hiding this comment.
As mentioned before, I have a OLS clone with 1MB of memory. This may be a bit too much to poll in one go? Dunno what a reasonable size for chunks would be? 64KB?
There was a problem hiding this comment.
I agree to not block the event loop, though the device would have respond pretty quickly to not cause serial_read_nonblocking to return nothing (assuming the latter really doesn't block). Nonetheless, to be safe, I added a safeguard to "ols: Capture multiple bytes at once".
| (num_bytes_read = serial_read_nonblocking( | ||
| serial, devc->raw_sample + devc->raw_sample_size, | ||
| num_changroups - devc->raw_sample_size)) > 0) { | ||
| received_a_byte = TRUE; |
There was a problem hiding this comment.
Timeout handling will still work the same, right?
Maybe 'received_a_byte' should be renamed to 'received_a_sample'?
There was a problem hiding this comment.
Yes, timeout handling is unaffected. We're just reading more at the same time.
About the variable: Since a sample can span multiple bytes but the serial interface might only deliver 1 byte (theoretically), I think received_a_byte is actually still correct.
| if (devc->trigger_at_smpl != | ||
| OLS_NO_TRIGGER && | ||
| devc->trigger_rle_at_smpl_from_end == | ||
| OLS_NO_TRIGGER && | ||
| (unsigned int)devc->trigger_at_smpl == | ||
| devc->cnt_rx_raw_samples) | ||
| devc->trigger_rle_at_smpl_from_end = | ||
| devc->cnt_samples; | ||
|
|
||
| /* |
There was a problem hiding this comment.
This if-condition is really hard to read. And unless I missed something, it is needed a second time later. I'd suggest an inline function here like "is_<some_good_description>_trigger(devc)"
There was a problem hiding this comment.
Good point! I extracted the ugliness to set_rle_trigger_point_if_unset
| @@ -112,6 +112,7 @@ struct dev_context { | |||
| uint64_t limit_samples; | |||
| uint64_t capture_ratio; | |||
| int trigger_at_smpl; | |||
| int trigger_rle_at_smpl_from_end; | |||
There was a problem hiding this comment.
But in genral, this patch is cool :)
| unsigned char *sample_buf; | ||
| unsigned int sample_buf_size; | ||
| unsigned int cnt_samples; /* number of final samples in sample_buf */ | ||
| uint64_t cnt_samples; /* number of final samples in sample_buf */ |
There was a problem hiding this comment.
Do all these type changes have to do with RLE mode 3? I agree that it is probably cleaner to have them, but I don't understand why this is connected to RLE mode 3.
That being said, I will stop here for this review round. The final patch left is huge and I think it makes sense to discuss my comments now up to here and see that we can get another PR in good shape. Do you agree?
There was a problem hiding this comment.
Ah, the type changes are more to simplify the pointer arithmetic by avoiding theoretical overflows (which usually pessimize the code that the compiler generates). Sorry, that was more a habit than a change motivated by the support of RLE mode 3.
|
Hi @v1ne, As discussed on HN, I'd like to help you get this reviewed and merged. Any progress so far? It might be helpful for both of us if you split your patchset into different PRs. Just an idea. |
|
Hi @biot, thank you again for your offer! I'm on it… I got my changes rebased and I'm processing the valuable comments that I got. After that, I'll give it a go on my OLS and then ping you with the new Pull Request. That's my plan. |
|
Thank you for the very helpful comments, @wsakernel! I processed all comments from here and checked that capturing data still works (it does, also in RLE mode, yay) with my Open Bench Logic Sniffer. |
This PR adds:
What do you think? Is something missing? Should it be in a different shape?