use crate critical-section in defmt-rtt

[jannic]

As an example how #637 could be solved, I changed defmt-rtt to use https://github.com/embassy-rs/critical-section.
Together with the implementation from https://github.com/9names/rp-hal/blob/critical_section/rp2040-hal/src/critical_section_impl.rs, I was able to run loop { info!("A!"); } and loop { info!("B!"); } on the two cores of an RP2040 concurrently, getting the desired output:

INFO  A!
└─ rp2040_project_template::__cortex_m_rt_main @ src/main.rs:66
INFO  B!
└─ rp2040_project_template::test @ src/main.rs:104
INFO  A!
└─ rp2040_project_template::__cortex_m_rt_main @ src/main.rs:66
INFO  B!
[...]

With the original defmt-rtt implementation, one of the cores panics immediately.

[Dirbaio]

Another advantage is this will allow it to work on more architectures such as riscv :)

[Urhengulas]

Thank you for the PR! Is there any drawback to using this?

[jannic]

The only drawback I'm aware of is the additional dependency.

[jannic]

@Urhengulas, what do you think about the additional dependency, is that a blocker?

[Urhengulas]

@Urhengulas, what do you think about the additional dependency, is that a blocker?

No, that is no blocker.

[jonas-schievink]

Generally in favor of this idea, but the critical-section crate could use some more documentation

[jonas-schievink]

What are the safety invariants of this function that we have to uphold?

[Urhengulas]

@jannic Could you shed light on this?

[jannic]

As @jonas-schievink already mentioned, critical-section is missing some documentation.
This is part of it: The intended safety-guarantees are not specified.

As far as I can tell, there are no real requirements. The current implementations of critical_section::acquire() are not really unsafe, but just disable interrupts.

My guess is that it's just for symmetry with critical_section::release(), which is obviously unsafe, as enabling interrupts could break other code expecting to run exclusively.

@Dirbaio, any comments?

[Urhengulas]

@jannic Could you please add safety comments for critical_section::acquire and critical_section::release, with what you wrote here? Then we should be good to go.

[Dirbaio]

I still have to write docs for critical-section (sorry! 🙈 ), but the safety contract is essentially:

• Each acquire must be paired with a release with the same token.
• acquire/release pairs must be "properly nested", ie it's not OK to do a=acquire(); b=acquire(); release(a); release(b);.

Code in this PR complies with the safety contract, so LGTM 👍

[jonas-schievink]

Thanks, that sounds good for now.

[jannic]

acquire() has to be unsafe because you can use it to break the "properly nested" requirement in someone else's acquire/release pair deeper in the call stack, if you don't do the corresponding release() call.

As fn acquire() is not unsafe, one could break this contract by just calling Logger::acquire() in some random location.
But as far as I can tell, nothing bad would happen with that alone? eg:

critical_section::with(|_| Logger::acquire());

This would lead to a sequence of calls like a=acquire(); b=acquire(); release(a).
But as long as you neither call release(b) (which would be unsafe) or rely on being inside a critical section after release(a), I don't see any bad effect of that call sequence.

[jannic]

As fn acquire() is not unsafe, one could break this contract by just calling Logger::acquire() in some random location.

One can't, as Logger::acquire() is private.
I still think that it wouldn't do harm if one could call it, but as one can't, either way it doesn't matter.

[jannic]

Oops, too fast...
There is defmt::export::acquire();, which is pub and not unsafe. So it's perfectly possible to call acquire() out of order. AFAIK the only thing bad which will happen is that the next call to a defmt logger will panic ("defmt logger taken reentrantly"), which seems to be a perfectly fine response.

However, there is something much more dangerous: defmt::export::release(). which allows to end a critical section unconditionally (or, before merging this pull request, to enable interrupts). I think this should be unsafe, right?

info!("something");  // log something while interrupts are enabled. `INTERRUPTS_ACTIVE` remembers that interrupts were enabled
cortex_m::interrupt::free(|cs| {
  // interrupts are disabled
  defmt::export::release();
  // oh no! interrupts are enabled, again
  // but we still have the cs token:
  let borrowed = cortex_m_mutex.borrow(cs);
  // do something with shared value while interrupts are enabled
}

[jannic]

→ #659

[mehmetalianil]

This will also resolve issues with defmt working with nrf-softdevice.

(nrf-softdevice is the crate to go in order to devlelop BLE applications on nrf52 because it uses the certified binaries of nRF, which eliminates a good amount of certification trouble. )

[bobmcwhirter]

Just a +1 that this would make working with nRF + softdevice much less painful.

[jonas-schievink]

bors r+

[bors]

Build succeeded:

• ci