<re_irc>
<@shakencodes:matrix.org> Thanks a lot! Your suggestion worked perfectly! (Had a failing integration test ahead of this, so it was easy to verify!)
explore has left #rust-embedded [#rust-embedded]
lehmrob has joined #rust-embedded
lehmrob has quit [Ping timeout: 248 seconds]
IlPalazzo-ojiisa has joined #rust-embedded
lehmrob has joined #rust-embedded
IlPalazzo-ojiisa has quit [Quit: Leaving.]
IlPalazzo-ojiisa has joined #rust-embedded
<re_irc>
<@amb12:matrix.org> I'm considering an SPI device driver, but there needs to be a minimum delay of a few microseconds between the first and subsequent words in the transaction. How would I achieve this within an SpiDevice transaction? Or wouldn't I?
<re_irc>
<@ryan-summers:matrix.org> I don't think you can do that with just the E-H traits. You'd have to do it as two transactions
<re_irc>
<@ryan-summers:matrix.org> If this isn't a hardware-agnostic driver, you can set up your SPI implementation to natively do that for you
<re_irc>
<@amb12:matrix.org> It would be nice if I could make it hardware agnostic, by performing the first transfer, then a (short) delay, then the rest. But it all needs to happen within a single assertion of the CS line, which I guess makes it a bit unusual.
<re_irc>
<@ryan-summers:matrix.org> Yeah, I've never seen a chip require that before. Mind if I ask what IC this is for?
<re_irc>
<@dirbaio:matrix.org> yes, this is not possible with the new SpiDevice :(
<re_irc>
<@dirbaio:matrix.org> it was possible with the previous closure-based SpiDevice
<re_irc>
<@dirbaio:matrix.org> you're the 2nd person to ask about it. We didn't think chips that required such delays existed, or were so common :D
<re_irc>
<@dirbaio:matrix.org> so perhaps we should switch back to the closure-based API
<re_irc>
<@dirbaio:matrix.org> or add "Operation::Delay(us)" or something 😓
<re_irc>
<@dirbaio:matrix.org> which chip is it, amb12 ?
<re_irc>
<@rmja:matrix.org> One more thing that the new api does not support:
<re_irc>
- Read one length byte = n
<re_irc>
- Assert CS
<re_irc>
- Read n bytes
<re_irc>
- De-assert CS
<re_irc>
<@dirbaio:matrix.org> do you have an example of a chip that requires that rmja ?
<re_irc>
<@rmja:matrix.org> ti cc1200
<re_irc>
<@ryan-summers:matrix.org> Does that operate like the cc1010 where the length byte is the length of the received packet?
<re_irc>
<@ryan-summers:matrix.org> If so, can't you do that in two SPI ops?
<re_irc>
<@ryan-summers:matrix.org> cc1101*
<re_irc>
<@rmja:matrix.org> Yes, it is the same, and that it what I have done.
<re_irc>
<@rmja:matrix.org> But it also has the delay requirement after hardware reset, where we need to wait for the xtal to stabilize
<re_irc>
<@rmja:matrix.org> That wait has to be within a spi transaction
<re_irc>
<@ryan-summers:matrix.org> So I guess the main desire of the E-H is to work around things that are required for normal operation. Forcing an insertion of a CS assert/deassert isn't the worst thing in the world IMO
<re_irc>
<@ryan-summers:matrix.org> Huh, that's weird, but makes sense given what I know about the cc1101
<re_irc>
<@ryan-summers:matrix.org> The CC radios are super funky to work with when compared to SPI sensors etc
<re_irc>
<@rmja:matrix.org> Basically you have to wait for the MISO pin to go low
<re_irc>
<@ryan-summers:matrix.org> Ah I see the issue. So the reset can't be done with a hardware agnostic driver. It could only be done at the end-user perspective where they can manually fiddle with SPI lines
<re_irc>
<@ryan-summers:matrix.org> But that's generally not a SPI transaction, so I don't know if the E-H is intended to support that
<re_irc>
<@rmja:matrix.org> It was supported before, as I was basically making a loop inside the closure, reading one byte at a time, and waiting for it to report 0x00
<re_irc>
<@ryan-summers:matrix.org> Theoretically, you could expose a static method to perform the reset that takes in the CS and SO as Gpios, but yeah, that's annoying
<re_irc>
<@ryan-summers:matrix.org> Hmm, good point
<re_irc>
<@dirbaio:matrix.org> between transfers in a single transaction (single CS assert)
<re_irc>
<@dirbaio:matrix.org> so perhaps adding a "Operation::Delay(us)" is the way to go?
<re_irc>
<@dirbaio:matrix.org> I'd really rather not go back to the closure API 😅
<re_irc>
<@rmja:matrix.org> It will work for me. It was possible for circumvent the "wait for xtal issue" by simply probing the status register over spi
<re_irc>
<@dirbaio:matrix.org> the "wait for xtal" requires waiting while CS is asserted?
<re_irc>
<@rmja:matrix.org> Year
<re_irc>
<@rmja:matrix.org> (as far as I can understand from the user guide)
<re_irc>
<@dirbaio:matrix.org> so you're instead polling the status register without deasserting CS?
<re_irc>
<@rmja:matrix.org> assert->read sr->deassert until it reports a stable xtal
<re_irc>
<@rmja:matrix.org> it seems to work
<re_irc>
<@ryan-summers:matrix.org> Tbh those radios likely don't care if CS is asserted or not
<re_irc>
<@ryan-summers:matrix.org> the XTAL is a completely independent part of the hardware from SPI and those CC processes probably have a whole processor doing management inside
<re_irc>
<@ryan-summers:matrix.org> * processors
<re_irc>
<@ryan-summers:matrix.org> * chips
<re_irc>
<@ryan-summers:matrix.org> There's a pretty complex radio state machine being managed
<re_irc>
<@dirbaio:matrix.org> rmja: so you do deassert CS
<re_irc>
<@rmja:matrix.org> yes
<re_irc>
<@dirbaio:matrix.org> so probably with a delay-based wait it's fine to deassert CS too
<re_irc>
<@dirbaio:matrix.org> assert, reset, deassert, wait, do more things
<re_irc>
<@dirbaio:matrix.org> ah wtf you have to use MISO as GPIO
<re_irc>
<@dirbaio:matrix.org> okay
<re_irc>
<@dirbaio:matrix.org> that's cursed
<re_irc>
<@ryan-summers:matrix.org> It's the init process before you're even setting up SPI though
<re_irc>
<@dirbaio:matrix.org> and I guess MISO doesn't go down if you deassert CS
<re_irc>
<@whitequark:matrix.org> a friend once had to program an FPGA whose config pins were connected to the DRAM controller
<re_irc>
<@whitequark:matrix.org> and you couldn't use the DRAM pins on that SoC as GPIO
<re_irc>
<@rmja:matrix.org> Dont know. The way they do it is that the xtal ready bit in the status register is the first bit clocked when we are making a transition on the clock line.
<re_irc>
<@rmja:matrix.org> Because the chip ready bit is the most significant bit in that byte
<re_irc>
<@rmja:matrix.org> So it is actually compatible
<re_irc>
<@rmja:matrix.org> See section 3.1.2 in the guide
<re_irc>
<@rmja:matrix.org> That bit 7 CHIP_RDYn is always the first bit transmitted by the chip in any spi transaction
<re_irc>
<@rmja:matrix.org> And I think that is what they describe in section 9.1.1
<re_irc>
<@rmja:matrix.org> It just never changes because they dont clock any spi bytes in that figure
<re_irc>
<@whitequark:matrix.org> she got it to work btw
<re_irc>
<@dirbaio:matrix.org> bitbanging it by poking the right addresses in the memory bus? :o
<re_irc>
<@adamgreig:matrix.org> hi @room, meeting time again! agenda is https://hackmd.io/k-YOuWJEQyy4qws760Esow, please add anything you'd like to announce or discuss, we'll start in about 5 min
<re_irc>
<@thejpster:matrix.org> Going to miss this one. At a work dinner.
<re_irc>
<@adamgreig:matrix.org> enjoy!
<re_irc>
<@adamgreig:matrix.org> ok, let's start! I don't have any announcements from last week, does anyone else?
<re_irc>
<@adamgreig:matrix.org> oh, I guess the translation is sort of an announcement, someone has added a start at a portugese translation of the disco book at https://allynaell.github.io/discovery/
<re_irc>
<@adamgreig:matrix.org> otherwise just a couple of points leftover from last week, first up is the bors retirement
<re_irc>
<@adamgreig:matrix.org> since then, bors has updated to leave a message in every comment about how it's soon shutting down, so we might not have as much grace time as it seemed last week
<re_irc>
<@adamgreig:matrix.org> though still no deadline I don't think
<re_irc>
<@adamgreig:matrix.org> my feeling is still to try out ghmq on some repo (e-h? c-m?) to see what else we run into - already found something that might catch us up where we have one ci job to batch together lots for bors to check - and then go from there
<re_irc>
<@adamgreig:matrix.org> has anyone else had any new thoughts since?
<re_irc>
<@therealprof:matrix.org> : Maybe is in the mood of reviewing that? 😅
<re_irc>
<@therealprof:matrix.org> : We can certainly try it on the WG repo, too.
<re_irc>
<@adamgreig:matrix.org> to be honest I dunno if the wg repo really needs bors, the ci is a no-op after all
<re_irc>
<@therealprof:matrix.org> It is, that's exactly why I think it's a good repo to test new workflows and migrating the rules, etc. 😉
<re_irc>
<@therealprof:matrix.org> Not a lot of breakage expected and no one will complain about dummy changes just to try things out.
<re_irc>
<@adamgreig:matrix.org> fair point, I'll enable it and we can test it for the minutes pr today, but I should hope this one is easy at least :P
<re_irc>
<@therealprof:matrix.org> There's a few bits to touch, branch protection etc.
<re_irc>
<@adamgreig:matrix.org> yea
<re_irc>
<@adamgreig:matrix.org> ok, i'll try it for this evening's pr as a first step
<re_irc>
<@romancardenas:matrix.org> I've been reading a bit about ghmq and seems fine to me. Couldn't try it, though
<re_irc>
<@romancardenas:matrix.org> Maybe we could set it in riscv and see how it behaves
<re_irc>
<@andre-richter:matrix.org> I would also like to raise attention again to look for a follow-up maintainer for aarch64-cpu. It got a couple of PRs lately that haven’t resulted in a new release on crates.io, and there is a nice new pending PR from AWS people.
<re_irc>
<@adamgreig:matrix.org> maybe I should open an issue on the repo about that too?
<re_irc>
<@andre-richter:matrix.org> Yeah maybe tag the AWS guys if they would like to jump in, they contributed some lately. 👍
<re_irc>
<@adamgreig:matrix.org> other point I had on the agenda is about cortex-m-rt and making HardFaultTrampoline optional, a few people have asked for this over the years and it seems like it might be a bit easier to support now we have inline asm rather than pre-built blobs
<re_irc>
<@adamgreig:matrix.org> I just mention it now in case anyone is interested in giving it a go :P
<re_irc>
<@diondokter:matrix.org> : Could be a nice hackday project. Got one coming up next week Fridays again. Is there an issue for it? Might try my hand at it if nobody's picked it up by then
<re_irc>
<@adamgreig:matrix.org> that's everything I had on the agenda, does anyone want to discuss anything else today?
<re_irc>
<@therealprof:matrix.org> e-h release when? 😅 Seems it's popping up again and again and today there was a discussion of the new SPI traits maybe being a bit too limiting?
<re_irc>
<@dirbaio:matrix.org> yeah, two people have asked for SpiDevice to support delays
<re_irc>
<@adamgreig:matrix.org> maybe it's enough to add a Delay operation... it seems like most HALs and Linux should be able to implement it
<re_irc>
<@adamgreig:matrix.org> a bit gross since it does raise the "time" spectre but I think just u32 microseconds ought to please everyone for this
<re_irc>
<@adamgreig:matrix.org> it still doesn't allow "read one byte, then read that byte's value worth of bytes" operations though
<re_irc>
<@dirbaio:matrix.org> : Neither does linux spidev, I think that's fine
<re_irc>
<@eldruin:matrix.org> time... 😱
<re_irc>
<@dirbaio:matrix.org> I imagine this will trigger bikeshedding about "but now the HAL SPI needs to own a hardware timer!"
<re_irc>
<@dirbaio:matrix.org> for HALs that don't have something like "embassy-time" :S
<re_irc>
<@adamgreig:matrix.org> hmmm yea...
<re_irc>
<@adamgreig:matrix.org> things like spi flash do a better job of this by just having dummy bytes in the spi protocol where a delay might be needed
<re_irc>
<@adamgreig:matrix.org> very inconsiderate of these weird devices to require a delay :P
<re_irc>
<@dirbaio:matrix.org> yeah it sucks
<re_irc>
<@adamgreig:matrix.org> though if it also requires you to toggle mosi like a gpio then it's a bit beyond our reach anyway
<re_irc>
<@adamgreig:matrix.org> good point though, for a lot of hals there's no immediately great way to do the delay. hmm
<re_irc>
<@adamgreig:matrix.org> i guess they'll hopefully know what the clock speed is and can just do a nop loop or something in the worst case
<re_irc>
<@adamgreig:matrix.org> still :/
<re_irc>
<@eldruin:matrix.org> then comes the issue of not knowing how long nop takes even within one arch (cf CM4 vs CM7 or so)
<re_irc>
<@dirbaio:matrix.org> HALs know the arch though
<re_irc>
<@adamgreig:matrix.org> that's ok on a hal though right?
<re_irc>
<@dirbaio:matrix.org> and the clock speed
<re_irc>
<@dirbaio:matrix.org> so "nop loop" is quite viable, yes
<re_irc>
<@thalesfragoso:matrix.org> : Nice, I wasn't awared of that. I can definitely take a look and help out.
<re_irc>
<@adamgreig:matrix.org> presumably the spec will have to be "delays at least this long, maybe longer"
<re_irc>
<@eldruin:matrix.org> yeah, unclear if that works for the users though
<re_irc>
<@eldruin:matrix.org> maybe it does?
<re_irc>
<@dirbaio:matrix.org> these delays are usually for "wait for X to be ready", if you wait too much is fine
<re_irc>
<@juliand:fehler-in-der-matrix.de> : Would be feature-gating it an option?
<re_irc>
HALs that are unable to provide a timer then just can't be used with drivers that use e-h with that SPI delay feature enabled. But that introduces other weirdness then, I presume...
<re_irc>
<@adamgreig:matrix.org> the other option is to not support delays and continue to consider adding the closure api back in some time later for those advanced use cases
<re_irc>
<@dirbaio:matrix.org> you can't easily feature-gate part of a trait
<re_irc>
<@adamgreig:matrix.org> but it's a bit meh
<re_irc>
<@dirbaio:matrix.org> the closure API was a pain
<re_irc>
<@dirbaio:matrix.org> and having two different APIs for the same thing is a pain too
<re_irc>
<@juliand:fehler-in-der-matrix.de> : I didn't say it would be easy ;p
<re_irc>
<@adamgreig:matrix.org> the other way out is for those drivers to use SpiBus and a GPIO CS instead, too, I suppose
<re_irc>
<@dirbaio:matrix.org> I think adding "Operation::DelayUs" is the right tradeoff
<re_irc>
<@dirbaio:matrix.org> apparently it's common enough that Linux decided to suppor tit
<re_irc>
<@dirbaio:matrix.org> * support it
<re_irc>
<@dirbaio:matrix.org> it seems like we're slowly converging with the linux api lol
<re_irc>
<@adamgreig:matrix.org> I wonder if there could be some way to support sharing an spibus where drivers can lock and unlock it but handle their own cs
<re_irc>
<@dirbaio:matrix.org> that's just a "Mutex<SpiBus>".. :D
<re_irc>
<@adamgreig:matrix.org> yea, I guess it is
<re_irc>
<@adamgreig:matrix.org> wonder how annoying that would be to share between some things wanting SpiDevice and one driver wanting SpiBus to do these annoying things with
<re_irc>
<@dirbaio:matrix.org> except mutex can be a std mutex, a refcell, a "critical_section::Mutex<RefCell<T>>"
<re_irc>
<@grantm11235:matrix.org> And you still can't "lock" an spi bus on linux
<re_irc>
<@adamgreig:matrix.org> sure, but I think we'll probably end up with SpiBus structs on linux where you have to pinky promise no other spidev on the same bus gets used at the same time
<re_irc>
<@adamgreig:matrix.org> since all the crates to drive ws2812s over spi will want SpiBus and people will want to use them from linux, so
<re_irc>
<@adamgreig:matrix.org> (and once you have such an spibus you can still use normal gpio as cs for things, if they're not already hard bound to be spidev cs, and plug them into whatever shared-bus provider you're using)
<re_irc>
<@dirbaio:matrix.org> that's the "SPI abuse" case in the docs
<re_irc>
<@dirbaio:matrix.org> that doesn't allow sharing anyway, so it's fine to use SpiBus
<re_irc>
<@dirbaio:matrix.org> for everything else, the current SpiDevice is perfectly implementable on Linux
<re_irc>
<@dirbaio:matrix.org> even if we add Delay
<re_irc>
<@dirbaio:matrix.org> perfectly implementable on Linux with kernel-managed SC
<re_irc>
<@dirbaio:matrix.org> * CS
<re_irc>
<@adamgreig:matrix.org> right, that was one of the points of the current design after all
<re_irc>
<@adamgreig:matrix.org> linux will have a very easy time adding Delay, maybe other hals less so, which is why I wonder how much we need it if the few devices that want it could possibly use spibus
<re_irc>
<@therealprof:matrix.org> : Well, the ability to drive WS2812s with SPI is a lucky win anyway...
<re_irc>
<@dirbaio:matrix.org> there's a fun issue
<re_irc>
<@dirbaio:matrix.org> the operation list is not actually handled by the HAL
<re_irc>
<@adamgreig:matrix.org> oh yea, but by the sharing thing
<re_irc>
<@adamgreig:matrix.org> so I guess it will want to consume a DelayUs instance or something alongside the SpiBus
<re_irc>
<@adamgreig:matrix.org> kinda annoying
<re_irc>
<@dirbaio:matrix.org> (yes, HALs can impl SpiDevice, but most shouldn't have to)
<re_irc>
<@dirbaio:matrix.org> yeah, it's one more generic param
<re_irc>
<@halfbit:matrix.org> Delay is kind of an issue in itself...
<re_irc>
<@halfbit:matrix.org> what is a long delay for one part, a lot of work could be done on another
<re_irc>
<@adamgreig:matrix.org> and means everyone using spi anywhere needs to create a DelayUs to pass in to their spidevice provider
<re_irc>
<@dirbaio:matrix.org> yeah
<re_irc>
<@dirbaio:matrix.org> even if the driver doesn't end up using it, lol
<re_irc>
<@adamgreig:matrix.org> not hugely appealing
<re_irc>
<@dirbaio:matrix.org> what's the alternative though?
<re_irc>
<@dirbaio:matrix.org> not support delays? go back to the closure API? add a 2nd "trait SpiDeviceThatCanAlsoDoDelays"?
lehmrob has joined #rust-embedded
<re_irc>
<@adamgreig:matrix.org> don't support Operation::Delay, drivers that really do need it have to take SpiBus and a GPIO CS?
<re_irc>
<@adamgreig:matrix.org> (and a DelayUs)
<re_irc>
<@dirbaio:matrix.org> not supporting it seems very lame :S
<re_irc>
<@eldruin:matrix.org> would that work for the l-e-h implementation? i.e. can l-e-h implement SpiBus or only SpiDevice?
<re_irc>
<@dirbaio:matrix.org> l-e-h can (and should) impl both
<re_irc>
<@adamgreig:matrix.org> it'd be the thing I mentioned earlier, SpiBus only if you promise no other device on that spidev bus is used at the same time
<re_irc>
<@adamgreig:matrix.org> but yea, you can have both
<re_irc>
<@halfbit:matrix.org> How would delay be expected to work?
<re_irc>
<@dirbaio:matrix.org> * both, adding "Operation::Delay" doesn't change that
<re_irc>
<@adamgreig:matrix.org> I don't think we found out what chip amb12 needed delays for, which was the other one?
<re_irc>
<@adamgreig:matrix.org> for the blocking spi trait, it would take anything that implements the DelayUs trait, which is probably polling a timer or a NOP loop or similar
<re_irc>
<@adamgreig:matrix.org> for the async spi trait, presumably you'd take something that did an async delay, which would then be via an async task queue
<re_irc>
<@halfbit:matrix.org> so call it a busy wait rather than delay
<re_irc>
<@adamgreig:matrix.org> I guess if you had an rtos you could probably use a DelayUs that "blocks" the thread by sleeping it
<re_irc>
<@halfbit:matrix.org> at least the terms better match the expectations
<re_irc>
<@dirbaio:matrix.org> "how" you do the delay is an implementation detail
<re_irc>
<@adamgreig:matrix.org> yea, it just needs to delay, it might put the chip to sleep while it waits or whatever you wanted
<re_irc>
<@halfbit:matrix.org> A detail that matters
<re_irc>
<@adamgreig:matrix.org> but no matter what you call it, having to pass a DelayUs every time anyone uses any spi device is kinda boring
<re_irc>
<@adamgreig:matrix.org> it matters, but the end user is in control of what it does and is the only person who knows
<re_irc>
<@adamgreig:matrix.org> so we can't name it in this generic trait
<re_irc>
<@adamgreig:matrix.org> one user might pass in a DelayUs that does a nop loop, another might be on an rtos and pass in one that sleeps the thread, but the spi driver won't know or care
<re_irc>
<@adamgreig:matrix.org> ("spi driver" here is the crate that talks to some particular chip over an SPI bus, not the MCU's HAL)
<re_irc>
<@dirbaio:matrix.org> so, back on topic, about the SPI transaction delay
<re_irc>
<@dirbaio:matrix.org> there's another option: make the "e-h-bus" impls fail with "not supported", add others that do take a "Delay" and do support it
<re_irc>
<@dirbaio:matrix.org> or add the generic param, but make it default to "Delay=NotSupported" which just fails
<re_irc>
<@dirbaio:matrix.org> it'd have to panic because we made Delay infallible :V
<re_irc>
<@adamgreig:matrix.org> the ONE trait
<re_irc>
<@dirbaio:matrix.org> (fwiw I think it should be infallible. Making it fallible only to supprt a fake "DummyDelay" that always fails would be stupid)
<re_irc>
<@dirbaio:matrix.org> so uh
<re_irc>
<@dirbaio:matrix.org> this is actually bikeshedding on how the "embedded-hal-bus" crate shold work though
<re_irc>
<@adamgreig:matrix.org> hmm
<re_irc>
<@adamgreig:matrix.org> you're suggesting you could add Operation::Delay(us) to the SpiDevice trait...
<re_irc>
<@grantm11235:matrix.org> I think "is this trait implementable in a sane way?" is still on topic
<re_irc>
<@eldruin:matrix.org> well, it is a whole sharing concept
<re_irc>
<@adamgreig:matrix.org> ...and just leave it up to the shared providers like e-h-b to do something good?
<re_irc>
<@halfbit:matrix.org> How would delay work in the context of a programmed DMA transfer list?
<re_irc>
<@dirbaio:matrix.org> independently of that, I think the core idea of adding "Operation::DelayUs(u32)" is worth it, yes
<re_irc>
<@adamgreig:matrix.org> like "just don't delay, lol" or "require the user provide a DelayUs impl" or something in between
<re_irc>
<@halfbit:matrix.org> e.g. I go to program the nxp edma with a nice list of spi work... and someone plops a delay in there
<re_irc>
<@adamgreig:matrix.org> Tom B: it's up to the HAL, but presumably it would program the first transfers, do the delay, then program the subsequent transfers?
<re_irc>
<@adamgreig:matrix.org> usually the DMA doesn't control CS, so the HAL is already "assert CS, trigger DMA transfers, deassert CS"
<re_irc>
<@halfbit:matrix.org> yes but with the op array today, its incredibly simple
<re_irc>
<@adamgreig:matrix.org> well, "trigger DMA, wait for DMA to complete, then deassert CS"
<re_irc>
<@dirbaio:matrix.org> plus HALs / RTOSs / frameworks can provide more "ready-to-use" versions of that
<re_irc>
<@halfbit:matrix.org> program dma, poll for completion
<re_irc>
<@dirbaio:matrix.org> for example embassy has "embassy-embedded-hal", which is sort of equivalent of "embedded-hal-bus" with a few embassy-specific additions
<re_irc>
<@adamgreig:matrix.org> would you usually program a whole list? I'd think often you program the dma one Operation at a time
<re_irc>
<@adamgreig:matrix.org> at which point it's fairly easy to do the delay business
<re_irc>
<@dirbaio:matrix.org> that one would simply do the delays with "embassy-time" and not bother the user with providing a "Delay"
<re_irc>
<@adamgreig:matrix.org> hmm
<re_irc>
<@adamgreig:matrix.org> the HAL doesn't actually implement the SpiDevice trait typically, sorry
<re_irc>
<@adamgreig:matrix.org> it's just doing the underlying SpiBus
<re_irc>
<@adamgreig:matrix.org> which doesn't have Delay anyway
<re_irc>
<@dirbaio:matrix.org> : it could, if it wants to do DMA batching
<re_irc>
<@dirbaio:matrix.org> or hardware CS
<re_irc>
<@halfbit:matrix.org> sam e70 has hardware cs and linked list transfers
<re_irc>
<@dirbaio:matrix.org> and then yes, it would have to "split" the batches when there's a delay
<re_irc>
<@halfbit:matrix.org> you could literally program 4 spi chip selects and any number of transfers in one go
<re_irc>
<@halfbit:matrix.org> like, here's all this work spi + dma, go
<re_irc>
<@adamgreig:matrix.org> and then busy wait polling for it to be done 😥
<re_irc>
<@adamgreig:matrix.org> or hopefully write an async hal too that can do something else
<re_irc>
<@grantm11235:matrix.org> I assume the delay op would have to be implemented with an implicit flush beforehand?
<re_irc>
<@adamgreig:matrix.org> Tom B: certainly it's kind of annoying, kind of unrelated to spi, and kind of mucks up the otherwise simple set of operations
<re_irc>
<@adamgreig:matrix.org> "delay us" is not obviously an spi primitive, lol
<re_irc>
<@halfbit:matrix.org> why can't the delay be done outside of the single transaction call?
<re_irc>
<@adamgreig:matrix.org> because apparently these devices need CS to remain asserted
<re_irc>
<@adamgreig:matrix.org> and the transaction is "assert CS, perform operations, deassert CS"
<re_irc>
<@adamgreig:matrix.org> I don't know what "these devices" are though :/
<re_irc>
<@halfbit:matrix.org> That seems like it should be done in a different manner then
<re_irc>
<@halfbit:matrix.org> let spi_periph = bus.txn(some_cs);
<re_irc>
<@adamgreig:matrix.org> "in a different manner" would probably be "using SpiBus instead of SpiDevice", where you control CS separately from transacting on the bus
<re_irc>
<@halfbit:matrix.org> spi_periph.transfer(transfer_list); delay(1); spi_periph.transfer(transfer_list2); // spi_periph is dropped and cs given back
<re_irc>
<@halfbit:matrix.org> right, so like that then
<re_irc>
<@adamgreig:matrix.org> you're more or less describing the closure based API we had before
<re_irc>
<@dirbaio:matrix.org> I don't think "it's hard to implement with DMA linked lists" is a solid argument against supporting "Operation::Delay"
<re_irc>
DMA linked lists are already hard anyway.
<re_irc>
<@adamgreig:matrix.org> you still can implement it fine with dma linked lists, it's just your implementation is a bit more fiddly to account for delays, right?
<re_irc>
<@dirbaio:matrix.org> yeah, implementing it is perfectly feasible
<re_irc>
<@halfbit:matrix.org> at the cost of code everyone pays for but only a few use
<re_irc>
<@adamgreig:matrix.org> and worse if it's at the cost of generic arguments and extra parameters that everyone must construct and pass in but almost never use :/
<re_irc>
<@adamgreig:matrix.org> well I guess the shared provider can make that pretty easy by having a constructor that generates its own dummy delay that panics or whatever
<re_irc>
<@adamgreig:matrix.org> so I suppose the usability for the end user isn't dramatically impacted, though still annoying that the type is longer to spell out for storing it places and such
<re_irc>
<@adamgreig:matrix.org> I guess it doesn't even need to be longer...
<re_irc>
<@dirbaio:matrix.org> IMO the whole point of "embedded-hal" is to make a standard API that as many device drivers can use as possible
<re_irc>
<@dirbaio:matrix.org> so we should prioritize that over it being "zero-cost"
<re_irc>
<@adamgreig:matrix.org> yea, but if you strictly follow that we're probably back to closures to support devices where you have to read a length byte and then read that many bytes or whatever
<re_irc>
<@dirbaio:matrix.org> okay, "as many device drivers can use as possible, and as many HALs can implement as possible"
<re_irc>
<@adamgreig:matrix.org> or something even more cursed to do bit twiddling on mosi to reset things?
<re_irc>
<@adamgreig:matrix.org> yea
<re_irc>
<@dirbaio:matrix.org> within reason lol
<re_irc>
<@grantm11235:matrix.org> : That would mean that adding a delay to a device driver could break an end user app at runtime
<re_irc>
<@adamgreig:matrix.org> just a case of where to draw the line I guess. it's a fair point though, there obviously are devices out there that need it if we've already heard from two people within a few weeks of an alpha of the new api
<re_irc>
<@adamgreig:matrix.org> : yea, indeed
<re_irc>
<@halfbit:matrix.org> isn't the delay predominantly needed between cs toggle and clocking?
<re_irc>
<@adamgreig:matrix.org> an annoying hazard
<re_irc>
<@adamgreig:matrix.org> Tom B: it seems the delay has often been between sending a command and sending the rest of the data or something like that
<re_irc>
<@halfbit:matrix.org> some hardware even supports this natively, so why not make it part of some cs setup
<re_irc>
<@dirbaio:matrix.org> in this case the "as many device drivers can use as possible" favors the closure API, and "as many HALs can implement as possible" favors the transaction list, because the closure can't be implemented on Linux and on ESP-IDF
<re_irc>
so we chose the transaction list API
<re_irc>
<@adamgreig:matrix.org> I think it's common for devices to have a min timing requirement between CS assert and first clock pulse, but it's usually small enough to be fine or whatever
<re_irc>
<@grantm11235:matrix.org> For example, if you test your code with MyDriver version 1.0.0, but it adds a delay in 1.0.1, you will get a runtime panic when you update, and the compiler can't help you at all
<re_irc>
<@adamgreig:matrix.org> yea, it's annoying for sure
<re_irc>
<@dirbaio:matrix.org> adding "Operation::Delay" improves "as many device drivers can use as possible", and doesn't hurt "as many HALs can implement as possible" at all AFAICT, it's implementable everywhere
<re_irc>
<@halfbit:matrix.org> the transaction list is actually really nice
lehmrob has quit [Ping timeout: 256 seconds]
<re_irc>
<@dirbaio:matrix.org> and it's especially critical IMO becaue SpiDevice is the key for unrelated drivers to be able to share the same bus
<re_irc>
<@adamgreig:matrix.org> : yea, I guess ultimately you're right here, the axis it's bad on is "it's annoying" and "it makes some hals harder but not impossible to implement"
<re_irc>
<@dirbaio:matrix.org> if you sabotage that and make drivers go to "SpiBus + OutputPin" then you're throwing bus sharing out of the window
<re_irc>
<@adamgreig:matrix.org> but on the most important metrics it's a benefit
<re_irc>
<@adamgreig:matrix.org> : I think that would just have to be covered in the docs for whatever provided the SpiDevice? probably not many device drivers will suddenly start needing a delay where they didn't before, I guess
<re_irc>
<@vollbrecht:matrix.org> the reason that some devices need that delay is that, there exist some devices where you talk to a device "full-duplex" style but you have to wait for the computation of the first bytes be computed, so they answer directly in the 2nd byte. Though i think if the bus is just slow enough it should often not be a problem
<re_irc>
<@halfbit:matrix.org> at least lpspi seems to natively support a delay between transfers which is cool
<re_irc>
<@halfbit:matrix.org> but it would need to be known in advance
<re_irc>
<@halfbit:matrix.org> so not in the op queue, but as part of the hardware config
<re_irc>
<@vollbrecht:matrix.org> * first bytes to
<re_irc>
<@halfbit:matrix.org> that's where linux and zephyr also provide delay options
<re_irc>
<@halfbit:matrix.org> +I believe
<re_irc>
<@adamgreig:matrix.org> can't the hal configure it at the start of the transaction?
<re_irc>
<@halfbit:matrix.org> Could the delay be done not in the op queue but with a spi peripheral configuration step?
<re_irc>
<@grantm11235:matrix.org> : I think that "oops, I forgot to add a delay because it worked on my machine" is quite common
<re_irc>
<@dirbaio:matrix.org> nothing prevents the impl from scanning the whole op list upfront
<re_irc>
<@adamgreig:matrix.org> Tom B: there's no configuration in embedded-hal, and there might be different length delays at different parts of the operation list
<re_irc>
<@halfbit:matrix.org> Yes but the implication of the delay being an op is that... if you go to scope it, you'll see that delay there
<re_irc>
<@halfbit:matrix.org> * expect to see that delay there, vs a configuration step prior to doing the transaction...
<re_irc>
<@halfbit:matrix.org> or what about delay(5) delay(5) delay(5) write(x)
<re_irc>
<@adamgreig:matrix.org> sure, exactly
<re_irc>
<@halfbit:matrix.org> cool, now I need to build logic for this
<re_irc>
<@halfbit:matrix.org> in the hal
<re_irc>
<@halfbit:matrix.org> which is supposed to work with hardware
<re_irc>
<@adamgreig:matrix.org> yea, but the logic is "step through the list, wait out any delays, batch up non-delay operations and run them on dma"?
<re_irc>
<@adamgreig:matrix.org> or just implement SpiBus with dma and let the SpiDevice provider take care of the delays for you, you can still use DMA in your SpiBus implementation
<re_irc>
<@adamgreig:matrix.org> (but you can't batch multiple transfers together because you don't see the list)
<re_irc>
<@adamgreig:matrix.org> like, it's definitely more annoying for the HAL implementer than not having it, no disagreement there
<re_irc>
<@dirbaio:matrix.org> yep, HALs usually impl just SpiBus
<re_irc>
<@adamgreig:matrix.org> but it doesn't make it impossible to implement the HAL, whereas not having it does make it impossible to create a shared driver for devices that need delays
<re_irc>
<@dirbaio:matrix.org> exactly
<re_irc>
<@adamgreig:matrix.org> so in the end, that's probably the balance that matters more
<re_irc>
<@adamgreig:matrix.org> oops, we're over time for the meeting, sorry
<re_irc>
<@adamgreig:matrix.org> given it's a very simple PR (I hope?), maybe worth putting it together for final discussion/voting?
<re_irc>
<@adamgreig:matrix.org> maybe not simple for the e-h-bus part
<re_irc>
<@dirbaio:matrix.org> I wonder if we could have 2 traits
<re_irc>
<@dirbaio:matrix.org> +instead
<re_irc>
<@dirbaio:matrix.org> or a trait generic on the "operation type"
<re_irc>
<@dirbaio:matrix.org> so it can be an enum with the Delay case, or without
<re_irc>
<@grantm11235:matrix.org> If we are going to have two traits, I think that linux spidev vs lowlevel spi is a much better split
<re_irc>
<@dirbaio:matrix.org> wdym with "lowlevel"?
<re_irc>
<@dirbaio:matrix.org> the closure API?
<re_irc>
<@grantm11235:matrix.org> Yeah
<re_irc>
<@dirbaio:matrix.org> that's a completely different API
<re_irc>
<@vollbrecht:matrix.org> problem with closure for us in rtos was that if we use the manged_cs we cant capture the status when all the transaction are finished, to deassert the cs at the end. we would need some info from the closure to get it back
<re_irc>
<@newam:matrix.org> : done
<re_irc>
<@adamgreig:matrix.org> thanks
<re_irc>
<@newam:matrix.org> Huh, looks like it worked
<re_irc>
<@adamgreig:matrix.org> my gut feeling is we don't need a merge queue on the wg repo and just have "approval required"
<re_irc>
<@adamgreig:matrix.org> and thus also delete the CI entirely
<re_irc>
<@adamgreig:matrix.org> but as a test run for other repos, promising start
<re_irc>
<@newam:matrix.org> Yeah there's nothing to build so any conflicts should appear as normal git merge conflicts.
<re_irc>
<@grantm11235:matrix.org> vollbrecht: Isn't that what flush is for?
<re_irc>
<@vollbrecht:matrix.org> the problem at the core for us was that the system api wants to know at the last call inside the closure if it still should hold the cs high in advanced. Otherwise one have to do an extra empty flush out ( that is totaly broken ) and creates an extra delay in the form of an minimal transaction length (even when empty)
<re_irc>
<@dirbaio:matrix.org> I really really really don't think we should have BOTH the oplist AND the closure APIs
<re_irc>
<@vollbrecht:matrix.org> * low
<re_irc>
<@dirbaio:matrix.org> having two completely differnet APIs has a very high complexity cost
<re_irc>
<@dirbaio:matrix.org> one thing is a variant of an API with more/less capabilities, another thing is two completely different APIs
<re_irc>
<@adamgreig:matrix.org> And the closure one was a real pain for async huh
<re_irc>
<@dirbaio:matrix.org> fwiw we already have such "variations" with SpiDevice / SpiDeviceRead / SpiDeviceWrite
<re_irc>
<@dirbaio:matrix.org> : yeah.. though that'll get fixed in Rust in the future, so I wouldn't say it has much eight
<re_irc>
<@dirbaio:matrix.org> if we do variations for "supports delays" we get SIX variations though:
<re_irc>
<@adamgreig:matrix.org> The more the merrier...?
starblue has quit [Ping timeout: 265 seconds]
starblue has joined #rust-embedded
<re_irc>
<@sourcebox:matrix.org> Since we are talking about time again, what is the recommended way if a driver wants to do an operation with some timeout?
<re_irc>
<@dirbaio:matrix.org> no trait in embedded-hal currently for that, no
<re_irc>
<@dirbaio:matrix.org> closest is probably "embassy-time", since the driver is supplied separately it can work everywhere
<re_irc>
<@dirbaio:matrix.org> but yeah it's not a trait
<re_irc>
<@sourcebox:matrix.org> Because sometimes you don't want a fixed delay time but just advance as quickly as possible after some condition is met.