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

Components #1012

Merged
merged 27 commits into from Jul 16, 2018

Conversation

Projects
None yet
7 participants
@phil-levis
Copy link
Collaborator

phil-levis commented Jun 20, 2018

Pull Request Overview

This pull request does most of transitioning the imix boot sequence over to the Component interface. It includes, the FXOS8700, SI7021, SPI, RF233, RF51822, non-volatile storage, ISL29035, alarm driver, and console. It does not include the 6lowpan stack, USB, ADC, GPIO, buttons, or AES.

Testing Strategy

Compiling the full kernel and running the accel-leds test appplication.

TODO or Help Wanted

Other system calls should be tested -- e.g., the networking stack -- to make sure that configuration is still correct.

One unresolved question is whether Components allocate their own buffers or use the ones defined in the capsules. Currently the SPI, SI7201, and ISL29035 Components allocate their own, while console, FXOS8700 and NRF51822 use the capsule ones.

Documentation Updated

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

Updated doc/Startup.md to explain the difference between capsules are peripherals, and what Components are.

Formatting

  • Ran make formatall.
@phil-levis

This comment has been minimized.

Copy link
Collaborator

phil-levis commented Jun 21, 2018

I implemented the second batch of components. I think that the RF233/Radio setup could be cleaned up, but mostly wanted to just translate the current boot sequence (rather than rewrite it). Looking at the old and new main.rs side-by-side helps:

https://github.com/tock/tock/pull/1012/files?utf8=%E2%9C%93&diff=split#diff-6b9e7a51595e226048498b2e7615d1eb

@bradjc

This comment has been minimized.

Copy link
Contributor

bradjc commented Jun 21, 2018

Question about the vision: is the idea to make improvements for the imix board, or for this to be a template for other boards as well? Have you looked at what the components look like if they are board agnostic and could be used with different pins and chip drivers, say?

@bradjc bradjc referenced this pull request Jun 21, 2018

Merged

capsules: add mx25r6435f flash chip #978

3 of 3 tasks complete
@phil-levis

This comment has been minimized.

Copy link
Collaborator

phil-levis commented Jun 21, 2018

The idea is for this to be a template for other boards as well. It's about simplifying main.rs so that one can much more easily configure a kernel to include/not include features.

There is a tension between simpler and more general Components. E.g., if it's general you'll need to pass in board-specific parameters (such as which USART, etc.). My thought was to start with just a board-specific set of Components; if we later want to generalize, we can.

@bradjc

This comment has been minimized.

Copy link
Contributor

bradjc commented Jun 26, 2018

Given this changes the kernel crate API I think that makes it P-Significant. If this was entirely contained in the boards/imix folder, then I think it is good to merge. But since it adds a kernel API, which implies that it should be used more broadly than on one platform, I think there is more to think about here.

I think a shared library of components that board creators can pull from would be very useful. It would also make it easier when capsules are updated because the new() function call would only have to get updated in one place.

But right now I think this leaves us in that gap where there is a new way to do things, but everyone is just going to copy and paste the old way until everything is updated (which isn't clear will happen).

@dverhaert

This comment has been minimized.

Copy link
Contributor

dverhaert commented Jun 27, 2018

I think this is generally a good pull request, making the functionality of main.rs more clear. However, there are two things that bother me.

First, it adds yet another file which needs to be created when adding a new component. At this point, I think it's worth it to write a small document/tutorial on how to write a component, describing which files exactly would have to be changed and how everything is linked together through the kernel: it has gotten pretty complicated to add a component.

Secondly, as @bradjc mentions, adding the kernel API implies this should in fact be done on all platforms, or at least on our main platforms. Is this something we want and are able to do in the short term without leaving a gap?

mod callback;
mod driver;
mod grant;
pub mod hil;
pub mod ipc;

This comment has been minimized.

@dverhaert

dverhaert Jun 27, 2018

Contributor

Why move these two lines? Weren't they set this way to have public and private modules separated?

This comment has been minimized.

@phil-levis

phil-levis Jun 27, 2018

Collaborator

Ah good point -- I think I just put them in alphabetical (with debug and common being above due to macro use).

@ppannuto ppannuto referenced this pull request Jun 30, 2018

Merged

cortex-m4: move hardfault handler to arch #1051

2 of 2 tasks complete
mac_device.set_device_procedure(radio_driver);
radio_mac.set_transmit_client(radio_driver);
radio_mac.set_receive_client(radio_driver);
radio_mac.set_pan(0xABCD);

This comment has been minimized.

@hudson-ayers

hudson-ayers Jun 30, 2018

Collaborator

With this design, it is now pretty non-obvious where values such as the radio PAN and address should be set. When these values were set in main.rs it was likely that anyone using the stack would stumble across them pretty quickly, but if we are going to bury these constants in imix/src/components, it seems like we should at least add a comment directing developers here, or, alternatively, make the PAN and address parameters to finalize()

This comment has been minimized.

@phil-levis

phil-levis Jul 2, 2018

Collaborator

I agree. I think the better approach is to make them as arguments to finalize.

This comment has been minimized.

@phil-levis

phil-levis Jul 2, 2018

Collaborator

They can't be arguments to finalize(), but can be arguments to new(). I updated components/radio.rs to do this. Now the boot sequence specifies the PanID and the short address.

One note -- the PanID is a type, but the short address isn't (it's just a u16). Might be useful to be consistent on this.


rf233_spi.set_client(rf233);
// Can this initialize be pushed earlier, or into component? -pal

This comment has been minimized.

@hudson-ayers

hudson-ayers Jun 30, 2018

Collaborator

On this note, could the initialization/finalization of RF233 and rf233 spi be pushed into the initialization/finalization for the radioComponent? Given that there wouldn't be a scenario where the Imix radioComponent is used without these, it seems like just having what is currently line 304 handle all of that might be the desired interface (though of course the RadioComponet::new() would have to take in the parameters required for RF233Component::new() and SpiComponent::new()

Perhaps this doesn't make sense, if so, let me know.

This comment has been minimized.

@phil-levis

phil-levis Jul 2, 2018

Collaborator

