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

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

Merged
merged 3 commits into from Jul 19, 2018

Conversation

Projects
None yet
4 participants
@bradjc
Copy link
Contributor

bradjc commented Jun 28, 2018

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

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

Formatting

  • Ran make formatall.
pub unsafe fn get_debug_writer() -> &'static mut DebugWriterWrapper {
match ptr::read(&DEBUG_WRITER) {
Some(x) => x,
None => panic!(),

This comment has been minimized.

@ppannuto

ppannuto Jul 2, 2018

Member

Could you put something informative in this panic, maybe Call to debug! before the board has initialized debug system (I think that's when this would happen?)

This comment has been minimized.

@bradjc

bradjc Jul 2, 2018

Contributor

Yeah, I was hoping someone with more Rust experience would know how to rewrite this function or something.

const BUF_SIZE: usize = 1024;
/// Wrapper type that we need a mutable reference to for the core::fmt::Write
/// interface.
pub struct DebugWriterWrapper {

This comment has been minimized.

@ppannuto

ppannuto Jul 2, 2018

Member

I'm missing something here, I don't understand why these have to be two separate structs, particularly since the impl of this struct is just passing things through? There's an 'is-init' problem, but couldn't that be resolved by making the uart field in the DebugWriter struct an option?

This comment has been minimized.

@bradjc

bradjc Jul 2, 2018

Contributor

Without the Wrapper, Rust complains that there cannot be a mutable reference in the kernel crate (needed for the Write) trait, and an immutable reference given to the uart in set_client. This was my workaround

@phil-levis
Copy link
Collaborator

phil-levis left a comment

Wow this would be awesome. I've always been a bit unhappy with the console weird kernel app special case.

That being said, I think the virtualized UART is broken in a bunch of ways: #1045 goes into some examples. A virtualized UART should behave almost exactly like a non-virtualized ones. If these virtualization bugs are fixed then I'm good with this PR.

@bradjc

This comment has been minimized.

Copy link
Contributor

bradjc commented Jul 2, 2018

That being said, I think the virtualized UART is broken in a bunch of ways: #1045 goes into some examples. A virtualized UART should behave almost exactly like a non-virtualized ones. If these virtualization bugs are fixed then I'm good with this PR.

Any chance you have some cycles to add a commit?

@ppannuto ppannuto force-pushed the debug-no-process branch from 73a26dd to 8f54a24 Jul 6, 2018

@bradjc bradjc force-pushed the debug-no-process branch 2 times, most recently from 109ed3d to a41c1dc Jul 9, 2018

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

@bradjc bradjc referenced this pull request Jul 10, 2018

Merged

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

2 of 4 tasks complete
@phil-levis

This comment has been minimized.

Copy link
Collaborator

phil-levis commented Jul 11, 2018

@bradjc I'm tied up with the NSF PI meeting this week, then IETF next week. I may be able to spend some cycles on UART virtualization at the IETF.

@bradjc bradjc force-pushed the debug-no-process branch from a41c1dc to 83c0b7e Jul 11, 2018

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

@bradjc bradjc force-pushed the debug-no-process branch from 83c0b7e to 3d7e643 Jul 17, 2018

@bradjc

This comment has been minimized.

Copy link
Contributor

bradjc commented Jul 17, 2018

Ok rebased. Please test on your board. I've tested on hail and nrf52840dk and both work.

@tock tock deleted a comment from bradjc Jul 17, 2018

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

@bradjc bradjc removed the blocked label Jul 17, 2018

@ppannuto
Copy link
Member

ppannuto left a comment

I futzed around a bit but didn't really come up with anything I liked better that the DebugWriterWrapper, so I think I'm happy with this.

@phil-levis, I think your comments on #1045 are right and the virtual uart interface has issues. That said, this PR only relies on the transmit path being virtualized (which until the HIL is fixed is intertwined with the receive path), which is much less problematic, so I'm inclined to move forward with this particular PR and keep working the UART issues in other PRs.

@phil-levis phil-levis referenced this pull request Jul 18, 2018

Merged

capsules: add virtual_uart #1045

2 of 2 tasks complete

@bradjc bradjc force-pushed the debug-no-process branch from 4b4e200 to a2a0b02 Jul 18, 2018

@phil-levis

This comment has been minimized.

Copy link
Collaborator

phil-levis commented Jul 19, 2018

I have some code that correctly virtualizes UART reception -- can I push to this branch? I've tested it on imix. In so doing discovered a low-level bug in the SAM4L UART implementation.

@alevy

This comment has been minimized.

Copy link
Member

alevy commented Jul 19, 2018

@phil-levis, I don't mind omnibusing that into this PR if others are on board, but a separate one might be simpler to test/review/and merge.

bradjc added some commits Jul 9, 2018

kernel: simplify type in Allocator
Rather than use an option and wait for something to use the allocator to
panic, just panic if an allocator is created for a nonexistent process.

@bradjc bradjc force-pushed the debug-no-process branch from a2a0b02 to 72cca90 Jul 19, 2018

@bradjc

This comment has been minimized.

Copy link
Contributor

bradjc commented Jul 19, 2018

Either way is fine.

@phil-levis
Copy link
Collaborator

phil-levis left a comment

I've committed changes to UartMux that correctly virtualizes reception, and included tests for the capsule as well as an exercise of the tests for imix.

@phil-levis

This comment has been minimized.

Copy link
Collaborator

phil-levis commented Jul 19, 2018

There are longer-term issues here which mean that we need to fix the UART HIL. E.g., the fact that transmission and reception are simultaneously virtualized, such that a client has to implement both callbacks, as well as the lack of separation between the control path and data path, which allows any virtualized instance to change the configuration of all of instances.

@bradjc bradjc force-pushed the debug-no-process branch from e3c3451 to 3362a5a Jul 19, 2018

@bradjc

This comment has been minimized.

Copy link
Contributor

bradjc commented Jul 19, 2018

This changes the behavior of abort_receive (the console_timeout test doesn't work). Now, the virtual_uart in master right now doesn't work with console_timeout either, but it panics on an explicit unimplemented!(), whereas on this pr it just returns "".

@alevy

This comment has been minimized.

Copy link
Member

alevy commented Jul 19, 2018

@phil-levis or @bradjc can you explain the changes you made. What does it mean to "correctly virtualize reception" (as I've stated elsewhere, I don't think that's actually possible without adding a protocol on top of UART, so I'm really just trying to understand what semantics you chose).

Because the changes resulted in formatting differences it's really tricky to figure out what important changed by just looking at the diff. A summary would be very useful for review.

@alevy

This comment has been minimized.

Copy link
Member

alevy commented Jul 19, 2018

Regarding the virtual_uart changes, not debug: OK, I think I follow now, and i think this belongs in a separate PR.

IMO, it is fine to merge for now, with the assumption that we're going to re-implement virtual_uart differently anyway after updating the UART HIL, which will happen presently. As it is, this seems functional, but slightly unclear what the semantics are, and seems fairly brittle (e.g. a few important invariants held across functions that aren't stated).

So I'd just like a separate PR to record to the feedback is all. i think these can both be merged today. I'll create the PR.

@phil-levis

This comment has been minimized.

Copy link
Collaborator

phil-levis commented Jul 19, 2018

This changes the behavior of abort_receive (the console_timeout test doesn't work). Now, the virtual_uart in master right now doesn't work with console_timeout either, but it panics on an explicit unimplemented!(), whereas on this pr it just returns "".

Well, I'm glad it changes the behavior from panicking to something more sane. What is the correct behavior you'd expect in the console_timeout test? Can you construct me a test case? I wrote the code to handle aborts as I understood they should behave: signal receive_complete with whatever bytes had been received so far (lines 91-127).

@phil-levis

This comment has been minimized.

Copy link
Collaborator

phil-levis commented Jul 19, 2018

I think the strength of my objection to this approach of lots of small PRs is growing. Small PRs are great for bug fixes. But not for re-architecting the system. I'm seeing lots of PRs that have unfinished bits/leave stuff elsewhere. That is technical debt. If all we do to pay off technical debt is take it out elsewhere we don't make any real progress. If I felt our code review process was fundamental, caught significant bugs, and discussed the design of the system, rather than being mostly stylistic, then I might buy that small PRs are the way to go. But it's not.

@phil-levis

This comment has been minimized.

Copy link
Collaborator

phil-levis commented Jul 19, 2018

"Correctly virtualize UART" means that when a virtualized client issues a read, it begins reading the UART stream at that point in time. It continues reading the stream until its read complete or the read is aborted early. A complete read will read the requested number of bytes. An aborted read will read all of the bytes until the moment in time when the abort was issued. Concurrent reads behave correctly. I.e.,

read A of length 10 starts
5 bytes arrive
read B of length 7 starts
5 bytes arrive
read A signals a callback of 10 bytes
read C of length 10 starts
1 byte arrives
read C aborts
read C signals a callback of 1 byte
1 byte arrives
read B signals a callback of 7 bytes

All of the changes for this functionality are in virtual_uart. The other changes are small tweaks because, for example, the baud rate for a virtualized UART should be set in the UartMux, not in the clients.

@bradjc

This comment has been minimized.

Copy link
Contributor

bradjc commented Jul 19, 2018

Well, I'm glad it changes the behavior from panicking to something more sane. What is the correct behavior you'd expect in the console_timeout test? Can you construct me a test case? I wrote the code to handle aborts as I understood they should behave: signal receive_complete with whatever bytes had been received so far (lines 91-127).

I meant it changes the behavior relative to using the UART directly (which I believe your intention is to make virtualized devices behave exactly as their non virtualized counterparts, which this does not do).

To test, simply run the console_timeout test app. You should be able to type in a few characters (less than 60) and wait 5 seconds and it should print what you have typed so far. On this branch it just prints "\n".

@alevy

This comment has been minimized.

Copy link
Member

alevy commented Jul 19, 2018

I'm making an executive decision that we won't have any more discussion about the virtual_uart semantics in this PR, because it is irrelevant. I'm moving that code to a different PR.

@alevy alevy referenced this pull request Jul 19, 2018

Merged

Fix virtual_uart receive semantics #1129

1 of 2 tasks complete

@alevy alevy force-pushed the debug-no-process branch from 3362a5a to 72cca90 Jul 19, 2018

@alevy

alevy approved these changes Jul 19, 2018

@bradjc

This comment has been minimized.

Copy link
Contributor

bradjc commented Jul 19, 2018

I just tested this again on hail and it is working as expected. I added a debug! print to the button capsule and it displays when the button is pressed, and the hail app works as expected.

@alevy

This comment has been minimized.

Copy link
Member

alevy commented Jul 19, 2018

bors r+

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>
@bors

This comment has been minimized.

Copy link
Contributor

bors bot commented Jul 19, 2018

@bors bors bot merged commit 72cca90 into master Jul 19, 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 debug-no-process branch Jul 19, 2018

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