[RFC] Simpler and more robust component interface

[gendx]

Pull Request Overview

Context

While writing my first components in #1556, I hit some difficulties in how to design things.

This pull request is an RFC to refactor the components interface, with the goals of (1) being less verbose and (2) more difficult to use incorrectly. It contains:

• New ways of initializing static objects, in kernel/src/common/utils.rs.
• An example of how the refactoring would look like on the component side (boards/components/src/alarm.rs) as well as the board side (boards/imix/src/main.rs). I picked the alarm component, which uses the static_init_half pattern, and therefore things should generalize well to the other components as far as I understand it. But please let me know if there are more complex cases that components can handle.

Refactoring of static_init

Current rationale for static_init_half

In principle, static objects in kernel boards are initialized with the static_init macro.

However, this relies on defining a static buffer inline in the current function, which doesn't work in Rust when the static type depends on a generic type of the function (because the compiler would somehow have to instantiate a distinct static object for each type of the function - which may not be well-defined in general).

This led to splitting static_init into *_helper macros defined in components to create static buffers with concrete types, and a static_init_half macro that initializes the buffer with some value. Besides creating some confusion about what this and that half mean, I don't think exposing the internal details of static_init in this way is very robust, as it relies on too many assumptions.

• static_init_half expects a MaybeUninit<T> buffer that contains uninit. Giving it an already initialized buffer would be incorrect as it would erase the previous value without notice. However, the current API of static_init_half just takes a MaybeUninit<T>.
• The *_helper macros return a &mut MaybeUninit<T>, which doesn't prevent the buffer from being used directly, or passed to various static_init_half. The API should only allow to pass such a buffer to static_init_half and only once.
• static_init_half doesn't have to be a macro, it can be implemented as a function. The only part that needs to be in a macro is the static mut BUF: MaybeUninit<$T> = MaybeUninit::uninit();, because that cannot be put in a function generic on $T (due to Rust rules). But this part is already in the *_helper macros.

Proposed new API for static initialization

Instead, I propose the following API (see the changes in kernel/src/common/utils.rs).

• An uninit_static_buf macro, which takes a type and instantiates inline a static mut buffer, containing MaybeUninit::uninit(). The role of this macro is similar to the *_helper macros in components.
• The result of it is stored in a new UninitBuf type, that privately wraps the uninitialized MaybeUninit - preventing anyone else from initializing it. This wrapper is repr(transparent), and just here to guarantee that the contents really are uninitialized. The only way to construct it is via a new() function which takes care of calling MaybeUninit::uninit().
• Another new type is StaticUninitRef, which privately wraps an UninitBuf into a &'static mut reference. This is so that uninit_static_buf returns this wrapper instead of a plain &mut BUF that could be misused.
• The StaticUninitRef wrapper has a single public method, initialize, which consumes the uninitialized self, stores a value in it, and returns the &'static mut T reference. This provides the same functionality as the previous static_init_half (i.e. doing the actual initialization), but:
  • this prevents double initialization, because self is consumed,
  • this prevents non-initialization, because the only way to get a &'static mut T reference is to actually initialize it via StaticUninitRef::initialize.

Then, the *_helper macros in components can use uninit_static_buf instead of directly using MaybeUninit. This removes some code duplication as the components don't have to create a MaybeUninit::uninit() and cast the reference, they just need to call uninit_static_buf!($T). I also suggest to rename *_helper macros into *_buf macros, because all that these helpers really do is creating uninitialized static buffers.

The static_init macro is also a straightforward combination of the uninit_static_buf macro and the initialize function.

Refactoring in components

I find the Component trait itself quite weird.

• The idea is that it is generic with a finalize() method. But in practice components need to add a new() method as well to pass arbitrary parameters, and this new() method is not part of the interface.
  • This leads to expressions of the form ComponentFoo::new(some, parameters).finalize(other, parameters) instead of a more straightforward single-function call ComponentFoo::create(some, parameters, other, parameters).
  • When defining the components themselves, a lot of verbosity happens as well in copying things in new() and finalize(). Defining a single create() function instead would decrease the number of lines of code.
• The finalize method in Component is ill-defined. It takes a &mut self parameter, but the documentation explains that this method may only be called once. A better API to enforce this constraint is to consume the component by taking a self parameter instead of a reference. This is an issue I've hit while trying to define my own components (Refactor Segger RTT and add a usb_debugging mode on the nRF52840-DK board #1556 (comment)).
• The Component trait doesn't mean much in itself. It requires a finalize method taking an arbitrary input type and returning an arbitrary output type, without any constraints on the input and output. Pretty much any function can be turned into this finalize interface.
• Although the Component trait brings genericity, it's never called in a generic context. Combined with the previous remark, I think that this trait mostly adds verbosity.
• The component objects are only constructed (new()) and finalized() (thereby returning a reference to something else). Other than that their value isn't used as an object. The only reason to have an object is so that it implements the Component trait.

Alarm example

As an example, I refactored the alarm component to get rid of the component interface, and this divides the number of lines by two.

A simpler API can also make it easier for everyone to define components. I was quite confused when defining my own components for the first time about what should be put in new() vs. in finalized() - and I think making it easy for anyone to get acquainted to the concept of components so that they can write their own components is important.

Misc

Testing Strategy

Not much at this stage. It's mostly an RFC on the design.

TODO or Help Wanted

This pull request still needs comments on the design.

Documentation Updated

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

Formatting

• Ran make formatall.

[bradjc]

I think it's great to have another look at components after they became generic. It took so long just to make components generic that I was excited about anything that worked.

What enforces that components are created with ::create()? Having the component trait ensures at least some sort of consistent implementation of components. A new component author, who doesn't know about the new()/finalize() roots of components may want to add additional functions, which complicates using components.

Now I wrote the static_init_half! macro, but I am partial to the naming relationship between that macro and static_init! as it provides an immediate connection between the two. And perhaps Rust has already sailed this ship, but I dislike how confusing "uninit" is. Is that supposed to be an adjective or verb? Is it uninitializing already initialized memory, or is the memory just uninitialized? The easy fix is just spelling it out, and I don't see why Rust wanted to save the few characters.

For components, is it important that the the type be something like UninitializedStaticBuffer versus just StaticBuffer? Would we ever need both types? I think what I find confusing is does the uninit part have any meaning to the component author, or is that just an artifact of how it is implemented in rust?

[ppannuto]

This is really exciting to see. I was just trying to teach someone new the component / initialization interface in the last few weeks and complexity of the explanation yielded a mental TODO that you've just checked off my list :)

---

I think I want to go farther than Brad on renaming things, as much as static_init has served us well, it's pretty Rust-y thing to read that and intuit what's going on. I might suggest:

• static_init -> create_and_initialize_static
• uninit_static_buf -> create_uninitialized_static
• UninitBuf -> UninitializedBuffer
• UninitStaticBuf -> UninitializedStaticBuffer

---

I don't really have a solution here, but losing the Component trait is unfortunate. Not because the trait itself was terribly valuable, but because it created a common interface for components [although, the absence of new in the trait lessens that argument].

What's missing with this then is some (ideally compiler-enforced) guidance of what a create method looks like, or really what a Component looks like.

The only thing I came to was that components themselves look pretty procedural, and possibly replaced by yet-another-macro, which could then present a structured interface, but would likely be prohibitively complex.

---

I think my current takeaway is that this has some easy wins (the buffer wrapping) that is strictly better and helps people to 'do the right thing', but the actual creation part may need more iteration.

[phil-levis]

A bit of history. I looked back, and initial approach with components was:

• new took no parameters, or sometimes a reference to the kernel
• finalize took no parameters
• dependency was a call to add an additional dependency to the component.

E.g..:

pub struct GpioComponent {
    pins: Option<&'static [PinHandle]>,
    board_kernel: &'static kernel::Kernel,
}

impl GpioComponent {
    pub fn new(board_kernel: &'static kernel::Kernel) -> Self {
        GpioComponent {
            pins: None,
            board_kernel: board_kernel
        }
    }
}

impl Component for GpioComponent {
    type Output = &'static capsules::gpio::GPIO<'static, mk66::gpio::Gpio<'static>>;

    unsafe fn finalize(&mut self) -> Option<Self::Output> {
        let grant_cap = create_capability!(capabilities::MemoryAllocationCapability);

        if self.pins.is_none() {
            return None;
        }

        let gpio = static_init!(
                capsules::gpio::GPIO<'static, mk66::gpio::Gpio<'static>>,
                capsules::gpio::GPIO::new(self.pins.unwrap(), self.board_kernel.create_grant(&grant_cap))
            );

        for pin in self.pins.unwrap().iter() {
            pin.set_client(gpio);
        }

        Some(gpio)
    }
}

impl ComponentWithDependency<&'static [PinHandle]> for GpioComponent {
    fn dependency(&mut self, pins: &'static [PinHandle]) -> &mut Self {
        self.pins = Some(pins);

        self
    }
}

I think that we realized that dependency could be merged into finalize, such that finalize takes parameters. As for the reason for dependency, the original comment read:

/// If component A requires a reference to component B, and B
/// requires a reference to A, then one of them can implement this
/// trait. For example, A can implement ComponentWithDependency<B>.
/// When A is constructed, B does not exist yet. When B is constructed,
/// A exists, so can be passed to new(). Then, B can be passed to A
/// via a call to dependency(). After both dependencies are resolved,
/// the boot sequence can call finalize() on both of them.

