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

capsules: add virtual_uart #1045

Merged
merged 2 commits into from Jul 17, 2018

Conversation

Projects
None yet
4 participants
@bradjc
Copy link
Contributor

bradjc commented Jun 28, 2018

Pull Request Overview

This pull request adds a virtual_uart.rs capsule in the tradition of the other virtual_x.rs capsules.

TX is virtualized. RX will pass all received messages to all clients who want to receive (whether or not to receive is set when calling new() on the UartDevice. receive_abort() is not virtualized.

Testing Strategy

This pull request was tested by running console and console_timeout on hail with hail also running. I also tested this with the new debug and I could get UART from both userland and debug!().

TODO or Help Wanted

If someone knows how I can get rid of the setup() command that would be nice.

Documentation Updated

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

Formatting

  • Ran make formatall.
@ppannuto
Copy link
Member

ppannuto left a comment

Regarding setup, didn't come up with a great answer. You could do something like add an attach method to the mux, such that you init the uart device and init the mux (without any relation to each other) and then attach each device to the mux. Could be reasonable


/// If we are not currently listening, start listening.
fn start_receive(&self, rx_len: usize) {
if self.receive_in_progress.get() == false {

This comment has been minimized.

@ppannuto

ppannuto Jun 28, 2018

Member

Not that it hurts anything to have, but is receive_in_progress actually doing anything? I think if you just eliminated the flag completely nothing would functionally change (i.e. here the takecell would be empty if one was already in flight and skip over the map, above once replaced it'd be ready to go again)

self.mux.start_receive(rx_len);
}

// This is not virtualized. Calling this will cause all receives to stop.

This comment has been minimized.

@ppannuto

ppannuto Jun 28, 2018

Member

I think this could virtualize fairly easily by changing this to set the receiver flags for this instance to false, then iterating all the mux'd devices and actually calling abort only if all devices are set not to receive

This comment has been minimized.

@bradjc

bradjc Jun 28, 2018

Contributor

That would be the opposite nonvirtualized behavior, correct? Right now it is first to call abort aborts for all, and that would be last to call abort blocks early receive for all.

This comment has been minimized.

@ppannuto

ppannuto Jun 28, 2018

Member

Hmm, I hadn't realized that aborting returned partial buffers. That's a bit unfortunate. As I don't think much (anything) is built on fixed buffer receive + abort yet, maybe we should have a discussion around this HIL and ABI.

This comment has been minimized.

@ppannuto

ppannuto Jul 17, 2018

Member

I think I'd prefer this being a no-op (or even an unimplemented!) in the short term, to discourage anything from trying to use it, as the current behavior can never virtualize and it will likely go away in eventual UART HIL refinements.

@bradjc bradjc referenced this pull request Jun 28, 2018

Merged

Refactor debug.rs to not be implemented as a kernel process #1046

2 of 2 tasks complete

@ppannuto ppannuto referenced this pull request Jun 29, 2018

Closed

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

2 of 2 tasks complete

@bradjc bradjc added the blocked label Jul 5, 2018

@bradjc

This comment has been minimized.

Copy link
Contributor

bradjc commented Jul 5, 2018

This is blocked on the UART HIL changes, I suppose.

@bradjc bradjc referenced this pull request Jul 10, 2018

Merged

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

2 of 4 tasks complete

@bradjc bradjc force-pushed the virtual-uart branch from a88b304 to 832ccc1 Jul 11, 2018

bradjc added a commit that referenced this pull request Jul 11, 2018

@bradjc bradjc removed the blocked label Jul 16, 2018

@bradjc bradjc force-pushed the virtual-uart branch from 832ccc1 to e3a22eb Jul 16, 2018

@bradjc

This comment has been minimized.

Copy link
Contributor

bradjc commented Jul 16, 2018

I think we need to unblock this and not wait for either the uart hil changes or the perfect virtualization scheme. We can make an issue if need be, but this PR is blocking too many other PRs, and is a reasonable attempt at a shared uart that has a use case and doesn't introduce new API to tock.

@bradjc bradjc force-pushed the virtual-uart branch from e3a22eb to 3db64d8 Jul 17, 2018

/// Must be called right after `static_init!()`.
pub fn setup(&'a self) {
self.mux.devices.push_head(self);
}

This comment has been minimized.

@ppannuto

ppannuto Jul 17, 2018

Member

I don't think this needs addressing addressing immediately, but is this something that could eventually be handled by the newly-merged Component interface? e.g. this capsule somehow provides a Component that boards instantiate and finalize (which I suppose is still a function that must be called, but is at least one standard)?

@ppannuto
Copy link
Member

ppannuto left a comment

I agree, this is a good improvement independent of the eventual UART HIL refinements, which will just have to update this now whenever they come through

@ppannuto

This comment has been minimized.

Copy link
Member

ppannuto commented Jul 17, 2018

bors r+

bors bot added a commit that referenced this pull request Jul 17, 2018

Merge #1045
1045: capsules: add virtual_uart r=ppannuto a=bradjc

### Pull Request Overview

This pull request adds a `virtual_uart.rs` capsule in the tradition of the other virtual_x.rs capsules.

TX is virtualized. RX will pass all received messages to all clients who want to receive (whether or not to receive is set when calling `new()` on the `UartDevice`. `receive_abort()` is not virtualized.


### Testing Strategy

This pull request was tested by running console and console_timeout on hail with hail also running. I also tested this with the new debug and I could get UART from both userland and `debug!()`.


### TODO or Help Wanted

If someone knows how I can get rid of the `setup()` command that would be nice.


### Documentation Updated

- [x] Updated the relevant files in `/docs`, or no updates are required.

### Formatting

- [x] Ran `make formatall`.


Co-authored-by: Brad Campbell <bradjc5@gmail.com>
@bors

This comment has been minimized.

Copy link
Contributor

bors bot commented Jul 17, 2018

@bors bors bot merged commit fd619eb into master Jul 17, 2018

4 checks passed

bors Build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details

@bors bors bot deleted the virtual-uart branch Jul 17, 2018

@phil-levis

This comment has been minimized.

Copy link
Collaborator

phil-levis commented Jul 17, 2018

I don't understand how we can merge broken code that is semantically and functionally incorrect.

@ppannuto

This comment has been minimized.

Copy link
Member

ppannuto commented Jul 17, 2018

We talked about this for a bit on the call (I'm sorry I didn't capture more of that in my approval, it should have been there).

Ultimately, the consensus was that it's really the UART HIL that needs refinement, and this PR---plus the many others blocking on it---is evidence that you can build a useful, if imperfect, virtual UART on top of the existing interfaces. An imperfect UART HIL is not a reason to block a working virtual UART implementation. (One caveat there is to try to minimize future changes, which is why abort was disallowed completely in virtual UART as-is, since there is consensus that that interface will change somehow)

From that, there are really two independent paths forward. One path is to continue with the UART interface refinements, that will necessarily include improving the virtual UART. The other path is now enabled to move forward with the 'sufficient but imperfect' virtual UART here.

I think "broken code that is semantically and functionally incorrect" is a bit too strong. As it stands, this code allows multiple clients to send messages and can pass messages back to multiple clients.

@phil-levis

This comment has been minimized.

Copy link
Collaborator

phil-levis commented Jul 17, 2018

I agree with the HIL point. I mean the virtualization is broken. This abstraction doesn't correctly virtualize a UART. Please see my long comment above.

@phil-levis

This comment has been minimized.

Copy link
Collaborator

phil-levis commented Jul 17, 2018

Try this as a use case: write capsules that read from the UART. One of them reads 20 bytes at a time and the other reads 40 bytes at a time. Both immediately read again after receiving their callback. Slowly send bytes to the device. What will the two caspules read? If this were correctly virtualized, then both of them should receive every byte correctly, the 20-byte read calls back with 20 bytes and the 40-byte read calls back with 40 bytes. But this isn't what will happen. Either the 20-byte read will miss half of the bytes or the 40-byte read will receive a callback with 20 bytes. And it's non-deterministic which one it will be.

@alevy

This comment has been minimized.

Copy link
Member

alevy commented Jul 17, 2018

@phil-levis, this PR had been open for 19 days and you didn't have a single comment on it before merging. In fact, no one except @ppannuto did.

It's fine to focus on specific ways in which the code could now be improved, or even to suggest it should be removed (although such a comment would really really need to include a reasoned argument, and must be proceeded by a "sorry I didn't catch this earlier")..

Here's one potential course forward:

The virtual_uart as it stands has an issue on the receive end: if a client has an RX buffer smaller than the length of another client's request receive transfer, it will only ever receive part of the bytes from that transfer (though how this happens is completely deterministic).

One productive observation is that this problem is avoidable with the current virtual_uart by just making sure all clients use the same sized RX buffers in the board configuration. It doesn't mean the problem couldn't manifest, but that virtual_uart is usable in such a way that it doesn't.

Another productive observation is that UART RX isn't really virtualizable "correctly"---the UART protocol doesn't lend itself to being shared, and there would probably have to have some sort of protocol on top of UART to identify the beginning and end of transactions. So any virtualization is going to be imperfect in some way, and we can get around it by noting the exact semantics in comments and recommending how it should/shouldn't be used.

A final productive observation is that we don't actually care about virtualizing the receive end, at least right now (and I can't think of compelling cases to virtualize it at this level, without really needing to add some protocol on top of UART). So, good news! It doesn't functionally break anything. More good news! We can fix this potential behavior in a number of ways.

  1. One way forward is to change to the virtual_uart to have multiple senders, but only one receiver. I believe this could be done without changing the HIL yet, by just no-opng the RX methods for non-receivers. Changing the HIL to explicitly separate RX from TX paths would help this even more. But I think that improving the virtual_uart's receive path need not block on changing the HIL.

  2. The virtual_uart could keep track of the minimum size buffer any client has, and cap rx_len at that minimum size. This would basically enforce that all clients have the same size (usable) buffers.

  3. We could do what the console driver does for processes, and only deliver receives to the client who requested a particular receive. It is, of course, not clear how usable this is. In practice it would mean a single client would have expected behavior, and multiple clients would have some behavior.

  4. Stick a protocol on top of UART that identifies transactions and includes virtual endpoints. If it were an ascii protocol, it could potentially degrade gracefully with a generic serial client, and tockloader could understand it explicitly and let the end-user interact with particular endpoints.

let len = cmp::min(rx_len, device.rx_len.get());
for i in 0..len {
rxbuf[i] = rx_buffer[i];
}

This comment has been minimized.

@phil-levis

phil-levis Jul 17, 2018

Collaborator

I think these virtualization semantics aren't quite correct. A couple of things:

If a short read request comes in while the MUX is performing a long read, then the short read will block on the long read. E.g., if a read(100) is ongoing and a client makes a read(40) call, then it will block until the read(100) completes.

If a short read request comes in while the MUX is performing a long read, then the short read will lose the data past the end of its own read. E.g., if a read(100) is ongoing and a client makes a read(40) call, then it will lose bytes 40-99. Even if the read calls are in a tight loop, it will lose 60 bytes each time.

If a long read request comes in while a short read is ongoing, then the long read will only receive a partial read. E.g., if a read(40) is ongoing and there is a read(100) request, then the read(100) will only read 40 bytes.

Virtualized reads can read data from before they were issued. E.g., if there is a read(50) request ongoing and a read(30) occurs after 15 bytes have already been received, then the read 30 won't return bytes 15-44 but rather bytes 0-29.

I think solving this is similar to how abort should be solved: the underlying virtualizer can break a single client read into multiple underlying reads (much as SPI does it). So, if one client aborts, then the UART aborts the current read, signals completion to that client, and restarts receive for the others. If a new read request comes in, then it aborts the current read and restarts all of the outstanding requests. They'll continue reading data into the middle of their buffers, while the new request will start receiving into the beginning of its buffer.

@phil-levis

This comment has been minimized.

Copy link
Collaborator

phil-levis commented Jul 17, 2018

Oh, I'm sorry, this is totally a SNAFU on my part. I wrote my comment 12 days ago and thought I'd hit submit, but hadn't. I interpreted the github UI to think it was entered, but now I see it had a little yellow box at the top saying "pending". I'd been wondering why nobody had responded... hence my "please read my long comment" statement...

@ppannuto ppannuto referenced this pull request Jul 18, 2018

Open

Tracking & RFC: UART HIL Refinements #1072

1 of 9 tasks complete
@phil-levis

This comment has been minimized.

Copy link
Collaborator

phil-levis commented Jul 18, 2018

Where can I find a code example of the virtualized UART being used correctly? I've been trying to get it to properly virtualize reception for the past 8 hours or so but am stuck on getting a console built on top of a virtualized UartDevice to work. Does the code in #1046 work?

@bradjc

This comment has been minimized.

Copy link
Contributor

bradjc commented Jul 18, 2018

Does the code in #1046 work?

Yes, I've tested that it works on hail and the nrf52840dk.

bors bot added a commit that referenced this pull request Jul 19, 2018

Merge #1046
1046: Refactor debug.rs to not be implemented as a kernel process r=alevy a=bradjc

### Pull Request Overview

This pull request changes the debug module to use a normal `uart::UART` interface rather than acting as a process. This simplifies some kernel data structures as they no longer have to handle the case where an app can be a normal app or a kernel app. It also allows boards to easily use a different uart interface (like segger RTT) without having to implement the `Driver` interface as well.

~~This is blocked on #1045 but you can help test anyway!~~


### Testing Strategy

This pull request was tested by running hail app and doing debug!()s from a capsule.


### TODO or Help Wanted

n/a


### Documentation Updated

- [x] Updated the relevant files in `/docs`, or no updates are required.

### Formatting

- [x] Ran `make formatall`.


Co-authored-by: Brad Campbell <bradjc5@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment