Skip to content

Re-work defmt-rtt#949

Merged
Veykril merged 4 commits intomainfrom
rtt-updates
Mar 27, 2025
Merged

Re-work defmt-rtt#949
Veykril merged 4 commits intomainfrom
rtt-updates

Conversation

@jonathanpallant
Copy link
Copy Markdown
Contributor

@jonathanpallant jonathanpallant commented Mar 27, 2025

  • Remove all static mut variables - now we use UnsafeCell, and we wrap all the state into a single static variable.
  • Moved all the static variables to the top and put the implementation below.

Tested with the rust-exercises code for the nRF52-DK.

) Now we use UnsafeCell, and we wrap all the state into a single static variable.
) Moved all the static variables to the top and put the implementation below.

Tested with the rust-exercises code for the nRF52-DK.
@cloudflare-workers-and-pages
Copy link
Copy Markdown

cloudflare-workers-and-pages Bot commented Mar 27, 2025

Deploying knurling-defmt-book with  Cloudflare Pages  Cloudflare Pages

Latest commit: 63bdd03
Status: ✅  Deploy successful!
Preview URL: https://5e3e0fe2.knurling-defmt-book.pages.dev
Branch Preview URL: https://rtt-updates.knurling-defmt-book.pages.dev

View logs

@jonathanpallant
Copy link
Copy Markdown
Contributor Author

The semver-check fail is only for defmt-parser, which should probably just be published.

TAKEN.store(false, Ordering::Relaxed);
/// Flush the encoder
unsafe fn flush(&self) {
if !self.taken.load(Ordering::Relaxed) {
Copy link
Copy Markdown
Member

@Veykril Veykril Mar 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think relaxed is correct here? Nothing here enforces an ordering relationship for this load if I see that right. Given that user code can end up calling into here, correct? (similar for the other relaxed ops)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah wait nevermind, that's the unsafe part, we expect the lock to be acquired here I suppose

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A correct user of this API will call {acquire, write*, flush?, release}, so this check will never fail in normal operation. The acquire and release calls use critical-section, which contains the appropriate fences.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@Veykril Veykril added this pull request to the merge queue Mar 27, 2025
Merged via the queue into main with commit 3e0568d Mar 27, 2025
23 of 24 checks passed
@Veykril Veykril deleted the rtt-updates branch March 27, 2025 15:27
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.

2 participants