Spi: Move SPI buffer size into SPI capsule

[v-thakkar]

Pull Request Overview

This pull request moves the SPI buffer size into capsule from the component, thus addressing the solution for #1462 .

Testing Strategy

This pull request was tested by make allcheck and make allboards, no device testing.

TODO or Help Wanted

Should be fine.

Documentation Updated

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

Formatting

• Ran make formatall.

[hudson-ayers]

So this is definitely the fix that #1462 was asking for, but looking at it now I am not sure about the philosophy behind putting buffers in the capsule instead of the component. For capsules where a specific size buffer is required, it makes sense. But the SPI syscall capsule already has machinery to accept different buffer sizes (config_buffers() checks the length of each passed buffer, for example), and these buffer sizes are big enough that it seems possible more limited boards could want to opt for something smaller -- like the Hail used to do.

If the buffers are in the component, any board wishing to choose a different buffer size can simply forego use of the component and declare its own buffers and initialize the component itself. If the buffers are in the capsule this isn't an option (unless rustc is smart enough to exclude the unused Capsule buffers in this case? either way it is less clear it is an option).

[bradjc]

I think the reason that new() functions in capsules require a buffer to be passed in is this provides the flexibility if a board wants to use a different buffer size. Specifying the buffer in the capsule is just a convenience mechanism.

The RTT capsule recently changed to move the buffer out of the capsule, and instead includes the buffer lengths in the capsule:

tock/capsules/src/segger_rtt.rs

Lines 101 to 105 in cfa0757

	/// Suggested length for the up buffer to pass to the Segger RTT capsule.
	pub const DEFAULT_UP_BUFFER_LENGTH: usize = 1024;
	/// Suggested length for the down buffer to pass to the Segger RTT capsule.
	pub const DEFAULT_DOWN_BUFFER_LENGTH: usize = 32;

That way the buffer can be created in a component or anywhere else. However, what I think is important is having the "suggested length" specified directly in the capsule as a starting point for board customization. While I agree many capsules will support buffers of any length, the capsule author probably knows what a "reasonable" size is given the use case, or a length that might result in less overhead. I think this should still be recorded in the capsule source directly. If it's recorded at the /boards layer, that can lead to confusion if two different boards use two different values. Which is the suggested value, and which is customized? What should a new board use by default?

[hudson-ayers]

I am in favor of SPI moving to the approach used by the RTT capsule then -- declaring the preferred lengths in the capsule makes a lot of sense, but I think putting the actual buffer in the component (or individual boards main.rs as needed) is preferable.

[v-thakkar]

Thanks for the reviews and detailed discussion. I was wondering then if in this case, should I just go for declaring preferred length in the SPI capsule and then using it in the component as it been done in RTT component? Also, does it makes sense to change the buffer size in Hail - back to 64-byte? [That can also provide an example of how it can be done at individual board level]

[hudson-ayers]

I was wondering then if in this case, should I just go for declaring preferred length in the SPI capsule and then using it in the component as it been done in RTT component?

That would be great, thank you.

Also, does it makes sense to change the buffer size in Hail - back to 64-byte? [That can also provide an example of how it can be done at individual board level]

I think it is fine for Hail to still use the buffer in the component (and stay at the default length specified by the capsule).

[hudson-ayers]

This looks good! Just one change leftover from the original approach

[hudson-ayers]

There should be no need for these buffers now that you have reverted to initializing in the component.

[v-thakkar]

Oops, of course. Sorry, I missed it in this patch while jumping between branches in the local repo. Thanks for notifying.

[bradjc]

bors r+

[bors]

This PR was included in a batch that successfully built, but then failed to merge into master (it was a non-fast-forward update). It will be automatically retried.