<re_irc>
<@thejpster:matrix.org> https://activities.esa.int/4000140241: "EVALUATION OF RUST USAGE IN SPACE APPLICATIONS BY DEVELOPING BSP AND RTOS TARGETING SAMV71"
<re_irc>
<@k900:0upti.me> SPAAAAAAAAAAAAAAAAACE
<re_irc>
<@mameluc:matrix.org> there is no rust in space as there is no oxygen
<re_irc>
<@k900:0upti.me> Do fungi need oxygen?
<re_irc>
<@diondokter:matrix.org> If you contributed to cortex-m(-rt) and stm32h7xx-hal, then your code is already in space :P
cr1901 has quit [Remote host closed the connection]
cr1901 has joined #rust-embedded
cr1901 has quit [Remote host closed the connection]
cr1901 has joined #rust-embedded
starblue has quit [Ping timeout: 264 seconds]
starblue has joined #rust-embedded
IlPalazzo-ojiisa has joined #rust-embedded
cr1901 has quit [Remote host closed the connection]
cr1901 has joined #rust-embedded
<re_irc>
<@imdoor:ilgt.lv> here's bit more detailed description https://github.com/probe-rs/probe-rs/issues/1617 i've no idea if it's actually an issue with probe-rs, but that seemed like the best bet
<re_irc>
<@dirbaio:matrix.org> : are you using embassy-nrf or nrf52840-hal?
<re_irc>
<@dirbaio:matrix.org> sounds like a HAL bug or a bug in your code
<re_irc>
<@dirbaio:matrix.org> if flashing succeeds, and then the firmware starts up after powercycle, then what probe was used to flash it can no longer have any effect
<re_irc>
<@dirbaio:matrix.org> it's just bytes on the flash, they don't "remember" how they were flashed
<re_irc>
<@dirbaio:matrix.org> (probably worth trying flashing with some other tool like nrfjprog or pyocd or openocd just to be 100% sure, but I'm 99% sure the probe isn't the issue)
<re_irc>
<@dirbaio:matrix.org> it might be some timing bug. When running with the probe attached, defmt is set to blocking mode which makes printing slower. This makes the code run slower which can change timings so that a bug no longer reproduces.
<re_irc>
<@imdoor:ilgt.lv> @dirbaio thanks for the insight. I should maybe try adding some delays to see if that changes anything. also thanks for the list of flashing tool alternatives
<re_irc>
<@imdoor:ilgt.lv> : but would defmt still be using blocking mode even when "cargo-flash" is used?
<re_irc>
i mean, what I see is that:
<re_irc>
1. I flash the program using "cargo-flash", it shows only a progress bar and once it's done, it's done (no logs from the device get shown or anything, it simply detaches)
<re_irc>
2. the program starts up and proceeds to do it's thing on the device, everything works as expected
<re_irc>
3. I power-cycle the device
<re_irc>
4. suddenly things break, some lines of code seem to get executed (some LEDs change colors), but it gets stuck right after (I can't change the color of LEDs twice, for example)
<re_irc>
is your hypothesis about timings compatible with these observations (the fact that no logs seem to be acquired from the device)? does cargo-flash still attach a probe even when it's not producing log output?
<re_irc>
<@dirbaio:matrix.org> there's another explanation: when the debug probe attaches it turns on some debug blocks inside the chip, which force HFCLK permanently on
<re_irc>
<@dirbaio:matrix.org> vs when you run it standalone, HFCLK is started/stopped on demand by the hardware as you enable peripherals that need it
<re_irc>
<@dirbaio:matrix.org> that might change timing
<re_irc>
<@dirbaio:matrix.org> it's very unlikely that makes a difference, I've never seen that happen
<re_irc>
<@dirbaio:matrix.org> but it could..?
<re_irc>
<@dirbaio:matrix.org> do you have the code somwhere?
<re_irc>
<@imdoor:ilgt.lv> I have it in a private repo, but there's no harm in publishing it, i guess. wait a sec
<re_irc>
<@imdoor:ilgt.lv> https://github.com/rihardsk/airlog the "neopixel" branch is the one with the issue. "master", however works without a problem
<re_irc>
<@dirbaio:matrix.org> ah you're using nrf-hal
<re_irc>
<@imdoor:ilgt.lv> oh, and "hello-co2" is the bin you should be looking at
<re_irc>
<@dirbaio:matrix.org> do you have a revisoin 3 chip? (build code Fxx)
<re_irc>
<@imdoor:ilgt.lv> I might, I have to look it up
<re_irc>
<@dirbaio:matrix.org> if you've bought the DK in the last ~year then you probably have a rev3 chip
<re_irc>
<@dirbaio:matrix.org> that's why you have to pass --erase-all every time
<re_irc>
<@dirbaio:matrix.org> it needs firmware support for keeping the debug port unlocked, that's what that issue is about
<re_irc>
<@dirbaio:matrix.org> I think this only affects flashing though, it sholdn't make the firmware run different after flashing
<re_irc>
<@imdoor:ilgt.lv> : yup, I bought it quite recently. and I already encountered a surprise, when I tried to follow the knurling sessions book and the device IDs were slightly different
<re_irc>
<@dirbaio:matrix.org> yeah I don't think those are updated either
<re_irc>
<@dirbaio:matrix.org> from looking at the code I don't see amy obvious issue :S
<re_irc>
<@dirbaio:matrix.org> might be worth trying setting CLOCKS to force HFCLK on
<re_irc>
<@dirbaio:matrix.org> so it matches what happens with debug on
<re_irc>
<@imdoor:ilgt.lv> is it something you do through nrf-hal, or should I look elswhere for info on how to do it?
<re_irc>
<@dirbaio:matrix.org> ah no that switches it to the external oscillator, but doesn't necessarily force it on
<re_irc>
<@dirbaio:matrix.org> your code is all hot loops with no "wfi", that should already force hfclk on
<re_irc>
<@dirbaio:matrix.org> perhaps some race condition with initializing the i2c chips?
<re_irc>
<@dirbaio:matrix.org> if you power them on all at the same time
<re_irc>
<@imdoor:ilgt.lv> oh, damn, I just tried adding two delays after setting colors with neopixels and it fixed the issue
<re_irc>
<@dirbaio:matrix.org> it behaves different
<re_irc>
<@dirbaio:matrix.org> than if you reset just the nrf, but the other chips were already powered from before
<re_irc>
<@dirbaio:matrix.org> try resetting just the nrf, with the reset button
<re_irc>
<@imdoor:ilgt.lv> i just pushed a commit with the change to the neopixel branch. it seems you were right that the issue is timing related. the diff looks like this
<re_irc>
<@imdoor:ilgt.lv> oh, I think i saw some blog post where someone was trying out different sleep states on nrf52840 and had to fork nrf-hal to be able to wake the device from some sleep state or something. i had a suspicion that it was relevant here, but wasn't sure
<re_irc>
<@imdoor:ilgt.lv> but yeah, thanks for the help! now i'm wondering whether I want to dig deeper to try to find out what exactly is causing the issue. maybe just the neopixel crate is buggy, maybe something else is going on, but either way I can at least move on with my project :)
emerent has quit [Ping timeout: 240 seconds]
emerent has joined #rust-embedded
smach has joined #rust-embedded
<re_irc>
<@dirbaio:matrix.org> it's manufacturing-related... blast it with a hot air gun for a bit and then it starts working.. grrrr
<re_irc>
<@dirbaio:matrix.org> and the factory tests didn't catch it because they run through SWD lol
<re_irc>
<@dirbaio:matrix.org> and it's so odd. ONLY the radio stops working, and ONLY when the debug probe is not attached
<re_irc>
<@dirbaio:matrix.org> and it's not the HFCLK thing. I change the firmware to never sleep and it still happens. yay
<re_irc>
<@mameluc:matrix.org> I had some similar gremlings not too long ago. Had to get defmt over serial to work to try to catch it. After a lot of work it just started working IIRC and I don't know why. Ran fine with swd but not without
<re_irc>
<@mameluc:matrix.org> I could blink leds reliably but trying to do lora stuff was too much. Never understood why but now everything works
<re_irc>
<@mameluc:matrix.org> I changed my shampoo, maybe that is important
<re_irc>
<@juliand:fehler-in-der-matrix.de> : Do you mean physically attached (vs. disconnected)?
<re_irc>
<@dirbaio:matrix.org> do something over swd, then it starts working
<re_irc>
<@dirbaio:matrix.org> reset the chip, still working
<re_irc>
<@dirbaio:matrix.org> power cycle, no longer works
smach has quit [Remote host closed the connection]
<re_irc>
<@dirbaio:matrix.org> do something over swd, it starts working
<re_irc>
<@dirbaio:matrix.org> detach probe, still works
<re_irc>
<@dirbaio:matrix.org> reset, still works
<re_irc>
<@dirbaio:matrix.org> power cycle, stops working
<re_irc>
<@juliand:fehler-in-der-matrix.de> Wow
<re_irc>
<@juliand:fehler-in-der-matrix.de> That sounds like quite some magic is going on here haha
<re_irc>
<@dirbaio:matrix.org> seems like it works as long as the DBGPWRUPREQ bit in the DP is set or something
<re_irc>
<@dirbaio:matrix.org> probe-rs sets it on attach and doesn't unset it on detach. Reset doesn't clear it, only power cycle does
smach has joined #rust-embedded
<re_irc>
<@dirbaio:matrix.org> symptoms seem consistent with that
<re_irc>
<@dirbaio:matrix.org> changing the firmware to never sleep doesn't fix it though. (changing WFI to "loop{}")
<re_irc>
<@dirbaio:matrix.org> so it's not the HFCLK
<re_irc>
<@dirbaio:matrix.org> but no idea what DBGPWRUPREQ does besides forcing HFCLK on
<re_irc>
<@henrik_alser:matrix.org> : Easy, just ship it with a debug probe hanging off there!
<re_irc>
<@dirbaio:matrix.org> lol
<re_irc>
<@henrik_alser:matrix.org> Problem solved
<re_irc>
<@dirbaio:matrix.org> if it's a soldering issue, probably nothing I can do to fix it in firmware
<re_irc>
<@henrik_alser:matrix.org> Ship it with a soldering iron
<re_irc>
<@dirbaio:matrix.org> some VCC/GND pad not connected maybe, and DBGPWRUPREQ turns power to some other parts of the chip which allow it to leak into the radio, who knows
<re_irc>
<@dirbaio:matrix.org> it's a new batch.. about 5% of the boards have this issue
<re_irc>
<@dirbaio:matrix.org> and the batch is many many thousands 😰
<re_irc>
<@henrik_alser:matrix.org> Oh no…
<re_irc>
<@henrik_alser:matrix.org> Have they shipped yet?
<re_irc>
<@dirbaio:matrix.org> yeah, just now they've started to come back 🤪
<re_irc>
<@henrik_alser:matrix.org> Reflowing fixes them permanently?
<re_irc>
<@dirbaio:matrix.org> from our field techs, not from customers, so it's not _that_ bad but still
<re_irc>
<@henrik_alser:matrix.org> Yeah..
<re_irc>
<@dirbaio:matrix.org> : seems like it, at least in 2 we tried
<re_irc>
<@dirbaio:matrix.org> bleh
<re_irc>
<@dirbaio:matrix.org> best I can hope for is to update the factory tests to catch it
<re_irc>
<@dirbaio:matrix.org> and retest eeeeeverything....
<re_irc>
<@dirbaio:matrix.org> I can repro it direclty. set CDBGPWRUPREQ=1, BLE works. Set CDBGPWRUPREQ=0, BLE stops working. No chip reset required even
starblue has quit [Ping timeout: 240 seconds]
starblue has joined #rust-embedded
<re_irc>
<@jannic:matrix.org> If you can set that from the firmware, the solution seems obvious :-)
<re_irc>
<@dirbaio:matrix.org> you can't :P
<re_irc>
<@adamgreig:matrix.org> hi @room, meeting time! agenda is https://hackmd.io/MDSIfjm2TcKxbvvPLs8NEw, please add anything you'd like to announce or discuss and we'll start in a few mins
<re_irc>
<@almindor:matrix.org> i'll be a bit tentative
<re_irc>
<@adamgreig:matrix.org> ok, let's start! I don't have any announcements this week, does anyone else?
<re_irc>
<@adamgreig:matrix.org> nice! i've put it on the agenda to discuss the pr
<re_irc>
<@adamgreig:matrix.org> ok, first up on the agenda is the ongoing bors retirement, no news this week as we continue to see how things go, but we can probably move e-h or c-m to ghmq and write a note on the process before kicking off other repos
<re_irc>
<@adamgreig:matrix.org> we didn't get to embedded hal last week so let's cover those points now
<re_irc>
<@almindor:matrix.org> if I understand correctly, this is usually handled by HW SPI implementations, is this PR about adding the ability to manually mimick the in-transaction SCK delays?
<re_irc>
<@adamgreig:matrix.org> no, but you're not the first person to think that and we should probably clear it up
<re_irc>
<@adamgreig:matrix.org> this is nothing to do with the hardware timing of sck-to-data or cs-to-sck and entirely about delays between transfers in a transaction
<re_irc>
<@adamgreig:matrix.org> or maybe I misunderstood what you said?
<re_irc>
<@adamgreig:matrix.org> this isn't something you'd expect hardware to do, in my experience
<re_irc>
<@adamgreig:matrix.org> this is more like "transfer one byte, wait 2ms, transfer 10 more bytes"
<re_irc>
<@adamgreig:matrix.org> all while CS remains asserted and the bus is exclusively locked
<re_irc>
<@almindor:matrix.org> so within one CS assertion, there need to be delays between some arbitrary data pieces, but the delays have nothing to do with SCK operation itself?
<re_irc>
<@adamgreig:matrix.org> yes
<re_irc>
<@almindor:matrix.org> ah ok, is this about framing?
<re_irc>
<@adamgreig:matrix.org> which you'd think isn't necessary but as soon as we removed the ability to do that with closures several people messaged with examples where it's required
<re_irc>
<@dirbaio:matrix.org> : some chips require it. See examples in the PR description
<re_irc>
<@adamgreig:matrix.org> the framing is the same (just the CS signal), this is more about crappy SPI chips that need time between parts of the same operation
<re_irc>
<@adamgreig:matrix.org> better chips define the amount of time they need in terms of sck cycles and insert those as dummy bits, removing this problem, like spi flash
<re_irc>
<@adamgreig:matrix.org> but these troublesome chips cannot tolerate dummy sck cycles between the two parts where they need a delay
<re_irc>
<@almindor:matrix.org> yeah this seems like an out-of-spi issue technically, but I understand why having is supported in-transaction might be necessary
<re_irc>
<@adamgreig:matrix.org> in the previous closure api, one could stick a delay inside the closure in drivers that needed it
<re_irc>
<@almindor:matrix.org> i'm somewhat against it the way it's presented though
<re_irc>
<@adamgreig:matrix.org> but with the new slice-of-operations api, that's not possible, so you can't write a driver for those chips using SpiDevice
<re_irc>
<@almindor:matrix.org> could we add "D: DelayUs" only to a specific method?
<re_irc>
<@adamgreig:matrix.org> you _could_ use SpiBus directly but we don't want to encourage that and it would prevent any bus sharing
<re_irc>
<@almindor:matrix.org> e.g. transaction_with_delays() kind of thing
<re_irc>
<@almindor:matrix.org> I'd like to avoid the generics hell
<re_irc>
<@almindor:matrix.org> or perhaps make it a separate SpiDevice implementation for these only?
<re_irc>
<@almindor:matrix.org> sorry, subtrait
<re_irc>
<@adamgreig:matrix.org> there's no new generic in the SpiDevice trait
<re_irc>
<@adamgreig:matrix.org> ah, well, not in the blocking one
<re_irc>
<@adamgreig:matrix.org> the async one however...
<re_irc>
<@dirbaio:matrix.org> : no, in the impls
<re_irc>
<@adamgreig:matrix.org> the impls could do all sorts of things that don't require generics though right?
<re_irc>
<@adamgreig:matrix.org> oh, hm
<re_irc>
<@almindor:matrix.org> right with blocking you can just impl on the HAL level
<re_irc>
<@adamgreig:matrix.org> I guess no, because it's mostly impls provided by eg embedded-hal-bus
<re_irc>
<@dirbaio:matrix.org> the "hal-independent" impls on embedded-hal-bus are generic on a Bus and a Delay
<re_irc>
<@dirbaio:matrix.org> * SpiDevice impls on embedded-hal-bus are generic on a SpiBus
<re_irc>
<@dirbaio:matrix.org> a HAL could also provide a SpiDevice impl directly, in that case it doesn't need these generics
<re_irc>
<@adamgreig:matrix.org> it is a pain to add generic spam when the huge majority of users and use cases don't ever require the delay
<re_irc>
<@adamgreig:matrix.org> but having to pass the delay provider in to the transaction methods is also pretty annoying and splitting them up is pretty weird
<re_irc>
<@almindor:matrix.org> we did make the HAL provide the SpiDevice for the e310x-hal
<re_irc>
<@dirbaio:matrix.org> : why
<re_irc>
<@diondokter:matrix.org> Having it so you have to pass the delay to the function would make it so the driver has to know about the delay impl too
<re_irc>
<@adamgreig:matrix.org> but I hoped the e-h-bus impls could have easy ways to construct them without a delay provider, for most users
<re_irc>
<@almindor:matrix.org> well you actually do bus.give_device(config)
<re_irc>
<@almindor:matrix.org> e.g.
<re_irc>
<@almindor:matrix.org> " let spi_display1 = spi_bus.new_device(cs0, &spi_config_display1);"
<re_irc>
<@dirbaio:matrix.org> does it use CS, RefCell, or some kind of mutex internally? is it configurable?
<re_irc>
<@almindor:matrix.org> so the HAL could have a separate constructor here for the special case, like bus.new_device_with_delay() to avoid needing it in the happy path
<re_irc>
<@almindor:matrix.org> yes RefCell
<re_irc>
<@almindor:matrix.org> configurable to HW capacity
<re_irc>
<@almindor:matrix.org> there's a HW specific config here, so you can set all the toggles you might need
<re_irc>
<@dirbaio:matrix.org> why aren't the impls in "embedded-hal-bus" not suitable?
<re_irc>
<@dirbaio:matrix.org> or is it just for convenience?
<re_irc>
<@dirbaio:matrix.org> why are the impls in "embedded-hal-bus" not suitable?
<re_irc>
<@almindor:matrix.org> this was written before that I think :D
<re_irc>
<@dirbaio:matrix.org> ah
<re_irc>
<@adamgreig:matrix.org> the e-h-bus ones wouldn't let you change bus mode per-device, right?
<re_irc>
<@almindor:matrix.org> it started off Mgiant's SPI split design PR IIRC
<re_irc>
<@adamgreig:matrix.org> whereas presumably the configs you pass to new_device allow a different bus config for each device
<re_irc>
<@almindor:matrix.org> yes everything is settable
<re_irc>
<@dirbaio:matrix.org> : ah yep. this is a missing feature in e-h today :(
<re_irc>
<@almindor:matrix.org> we reconfigure the bus on each switch, seems to not impact performance whatsoever but this MCU has a beefy core
<re_irc>
<@dirbaio:matrix.org> Embassy also had to add it on its own, it with a "SetConfig" trait
<re_irc>
<@almindor:matrix.org> tbf I don't think this needs to be "codified"
<re_irc>
<@dirbaio:matrix.org> the intent with "embedded-hal-bus" was to allow the user to choose which kind of mutexing
<re_irc>
and there's one with no mutex at all, so you don't pay any cost if you don't want sharing
<re_irc>
<@almindor:matrix.org> yeah but this is still HAL implementor level, not driver
<re_irc>
<@dirbaio:matrix.org> if you build SpiDevice impls into the HAL, you either have to replicate all of that, or you offer less flexibility
<re_irc>
<@almindor:matrix.org> so it can be left to "implement to the best capability of your HW"
<re_irc>
<@almindor:matrix.org> e.g. for us we're single-core so we just do interrupt safety
<re_irc>
<@almindor:matrix.org> : well true, but my understanding was that that'd be the default take
<re_irc>
<@almindor:matrix.org> anyway, I don't want to derail the Delay issue
<re_irc>
<@adamgreig:matrix.org> yea, maybe "can e-h-bus be extended to allow configuration" can happen later, i don't think it should affect the core e-h traits?
<re_irc>
<@adamgreig:matrix.org> probably something akin to embassy's SetConfig trait on the underlying Bus
<re_irc>
<@dirbaio:matrix.org> yeah, it can be added later
<re_irc>
<@almindor:matrix.org> yes it's more a "embedded-hal-bus" question
<re_irc>
<@adamgreig:matrix.org> for Delay I think the question is basically how much we inconvenience all other users to enable delaying in a transaction; jannic's comment on the pr has some good points about that too
<re_irc>
<@adamgreig:matrix.org> which basically come down to "sort of allow some implementations to not support it, perhaps with a new error type rather than just panicking"
<re_irc>
<@adamgreig:matrix.org> but reasoning about it is a bit complicated by it often being provided by something outside the actual hal, like e-h-bus
<re_irc>
<@adamgreig:matrix.org> I think even just having the e-h-bus structs have a convenience method to construct it without a delay - perhaps just "new", and add "new_with_delay"...
smach has quit [Quit: Leaving]
<re_irc>
<@almindor:matrix.org> we did something similar to allow a "NoPin" type with some GPIO stuff, maybe an approach like that can be taken for the "nodelay" but that doesn't solve the "one more generic param to wonder about"
<re_irc>
<@adamgreig:matrix.org> and then they could either return a new errorkind, or panic, or something, if someone tries to use them with a delay?
<re_irc>
<@adamgreig:matrix.org> yea, and if you want to store the spidevice reference somewhere and it has another generic param now, ugh
<re_irc>
<@almindor:matrix.org> what if we make a new trait and add an optional ".with_delay" to the bus/devices that returns Spi[Bus|Device] + Spi[Bus|Device]WithDelay trait-implementor ?
<re_irc>
<@dirbaio:matrix.org> : generic drivers just have a "where D: SpiDevice" bound, they don't need another generic param for the delay
<re_irc>
<@dirbaio:matrix.org> the delay is within the type: "D = MyDevice<'a, MyBus, MyDelay>"
<re_irc>
<@dirbaio:matrix.org> and non-generic code can use concrete types, so there's no extra generics there either
<re_irc>
<@dirbaio:matrix.org> I don't think it's a good idea to split the trait into SpiDevice / SpiDeviceWithDelay
<re_irc>
<@dirbaio:matrix.org> it means you have to implement them 2x
<re_irc>
<@dirbaio:matrix.org> * bloat. It's the same issue we had with SpiDevice/SpiDeviceRead/SpiDeviceWrite
<re_irc>
<@almindor:matrix.org> i mean it's true that this just impact the "HAL level implementors"
<re_irc>
<@almindor:matrix.org> * impacts
<re_irc>
<@almindor:matrix.org> i wish we didn't have to hack things due to damn linux though
<re_irc>
<@adamgreig:matrix.org> this one isn't really linux's fault at least
<re_irc>
<@almindor:matrix.org> it is due to the closure issue no?
<re_irc>
<@adamgreig:matrix.org> I guess, but we liked the slice api for reasons beyond just being implementable in linux
<re_irc>
<@jannic:matrix.org> Wasn't esp another case where the closure api wasn't possible?
<re_irc>
<@adamgreig:matrix.org> it was also a pain on async and perhaps less good for dma?
<re_irc>
<@adamgreig:matrix.org> hmm, i guess one downside of the new OperationNotSupported ErrorKind and a dummy Delay that returns it is that DelayUs is infallible so
<re_irc>
<@adamgreig:matrix.org> but you could have that new ErrorKind and the SpiDevice impl itself returns it if it wasn't created with a real delay
<re_irc>
<@dirbaio:matrix.org> downsides of the closure API:
<re_irc>
- not implementable on esp-idf
<re_irc>
- not implementable on linux
<re_irc>
- Makes the SpiDevice trait not dyn-safe (!!)
<re_irc>
- code size bloat due to monomorphizing code around the closure
<re_irc>
- needs hacks on async
<re_irc>
<@dirbaio:matrix.org> +- More complicated overall
<re_irc>
<@dirbaio:matrix.org> I haven't seen dyn-safety mentioned before
<re_irc>
<@dirbaio:matrix.org> imo that's a strong downside
<cr1901>
I can't really hide my biases here... Linux has too much say over the direction of an API meant for a world that isn't solely Linux lmao
<re_irc>
<@jannic:matrix.org> : Yes, that would basically mean that the SpiDevice would contain an "Optional<DelayUs>" instead of just a "DelayUs".
<re_irc>
<@dirbaio:matrix.org> we can have two SpiDevice _impls_, one with and one without
<re_irc>
<@adamgreig:matrix.org> cr1901: linux was just the first place we noticed that the closure api wasn't implementable, but it's not the only reason to move to the new api
<re_irc>
<@dirbaio:matrix.org> one thing is _the trait_
<re_irc>
another thing is _the impls in embedded-hal-bus_
<re_irc>
<@adamgreig:matrix.org> both are in the PR though :P
* cr1901
nods, well I'm gonna be in favor of both APIs most likely
<re_irc>
<@adamgreig:matrix.org> I think adding the extra variant to Operation is good, but we should document more explicitly what it means and when it's useful and maybe even what to do if you can't support it, which might also mean a new ErrorKind
<re_irc>
<@adamgreig:matrix.org> but for the e-h-b impls it seems very unergonomic to require all spi users to pass a delay provider in to make SpiDevices when they almost always won't be used
<re_irc>
<@dirbaio:matrix.org> we should focus the bikeshed energy on the traits
<re_irc>
<@dirbaio:matrix.org> that's what will be perma-stabilized as e-h 1.0
<re_irc>
<@dirbaio:matrix.org> we can always make breaking changes to the impls
<re_irc>
<@dirbaio:matrix.org> for the trait, we have a few options:
<re_irc>
2- Mandate SpiDevice supports delays
<re_irc>
1- Simply not support delays. (status quo)
<re_irc>
4- split the trait into SpiDevice, SpiDeviceWithDelay
<re_irc>
3- Allow SpiDevices to optionally support delays, return DelayNotSupported otherwise
<re_irc>
<@adamgreig:matrix.org> sure, just a focus on the documentation for the trait change then
<re_irc>
<@dirbaio:matrix.org> I would prefer 2 or 3, somewhat leaning towards 2
<re_irc>
<@jannic:matrix.org> The only disadvantage of a not-always-supported delay operation I see is the missing type safety. If a device driver starts using the delay operation in an updated, is that a semver breaking change? The API stays the same.
<re_irc>
<@dirbaio:matrix.org> splitting is a mistake (we been there, done that)
<re_irc>
<@jannic:matrix.org> * update,
<re_irc>
<@adamgreig:matrix.org> my problem with 2 is that it means you can't allow constructing an spidevice provider without passing in a delay provider
<re_irc>
<@adamgreig:matrix.org> and there's no way for the spidevice provider to guess at a "default" delay provider, so the user always has to give it one
<re_irc>
<@almindor:matrix.org> if we use "Option<D>" with "NoDelay" trait it'd work
<re_irc>
<@almindor:matrix.org> it could even be the standard default for the construction then?
<re_irc>
<@therealprof:matrix.org> Maybe people should simply bitbang such broken chips? 😛
<re_irc>
<@dirbaio:matrix.org> I lean towards 2 because
<re_irc>
- Any decent HAL will have a Delay impl. Ideally clonable to infinity. We should stop doing "a Delay needs exclusive ownership of a hardware timer" asap...
<re_irc>
- HALs can give convenience functions to construct the SpiDevices that automatically set the right delay
<re_irc>
<@diondokter:matrix.org> For option 3 what are you realistically gonna do with a DelayNotSupported error?
<re_irc>
<@dirbaio:matrix.org> return it to the caller
<re_irc>
<@adamgreig:matrix.org> I guess just panicking is probably just as good
<re_irc>
<@almindor:matrix.org> if we use &[&operation] kind of thing, could the delay be a part of the operation instead then? 🤔
<re_irc>
<@dirbaio:matrix.org> : that's exactly what the proposed trait does
<re_irc>
<@diondokter:matrix.org> Yeah, but what's the caller gonna do with it? Probably just panic because it shouldn't happen in production code.
<re_irc>
Might as well do option 2 where a delay is gonna panic if not implemented
<re_irc>
<@dirbaio:matrix.org> : then people will go "waaaaah panics in libraries bad"
<re_irc>
<@dirbaio:matrix.org> (even though they unwrap everyting)
<re_irc>
<@almindor:matrix.org> no, I mean have Bus/Device no delay in itself and have "Operation::Delay(D, amount)" kind of thing rather than D being inside the Bus/Device?
<re_irc>
<@almindor:matrix.org> oh I guess there's no way to indirect the type there
<re_irc>
<@jannic:matrix.org> Is trying to write firmware without any panic call (so it can be optimized away) still a thing?
<re_irc>
<@almindor:matrix.org> libraries must not panic in kernel mode, I agree with that
<re_irc>
<@adamgreig:matrix.org> you'd have every HAL provide a method that constructs a CriticalSectionDevice, ExclusiveDevice, MutexDevice, RefCellDevice but passing in a hal-appropriate delay?
<re_irc>
<@dirbaio:matrix.org> : then you need to make Operation generic on the Delay, which makes SpiDevice non-dyn-safe
<re_irc>
and you'll get "can't infer D" errors if you don't have any Operation::Delay, which you'll have to fix with turbofish.
<re_irc>
<@diondokter:matrix.org> : Ha, what if it's "Operation::Delay(&dyn DelayUs)"
<re_irc>
<@dirbaio:matrix.org> : nope, it's not viable for non-toy projects
<re_irc>
<@adamgreig:matrix.org> clonable easily-accessible Delays would be nice but they definitely don't exist everywhere yet either
<re_irc>
<@adamgreig:matrix.org> but my main concern is the ergonomics of having to clone one to make the Device, regardless of how easy the HAL makes it to cloen one
<re_irc>
<@dirbaio:matrix.org> : that's an implementation quality issue. We shouldn't make the embedded-hal traits worse because some hals are bad
<re_irc>
<@dirbaio:matrix.org> like
<re_irc>
<@dirbaio:matrix.org> the HAL knows the core freq
<re_irc>
<@dirbaio:matrix.org> it could even provide one using "cortex_m::asm::delay()"
<re_irc>
<@dirbaio:matrix.org> +so
<re_irc>
<@adamgreig:matrix.org> presumably we don't want/need HAL crates depending on e-h-bus, which they'd have to do to provide wrappers?
<re_irc>
<@dirbaio:matrix.org> : that's true
<re_irc>
<@dirbaio:matrix.org> :S
<re_irc>
<@almindor:matrix.org> I don't plan to use e-h-bus with e310x-hal for example
<re_irc>
<@almindor:matrix.org> brings nothing to the table
<re_irc>
<@adamgreig:matrix.org> sure, but your users will quite probably use e-h-bus
<re_irc>
<@adamgreig:matrix.org> since it provides a ton of convenient ways to turn your SpiBus into a bunch of SpiDevices
<re_irc>
<@dirbaio:matrix.org> : if you hardcode RefCell-based mutexes then you can't share across priority levels
<re_irc>
<@dirbaio:matrix.org> while you'd be able to with CriticalSectionDevice
<re_irc>
<@adamgreig:matrix.org> so the question is how easily those users can construct a e-h-b Device when all the constructors require a DelayUs impl
<re_irc>
<@dirbaio:matrix.org> PanickingDelay?
<re_irc>
<@adamgreig:matrix.org> and e-h-b gets new constructors that use it?
<re_irc>
<@dirbaio:matrix.org> or add impls with and without delay, and the ones without delay panic
<re_irc>
<@adamgreig:matrix.org> or just say "meh, users have to pass in one more argument, whatever"
<re_irc>
<@dirbaio:matrix.org> but again that's impl
<re_irc>
<@dirbaio:matrix.org> please let's decide what we do with the trait
<re_irc>
<@adamgreig:matrix.org> : yea, this sort of thing, though it could be two constructors and an Option<D> on the type, whatever
<re_irc>
<@dirbaio:matrix.org> what to do with the impl follows from that
<re_irc>
<@adamgreig:matrix.org> does anyone disagree with adding the Operation::DelayUs to the trait?
<re_irc>
2- Mandate SpiDevice supports delays
<re_irc>
3a- Allow SpiDevices to optionally support delays, document that they should return DelayNotSupported otherwise if a transaction has a delay op
<re_irc>
<@dirbaio:matrix.org> 1- Simply not support delays. (status quo)
<re_irc>
3b- Allow SpiDevices to optionally support delays, document that they should panic otherwise if a transaction has a delay op
<re_irc>
4- split the trait into SpiDevice, SpiDeviceWithDelay, Operation, OperationWithDelay
<re_irc>
<@adamgreig:matrix.org> I think the discussion has primarily been around how we handle the downstream implications in e-h-b and in hals
<re_irc>
<@almindor:matrix.org> i'll abstain it's not strong enough of an issue to block things on
<re_irc>
3a- Allow SpiDevices to optionally support delays, document that they should return DelayNotSupported otherwise if a transaction has a delay op
<re_irc>
<@dirbaio:matrix.org> IMO the only valid ones are these, the rest is bad
<re_irc>
2- Mandate SpiDevice supports delays
<re_irc>
3a- Allow SpiDevices to optionally support delays, document that they should return DelayNotSupported otherwise if a transaction has a delay op
<re_irc>
<@almindor:matrix.org> i'd rather do #2 if we have to then to keep it straightforward
<re_irc>
<@almindor:matrix.org> you can always provide a NoDelayPanic thing that implements DelayUs on MCUs without delay :D
<re_irc>
<@diondokter:matrix.org> 2 and 3b are the same traits, but only have a small difference in documentation
<re_irc>
<@adamgreig:matrix.org> I think the only difference in 2 and 3b could be that the impls we provide in e-h-b document that they'll panic if there's a delay
<re_irc>
<@adamgreig:matrix.org> you could have the actual trait be identical either way
<re_irc>
<@dirbaio:matrix.org> or actually, a variant of 3b:
<re_irc>
- document "SpiDevice must support delays" in e-h. (i.e. encourage full support. Don't add an error kind, don't encourage panicking)
<re_irc>
- impls are still free to panic. Provide one in e-h-b, with a warning in the docs.
<re_irc>
<@diondokter:matrix.org> Yeah, I like that
<re_irc>
<@dirbaio:matrix.org> but at least we don't make it an "officially sanctioned" thing to do
<re_irc>
<@almindor:matrix.org> wait so will e-h-b require the delay provider (in the happy path), or will it panic by default in case of delay call?
<re_irc>
<@adamgreig:matrix.org> irrelevant to the trait itself
<re_irc>
<@adamgreig:matrix.org> we can do one or the other or change our mind later or whatever
<re_irc>
<@almindor:matrix.org> yes, true, but I want to make sure we don't default to shadow-panics
<re_irc>
<@dirbaio:matrix.org> I'd do "new(bus, delay)" and "new_panicking_delay(bus)" or something like that
<re_irc>
<@adamgreig:matrix.org> maybe new_no_delay would be a less scary way to phrase it but yea
<re_irc>
<@dirbaio:matrix.org> making it scary is good
<re_irc>
<@dirbaio:matrix.org> idk lol
<re_irc>
<@adamgreig:matrix.org> I don't think it needs to be scary
<re_irc>
<@adamgreig:matrix.org> I expect it will be the method I use every single time lol
<re_irc>
<@dirbaio:matrix.org> yeah...
<re_irc>
<@almindor:matrix.org> i'd avoid the panic one like the plague :D
<re_irc>
<@almindor:matrix.org> IMO it should be "unsafe"
<re_irc>
<@adamgreig:matrix.org> why?
<re_irc>
<@jannic:matrix.org> I'd say just "new(bus, delay)" is enough. Authors of toy firmwares will likely copy some example anyway, and if you are developing something serious, the additional effort to provide a delay is insignificant.
<re_irc>
<@dirbaio:matrix.org> "unsafe" has a very concrete meaning: may cause undefined behavior if you use it wrong
<re_irc>
<@almindor:matrix.org> because then someone ends up debugging their edge case panic the kernel for 2 days coz "abstraction reasons". If I wanted super-simple badly-designed abstractions I'd be using the Go standard library
<re_irc>
<@dirbaio:matrix.org> panics are not undefined behavior
<re_irc>
<@adamgreig:matrix.org> panicking's not unsafe, and it's only going to panic when you end up using a driver that requires a delay, which I think will be very rare, and when it does you can then change to giving it a delay
<re_irc>
<@adamgreig:matrix.org> the edge case panic's going to be very easy to debug
<re_irc>
<@adamgreig:matrix.org> "oops: tried to delay but can't"
<re_irc>
<@dirbaio:matrix.org> and the panic literally will say "you tried to do a delay on a SpiDevice created with "new_panicking_delay()", don't do that"
<re_irc>
<@adamgreig:matrix.org> "oh, I guess i'd better give this device a delay then"
<re_irc>
<@almindor:matrix.org> i know, but I'd def. wouldn't want to not be aware something is panic capable on kernel level development
<re_irc>
<@dirbaio:matrix.org> then you're free to not choose that
<re_irc>
<@adamgreig:matrix.org> I think it would be documented just like any other method that might panic
<re_irc>
<@adamgreig:matrix.org> i.e. a "panics" section in the doc string
<re_irc>
<@dirbaio:matrix.org> there's lots of stuff in Rust that panic and are not unsafe
<re_irc>
<@adamgreig:matrix.org> we don't usually put "panic" in the method _name_ for any method that might panic
<re_irc>
<@jannic:matrix.org> Bastard driver author from hell writes driver which inserts delay(0) operation into every 10000th transaction.
<re_irc>
<@adamgreig:matrix.org> that author can just insert a panic() call...
<re_irc>
<@almindor:matrix.org> this is also more of a "second hand" panic, coz it happens way down the drain so I'd rather have a "new_device_that_panics" kind of thing for it
<re_irc>
<@diondokter:matrix.org> The only problem with the panic thing is, is that a library could make a minor update that adds a delay. So your build could break.
<re_irc>
<@diondokter:matrix.org> * An extra
<re_irc>
<@eldruin:matrix.org> if this gets so complicated, we should keep in mind the option to not support it in "SpiDevice" and tell people to use "SpiBus" for those weird devices. From what I read it would be the simpler version for the 90%
<re_irc>
<@jannic:matrix.org> Not literally. More like: Only if the connected spi device, for some reason, responded with an error to the previous call. Just to be safe, add a delay into the next one.
<re_irc>
<@adamgreig:matrix.org> I admit it could happen, but I think it's exceedingly unlikely to cause problems, and if it did, I think they'd be easy to fix
<re_irc>
<@adamgreig:matrix.org> but, we can have this discussion about the e-h-b methods another time?
<re_irc>
<@jannic:matrix.org> Yes, I agree, it's probably completely theoretical and won't happen in practice.
<re_irc>
<@adamgreig:matrix.org> the naming/existence of an e-h-b convenience to make a panicking delay aside, is everyone happy with the change the spi trait?
<re_irc>
<@adamgreig:matrix.org> : i.e. this
<re_irc>
<@dirbaio:matrix.org> rough sketch of the e-h-b impls:
<re_irc>
pub struct NoDelay;
<re_irc>
/// Dummy `DelayUs` implementation that panics on use.
<re_irc>
fn delay_us(&mut self, us: u32) {
<re_irc>
impl embedded_hal::delay::DelayUs for NoDelay {
<re_irc>
panic!("You've tried to execute a SPI transaction containing a `Operation::Delay` in a `SpiDevice` created with `new_no_delay()`. Create it with `new()` instead, passing a `DelayUs` implementation.")
<re_irc>
<@adamgreig:matrix.org> oh I had ignored that but have now responded
<re_irc>
<@dirbaio:matrix.org> I dont think codespaces need any setup in the repo. you click a button and you get dropped to a vscode with the repo cloned and that's it
<re_irc>
<@therealprof:matrix.org> : I've no idea what a GH codespace/workspace even is.
<re_irc>
<@adamgreig:matrix.org> yea, I don't think it's a request that makes any sense
<re_irc>
<@dirbaio:matrix.org> approvals appreciated and encouraged :D
<re_irc>
<@innerand:matrix.org> Hi! Is there a room for (embedded related) beginner questions?
<re_irc>
<@diondokter:matrix.org> innerand: If it's Rust related, you're in it
dc740 has joined #rust-embedded
<re_irc>
<@innerand:matrix.org> Ok, thanks . I am looking for kind of a best practice example for register access within interrupt handlers (PAC already taken). At least I'd like to clear the interrupt flag within the interrupt handler.
<re_irc>
What I found is this is some mutex/RefCell thingy, which looks like a bit exaggerated to me...
<re_irc>
<@diondokter:matrix.org> Right, well using a mutex is best practise!
<re_irc>
<@diondokter:matrix.org> You could also look at using an executor like RTIC that manages the global resources for you
<re_irc>
<@diondokter:matrix.org> But often using the Mutex of the critical-section crate is sufficient. It has almost no memory overhead and if you don't have the requirement of real-time execution, that critical section won't do much harm
<re_irc>
<@dirbaio:matrix.org> or just unsafely steal the peripherals in the interrupt
<re_irc>
<@diondokter:matrix.org> : Well yeah, if you don't keep an instance of the peripheral in your non-interrupt code around
<re_irc>
<@dirbaio:matrix.org> there's nothing wrong with keeping multiple instances, it's not UB
<re_irc>
<@dirbaio:matrix.org> if you do just register access
<re_irc>
<@innerand:matrix.org> Ok, I think RTIC is bit too much abstraction for me at the moment (but I really like the approach). Will have a look at the critical section crate.
<re_irc>
<@diondokter:matrix.org> Surely getting an interrupt while doing a read-modify-write operation is UB, right?
<re_irc>
<@diondokter:matrix.org> Agreed that the instance itself isn't a problem
<re_irc>
<@dirbaio:matrix.org> it's not UB, it's just wrong
<re_irc>
<@diondokter:matrix.org> Hmmmm
<re_irc>
<@dirbaio:matrix.org> UB means "anything can happen"
<re_irc>
<@dirbaio:matrix.org> ie the compiler is allowed to miscompile your code
<re_irc>
<@dirbaio:matrix.org> a race on a reg is not UB
<re_irc>
<@dirbaio:matrix.org> say value is initially 10, main does "read, add 1, write", interrupt does "store 10"
<re_irc>
<@dirbaio:matrix.org> if the interrupt runs before the RMW, the result is 11
<re_irc>
if the interrupt runs interleaved with the RMW, the result is 1
<re_irc>
if the interrupt runs after the RMW, the result is 10
<re_irc>
<@thejpster:matrix.org> It's UB if you have two mutable references to the same memory location at the same time. But if you're writing through a volatile pointer, it's not UB. Just hazardous.
<re_irc>
<@dirbaio:matrix.org> that's all the 3 possible outcomes
<re_irc>
<@dirbaio:matrix.org> behavior is perfectly defined, is exactly one of these three
<re_irc>
<@dirbaio:matrix.org> it will probably be not the desired behavior, but it's definitely not UB
<re_irc>
<@adamgreig:matrix.org> how do you get a result of 1?
<re_irc>
<@thejpster:matrix.org> Maybe we should add an alias for Mutex<RefCell<Option<T>>> to the embedded-hal ...
<re_irc>
<@thejpster:matrix.org> Might be less scary then.
<re_irc>
<@dirbaio:matrix.org> : main reads 0
<re_irc>
interrupt writes 10
<re_irc>
main writes 0+1
<re_irc>
<@adamgreig:matrix.org> how does main read 0?
<re_irc>
<@explodingwaffle101:matrix.org> i always thought data races were UB? though I admit I never understood how
<re_irc>
<@dirbaio:matrix.org> inital value is 0
<re_irc>
<@adamgreig:matrix.org> : > <@dirbaio:matrix.org> say value is initially 10, main does "read, add 1, write", interrupt does "store 10"
<re_irc>
<@dirbaio:matrix.org> sorry, typoed 👊
<re_irc>
<@dirbaio:matrix.org> * 🤦♂️
<re_irc>
<@adamgreig:matrix.org> "what sorcery is this???"
<re_irc>
<@dirbaio:matrix.org> say value is initially 0, main does "read, add 1, write", interrupt does "store 10"
<re_irc>
<@thejpster:matrix.org> or, a "finger race hazard"
<re_irc>
<@dirbaio:matrix.org> it is UB if you use "*mut u8" and ".read()", ".write()"
<re_irc>
<@dirbaio:matrix.org> it is not UB if you use "AtomicU8" and ".load()", ".store()"
<re_irc>
<@explodingwaffle101:matrix.org> we may have strayed from the "beginner question" 😅
<re_irc>
<@innerand:matrix.org> How about putting every register modification into a critical section (disabling interrupts should be cheap, register modify / write is short)?
<re_irc>
<@innerand:matrix.org> Stealing in the interrupt handler should be fine, or do I miss something?
<re_irc>
<@innerand:matrix.org> * fine then,
<re_irc>
<@diondokter:matrix.org> innerand: Yes, that's what the critical-section Mutex does. It allows you to only access the inner value if a critical section is active. Because it enforces it, we have a guarantee and don't need unsafe in the API
<re_irc>
<@diondokter:matrix.org> * only allows you to
<re_irc>
<@thejpster:matrix.org> But the PAC doesn't do that because Rust is _explicit_ and implicit interrupt disables are not ideal. If you want them, you opt-in and you know you're paying the price.
<re_irc>
<@innerand:matrix.org> Ah, I see..
<re_irc>
<@innerand:matrix.org> Thanks a lot guys!
<re_irc>
<@adamgreig:matrix.org> it's a bit of a stretch but if anyone here happens to be planning on/thinking of going to the ESA software product assurance workshop this year and would be interested in giving a talk about Rust there (I guess in exchange for tickets but not in exchange for any travel allowance/accommodation), it sounds like they'd be interested
<re_irc>
<@thejpster:matrix.org> Ferrous was asked. We're strongly considering it.
<re_irc>
<@diondokter:matrix.org> Sounds cool. I'll ask if we want to do anything with this
crabbedhaloablut has quit [Ping timeout: 240 seconds]
<re_irc>
<@thalesfragoso:matrix.org> : Well, you can't use atomics for device memory. I get that it will get lowered to normal ldr/str in thumb, but that seems dangerous
cr1901 has quit [Read error: Connection reset by peer]
cr1901 has joined #rust-embedded
<re_irc>
<@adamgreig:matrix.org> sorry to be nitpicking wording , I saw the Delay/DelayUs typo and just deleted the 'one' while I was making the suggestion
<re_irc>
<@dirbaio:matrix.org> 🤣
<re_irc>
<@dirbaio:matrix.org> perhaps "that contains an "Operation::DelayUs""?
<re_irc>
<@dirbaio:matrix.org> I agree the original "that contains one"Operation::DelayUs"" is not quite right, it implies it wouldn't panic if there's two
<re_irc>
<@dirbaio:matrix.org> you could also read "an" that way though lols
<re_irc>
<@dirbaio:matrix.org> "that contains one or more operations of type "Operation::DelayUs""?
<re_irc>
<@adamgreig:matrix.org> "an" reads totally fine to me but I agree in theory one could argue it implies singular lol
<re_irc>
<@adamgreig:matrix.org> the final one is very spec-lawyering but totally precise and unambiguous... you could say "that contains any operations of type Operation::DelayUs"?
<re_irc>
<@dirbaio:matrix.org> changed
dc740 has quit [Remote host closed the connection]