probe-rs / rtt-target

Target side implementation of the RTT (Real-Time Transfer) I/O protocol
MIT License
115 stars 29 forks source link

Review usage of critical-sections by default #27

Closed ryan-summers closed 2 years ago

ryan-summers commented 2 years ago

Currently, RTT channels will leverage a critical section when writing data to them. While this makes RTT usable out of the box in multiple ISR priorities, this can be a stumbling block when developing real-time applications.

The usage of the critical section is not easily seen from outside of the library rtt-target library, and many users do not know it exists.

There are mechanisms to work around the usage of critical sections when writing to RTT channels, such as that employed by Stabilizer, where the higher priority log is simply dropped instead of being logged if the critical section check fails: https://github.com/quartiq/stabilizer/blob/e75eff0bbd0762b5b6a22b2292d084c26d91f4d5/src/hardware/setup.rs#L203-L231

Does it make sense to keep using critical sections when the user may not know they exist? Perhaps we should instead panic whenever attempting to pre-empt and allow the user to explicitly opt-in to critical section usage?

mvirkkunen commented 2 years ago

The critical section is only used for the "just works" rprintln! macro, the channels themselves don't take a critical section. If you use the channels directly there are no locks, but it's your job to synchronize if they're used from multiple contexts (the channels take a &mut self for writing).

You can even create multiple channels and use them at different interrupt levels and never block, at the cost of more buffer RAM and slower polling.

ryan-summers commented 2 years ago

Hmm, you make a valid argument. This might be a general issue with the logger sitting on top of rtt-target, which makes use of rprintln!(): https://docs.rs/crate/rtt-logger/0.2.0/source/src/lib.rs

Generally, I'm just trying to figure out how to make RTT-based logging work out-of-the-box with minimizing assumptions

ryan-summers commented 2 years ago

Closing this. Definitely doesn't appear to be the fault of rtt-target, thanks for the feedback here.

Unfortunately, rtt-target doesn't even have a repository for the code, so there's no further place to open an issue.