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

Feature/logging #86

Merged
merged 6 commits into from
Oct 8, 2020
Merged

Feature/logging #86

merged 6 commits into from
Oct 8, 2020

Conversation

ryan-summers
Copy link
Member

This PR adds a logging facade for booster. The logger implemented serializes the log and stores it in an internal buffer for later transmission by the serial terminal.

When the USB process eventually executes, logs will be written out over the USB serial port.

This fixes #14

Background

  • Implementers of log::Log must be static, which imposes the following design restrictions:
    • Cannot directly contain any RTIC resources
    • Should be const fn to avoid static mut and unsafe access

Heapless Vec::new() is not a const fn, so we have to implement our own minimal vector for data buffering, which is implemented here as the LogBuffer.

@jordens
Copy link
Member

jordens commented Oct 7, 2020

Hmm. Ack the static issue.
Doesn't bbqueue or heapless::spsc, mpmc help here (compared to the custom buffer and compared to the potentially long critical sections)?

src/logger.rs Outdated
pub fn process(&self, terminal: &mut SerialTerminal) {
cortex_m::interrupt::free(|_cs| {
let buffer = unsafe { &mut *self.buffer.get() };
terminal.write(buffer.data());
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't this take quite a while?
It's in a critical section.

Copy link
Member Author

Choose a reason for hiding this comment

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

This boils down into a copy into the USB serial output port buffer, so the actual blocking time should just be the amount of time to copy the data - the output isn't actually transmitted over USB here.

src/logger.rs Outdated Show resolved Hide resolved
@ryan-summers
Copy link
Member Author

ryan-summers commented Oct 8, 2020

Doesn't bbqueue or heapless::spsc, mpmc help here (compared to the custom buffer and compared to the potentially long critical sections)?

heapless structures work perfectly here, but they don't provide a const fn constructor, so we can't use them unless we store them in an internal Option and initialize them later (which is a potential path forward).

I think a bbqueue fits our needs, but it won't eliminate the need for critical sections. The problem is that the global logger is being used across multiple contexts (any task that creates a log will then refer to the global logger). Thus, we need to take care about preemption of the write into the queue from a higher priority task. Essentially, the issue is that bbqueue's Producer is not Sync (which implies it is not safe to use the producer across multiple contexts).

The heapless::mpmc does implement Send and Sync, so it is safe to share, but only supports up to 64 elements in the queue. Having a queue of heapless::String would then resolve this issue.

@ryan-summers
Copy link
Member Author

Usage of the heapless::mpmc queue allows us to avoid critical sections entirely here if we limit ourselves to N queued logs, each with a maximum of 128 bytes (which should be fine).

@ryan-summers ryan-summers requested a review from jordens October 8, 2020 07:32
@ryan-summers ryan-summers merged commit e18c1b2 into develop Oct 8, 2020
@ryan-summers ryan-summers deleted the feature/logging branch October 8, 2020 11:00
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.

Add logging support
2 participants