In practice, though, this turned out to not be important (plus it's not exactly clear how finalization will work).

[phil-levis]

I'm in favor of cleaning up components -- the strange overlap between new and finalize is not cleanly defined. But it is important that Component remain a trait. If we just rely on functions named create, if we ever want to layer software we'll need to parameterize by types. Everything standard in the kernel should be a trait.

[gendx]

I'm not against a trait, but as mentioned above the trait would not do anything else than enforce a name.

trait Component {
    type InputType;
    type OutputType;
    unsafe fn create(input: InputType) -> OutputType;
}

impl<A: 'static + time::Alarm<'static>> Component for AlarmComponent<A> {
    type InputType = (
        &'static kernel::Kernel,
        &'static MuxAlarm<'static, A>,
        (
            UninitStaticBuf<VirtualMuxAlarm<'static, A>>,
            UninitStaticBuf<AlarmDriver<'static, VirtualMuxAlarm<'static, A>>>,
        ),
    );
    type OutputType = &'static AlarmDriver<'static, VirtualMuxAlarm<'static, A>>;

    unsafe fn create(input: InputType) -> OutputType {
        let (board_kernel, alarm_mux, static_buffer) = input;
        ...
    }
}

I personally don't think the extra verbosity is worth it. I find it less readable to have an InputType tuple rather than named arguments.

As for the benefits, they are limited to a consistent naming. Because each component has its own input/output types there's not much else one can do than to instantiate each component separately.

---

In any case, the Component part of this RFC can be done independently from the changes in static_init/static_init_half macros. Shall I create a separate PR for these macros first?

[ppannuto]

I think splitting out the buffer parts would make thinking about the component interface easier, yeah — thanks!

…

On Fri, Feb 28, 2020 at 4:42 AM gendx ***@***.***> wrote: I'm not against a trait, but as mentioned above the trait would not do anything else than enforce a name. trait Component { type InputType; type OutputType; unsafe fn create(intput: InputType) -> OutputType; } impl<A: 'static + time::Alarm<'static>> Component for AlarmComponent<A> { type InputType = ( &'static kernel::Kernel, &'static MuxAlarm<'static, A>, ( UninitStaticBuf<VirtualMuxAlarm<'static, A>>, UninitStaticBuf<AlarmDriver<'static, VirtualMuxAlarm<'static, A>>>, ), ); type OutputType = &'static AlarmDriver<'static, VirtualMuxAlarm<'static, A>>; unsafe fn create(intput: InputType) -> OutputType { let (board_kernel, alarm_mux, static_buffer) = input; ... } } I personally don't think the extra verbosity is worth it. I find it less readable to have an InputType tuple rather than named arguments. As for the benefits, they are limited to a consistent naming. Because each component has its own input/output types there's not much else one can do than to instantiate each component separately. ------------------------------ In any case, the Component part of this RFC can be done independently from the changes in static_init/static_init_half macros. Shall I create a separate PR for these macros first? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#1618?email_source=notifications&email_token=AACS3XUZB7N6MCJ2REYYEJTRFDL6TA5CNFSM4KZEFBCKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOENH476A#issuecomment-592433144>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACS3XQNYV5SJ4E3LMIHSMTRFDL6TANCNFSM4KZEFBCA> .

[gendx]

I updated the draft to use a trait. For now it's called CreateComponent to ease prototyping, but should supersede the previous Component trait once there is agreement on the design.

[bradjc]

👍

I would like UninitBuf to be named based on what it is doing for Tock, rather than how it is implemented in Rust.

[gendx]

I would like UninitBuf to be named based on what it is doing for Tock, rather than how it is implemented in Rust.

What name would you suggest instead? Uninitialized? Is your concern about the shortcut "uninit" vs. spelling "uninitialized" (I reused "uninit" from MaybeUninit)?

This type is just a wrapper around an uninitialized T, that's what it is, there isn't much more to say about it. I don't see much other meaningful naming option.

Then UninitStaticBuf (or UninitializedStatic?) contains a static reference to such uninitialized T. That's again pretty descriptive.

And the initialize() function actually initializes it and returns a plain static reference to T.

I find these semantics much clearer than the former static_init_half which doesn't describe what "half" it's dealing with.

[bradjc]

Take a look at the commit I added. I wanted to capture what you wrote in the PR description in the code comments for future readers. That also helped me understand the static initialization design, and I'm a fan. The commit also includes my opinion on the names:

• static_init!()
• static_buf!()
• UninitializedBuffer
• StaticUninitializedBuffer

My comments might be a little verbose, but I really wanted to capture why we have all of this complexity.

[gendx]

The names look good! Thanks for taking the time documenting it as well. I'll take a closer look tomorrow.

[bradjc]

I'm convinced on the static_init changes.

I'm less sure about the component trait changes. The current component has a relatively familiar feel. The author defines a struct with all of the resources it needs, creates a new() function, and them implements the component trait. The weirdness comes with the component associated types and the macro creation.

This PR keeps that existing weirdness, but also requires the resources to be specified as a tuple, and the struct now has to contain a phatomdata type. That might save a couple lines of code, but adds a bit of complexity to do it. My opinion is this makes components harder to use. But even ignoring that, do you think there is enough benefit to warrant refactoring all of the components?

[gendx]

I'm convinced on the static_init changes.
I'm less sure about the component trait changes.

I was quite ambitious in proposing both changes in this pull-request, but they can indeed be split.

One thing to note is that deprecating the former static_init_half macro (which can be done separately than introducing the new static_init implementation) will require to refactor all components, so if we go that way I think it would be better to also rethink the components' design.

The current component has a relatively familiar feel. The author defines a struct with all of the resources it needs, creates a new() function, and them implements the component trait. The weirdness comes with the component associated types and the macro creation.

My concerns, are mainly about:

• The big layer of verbosity required to write a component. For usually 5-10 lines of actual business logic, one needs to define a struct, then a new function that just copies arguments to this struct, and then a finalize function to use those arguments to construct the actual objects. Additionally, associated types are also redundant with the function's parameter. Adding to that the documentation, which often paraphrases the corresponding capsule, and a component takes 50-100 lines for the 5-10 lines of business logic. That's a big overhead to take into account.
• I disagree with the "relatively familiar feel". Although components have a familiar feel to a developer who has already written some, I personally found their logic hard to grasp when writing my own for the first time. What should I put in new vs. finalize? When should I use static_init_half? Ultimately, this extra complexity (mostly due to the verbosity) means a steep initial learning curve for someone new to (this part of) the project, so only "experts" will tend to write components. I think that the code patterns should be as simple as possible so that new contributors can easily adopt the code base and make meaningful contributions. Simplification was my goal in proposing to remove the Component trait altogether.

This PR keeps that existing weirdness, but also requires the resources to be specified as a tuple, and the struct now has to contain a phatomdata type. That might save a couple lines of code, but adds a bit of complexity to do it. My opinion is this makes components harder to use.

As mentioned in the previous comments, the extra weirdness (tuple, phantomdata) are necessary if we keep a trait for something that shouldn't be a trait. Given that each component has its own list of parameters of various types, there's no other way of "unifying" all components behind a common trait. I again don't think it's worth having all this extra complexity just to make sure developers use the same function name. Rather than a completely artificial trait, we should use code reviews to make sure that the style is consistent among components.

But even ignoring that, do you think there is enough benefit to warrant refactoring all of the components?

• Most capsules still don't have a component, so improving the code pattern now will benefit all the currently component-less capsules.
• It will be beneficial to have a simpler interface for future contributors who will create their components.
• If the new interface has 5-10x fewer lines of code, migrating existing components will reduce the code base's size accordingly, which I also see as a benefit.
• On the other side, the migration cost can be spread in time by refactoring components one-by-one if migrating them all is too big of a change.
• Deprecating static_init_half will already have some refactoring cost, so if we also refactor components to a new pattern at the same time, the total cost will be less than the sum of the two refactorings.

So yes, all in all I think that the benefits outweigh the cost of migration.

---

All in all, I suggest to split this pull-request into:

1. Refactoring the static_init implementation (which we now seem to have agreed upon). The static_init_half macro isn't deprecated yet, because all components still use it.
2. Refactoring of the components, to migrate away from static_init_half, and hopefully we can agree on a simpler pattern for components. The actual migration of all the components can be split into multiple pull-requests if that's easier.

[bradjc]

I almost suggested separating into multiple PRs, but you are right the re-factoring is a big overhead even with just the static init changes. I don't think it is worth doing that twice.

familiar feel and verbosity

Here I am referring to the use of a struct that holds resources and settings, and is created with a new() function. This pattern happens all over Tock. I'm not necessarily referring to the component trait itself.

I still believe components have to be based on a trait. We are rather terrible at enforcing code consistency in PRs. With multiple reviewers it just will not work to enforce consistency at PR time. And the reason is is that components are going to come in PRs that also include new capsules or peripheral drivers, which often require numerous notes. But there is only so much that human reviewers are willing to request of a PR author, and after so many back-and-forth rounds components will be an easy place to say "eh, good enough".

PhantomData may be obvious to experienced Rust developers, but it is not to average developers or embedded developers. In cases where it exactly solves a problem and can be hidden behind an abstraction it is great. However, requiring it to write the more "user facing" kernel code is not great for tock usability or friendliness. (We expect that capsules and their corresponding components is what a lot of developers will need to write to support particular hardware platforms in the future.)

If both of these issues can be solved with a bit of verbosity, I think it is well worth it.

effects of new static buffer handling

I agree that static_init_half is confusing. I am curious if the refactoring of how static buffers work makes the current new/finalize interface more intuitive. Particularly if we add a guide on what new and finalize should be used for.

• new: must pass in all resources the component needs
• finalize: pass in any static buffers and configuration parameters

Or, maybe a third function would be helpful:

• new: pass in all resources
• configure: pass in all settings
• finalize: pass in needed static buffers

Perhaps we should remove static_init from components, so that all components require their static buffers to be passed in. This would make copying a component easier, since you could start from the most similar component without having to worry if it matches your requirement for needing separated buffer creation from initialization.

[gendx]

effects of new static buffer handling

I agree that static_init_half is confusing. I am curious if the refactoring of how static buffers work makes the current new/finalize interface more intuitive. Particularly if we add a guide on what new and finalize should be used for.

• new: must pass in all resources the component needs
• finalize: pass in any static buffers and configuration parameters

Actually, with the new StaticUninitializedBuffer type, the finalize function can be made more specific (which would also give more sense to a trait):

pub trait Component {
    type StaticInput = ();
    type Output;
    unsafe fn finalize(
        self,
        static_memory: StaticUninitializedBuffer<Self::StaticInput>
    ) -> Self::Output;
}

// ...

impl<A: 'static + time::Alarm<'static>> Component for AlarmMuxComponent<A> {
    type StaticInput = MuxAlarm<'static, A>;
    type Output = &'static MuxAlarm<'static, A>;

    unsafe fn finalize(
        self,
        static_buffer: StaticUninitializedBuffer<Self::StaticInput>
    ) -> Self::Output {
        let mux_alarm = static_buffer.initialize(MuxAlarm::new(self.alarm));
        mux_alarm
    }
}

// ...

impl<A: 'static + time::Alarm<'static>> Component for AlarmDriverComponent<A> {
    type StaticInput = (
        VirtualMuxAlarm<'static, A>,
        AlarmDriver<'static, VirtualMuxAlarm<'static, A>>,
    );
    type Output = &'static AlarmDriver<'static, VirtualMuxAlarm<'static, A>>;

    unsafe fn finalize(
        self,
        static_buffer: StaticUninitializedBuffer<Self::StaticInput>
    ) -> Self::Output {
        // ...
    }
}

One question that arises from the latter example (AlarmDriverComponent) is how to "split" a StaticUninitializedBuffer<(Foo, Bar)> into (StaticUninitializedBuffer<Foo>, StaticUninitializedBuffer<Bar>) (and more generally converting a buffer of tuple into a tuple of buffers).

[ppannuto]

I've been staring at this for a while, and I'm honestly not sure what the most useful feedback I can give is. I think what is proposed thus far is better, but the whole initialization process is still quite verbose and confusing to newcomers.

In the most recent proposal, would only finalize be part of the Component trait? I liked @bradjc's notion of the 'three phases' of component creation.

---

To what extent is this hard because we are relying on static lifetimes, which would require richer const fn's to directly create objects -- would the adoption of a kernel lifetime, as proposed elsewhere, allow components to more directly initialize themselves by creating their own buffers (now a safe operation) rather than needing to outsource their own allocation to board main files to allocate buffers and resulting in this split-phase initialization complexity?

[gendx]

To what extent is this hard because we are relying on static lifetimes, which would require richer const fn's to directly create objects -- would the adoption of a kernel lifetime, as proposed elsewhere, allow components to more directly initialize themselves by creating their own buffers (now a safe operation) rather than needing to outsource their own allocation to board main files to allocate buffers and resulting in this split-phase initialization complexity?

My intuition is that this would be an interesting path to explore, but this would first require quite a bit of untangling of the static lifetimes in the kernel, to be able to prototype it. #1628 would be a step in this direction. Completing #1074 would be another step.

The good news is that if it turns out not working, the more generic lifetime would still be compatible with the current implementation, by setting 'kernel to 'static.

[bradjc]

PR Summary

This PR right now does two things:

1. Re-designs how static_init works. There is now a uninitialized buffer type, and the only way to get a &'static mut is to call initialize(), ensuring that memory has been statically allocated and initialized.
2. Adds an input parameter to the Component trait. This makes it clearer how to pass more parameters to the finalize() function (called create() in this PR) rather than adding them as "static input" (when they really aren't).

Status

The static_init changes have been reviewed and so far the consensus is they are a great improvement.

However, changing static_init means changing every component. So, it is a lot of work to merge just half of this PR, and then make a change to the component trait. Therefore, this is blocking on deciding on what (if any) changes to make to Component.

Component Change Motivation

More details are in this thread, but to summarize:

• Components provide little framework for users, so it is difficult to know what to pass where.
• Creating a struct, and writing a new() function, adds a fair bit of verbosity to the code.
• However, having a trait in the kernel adds at least some structure, and we can't expect code reviews to enforce structure.

[bradjc]

My thoughts:

Convention vs. Rigorous Traits

I think the more we can do with the Component trait the better. We can have conventions here, but I think they will be difficult to rely on since the component is likely to be the "highest level" code in any PR, and therefore is least likely to receive much scrutiny.

However, it seems to be difficult to enforce much in the component trait, since every type has to be very flexible given the nature of components. Even in the current revision, a component author could still use the new() function and struct fields, and not require PhantomData.

PhantomData

I think that requiring (or expecting) a PhantomData type in /boards adds unnecessary complexity for developers not experienced in Rust. If it was essential for components to be shared I would feel differently, but it just needed to reduce a small amount of duplicated code. If the component only needs one "resource" (e.g. something that implements the i2c HIL), then it saves very little code.

My Proposed Component Trait

pub trait Component {
    /// A type or tuple of types that includes all of the hardware resources
    /// that this component requires. Hardware resources are generally objects
    /// that implement kernel HILs.
    type InputResources;

    /// A type or tuple of types that represents any configuration options this
    /// capsule provides. For example, this could include setting a baud rate,
    /// or the start and end addresses of a writeable flash region.
    type InputConfiguration;

    /// A type or tuple of types of static, uninitialized buffers that will be
    /// used as the memory to store the objects created by this component. This
    /// _must_ be either empty or one or more `StaticUninitializedBuffer` types.
    /// All other inputs must be include in the other associated types.
    type InputStatic;

    /// A type or tuple of types of additional objects that the component needs
    /// to successfully create the output type. An example is a reference to the
    /// kernel for creating a grant region.
    type InputHelpers;

    /// The `&'static` reference type the component will create.
    type Output;

    /// Creates and returns the component struct object.
    fn new(resources: Self::InputResources) -> Self;

    /// Setup the static buffers for this component.
    unsafe fn finalize(self,
                       configuration: Self::InputConfiguration,
                       static_memory: Self::InputStatic,
                       helpers: Self::InputHelpers) -> Self::Output;
}

This provides as much structure as I can some up with, while still, of course, not really providing any structure at all since everything is based on associated types. But, my goal is that the associated types map well to what needs to be passed to a component, and that it would make sense to implements what needs to be passed to what.

It also enforces that there has to be a new() function.

• An implementer using new() as intended would also have to create a struct and save references to all hardware resources (as the current component convention requires). I think it is useful to note that our components use very few hardware resources, and I don't think any components would require storing more than two in their struct. Also, if a type/lifetime needs to be referenced, the minimum is one.
• Next, by making new() part of the trait and having an associated type as its parameter, one copy of writing out the types is removed relative to the current design.
• Finally, I want to weakly make the argument that this approach with new() in the trait would impose more structure than the version that passes everything to finalize(). The version in this PR, for example, does not suggest that keeping a struct and a new() function (as is the current convention, and used throughout Tock) is not recommended. By making new() in the trait an implementer could still make it a no-op, but at least would have to ignore the descriptions of the associated types to do so.

[ppannuto]

I think I'm actually pro PhantomData. It helps in its way to crystallize a somewhat subtle concept. When you look at something like an AlarmMuxComponent, the actual thing, the Alarm Mux, is not the struct AlarmMuxComponent rather the mux is made up of the static buffers that are passed in. The "component" here is nothing, it's just wasted space when we make a struct for it.

What the PhantomData helps make clear is that AlarmMuxComponent is not actually a component, it's a factory for making components (and in the common case, that factory doesn't need anything -- i.e. the factory has no objects or state, it's just a non-thing to implement methods on). Indeed, I might suggest renaming the trait to something like Factory and then having an AlarmMuxFactory, as that's better suited to what is happening.

I think what @gendx's refactor makes clear is that there is no purpose to ever actually creating a AlarmMuxComponent object, and hence there is no need for a new method.

---

That said, I do like the types and descriptions you've provided for them, I think that's much clearer. Given the above, I might propose something like:

pub trait ComponentFactory {
    /// A type or tuple of types that includes all of the hardware resources
    /// that this component requires. Hardware resources are generally objects
    /// that implement kernel HILs.
    ///
    /// These are resources the component will have access to forever.
    type ComponentResources;

    /// A type or tuple of types that represents any configuration options this
    /// capsule provides. For example, this could include setting a baud rate,
    /// or the start and end addresses of a writeable flash region.
    ///
    /// These should generally be enums or other plain-old-data types that do
    /// not have any lifetime considerations.
    type InputConfiguration;

    /// A type or tuple of types of static, uninitialized buffers that will be
    /// used as the memory to store the objects created by this component. This
    /// _must_ be either empty or one or more `StaticUninitializedBuffer` types.
    /// All other inputs must be include in the other associated types.
    ///
    /// These buffers must last for the lifetime of the component.
    type BuffersToBecomeComponent;

    /// A type or tuple of types of additional objects that the component needs
    /// to successfully create the output type. An example is a reference to the
    /// kernel for creating a grant region.
    ///
    /// These are ephemeral references that are only available to a component
    /// during its creation.
    type CreationHelpers;

    /// The `&'static` reference type the component will create.
    type Output;

    /// Setup the static buffers for this component.
    unsafe fn create(self,
                       configuration: Self::InputConfiguration,
                       static_resources: Self::ComponentResources,
                       static_memory: Self::BuffersToBecomeComponent,
                       helpers: Self::InputHelpers) -> Self::Output;
}

(Edited by Branden to add syntax highlighting to Pat's example)

[gendx]

Although putting more associated types may sound appealing in terms of giving semantics, I worry about the additional bloat created by forcing components into boxes. To me, this is the trap of creating a trait for a trait's sake, when there's no programming reason to do so.

With this, we end up with associated types that have no use for the majority of components (so we'll end up calling ComponentFoo::create((), (), meaningful_parameter, (), ())), with artificial grouping of function parameters in tuples to fit multiple values in an InputFoo type (ComponentFoo::create((foo, bar), other_parameter, buffer), and with the developers of components having to use PhantomData to fit their components in the pre-defined boxes. All of this with additional verbosity in the code.

As a newcomer, I'd probably spend quite some time figuring out what all the types in the component trait are supposed to mean, and what to set for each of them.

---

As I understand it, the main motivation for having a Component trait is still to force a consistent naming of a create function, along with a certain order of parameters.

However, we can also have clear conventions that are not enforced by any code. For example, it's common for Rust structs to implement a new function that returns Self. This is some kind of "constructor", even though the Rust language itself doesn't have any concept of constructor. Each struct is free to name such constructors new or something else, yet in practice there is broad consistency in the ecosystem of calling them new.

This T::new(foo, bar) pattern works well across the Rust ecosystem, so in the same way I don't see why a ComponentFoo::create(foo, bar) pattern wouldn't work well for Tock. Ultimately, all of the components are instantiated in boards, so inconsistent naming would be easy to spot in code reviews, without requiring an immense amount of in-depth scrutiny by the code reviewer.

The difference is that creating a component is much simpler if it boils down to writing one utility function, rather than splitting it between multiple functions (new, finalize, etc.) and having to define a bunch of associated types and copying data between function parameters and member fields.

---

Ultimately, it should be easy for anyone to create their own components. It would be useful to have feedback from other people who have written them, or who haven't written any yet. For now, I've seen @alevy, @alexandruradovici, @bradjc, @hudson-ayers (in alphabetical order) creating new components in boards/components.

In contrast, a recent pull request to add an HMAC capsule doesn't have a component yet (https://github.com/tock/tock/pull/1702/files#r408097390). It would be interesting to know whether that's because components are too cumbersome to create, and whether any of the proposed designs in this thread would be better to grasp in that respect.

[bradjc]

I don't see a component trait as a tool for component authors, but rather a tool for component users. A component author probably doesn't need a component, they know what types need to be created, what set_client calls need to be called, and how to quickly debug if things don't work as expected. It's users later on that have the difficult task. I know from experience how annoying it is to track down why a capsule isn't working because I forgot the set_client() call. The trait provides structure for users who don't have to learn how individual component authors structured their components.

I don't see how it is possible to both enforce convention but not encode it in some sort of trait. It's not really fair for someone to copy an existing component, modify it, and then be told during a review that they didn't quite capture the spirit of the convention, when there isn't a good way to write what that convention is down. If we want complete flexibility for component authors, what conventions would we even have? Further, I stand by my argument that components are difficult to nitpick and review in PRs, and use a recently merged component that doesn't quite follow the existing convention as evidence.

[gendx]

I don't see a component trait as a tool for component authors, but rather a tool for component users.

I definitely agree.

And that's why I think it should be as easy as possible for capsules authors to write components. If writing a component is unnecessarily complex, developers will write capsules without components (possible example: https://github.com/tock/tock/pull/1702/files#r408097390). So users won't have the any component tool at all - which would be a net negative for component users.

The trait provides structure for users who don't have to learn how individual component authors structured their components.

I don't think it's that complicated to use a component. If the component consists of a single create() function, users simply have to learn which parameters to pass to this function (all the set_client magic happens inside the component so they don't have to worry about that).

I don't see a lot of difference between learning to write:

let alarm = components::alarm::AlarmDriverComponent::new(board_kernel, mux_alarm)
    .finalize(components::alarm_component_helper!(sam4l::ast::Ast));

versus:

let alarm = components::alarm::AlarmDriverComponent::create(
    board_kernel,
    mux_alarm,
    components::alarm_component_buf!(sam4l::ast::Ast)
);

but the latter is simpler to write for a component developer: essentially move a block of code from a board into some create function with the suitable parameters - without any extra verbosity nor headache in figuring out the associated types.

I don't see how it is possible to both enforce convention but not encode it in some sort of trait.

I've given an example with "constructors" in Rust - functions of the form new(...) -> Self - that are a convention without any sort of trait.

Another example is the component_foo_helper macros. These aren't encoded in any sort of trait, yet are a convention for components. Has there ever been any issue with these?

It's not really fair for someone to copy an existing component, modify it, and then be told during a review that they didn't quite capture the spirit of the convention, when there isn't a good way to write what that convention is down.

Can't we write this convention down in some documentation, for example in boards/kernel/README.md (or any other suitable location)?

If we want complete flexibility for component authors, what conventions would we even have?

I don't think we need a lot of conventions, because components are in essence not that complex. Here are my thoughts.

• A function named create to initialize the component.
• The inputs to create should be, in order:
  • The board's kernel (if needed).
  • Dependencies from other components (e.g. an alarm).
  • A static buffer (if needed). This should be of type StaticUninitializedBuffer<T> (or a tuple of such StaticUninitializedBuffers). The caller should provide it via a component_foo_buf macro that creates a suitable static buffer.

In these example conventions, one thing that cannot be enforced by any trait is that there is a macro named component_foo_buf to create a suitable static buffer. This is currently not part of the Component trait, yet there is already such convention, and as far as I see things, this convention works.

Further, I stand by my argument that components are difficult to nitpick and review in PRs, and use a recently merged component that doesn't quite follow the existing convention as evidence.

Do you have any example of what could go wrong in a component, which wouldn't follow the convention(s). I don't see many ways of getting the API wrong if it's just a create function with a certain order of parameters.

Documenting the convention would avoid the nitpicking, and on the other side of things, also help developers to prepare their code and write good components before sending their pull request.

[ppannuto]

I think part of what makes this different than the simple new -> Self convention is that component creation is necessarily unsafe (due to the static....) and thus trading a little structure for correctness can be valuable.

I don't think it's that complicated to use a component. If the component consists of a single create() function, users simply have to learn which parameters to pass to this function

(emphasis mine); in the case of the more complex and possibly unsafe interface, this learning is a possibly higher bar.

With components, I think we are trying to impose a little bit more structure than an arbitrary block of code that could be moved freely between a board main file and create function. Specifically, the four types proposed above make very clear the lifetime and access expectations of each list of things -- in a free-form create method, this structure is lost.

---

Regarding the interface:

The inputs to create should be, in order:

• The board's kernel (if needed).
• Dependencies from other components (e.g. an alarm).
• A static buffer (if needed). This should be of type StaticUninitializedBuffer (or a tuple of such StaticUninitializedBuffers). The caller should provide it via a component_foo_buf macro that creates a suitable static buffer.

I think this bulleted list is illustrative, as it helps demonstrate that there are categories of things used during component creation. In the simple create function approach, the categories all munge together. In contrast, the types are used to bin arguments into one of four categories:

// This requires authors to understand how each argument is used by create
//
// We could impose a convention of commenting use
let alarm = components::alarm::AlarmDriverComponent::create(
    // objects this component will have references to
    board_kernel,
    mux_alarm,
    // buffers that will make up this component
    components::alarm_component_buf!(sam4l::ast::Ast)
);

versus using the type system to identify and enforce the four types of things component creation might require:

let alarm = components::alarm::AlarmDriverComponent::create(
    (board_kernel, mux_alarm),                         // the first argument (the ComponentResources) is list of things this component will have references to
    (),                                                // the second argument (the InputConfiguration) is any configuration required, here there is none
    components::alarm_component_buf!(sam4l::ast::Ast)  // the third argument (the BuffersToBecomeComponent), defines what will make up the component
    (),                                                // the fourth argument (the CreationHelpers) is anything emphemeral
);

This means the type signature of create is effectively a series of four lists, which are sometimes empty, but consistent and clear across all invocations of create, regardless of the size of each individual list for each use of create.

---

Again, this structure certainly could be done by convention alone, but I think this is a case where we can use the type system a bit to help automatically enforce things.

As a newcomer, I'd probably spend quite some time figuring out what all the types in the component trait are supposed to mean, and what to set for each of them.

Do you think the proposed doc comments are unclear? Would examples for each help?

We could/should also encode the purpose a bit in a top-level doc-comment:

/// Components are used to make instantiating capsules easier for board authors.
///
/// A `ComponentFactory` is similar to a constructor, however, unlike an open-ended new()
/// method, we provide some structure to create() to help ensure consistency across
/// components. The associated types below describe the four classes of arguments
/// that are valid for a component constructor to use. Not all components will need
/// arguments from each class.
pub trait ComponentFactory {
     ...

[gendx]

While working on #1765, I also realized that it's not clear to me what should go in finalize today.

For some components, e.g. alarm, the finalized argument (StaticInput) is a (tuple of) &'static mut MaybeUninit type (coming from component_foo_helper! macro), on which static_init_half! is applied by the component to finish its initialization.

For other components, e.g. button, the StaticInput is an slice, coming from a component_foo_helper! macro as well, but which doesn't have the same purpose. This slice isn't MaybeUninit and isn't here to be consumed by static_init_half!. This slice is indeed &'static but not mut nor MaybeUninit.

So the current component interface is already somewhat blurry in that regard.

[bradjc]

@tock/core-wg We should make progress on this. I'm pretty supportive of @ppannuto's ComponentFactory approach which I think is clear and well-matched to the purpose of components. I'm hopeful that the required types provide some guidance and structure, and they also provide a place for documentation.

[ppannuto]

This was discussed on the core call today.

The major results:

• This week, @tock/core-wg members and others should weigh in on the proposed design with a goal of settling on component design by next week's call.
• Path the implementation: Phased vs flag? Phased in should be viable. Proposal: (1) Add a PR with the new static init, in a new namespace; (2) Update components to new interface; (3) Remove legacy interface

[hudson-ayers]

I support the ComponentFactory approach.

In terms of implementation approach, I think it makes the most sense to add just the static_init!() changes in this PR, then let updates to components trickle in individually, then remove static_init_half!() once all of the old components have been updated.

I am happy to convert a number of components to the ComponentFactory approach.

[gendx]

So is my following understanding correct?

• The changes in kernel/src/common/utils.rs (uninitialized buffer types + static_buf! macro + migrating static_init! implementation to use static_buf!) are agreed upon. I can make a pull request focused on these changes.
• The exact component design, which builds on top of it, as well as the migration process to new components, is still pending discussion.
• The removal of static_init_half! can be deferred to when the migration will be complete.

[ppannuto]

• buffer types + new static_buf! agreed upon
• The removal of static_init_half! can be deferred to when the migration will be complete.

Yes and yes. The idea I think is that the new interface is preferred, but since it's named differently can co-exist with the old static_init! during migration. Once everything is moved over, static_init! will be removed.

• exact component design still pending discussion

Yes; on the call, only a few of us (@hudson-ayers, @bradjc, and myself) had read this deeply. We are currently in favor of the ComponentFactory (as described in this comment), but wanted more folks to weigh in. So during this week, @tock/core-wg (and anyone else interested) will review this thread and the proposed design.

[bradjc]

Half of this PR (the static init changes) have been merged. The remainder is re-vamping the component interface, and the discussion here lead to a good design for components. However, it is a fair bit of effort to update all of the components to it. If anyone is motivated to do the update, we would be happy to have the component interface updated to the design presented here.