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

Add `ReturnCode`s to UART HIL, change `abort` policy #1049

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@ppannuto
Copy link
Member

ppannuto commented Jun 29, 2018

Pull Request Overview

This updates the UART HIL to allow methods to fail. Previously, they would silently drop errors internally. The console should also bubble these errors back to userland correctly now. There are probably some more cases where underlying implementations can catch and report more errors, but this at least allows them to do that in the future.

This also updates abort_receive to no longer guarantee the return of partial buffers. As noted in #1045, partial buffers causes problems for virtualization, as one client's abort request requires all other clients to abort in order to recover the partial buffer. This changes our ABI semantics slightly. I do not believe anything is relying on this behavior currently, and I believe that not guaranteeing partial buffers on abort is generally a better long-term interface.

Testing Strategy

Hail + various other printing apps.

Documentation Updated

  • Updated the relevant files in /docs, or no updates are required.

Formatting

  • Ran make formatall.
@bradjc
Copy link
Contributor

bradjc left a comment

The HIL doesn't seem to say anything about what abort_receive will actually guarantee or not, which I think is fine. I would be concerned if it did match the PR description, because I think being able to implement receive_automatic in software is much more important than being able to virtualize the UART bus.

if buffer.len() <= i {
break;
self.tx_buffer.take().map_or_else(
|| panic!("Console internal consistency error"),

This comment has been minimized.

@bradjc

bradjc Jun 29, 2018

Contributor

Crash in a capsule seems bad.

This comment has been minimized.

@ppannuto

ppannuto Jun 29, 2018

Member

Yes.., but before this was an error that was silently dropped, now it's noted. There are 28 other panic's in capsules as well, most similar consistency issues. I think that the console capsule could be re-architected a bit to make this case impossible, but that's a different PR.

This comment has been minimized.

@bradjc

bradjc Jun 29, 2018

Contributor

Doesn't adding return codes give it a way to not be silently dropped but without crashing the entire system?

/// No error occurred and the command completed successfully
CommandComplete,
}

pub trait UART {
/// Set the client for this UART peripheral. The client will be
/// called when events finish.
fn set_client(&self, client: &'static Client);
fn set_client(&self, client: &'static Client) -> ReturnCode;

This comment has been minimized.

@bradjc

bradjc Jun 29, 2018

Contributor

Why would setting a client not succeed?

@phil-levis phil-levis referenced this pull request Jun 30, 2018

Merged

cortex-m4: move hardfault handler to arch #1051

2 of 2 tasks complete

@ppannuto ppannuto added the blocked label Jul 4, 2018

@ppannuto ppannuto referenced this pull request Jul 5, 2018

Open

Tracking & RFC: UART HIL Refinements #1072

1 of 9 tasks complete

ppannuto added some commits Jul 5, 2018

uart: remove `initialize` from the UART HIL
This implements RFC #1035 for the UART HIL. It splits the hardware-specific
initialization (power, clock assignment, pin assignment) from the uart-specific
configuration (baud rate, parity, stop bits, flow control), which is now
controlled by a new `configure` method for the UART HIL.
@ppannuto

This comment has been minimized.

Copy link
Member

ppannuto commented Jul 5, 2018

This has been rebased on top of #1073 as they conflicted.

This PR also needs to more deeply consider and enumerate the possible types of failures for transmit and receive before it is ready to merge.

kernel: Add ReturnCode's to UART HIL
Also corrects the console driver to propogate errors when they occur.

@ppannuto ppannuto force-pushed the uart-hil branch from e7b392a to 27203fb Jul 5, 2018

@bradjc bradjc referenced this pull request Jul 10, 2018

Merged

Kernel: Move `PROCS` array to `Kernel` struct #1111

2 of 4 tasks complete
/// active receive may complete before this call occurs, in which case the
/// `receive_callback` will be called with `CommandComplete` and this method
/// will return `EALREADY`.
fn abort_receive(&self) -> ReturnCode;

This comment has been minimized.

@bradjc

bradjc Jul 17, 2018

Contributor

The PR description doesn't seem to match this comment. This doesn't say whether data will be returned or not.

In either case, returning what has been received so far was the entire purpose of adding this function. Without it, there is no way to implement a UART receiver that can take in variable length messages without doing it byte-by-byte.

@phil-levis

This comment has been minimized.

Copy link
Collaborator

phil-levis commented Jul 19, 2018

I'll work on this PR over the weekend, to redesign the UART HIL. Let's get it over with.

@phil-levis-google
Copy link
Contributor

phil-levis-google left a comment

I think this will be subsumed by the UART rewrite, so should we cancel it?

@ppannuto

This comment has been minimized.

Copy link
Member

ppannuto commented Nov 11, 2018

Agreed. I think the high-level changes should be preserved, but we can cross that bridge with the new UART HIL.

@ppannuto ppannuto closed this Nov 11, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment