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.

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

modules/rle: consider offseting symbol count by one #1070

Open
proppy opened this issue Jul 19, 2023 · 1 comment
Open

modules/rle: consider offseting symbol count by one #1070

proppy opened this issue Jul 19, 2023 · 1 comment
Labels
app Application level functionality (examples, uses of XLS stack) enhancement New feature or request

Comments

@proppy
Copy link
Member

proppy commented Jul 19, 2023

per @grebe on #974 and #1006:

I think it is useful to treat count as being offset by 1 so count=0 means 1 symbol, count=1 means 2 symbols, etc. Disallowing count=0 is wasteful when count is narrow, e.g. in the case of count being 1 bit it will always be longer. If you encode count with an offset, it should probably be named something like offset_count or count_minus_one to avoid confusion.

@proppy proppy changed the title modules/rle: consider offseting count by one modules/rle: consider offseting symbol count by one Jul 19, 2023
@proppy proppy added enhancement New feature or request app Application level functionality (examples, uses of XLS stack) labels Jul 19, 2023
@mtdudek
Copy link
Contributor

mtdudek commented Jul 19, 2023

In PR #1052 I take advantage of counter value being 0 to indicate invalid pair.
#1052 PR also adds valid signal to the symbols stream.
Decoder is or will be updated to handle and emit empty last packets.
So I would leave count as is, and fix decoder in the near future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
app Application level functionality (examples, uses of XLS stack) enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants