Skip to content
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

SpiDevice's interface can't be used for streaming transactions #583

Open
sky-dragn opened this issue Mar 28, 2024 · 8 comments
Open

SpiDevice's interface can't be used for streaming transactions #583

sky-dragn opened this issue Mar 28, 2024 · 8 comments

Comments

@sky-dragn
Copy link

There are applications where, when writing data to an SPI device, it is more efficient to write the data in a single transaction rather than multiple transactions (my use case is an SPI screen that can write multiple lines in a single transaction).

However, with the current SpiDevice interface, I would need to create a slice of Operations in order to be able to write all the lines in a single transaction. Because I need a new Operation for each separate line, this requires 128 lines * 8 bytes per slice = 1KiB of additional data just for the pointers (the framebuffer itself is only 2 KiB!). The chip I am targeting only has 40KiB of memory total, so this is a substantial amount of overhead.

The interface is now stabilized because the crate is at 1.0, but is there some change that can be made to improve this situation? (e.g. an interface that takes an iterator of Operations, a closure-based interface, or something similar)

@codertao
Copy link

codertao commented Apr 2, 2024

+1 ; though from a bit of research I think this might have been a design decision at some point. (the original interface seems to have been closure based, and changed in #443 ; but I haven't finished reading the history on it ) (edit: #478 seems to have a lot of discussion on this topic in it)

The lack of some way to acquire CS leads to "interesting" solutions, like in the embedded-sdmmc crate: SdCard relies on an SpiDevice, and a separate CS pin, because it needs to perform multiple and variable transactions while the CS pin is asserted.

For one project I have SPI Devices (custom SPI slave) that take a command, process it, and while processing will generate a 'busy' byte until the response is ready, and then play the response, all within a single CS assertion. That interaction isn't allowed with the current SpiDevice trait.

@Rahix
Copy link

Rahix commented Apr 2, 2024

The problem is that not all SPI implementations support this. Notably, I think the Linux HAL implementation runs into this problem.

The traits here in embedded-hal have to be the common denominator so that's why the decision was made to require prepared operations.

However, keep in mind that individual HAL implementations are free to add additional APIs to expose such features. You won't get a HAL generic interface from this, but when writing an application, this may not even be necessary.


If embedded-hal wants to also start supporting streaming via its traits, I guess additional traits next to SpiDevice/SpiBus could facilitate this. Something like a StreamingSpiDevice/StreamingSpiBus. Then drivers which explicitly require the streaming APIs can depend on these extended traits instead.

@Dirbaio
Copy link
Member

Dirbaio commented Apr 2, 2024

The lack of some way to acquire CS leads to "interesting" solutions, like in the embedded-sdmmc crate: SdCard relies on an SpiDevice, and a separate CS pin, because it needs to perform multiple and variable transactions while the CS pin is asserted.

for anyone reading this: don't do this, it breaks bus sharing. issue filed.

@codertao
Copy link

codertao commented Apr 2, 2024 via email

@sky-dragn
Copy link
Author

In the meantime, does it seem like a reasonable stop-gap for my project to handle SPI bus sharing by just passing the &'static Mutex<RefCell<SpiBus>> around? This would allow for the more complicated transactions I need. It would also require a modification of embedded-sdmmc, but a change there is necessary anyway because it's currently unsound.

@adamgreig
Copy link
Member

In the meantime, does it seem like a reasonable stop-gap for my project to handle SPI bus sharing by just passing the &'static Mutex<RefCell<SpiBus>> around? This would allow for the more complicated transactions I need. It would also require a modification of embedded-sdmmc, but a change there is necessary anyway because it's currently unsound.

Yep, that's an expected way to share SpiBus while getting exclusive bus access to handle your own CS or other non-SpiDevice communications. We'd like to write it up along with other options for sharing SpiBus soon.

@Vexs
Copy link

Vexs commented Feb 19, 2025

This one hit me yesterday- the sharp memory display allows you to write multiple lines at once but it requires CS to be held high for the entire transaction.. The sharp memory display is somewhat notable in that it requires the device to remember the state of the screen, as you can't write single pixels- only entire rows, or multiple rows.
Writing multiple rows involves a transaction like:

CS_HIGH
first: [writecommand, linenumber, ....pixels, 0x00]
intermediates: [linenumber, ...pixels, 0x00]
last: [linenumber, ...pixels, 0x00, 0x00]
CS_LOW

If CS is dropped, the transaction is ended. If you do multiple single line writes very quickly (ie, just the first with two 0x00), things get funny, due to timing on the screen( maybe?).

With the current SPIDevice implementation, this becomes grossly complicated. I have to either make an array of Operation::write()s for each line and feed them to a transaction (I gave up on this) or I can populate a single array with every command I want to send to the screen, and then .write() it in one go.

With the small screen size (400x240) px, or ~12000 bytes (with command sequences) this is fairly manageable, but I can imagine this becoming a problem- it's also just kind of annoying to write. Just being able to leave a "transaction" "open" surrounding a loop where I could do arbitrary writes would make this significantly easier.

An existing implementation for these displays takes the route of not having any locks at all by using spibus.

@sky-dragn
Copy link
Author

The Sharp Memory Display protocol was actually exactly what prompted me opening this issue in the first place :)

The issue we encountered was that the buffer of Operations which was needed in order to write the entire screen became similar in size to the actual framebuffer which we were attempting to write to the screen, which on our system was more memory than we could afford (and also just plain silly). Our solution was unfortunately to port the entire codebase back to embedded-hal 0.2, and just do our own bus management to prevent conflicts. You can see our display driver implementation here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants