Skip to content

Bit banging SPI implementation #42

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

Closed
maikelwever opened this issue Feb 22, 2018 · 6 comments
Closed

Bit banging SPI implementation #42

maikelwever opened this issue Feb 22, 2018 · 6 comments

Comments

@maikelwever
Copy link

For my MAX7219 crate I've translated the shiftOut function from the Arduino platform to rust, in order to aid with bit-banged serial transfer.

As proposed at rust-embedded/wg#39 (comment), it might be an idea to move this function to the embedded-hal crate.

I am willing to send a PR to move this to the embedded-hal crate, and extend the implementation to be on par with the Arduino one (https://github.com/arduino/Arduino/blob/master/hardware/arduino/avr/cores/arduino/wiring_shift.c#L40).

But before I make the PR, I'd like to check if there are any objections/ideas/comments regarding this, so here's what I was thinking:

  • Be on par with the Arduino implementation (MSB+LSB support).
  • Renaming the function to shift_out instead of shiftOut.
  • Moving it to the spi.rs file here in embedded-hal, so it's importable like this: use embedded_hal::spi::shift_out.
  • Implementing the shiftIn function is for another time and PR.
  • Making clear in the documentation comments that this is a bit-banged implementation.

For reference, my current implementation lives here: https://github.com/maikelwever/max7219/blob/master/src/lib.rs#L127

@austinglaser
Copy link
Contributor

I love the idea of having bit-banged implementations of common serial protocols -- but does it make sense in this this crate? My understanding is that embedded-hal's purpose is to define an abstraction layer, not to implement anything directly. I'd definitely favor a separate bit-bang-hal crate that implements the protocol, and possible others (like I2C) in the future.

I also think the API bears careful thinking on. Instead of a standalone function, it should probably be a type implementing the embedded_hal::spi::FullDuplex trait. Making it a function forces driver developers to choose between supporting a bit-banged or hardware-based implementation -- which should really be a choice made when putting together an application.

Perhaps something like bit_bang_hal::spi::Spi?

@japaric
Copy link
Member

japaric commented Mar 10, 2018

I'd definitely favor a separate bit-bang-hal crate that implements the protocol, and possible others (like I2C) in the future.

👍 though I would probably put embedded-hal in the name. Alternatively, it could be one of the keywords of the crate.

it should probably be a type implementing the embedded_hal::spi::FullDuplex trait.

I also think it should be a (generic) type implementing the SPI trait(s). Then it can be used with SPI based driver crates.

@japaric japaric mentioned this issue Mar 10, 2018
@Kixunil
Copy link

Kixunil commented Mar 13, 2018

I've proposed Wave generation trait. Could this be implemented on top of that trait or should it be different?

Obviously, receiving end will need something else, but it doesn't conflict with transmitting.

@sajattack
Copy link

@eldruin
Copy link
Member

eldruin commented Jul 15, 2020

As stated above, there is a working bitbanging SPI implementation in the bitbang-hal.
I think @sajattack will probably welcome any improvements.
@maikelwever can this issue be closed?

@maikelwever
Copy link
Author

It's been a while for me, but I think this issue can be closed indeed.

peckpeck pushed a commit to peckpeck/embedded-hal that referenced this issue Nov 10, 2022
42: Update gpio-cdev and add MSRV r=posborne a=eldruin

Using `gpio-cdev` 0.3 we have a MSRV of 1.36.0 due to `nix`.
This superseeds rust-embedded#41 

Co-authored-by: Diego Barrios Romero <[email protected]>
peckpeck pushed a commit to peckpeck/embedded-hal that referenced this issue Nov 10, 2022
The README.md was previously updated due to the dependency on `nix` with PR rust-embedded#42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants