RFC: How much stuff should go into board support crates?

[sajattack]

I'd like to have a discussion on this. Currently, our board support crates are quite light, basically just pin definitions. @tarcieri is looking to integrate drivers for sensors, and possibly additional abstractions based on those drivers for trellis_m4. I'm wondering if we should add neopixel support via smart-leds to all the board support crates with neopixels. Any comments for or against are appreciated.

[tarcieri]

As @sajattack already mentioned, personally I'd be a fan of "batteries included" board support crates that make it convenient to use all of the available board hardware without having to import a bunch of additional crates. There's a certain amount of "wiring up" that has to happen to use those crates, and I think it's nice when all of that functionality is readily available with convenient support functions.

The "batteries included" support for using hardware via 3rd party crates can be gated behind cargo features, e.g. smart-leds can be an optional = true dependency in Cargo.toml, and then when its cargo feature is enabled, corresponding support functions can be enabled in the board support crate as well.

[tarcieri]

Here's a PR which I think is a good example of this particular issue:

PR #48: trellis_m4: Orientation detector

I implemented a simple orientation detector for the board which uses the accelerometer feature (also in an unmerged PR, #47). It's gated on the adxl343 accelerometer crate, which it requires as a dependency.

It doesn't really belong in the accelerometer crate, IMO: the algorithm is specific to the NeoTrellis M4. I also think it's generally useful functionality anyone writing programs for one of these boards would probably like.

Does it belong in the board support crate? Or should it be spun out as a separate crate? (e.g. trellis_m4_orientation).

I will also say that I do plan on sticking around and improving this code, lest anyone is worried they might end up being responsible for it.

[wez]

I'm +1 on making the default feature selection batteries-included.

If that results in pulling in some heavy deps that someone might conceivably want to disable then we should add a feature gate for that, but have that feature be enabled on the list of default features.

[jacobrosenthal]

Maybe this is a place to hijack to a similar conversation. Do we have agreement on the .split() pattern? Seems like some crates are using it to clean up a nice (though small amount) of config boilerplate in the examples at the cost of quite a bit in the bsp.

One complaint I have is the repetition in current naming, (not to pick on just as an example) https://github.com/jacobrosenthal/atsamd/blob/master/boards/itsybitsy_m4/src/pins.rs#L262

• led.led()
• i2c.i2c_master()
• spi.spi_master()
• display.display()
• uart.uart()
• dotstar.dotstar()

as well as

usb_allocator() https://github.com/atsamd-rs/atsamd/blob/master/boards/itsybitsy_m4/src/pins.rs#L276

accel.open() https://github.com/atsamd-rs/atsamd/blob/master/boards/trellis_m4/examples/neopixel_accel.rs#L44

and Im thinking of using .init() so you get

• usb.init()
• led.init()
• i2c.init_master()
• spi.init_master()
• display.init()
• uart.init()
• dotstar.init()

Anyone like anything better?

[sajattack]

Yeah if you want to add more to the BSPs that's good by me. A lot of them are fairly barebones. It hasn't been a priority for me, I like to focus on HAL and when I do a BSP it's usually just enough to get a board up and running.

[tarcieri]

Do we have agreement on the .split() pattern?

I'll note the trellis_m4 uses split() to split up the big bucket-o-pins generated by device_pins into device-specific pin sets, as many of its components have a lot of pins:

https://github.com/atsamd-rs/atsamd/blob/master/boards/trellis_m4/src/pins.rs#L80

This was helpful in splitting up ownership of the pins and handing off whole components at a time, rather than the individual pins, especially in cases like the keypad:

https://github.com/atsamd-rs/atsamd/blob/master/boards/trellis_m4/src/pins.rs#L288

[sajattack]

Oh yeah, I just remembered, I found it less than helpful to implement split, when there were less well-defined "sets"

[jacobrosenthal]

Thoughts on naming consistency? Were all over the place right now. Using Init cleans up the pins.display.display() problem to pins.display.init() which i prefer.

I would argue even pins.usb.usb_allocator() and pins.i2c.i2c_master() should just become pins.usb.init() and pins.i2c.init() as well. I presume we COULD include an i2c_slave() someday, but actually I dont see that happening in a BSP. In my mind a BSP is batteries included for a happy path based on specific vendor shipped hardware. I dont see them having tons of options like i2c_master and i2c_slave. If you get that far in your hacking I think you've graduated to forking the bsp to your own project.

[sajattack]

Yeah init sounds good.

[tarcieri]

I have no horse in this bikeshedding race and init() sounds fine to me.

I guess the only thing I'd suggest is doing a cursory survey of some other embedded Rust projects and see if what they're doing is in anyway consistent and, if so, if we can align with other projects.

[jacobrosenthal]

Looked around a bit.

• stm32, nrf51, and others don't appear to be tackling bsps yet.
• nrf52 impls new() for the entire Board type, and creates all objects in one. However they only have very simple periphs like uart and buttons. I wonder if that will run into any trouble with say, an i2c or spi device not responding taking down unrelated examples. https://github.com/nrf-rs/nrf52-hal/blob/master/boards/actinius-icarus/src/lib.rs#L430
• microbit and others scattered around Ive seen 'new()' on a per peripheral basis as well https://github.com/therealprof/microbit/blob/master/src/led.rs

I think new() is a decent contender with init()

On Wed, Nov 20, 2019 at 6:56 PM Tony Arcieri notifications@github.com wrote:

I have no horse in this bikeshedding race and init() sounds fine to me.

I guess the only thing I'd suggest is doing a cursory survey of some other embedded Rust projects and see if what they're doing is in anyway consistent and, if so, if we can align with other projects.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/atsamd-rs/atsamd/issues/46?email_source=notifications&email_token=AADPI5FD4Z2DXLM2ELEMK5DQUXTDXA5CNFSM4HAJOC22YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEEYICSQ#issuecomment-556826954, or unsubscribe https://github.com/notifications/unsubscribe-auth/AADPI5H237LQQMUQLVVFX6DQUXTDXANCNFSM4HAJOC2Q .

[jacobrosenthal]

Im using pub structs so that in the case of rtfm or something, people can create the structure themselves without the split call. With new it looks like this

    let mut buttons = Buttons {
        latch: pins.button_latch,
        data_in: pins.button_out,
        clock: pins.button_clock,
    }
    .new(&mut pins.port);

We could obviously make a boilerplate call of some kind to hide that. but why add more. init is a little better in this case

    let mut buttons = Buttons {
        latch: pins.button_latch,
        data_in: pins.button_out,
        clock: pins.button_clock,
    }
    .init(&mut pins.port);

And note, I think the current split pattern purposely separates struct filling and init call presumably so that the .split() doesnt cause every peripheral to to be init immediately but rather allows the user to individually init selected peripherals per example.

[jacobrosenthal]

Though I guess since we dont take in our hal, you can still split in the rtfm context just fine. I guess its still nice to offer a clean way not to

Does anyone know if theres any other benefit to this pattern in upstream right now? https://github.com/atsamd-rs/atsamd/blob/master/boards/itsybitsy_m4/src/lib.rs#L87