I think that's another way to do it. I kept them separate for the case when someone wants to use the radio but not 6lowpan. I don't have a great answer for the case when multiple Components all initialize the same subsystem (e.g., suppose 6lowpan initializes Radio, but you include both the Radio and the 6lowpan Components: I'm sure hard-to-debug errors will occur). So I chose to keep the dependencies explicit.

@hudson-ayers
Copy link
Collaborator

hudson-ayers left a comment

I looked through this whole PR and I think it generally makes sense and moves things in the right direction. The resulting main.rs is much cleaner, and the changes seem simple enough that it is unlikely this design will lead to any problems. I will try to work with Conor to move the 6LoWPAN stack over to this interface early next week, and test that everything still works fine.

/// This abstraction is intended to make the kernel boot sequence simpler:
/// without it, the reset_handler involves lots of driver-specific
/// initialization. The Component trait encapsulates all of the
/// initialziation and configuration of a kernel extension inside

This comment has been minimized.

@brghena

brghena Jul 2, 2018

Contributor

initialziation -> initialization


pub trait Component {
type Output;
unsafe fn finalize(&mut self) -> Self::Output;

This comment has been minimized.

@brghena

brghena Jul 2, 2018

Contributor

Can finalize fail? For example, if a dependency was not provided?

This comment has been minimized.

@phil-levis

phil-levis Jul 2, 2018

Collaborator

I think finalize() can fail, but in that case it should panic. I.e., there is something wrong with your boot sequence/initialization.


pub trait ComponentWithDependency<D>: Component {
fn dependency(&mut self, _dep: D) -> &mut Self {
self

This comment has been minimized.

@brghena

brghena Jul 2, 2018

Contributor

Why include a default implementation here? It seems like just dropping the dependency for something implementing ComponentWithDependency will always be the wrong choice.

/// via a call to dependency(). After both dependencies are resolved,
/// the boot sequence can call finalize() on both of them.

pub trait ComponentWithDependency<D>: Component {

This comment has been minimized.

@brghena

brghena Jul 2, 2018

Contributor

Do you have an example of this in use? It doesn't seem like it's needed at all for Imix right now.

This comment has been minimized.

@phil-levis

This comment has been minimized.

@phil-levis

phil-levis Jul 9, 2018

Collaborator

Looking back at Shane's code, I think that ComponentWithDependency isn't actually needed. He uses it in cases where the argument could have been passed in new(). ComponentWithDependency can't be used in circular dependencies (e.g., resolving before finale()) because the actual generated types don't exist yet. I think we should cut it.

phil-levis added some commits Jul 2, 2018

Update 802.15.4 component so that PAN ID and address can be
specified in new(), so easily configurable in the boot sequence.
/// is called.

pub trait Component {
type Output;

This comment has been minimized.

@alevy

alevy Jul 4, 2018

Member

Document what this is expected to be and how it will/could be used. I believe generally it should be a &'static, right?

This comment has been minimized.

@phil-levis

phil-levis Jul 9, 2018

Collaborator

Comment added.


pub trait Component {
type Output;
unsafe fn finalize(&mut self) -> Self::Output;

This comment has been minimized.

@alevy

alevy Jul 4, 2018

Member

Document this method, in particular what kinds of behaviors should an implementer put in finalize, and what kinds of operations should a client expect to happen (and what won't happen before calling finalize).

@alevy

This comment has been minimized.

Copy link
Member

alevy commented Jul 4, 2018

I generally like this as a step. I don't know that I'm convinced it's the final design, but I also don't know what bar we should set for this. Since it's completely opt-in at the board level, I think it's reasonable to basically get the documentation about right, merge it, then iterate on it as we start using it more.

@bradjc
Copy link
Contributor

bradjc left a comment

I think as a kernel interface it needs at least some mention in the docs, like with the capabilities feature: https://github.com/tock/tock/pull/975/files#diff-d19431a79d5fa77aaae12a4dec6482eb

@alevy
Copy link
Member

alevy left a comment

Agree with @bradjc about docs, but otherwise approve.

@phil-levis

This comment has been minimized.

Copy link
Collaborator

phil-levis commented Jul 8, 2018

I'll expand the inline documentation and add Components to the discussion of the boot sequence in docs/.

@bradjc bradjc dismissed their stale review Jul 10, 2018

I still think this should demonstrate components that can be shared among boards, but I will get out of the way of progress.

@phil-levis

This comment has been minimized.

Copy link
Collaborator

phil-levis commented Jul 10, 2018

@bradjc @alevy -- I added documentation to doc/Startup.md, documented the interface more.

@alevy

alevy approved these changes Jul 12, 2018

@ppannuto

This comment has been minimized.

Copy link
Member

ppannuto commented Jul 14, 2018

I'll look over this in a moment, but I think it's been out here long enough (and then some) that it's time to say last-call. Current disposition is to merge tomorrow.

@ppannuto ppannuto added the last-call label Jul 14, 2018

@ppannuto
Copy link
Member

ppannuto left a comment

I'll echo some of the other reservations:

  • It's not quite clear to me how components will generalize. It'd be nice to see an example of something simple like an LED or Button component shared across similar boards (e.g. Imix and Hail should be pretty close as they're both Sam4Ls).
  • Right now this feels like a just a copy paste of the old contents of main to 20 different files. It does make the main reset_handler less of a monolith, but I'm not totally convinced that the added indirection is currently providing value.

That said, as this is currently contained to the imix platform, I'm in favor of merging this and seeing where it progresses.

One other slightly confusing thing is that it looks like every component implements new, yet new isn't a method of the Component trait. Something perhaps to consider adding to the Component interface moving forward.

@phil-levis

This comment has been minimized.

Copy link
Collaborator

phil-levis commented Jul 14, 2018

The purpose of Component is to simplify the boot sequence, such that we'll be able to have a configurable kernel.

@ppannuto ppannuto merged commit cebd681 into master Jul 16, 2018

3 checks passed

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

@ppannuto ppannuto deleted the components branch Jul 16, 2018

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