Implement embedded_hal_async::spi

[newAM]

This is a draft because I wanted to get feedback on the implementation before filling out the tests.

[ryankurte]

hmm, seems good to me! i wonder whether it'd be useful to share the expectations for sync / async so when you're testing drivers if you were to have both interfaces you could use the same set of vectors? or alternately, whether there's a particular advantage to having separate sync / async expectations?

[newAM]

i wonder whether it'd be useful to share the expectations for sync / async so when you're testing drivers if you were to have both interfaces you could use the same set of vectors? or alternately, whether there's a particular advantage to having separate sync / async expectations?

This was one of the design decisions I wasn't sure about, I can see arguments for both, being specific vs having portable testcases. Do you have a preference towards one or the other? I'm leaning more more towards shared expectations now that I think about it more, but I don't feel strongly about it.

[ryankurte]

I'm leaning more more towards shared expectations now that I think about it more, but I don't feel strongly about it.

i think same, it'd be nice to be able to run the same tests for both (if we end up having to do this in drivers), and if we need to be able to describe async expectations specifically we can probably expand that later

[newAM]

This is ready now, if anyone wants to see it in action I have doctests for bme280-multibus

[ryankurte]

looks good to me! imo sometimes it's nice to have an explicit [feature] to make it more obvious when you're looking at the manifest trying to work out what you can enable / disable.

[dbrgn]

@newAM thanks for your contribution!

looks good to me! imo sometimes it's nice to have an explicit [feature] to make it more obvious when you're looking at the manifest trying to work out what you can enable / disable.

I agree, that would be good. Could be used explicitly in CI with --features.

@ryankurte I don't really have much experience with SPI. Feel free to merge if you think the PR looks OK (once the CI issue is resolved)!

[newAM]

looks good to me! imo sometimes it's nice to have an explicit [feature] to make it more obvious when you're looking at the manifest trying to work out what you can enable / disable.

Good point, that is handy! Done.

[dbrgn]

@newAM can you rebase against master?

[newAM]

@newAM can you rebase against master?

I pushed this, and also squashed the commits here down to a single commit. It's going to look weird on GitHub because it rebases all commits made to 1-alpha. Will probably require a force push from the command line when it is time to merge.

[newAM]

I pushed this, and also squashed the commits here down to a single commit. It's going to look weird on GitHub because it rebases all commits made to 1-alpha. Will probably require a force push from the command line when it is time to merge.

I went to use this again and noticed the rebase took some changes from master that it should not have. It would probably be best to rebase or merge 1-alpha and master in a separate PR, could also do that after this PR.

For now I reverted this to just the commit to add async SPI.

[elpiel]

Any chance to get this PR going?

[newAM]

Another option is that I could add support for both the 0.2 HAL and the 1.0 alpha HAL in the same branch, removing the need for separate master/1-alpha branches. Happy to implement any of these pending feedback!

[elpiel]

Sound good to me 🙌
I've already seen it in your w5500 crate and I like the approach.

[dbrgn]

Any chance to get this PR going?

Sorry for the delay, this fell off my radar. I'm quite busy at the moment (planning to move to a new place), but will try to take a look at the PR soon-ish. If I forget about it, don't hesitate to post a reminder comment 🙂

[elpiel]

Is not a good time for a reminder @dbrgn ? 😋
I'm still looking to use this impl for test when ready.

[newAM]

This should probably go in after #66 so I can update to the latest async HAL

[dbrgn]

Is not a good time for a reminder @dbrgn ? 😋

It is 😄

This should probably go in after #66 so I can update to the latest async HAL

@newAM #66 is merged, so you can update your branch!

[newAM]

Should be good to go now.

I also pinned nightly to 2023-04-10 since there's an ICE in newer nightlies at the moment.

[elpiel]

Thank you for updating the PR @newAM

[dbrgn]

Finally, thanks for the patience!

I have barely any experience with SPI and none with embedded-hal-async, but the code looks good overall and both @ryankurte and @elpiel gave positive feedback, so let's get this merged 🙂