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

Another fix to the Buffer SaveStates #2369

Merged
merged 2 commits into from
May 15, 2024

Conversation

jlitewski
Copy link
Contributor

I updated the tests for the Buffer SaveStates and noticed another issue with it, and this PR fixes that issue.

So with my galaxy sized brain (implied sarcasm, btw), in the save/restore code for 8-bit buffers, I forgot that not all numbers are divisible by 4, so Iceman fixed that flaw. In doing so, the outputted size of the buffer was bigger than the original size, causing issues when reconstructing data. I modified the buffer_savestate_t structs to have a const uint8_t padding which keeps track of any padding that is needed for restoring the buffer. Now, no matter what buffer goes in, it comes out correctly and is the right sized.

Copy link

You are welcome to add an entry to the CHANGELOG.md as well

@iceman1001
Copy link
Collaborator

I don't think we wanna have four if statements for each iteration.

Instead of all this extra, why not just adjust the original length with the extra bytes?

@jlitewski
Copy link
Contributor Author

I don't think we wanna have four if statements for each iteration.

Instead of all this extra, why not just adjust the original length with the extra bytes?

Because it's unknown what size the buffer is with the restore commands. The original design was that buffer_savestate_t would work with any buffers used with it, not just the Graph/Operation/Demod buffers. Adding the bytes to the length would mean whatever buffer that was originally passed into it would have to be either larger than the original length passed in or be reallocated for the extra bytes after creating a save state. The Graph/Operation/Demod buffers wouldn't have issues because they are allocated larger than what they use, but any other 8-bit buffer would have issues if they only allocated enough space to fit the data they are holding.

Even though this fix isn't the cleanest, it guarantees that it'll be the same length going in as it would be coming out. The testing command explicitly doesn't use the same buffer to test and compare the data that went in and came out to make sure it's the same, but both buffers are the same length because in actual use, you would pass the same buffer into both commands to save/restore that state. If it's larger on one side than the other, an overflow would happen unless you made sure that the buffer going in was always allocated to a multiple of 4.

And if there's a better way to loop through an array while bit shifting stuff around that doesn't require four if statements to break out of that loop early, I'll be happy to implement it since I also don't like this solution but couldn't come up with another solution that would also not break that guarantee.

@iceman1001 iceman1001 merged commit 3d4b5fc into RfidResearchGroup:master May 15, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants