Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OLS Improvements #51

Draft
wants to merge 20 commits into
base: master
Choose a base branch
from
+981 −221
Draft

OLS Improvements #51

Changes from 1 commit
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
8ea06d6
ols: Clean up: Obey code style
v1ne Jan 23, 2021
e5dc9e6
ols: Provide sample count in RLE mode
v1ne Feb 23, 2021
86bf1d5
ols: Don't store temporary data in device context
v1ne Feb 23, 2021
fd87a59
ols: Don't silently ignore error when setting up the device
v1ne Feb 23, 2021
23f9047
ols: Fix demuxer operation
v1ne Feb 23, 2021
b3ef78e
ols: Fix off-by-one when setting up trigger stages
v1ne Feb 23, 2021
6621a22
ols: Adjust pre-trigger delay
v1ne Feb 23, 2021
c67f087
ols: Flip samples after reading everything
v1ne Feb 23, 2021
f05c040
ols: Use a dynamic sample buffer
v1ne Feb 23, 2021
84d6e00
ols: Be more robust against short reads
v1ne Feb 23, 2021
72fae1a
ols: Make resetting more robust, reset at the end
v1ne Feb 23, 2021
7fc0db4
ols: Clean up: Rename variables, remove misleading counter
v1ne Feb 23, 2021
3a39f13
ols: Clean up: Always keep sample_buf_size up to date
v1ne Feb 23, 2021
05e39c5
ols: Communicate internally that the number of samples is a multiple …
v1ne Feb 23, 2021
1a2437b
ols: Immediately process data after expected sample count
v1ne Feb 23, 2021
e068d80
ols: Receive many bytes in a row
v1ne Feb 23, 2021
fadfd5e
ols: Capture multiple bytes at once
v1ne Feb 23, 2021
e58ecb2
ols: Determine the right trigger point when using RLE
v1ne Feb 23, 2021
6af1de2
ols: Support Demon Core RLE mode 3
v1ne Feb 23, 2021
5696a4d
ols: Add support for advanced triggers
v1ne Feb 23, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 19 additions & 7 deletions src/hardware/openbench-logic-sniffer/protocol.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,14 +73,28 @@ 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)
{
unsigned int i;
int i, ret, delay_ms;
char dummy[16];

/* Drain all data so that the remote side is surely listening. */
while ((ret = serial_read_nonblocking(serial, &dummy, 16)) > 0);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

@v1ne v1ne Oct 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
Comment on lines +86 to +87
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@v1ne v1ne Oct 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


for (i = 0; i < 5; i++) {
if (send_shortcommand(serial, CMD_RESET) != SR_OK)
return SR_ERR;
if ((ret = send_shortcommand(serial, CMD_RESET)) != SR_OK)
return ret;
Comment on lines +90 to +91
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto.

}

return SR_OK;
/*
* 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.
*/
delay_ms = serial_timeout(serial, 16);
while ((ret = serial_read_blocking(serial, &dummy, 16, delay_ms)) > 0);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

";" to an extra line, please


return ret;
}

/* Configures the channel mask based on which channels are enabled. */
Expand Down Expand Up @@ -365,13 +379,11 @@ SR_PRIV int ols_set_samplerate(const struct sr_dev_inst *sdi,

SR_PRIV void abort_acquisition(const struct sr_dev_inst *sdi)
{
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);

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, indeed! I removed that part from the commit message – there is no resetting after capture.

std_session_send_df_end(sdi);
}

Expand Down