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

Fix bug when deleting from flash and add unit tests #99

Merged
merged 3 commits into from
Mar 10, 2023

Conversation

ali1234
Copy link
Contributor

@ali1234 ali1234 commented Mar 5, 2023

The terminating null should be sent after the offset. Sending it before causes the offset to be 256x too big, because it is little endian.

Also adds a log message, not shown by default.

@ali1234
Copy link
Contributor Author

ali1234 commented Mar 5, 2023

I think this is right. The firmware CDC parser code is not very clear about where null terminators are supposed to be, but other commands that have parameters don't have a null between the 32BLxxxx and the first parameter, and they do have one at the end.

@Daft-Freak
Copy link
Collaborator

Not sure the null is even needed for the commands that don't send strings, but all of them are doing it 🤔

Also, looks like this has always been wrong, no idea how I didn't notice that.

@ali1234
Copy link
Contributor Author

ali1234 commented Mar 5, 2023

I thought I had used this successfully in the past, but it does look like that extra null was always sent. Maybe something changed on firmware side.

@Gadgetoid
Copy link
Contributor

Need to fix py.test (who even came up with that absurd namespacing horror) to pytest in the CI. I'd better sort that.

@ali1234
Copy link
Contributor Author

ali1234 commented Mar 6, 2023

I already sent a PR for that too.

@ali1234
Copy link
Contributor Author

ali1234 commented Mar 7, 2023

Wrote some unit tests for this, and found a bug in the process.

The terminating null should be sent after the offset. Sending it before
causes the offset to be 256x too big, because it is little endian.

Also adds a log message, not shown by default.
@ali1234
Copy link
Contributor Author

ali1234 commented Mar 8, 2023

Rebased so the tests can run.

@ali1234
Copy link
Contributor Author

ali1234 commented Mar 8, 2023

Hmm okay well that clearly needs some work.

@ali1234 ali1234 changed the title Fix bug when deleting from flash Fix bug when deleting from flash and add unit tests Mar 8, 2023
@coveralls
Copy link

Pull Request Test Coverage Report for Build 4365346862

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+5.7%) to 86.102%

Files with Coverage Reduction New Missed Lines %
ttblit/core/blitserial.py 1 92.37%
Totals Coverage Status
Change from base Build 4363815723: 5.7%
Covered Lines: 1270
Relevant Lines: 1475

💛 - Coveralls

@Gadgetoid
Copy link
Contributor

Hoorah! Tests! Thank you.

@Gadgetoid Gadgetoid merged commit ae1782b into 32blit:master Mar 10, 2023
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

4 participants