-
Notifications
You must be signed in to change notification settings - Fork 221
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
Stable driver: SPI master #2494
Comments
What exactly does this mean, we're only aiming at stabilizing |
Start small, and incrementally stabilize. It's easier to stabilize Remember folks can always opt in to (I just realized I basically summerised what I wrote in #2491, in particular the "Can the driver as a whole be stabilized" section.) |
Hello, I would prefer that |
Does |
Just noting that we could provide a What I'm unsure about, is, whether we have to provide any chip select functionality for the bus driver in the first place. |
I think so, otherwise how do people select which device they're talking to on the bus. |
With a GPIO, as they already do when using |
Ah I see. Fwiw, when I created the issue asking to expose the CS signals, I meant adding it to |
They are still bus drivers, not device drivers. The busses shouldn't care much about chip select, that's the responsibility of the device drivers. Whether the chip select ends up being implemented as a hardware signal or a software GPIO write, isn't something the bus driver have to know about. For single transfers, there is essentially no difference (you still need to write a register to select a particular chip select and make sure that chunking doesn't deassert it), except with hardware chip select, you have less flexibility in the CS-to-SCLK delay. |
Hmm, I guess I just want an inherent GSPI-2/3 driver without the connotations of "bus" and "device", which will let me select a CS pin and create my own "device"s. Also on the DMA side, whilst the |
I think the best resolution for this conversation is that we don't stabilize anything CS-related initially, and come up with a solution that everyone likes. |
As part of #2491, which has more details on driver analysis.
Note: we are not stabilizing DMA support right now.
esp-hal API-GUIDELINE omissions
Construction
into_async
andinto_blocking
should be available ontyped
instances too, currently its only implemented onSpi<'d, Blocking, AnySpi>
/Spi<'d, Async, AnySpi>
. #2673Interop
- Something like embassy-hal spi bus has
- Related to hardware cs management item below
- Note from danielb: device shouldn't be implemented for bus driver structs.
embassy_embedded_hal::SetConfig
impl needs to be marked unstable #2675Debug
/Format
missing on the driversSpi
#2678PartialEq/Eq
#2791Hash
#2792API surface
Info
struct? - I think we probably want to sit on this a bit longer before exposing it as a stable API#[non_exaustive]
missing on Half duplex enumsCommand
,Address
#2679#[non_exaustive]
missing onesp_hal::spi::Error
#2680assert
(and its derivatives) macro. I believe we can avoid it (not talking aboutdebug
ones though)Documentation
Maintainability
cfg_if!
over MULTIPLE exclusive#[cfg]
attributes (some of below snippets are not public, but still would be good to make them more maintainable)Driver Implementation
State
struct is "yet-to-be-created" - changed in SPI: Implement interrupt-driventransfer_in_place_async
#2691Rust API guideline omissions
PartialEq
andEq
onspi::master::Config
(SPI: Driver structs don't derivePartialEq/Eq
#2791)C-QUESTION-MARK
: Examples use?
, nottry!
, notunwrap
.C-FAILURE
: Function docs include error, panic, and safety considerations. (again, good inSafety
, butErrors
andPanics
are missing. #2794C-CASE
:SpiBitOrder
variants should beMsbFirst
andLsbFirst
Hardware feature omissions
0-2
for most Instances/chips but some support0-5
with_cs1
,with_cs2
etcExpose bit ordering for tx and rxread/write/transfer_bits
on devices that support it. (C6:To transfer bits that is not an integral multiple of byte (8 bits), consider implementing it in CMD state or ADDR state.
)Mode/features of the driver that are lacking
Misc
spi::master::Config uses HertzU32 etc from fugit, which aren't stable #2804
SpiBitOrder, SpiDataMode, SpiMode: the Spi prefix seems redundant #2805
Variants of
Address
andCommand
could be changed into s.th. likeBits1
,Bits2
or similar to avoid repeating the enum name #2806Should we get rid of nb::Result for inherent functions? #2807
Do we want the single byte read/write inherent functions? or are they optimized for the job and are much more efficient? is it a common use case? #2808
SPI: We already started marking parts of the driver as unstable but there are some inconsistencies currently #2793
half_duplex_XXX
is marked unstable inSpi
but not in other places (i.e.SpiDmaXXX
)with_sio2
/with_sio3
is not marked unstable - but ifhalf_duplex_XXX
is unstable these should be marked, tooSpi::with_dma
is marked unstable but notSpiDmaXXX
The text was updated successfully, but these errors were encountered: