Skip to content

Conversation

@Oveln
Copy link
Contributor

@Oveln Oveln commented Oct 28, 2025

In the original implementation, if read < write, the available space between 0 and read would be ignored.

@jonathanpallant
Copy link
Contributor

I'm staring at this and struggling to convince myself that either what we had is correct, or what you propose is correct.

Perhaps we could use some unit tests for that function, covering the various conditions?

@Oveln Oveln force-pushed the main branch 2 times, most recently from c0cdb78 to 14dc29a Compare November 3, 2025 17:34
@Oveln
Copy link
Contributor Author

Oveln commented Nov 3, 2025

Alright, I'll write a few tests for this function. Additionally, in fact, in the official RTT implementation (https://github.com/SEGGERMicro/RTT/blob/main/RTT/SEGGER_RTT.c , line 520), the available space in the _GetAvailWriteSpace() function is calculated exactly this way.

@Oveln
Copy link
Contributor Author

Oveln commented Nov 3, 2025

Oh... Now this seems like there's something wrong with the testing tool?

@Hoverbear
Copy link
Member

@Oveln Seems the most recent Rust improved their error messages and broke a snapshot. It should be fixed with #1009. :) It should pass if you rebase.

@jonathanpallant
Copy link
Contributor

The changes here to the algorithm look OK, as do the tests.

Please don't do the version bump though. We'll collect a bunch of changes on the main branch and then do a release PR that combines them together in one bump.

@Oveln
Copy link
Contributor Author

Oveln commented Nov 5, 2025

Okay, but how should I modify the changelog?

@Oveln
Copy link
Contributor Author

Oveln commented Nov 5, 2025

Fine, I consider I fix all problem.

@jonathanpallant
Copy link
Contributor

Brilliant, thank you. I hope we can get this released soon.

@jonathanpallant jonathanpallant added this pull request to the merge queue Nov 5, 2025
Merged via the queue into knurling-rs:main with commit 440a400 Nov 5, 2025
26 checks 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.

3 participants