-
Notifications
You must be signed in to change notification settings - Fork 357
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.
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
v1ne
wants to merge
20
commits into
sigrokproject:master
Choose a base branch
from
v1ne:ols-improvements
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
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 e5dc9e6
ols: Provide sample count in RLE mode
v1ne 86bf1d5
ols: Don't store temporary data in device context
v1ne fd87a59
ols: Don't silently ignore error when setting up the device
v1ne 23f9047
ols: Fix demuxer operation
v1ne b3ef78e
ols: Fix off-by-one when setting up trigger stages
v1ne 6621a22
ols: Adjust pre-trigger delay
v1ne c67f087
ols: Flip samples after reading everything
v1ne f05c040
ols: Use a dynamic sample buffer
v1ne 84d6e00
ols: Be more robust against short reads
v1ne 72fae1a
ols: Make resetting more robust, reset at the end
v1ne 7fc0db4
ols: Clean up: Rename variables, remove misleading counter
v1ne 3a39f13
ols: Clean up: Always keep sample_buf_size up to date
v1ne 05e39c5
ols: Communicate internally that the number of samples is a multiple …
v1ne 1a2437b
ols: Immediately process data after expected sample count
v1ne e068d80
ols: Receive many bytes in a row
v1ne fadfd5e
ols: Capture multiple bytes at once
v1ne e58ecb2
ols: Determine the right trigger point when using RLE
v1ne 6af1de2
ols: Support Demon Core RLE mode 3
v1ne 5696a4d
ols: Add support for advanced triggers
v1ne File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.