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

Fix virtual_uart receive semantics #1129

Merged
merged 4 commits into from Jul 31, 2018

Conversation

Projects
None yet
3 participants
@alevy
Copy link
Member

alevy commented Jul 19, 2018

Pull Request Overview

Fixes the semantics of the virtual_uart on the receive path. In particular, it fixes a problem that occurs when client reads of different sizes are pending.

This changes the semantics to copy the underlying UART read into each of the client buffers. If the underlying read completes a client read, issue a callback to that client. In the meanwhile, compute the length of the next underlying UART read: if any client has more to read, issue another underlying read.

Note to @phil-levis or @bradjc, the description above ^ is unclear to me, I mostly just copied it from the comments. Please fix the description if you can.

This also makes two small changes to the SAM4L USART driver. All state updates need to be done before callbacks or other modules are called, in case those callbacks or other modules call the USART driver. If not, then the USART has the wrong state and those function calls will likely fail.

Testing Strategy

The PR includes a test for the virtual uart in capsules/src/test/virtual_uart.rs

TODO or Help Wanted

Documentation Updated

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

Formatting

  • Ran make formatall.

@alevy alevy force-pushed the alevy:bug/fix_virtual_uart branch from 1286313 to 9088bce Jul 19, 2018

@alevy alevy requested review from bradjc and phil-levis Jul 19, 2018

@alevy

This comment has been minimized.

Copy link
Member

alevy commented Jul 19, 2018

Moved here from the commits in #1046.

@alevy alevy changed the title Bug/fix virtual uart Fix virtual_uart receive semantics Jul 19, 2018

@alevy alevy force-pushed the alevy:bug/fix_virtual_uart branch from 9088bce to f85287b Jul 19, 2018

@bradjc
Copy link
Contributor

bradjc left a comment

I tested this with hail and the console_timeout test app. On master, when I type in "hi brad" the app after 5 seconds displays "hi brad". On this branch it just prints a new line after 5 seconds.

@phil-levis

This comment has been minimized.

Copy link
Collaborator

phil-levis commented Jul 28, 2018

OK, I'll take a look. My test did not test aborting, only overlapping but desynchronized reads. I'll update the test and use that to fix it.

@phil-levis

This comment has been minimized.

Copy link
Collaborator

phil-levis commented Jul 31, 2018

The bug is in console.rs, but I can't get Amit's branch to run on imix: the debug interface isn't configured so the kernel panics.

The bug is in receive_complete; an aborted UART receive returns an uart::Error::Aborted, but receive_complete only copies data if it's uart::Error::CommandComplete. So the function needs to be this:

    fn receive_complete(&self, buffer: &'static mut [u8], rx_len: usize, error: uart::Error) {
        self.rx_buffer.replace(buffer);
        self.rx_in_progress.take().map(|appid| {
            self.apps
                .enter(appid, |app, _| {
                    app.read_callback.map(|mut cb| {
                        let (result, len) = match error {
                            uart::Error::CommandComplete | uart::Error::Aborted => {
                                // Copy the data into the application buffer, if it exists
                                match app.read_buffer.take() {
                                    Some(mut app_buffer) => {
                                        // We used UART::receive(),
                                        // so we received the requested length
                                        self.rx_buffer.map(|buffer| {
                                            // Copy our driver's buffer into the app's buffer
                                            for (i, c) in app_buffer.as_mut()[0..rx_len]
                                                .iter_mut()
                                                .enumerate()
                                            {
                                                *c = buffer[i]
                                            }
                                        });
                                        match error {
                                            uart::Error::CommandComplete => (ReturnCode::SUCCESS, rx_len),
                                            uart::Error::Aborted => (ReturnCode::ECANCEL, rx_len),
                                            _ => (ReturnCode::FAIL, rx_len) // should never be triggered
                                        }
                                    }
                                    None => (ReturnCode::EINVAL, 0),
                                }
                            }
                            _ => {
                                // Some UART error occurred
                                (ReturnCode::FAIL, 0)
                            }
                        };

                        // Schedule the app's callback
                        cb.schedule(From::from(result), len, 0);
                    });

                    // If the enter() above fails because the app has disappeared,
                    // we simply drop the received data.
                })
                .unwrap_or_default();
        });
    }

