Update nordic board documentation [was: Rename nrf52dk_base to nrf52_base]

[ppannuto]

Pull Request Overview

The base isn't just for things that are DK-like, it's really for anything in the nrf52 family.

This also adds a README to the nordic directory in the boards file, because I mix these names up all the time.

Testing Strategy

Compiling.

TODO or Help Wanted

Re-read the names to make sure I wrote all of this correctly.

Documentation Updated

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

Formatting

• Ran make formatall.

[lschuermann]

I generally like the idea. Also appreciate the source branch name. :)

However when we're deciding to makes this a common base for all things nRF52 we could run into problems in the long run. For instance, the nRF52840DK already contains a few things that the others don't have, like the SPI flash.

tock/boards/nordic/nrf52_base/src/lib.rs

Lines 323 to 329 in b1400b8

	let nonvolatile_storage: Option<
	&'static capsules::nonvolatile_storage_driver::NonvolatileStorage<'static>,
	> = if let Some(driver) = mx25r6435f {
	// Create a SPI device for the mx25r6435f flash chip.
	let mx25r6435f_spi = static_init!(
	capsules::virtual_spi::VirtualSpiMasterDevice<'static, nrf52::spi::SPIM>,
	capsules::virtual_spi::VirtualSpiMasterDevice::new(

Adding more boards could make this additional complexity an issue. The setup_board() argument list and function is already really long. I'd appreciate if we found a way to call back into the board specific crate for further HW initialization (for instance bus peripherals). Think an array of components, getting passed all of the base peripherals. I'm not sure whether that's possible currently.

[niklasad1]

Adding more boards could make this additional complexity an issue. The setup_board() argument list and function is already really long. I'd appreciate if we found a way to call back into the board specific crate for further HW initialization (for instance bus peripherals). Think an array of components, getting passed all of the base peripherals. I'm not sure whether that's possible currently.

That's a good point but I don't think it is possible current because the shared nrf52 boards crate loads the kernel. We should consider using the builder pattern for such complex methods it just really hard to understand and use. However, it is out-of-scope for this PR and perhaps open issue/RFC regarding this and continue the discussion?

[gendx]

Adding more boards could make this additional complexity an issue. The setup_board() argument list and function is already really long. I'd appreciate if we found a way to call back into the board specific crate for further HW initialization (for instance bus peripherals). Think an array of components, getting passed all of the base peripherals. I'm not sure whether that's possible currently.

I think the right way of solving this would be to refactor the relevant code into components, so that the setup_board function of nrf52_base becomes quite short, at which point it would be acceptable to remove nrf52_base and duplicate its contents into each of the boards instead.

I'm also curious why the Nordic boards are the only ones to share such a base module. Aren't there other Tock boards that share a chip (or have a similar chip)?

[ppannuto]

I think the answer is that the nordic family happened to be the first with a lot of overlap, and so it was the first way we experimented with some way of sharing code. I don't think we were ever totally satisfied with the ergonomics, so it didn't propagate to other boards.

I agree we should consider other approaches moving forward, but I would also like to pull this in (post-1.5) still, as I think it clarifies the current state of things. A more substantial refactor of sharing board code should likely begin with an RFC.

[hudson-ayers]

This looks good to me, the current naming is confusing. I think its funny this saves 12 bytes of flash on the nrf52dk (presumably because paths are stored in the binary and are now slightly shorter).

[gendx]

This looks good to me, the current naming is confusing. I think its funny this saves 12 bytes of flash on the nrf52dk (presumably because paths are stored in the binary and are now slightly shorter).

Yes, see #1668. The paths are stored for panic messages. Incidentally, the acd52832 board shouldn't see any change because panic messages are dead code on this board (the panic handler only blinks LEDs).

[bradjc]

I don't think we should rename the crate (but the readme is a nice addition). The crate is a nrf52_dk base platform, and will not reasonably support all nrf52 boards. Naming it nrf52_base suggests that it is a base for any nrf52 board, and that is not something we can realistically support.

[hudson-ayers]

I don't think we should rename the crate (but the readme is a nice addition). The crate is a nrf52_dk base platform, and will not reasonably support all nrf52 boards. Naming it nrf52_base suggests that it is a base for any nrf52 board, and that is not something we can realistically support.

But then you keep the confusion that one of the 3 boards that currently uses the base is not a development kit (the nrf52840 dongle) 😕

[ppannuto]

Yeah, naming it _dk is strictly wrong. What about nrf528xx_base as it's supporting xx=40 and xx=32 right now?

[bradjc]

But then you keep the confusion that one of the 3 boards that currently uses the base is not a development kit (the nrf52840 dongle) 😕

It's a development board sold by Nordic. Doesn't seem that odd that it would use a dk base crate.

I think the name reflecting its purpose is more useful than one reflecting how it is being used.

It does seem like components would be a better option than _base, but we didn't have them when i created _base.

[ppannuto]

I think the name reflecting its purpose is more useful than one reflecting how it is being used.

I think a name reflecting what it is is more useful than either its purpose or use. nrf528xx_components ?

[bradjc]

I say we follow:

I think the right way of solving this would be to refactor the relevant code into components, so that the setup_board function of nrf52_base becomes quite short, at which point it would be acceptable to remove nrf52_base and duplicate its contents into each of the boards instead.

And then if there is anything left over it would be nrf52_components.

[gendx]

The crate is also a base for acd52832. Is this one at all related to nrf52dk? Otherwise I think that nrf52_base would be more meaningful. Besides, nrf52dk_base make it sound like it's closer to the nrf52dk board than any other board, which isn't the case.

It's like the nrf5x chip: it's here as a common denominator of several supported chips, but doesn't mean that Tock intends to support all possible nrf5xxxx chips.

[bradjc]

It is not a base for acd52832, the ACD just uses the components from the crate.

[bradjc]

It's like the nrf5x chip: it's here as a common denominator of several supported chips, but doesn't mean that Tock intends to support all possible nrf5xxxx chips.

But, if someone added support for nRF53 to the nrf5x that would be perfectly acceptable. However, if someone added support for the ardiuno nano ble sense (for example) to nrf52dk_base, I think that would be very difficult to maintain. But if it is called nrf52_base, I don't know why we would say no and not merge it.

[gendx]

I think a name reflecting what it is is more useful than either its purpose or use.

I definitely agree on this one. I've seen the same reasoning of "what it is" vs. "what it's used for" in other pull requests, and I think the former is more meaningful.

The use cases of something (speaking of a module, a type, or a function) can and do definitely change over time. And when you're at a call site, you know the purpose (the call site is the purpose/use case), so IMO the goal of the module/type/function's name is to be easy for the reader to understand what the thing is/does, without having to look for implementation details of the thing in the source code.

It's a bit like naming a "Vec" a "Stack" because it's used once as a stack (push_back, pop_back). The variable should be named "stack" to show the local purpose in that context, but the type should still be a "Vec" because that's what it is, and other users may not use it as a stack.

[gendx]

It's like the nrf5x chip: it's here as a common denominator of several supported chips, but doesn't mean that Tock intends to support all possible nrf5xxxx chips.

But, if someone added support for nRF53 to the nrf5x that would be perfectly acceptable. However, if someone added support for the ardiuno nano ble sense (for example) to nrf52dk_base, I think that would be very difficult to maintain. But if it is called nrf52_base, I don't know why we would say no and not merge it.

I'm not familiar with the specifics of the ardiuno nano ble sense, but if it can use the nrf52_base::setup_board function as is, then great. If it can't, it can just do its own setup and I don't see the issue.

And ultimately, components are probably the right level of code factoring, neither too coarse nor too fine-grained.

[bradjc]

I'm not familiar with the specifics of the ardiuno nano ble sense, but if it can use the nrf52_base::setup_board function as is, then great. If it can't, it can just do its own setup and I don't see the issue.

At the risk of not progressing this PR at all: I would think that someone looking to develop tock and who sees an nRF52_base crate can reasonably expect that that crate is the base for any nrf52-based board. Then if the Arduino Nano BLE, which uses an nrf52840, can't use nrf52_base::setup_board directly, the setup_board function must have a bug, and a patch so that setup_board works is warranted. This is OK, but I have two issues with this:

1. We know that what is now nrf52dk_base is not a great base for nrf52 boards. It includes a Platform and assumes other things about the hardware which I don't think we would if we built an intentional nrf52_base. If we want an nrf52 base we should look at a handful of nrf52 boards and make it suitably generic and useful.
2. The patch will likely make nrf52_base more difficult to maintain, when we should be prioritizing components. We also shouldn't reject the patch, because it would correctly make nrf52_base a better base for an nrf52-based board.

Perhaps this rename is actually appropriate (which I am not sure about), but I don't think it is the fix that we want.

[jmichelp]

Now that I want to plumb the CryptoCell-310 into the relevant boards, I'm actually facing the limits of this nrf52dk::setup_board() function.

How about the following:

• modify setup_board() so that it doesn't start the kernel loop anymore but return the platform object and make it a method of the struct Platform
• use struct composition. nrf5280dk::Platform would contain a nrf52dk_base::Platform and nrf52840dk::Platform::setup_board() would first call nrf52dk_base::Platform::setup_board() before configuring its own components. This would also make it easier to implement kernel::Platform trait without duplicating code.
• the reset handler would call the setup_board() method on the corresponding board and start the kernel loop using the returned platform object.

This would allow us to have the USB and the CryptoCell (and potentially the NFC too if we end up using it in OpenSK) in the nrf52840* boards without making nrf52dk_base::setup_board() function taking even more arguments and being too hard to maintain.

[bradjc]

Let's just delete nrf52dk_base and use components instead. I will try to help.

[hudson-ayers]

I created #1839 to organize progress towards deleting nrf52dk_base. Feel free to make edits or self-assign portions.

I vote that we either close this PR or only merge the README changes, and then iterate on #1839 instead of worrying about the names.

[ppannuto]

I've updated this to be just the README changes.