phil-levis and others added some commits Jul 19, 2018

@bradjc bradjc force-pushed the alevy:bug/fix_virtual_uart branch from f85287b to 2244db3 Jul 31, 2018

@bradjc
Copy link
Contributor

bradjc left a comment

console_timeout test works!

@phil-levis

This comment has been minimized.

Copy link
Collaborator

phil-levis commented Jul 31, 2018

More generally, I think we want want callbacks to distinguish between canceled and not-canceled reads; sometimes the canceler is in a logically separate part of the code, and trying to figure out why your read didn't return everything you requested can be a pain.

@phil-levis
Copy link
Collaborator

phil-levis left a comment

LGTM.

@bradjc

bradjc approved these changes Jul 31, 2018

Copy link
Contributor

bradjc left a comment

console recv timeout still works on hail

@alevy

This comment has been minimized.

Copy link
Member

alevy commented Jul 31, 2018

Happy to merge this as is, but...

My last question: do we actually want to return EINVAL if the process didn't allow a buffer? Or could we just silently not fill any buffer and return the same values. (My bias is that no doing so would simplify the code even more)

Pros: it gives the process an error if it forgot to add a buffer

Cons: for processes that intentionally don't allow a buffer (e.g. if I just care about counting that n bytes have been received, but I don't need to actually receive them), it elides the real error code (whether the reception completed or if it was aborted). It also elides the length that has actually been received, though that's easy to fix by just including rx_len

@bradjc

This comment has been minimized.

Copy link
Contributor

bradjc commented Jul 31, 2018

Defining that calling receive without a buffer generates an error on reception seems both fine and preferable, since it is most likely an error and it doesn't really match the expected semantics of receive. So, I think we should add the rx_len in case someone does want what you described (let me know when X bytes have come in, but I don't care what they are), but I think it is fine for it to be on them to check if the number of received bytes is what they asked for (and not rely on just the error code telling them).

@alevy

This comment has been minimized.

Copy link
Member

alevy commented Jul 31, 2018

OK, we can just figure this out when we discuss stabilizing the reception end of the console driver.

@alevy

This comment has been minimized.

Copy link
Member

alevy commented Jul 31, 2018

bors r+

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

Merge #1129
1129: Fix virtual_uart receive semantics r=alevy a=alevy

### Pull Request Overview

Fixes the semantics of the `virtual_uart` on the receive path. In particular, it fixes a problem that occurs when client reads of different sizes are pending.

This changes the semantics to copy the underlying UART read into each of the client buffers. If the underlying read completes a client read, issue a callback to that client. In the meanwhile, compute the length of the next underlying UART read: if any client has more to read, issue another underlying read.

Note to @phil-levis or @bradjc, the description above ^ is unclear to me, I mostly just copied it from the comments. Please fix the description if you can.

This also makes two small changes to the SAM4L USART driver. All state updates need to be done before callbacks or other modules are called, in case those callbacks or other modules call the USART driver. If not, then the USART has the wrong state and those function calls will likely fail.

### Testing Strategy

The PR includes a test for the virtual uart in `capsules/src/test/virtual_uart.rs`

### TODO or Help Wanted

### Documentation Updated

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

### Formatting

- [X] Ran `make formatall`.


Co-authored-by: Philip Levis <pal@cs.stanford.edu>
Co-authored-by: Brad Campbell <bradjc5@gmail.com>
Co-authored-by: Amit Aryeh Levy <amit@amitlevy.com>
@bors

This comment has been minimized.

Copy link
Contributor

bors bot commented Jul 31, 2018

@bors bors bot merged commit 5100f27 into tock:master Jul 31, 2018

3 checks passed

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

@alevy alevy deleted the alevy:bug/fix_virtual_uart branch Jul 31, 2018

@phil-levis

This comment has been minimized.

Copy link
Collaborator

phil-levis commented Jul 31, 2018

Wanting to count received bytes without receiving them seems like a different API.

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