crabbedhaloablut has quit [Remote host closed the connection]
crabbedhaloablut has joined #rust-embedded
dc740 has quit [Ping timeout: 268 seconds]
<re_irc>
<adamgreig> dirbaio: I wonder if CriticalSection could be a tuple struct like "struct CriticalSection<'cs>(PhantomData<&'cs>)" instead of having a "_0" member
starblue has quit [Ping timeout: 255 seconds]
starblue has joined #rust-embedded
causal has joined #rust-embedded
crabbedhaloablut has quit [Remote host closed the connection]
<re_irc>
<adamgreig> since it's private anyway I feel like it shouldn't be breaking, and also it's effectively a whole new type in c-s 1.0 so just a question of whether it changes migration effort I guess?
<re_irc>
<dirbaio> well, compat matters if we want to do the "change bare-metal to reexport" thing
<re_irc>
<adamgreig> yea, that's true
<re_irc>
<adamgreig> I feel like it's probably just used opaquely by anything outside the crate, since it can't be constructed except using "new()"
<re_irc>
<dirbaio> btw cortex-m 0.7 is still on bare-metal 0.2
<re_irc>
<adamgreig> yea, iirc bumping that to 1.0 was a breaking change
<re_irc>
<dirbaio> adamgreig: perhaps keep the "{}" syntax, but change "_0" to "_private"?
<re_irc>
<dirbaio> that's what I've seen done most often
<re_irc>
<dirbaio> it makes it clear it's a "dummy private field" to ensure the struct can't be constructed in other crates
<re_irc>
<adamgreig> hmm yea, I guess that works
<re_irc>
<adamgreig> looks less weird than _0 and clearly isn't a compatibility hazard
dc740 has quit [Ping timeout: 244 seconds]
<re_irc>
<adamgreig> shall I stick that into my pr?
brazuca has joined #rust-embedded
<re_irc>
<dirbaio> yes please! :)
<re_irc>
<dirbaio> hmm something wrong with bors :D
<re_irc>
<adamgreig> as usual :P the checks in bors.toml look to be spelt correctly but this failure usually indicates it's just not picked them up...
<re_irc>
<dirbaio> is it "Clippy check / clippy (pull_request)", "Clippy check / clippy", or just "Clippy check"
<re_irc>
<dirbaio> * check"? 🤣
<re_irc>
<dirbaio> there's the latter
<re_irc>
<dirbaio> * right now it's
<re_irc>
<adamgreig> perhaps it needs to be the middle one
<re_irc>
<dirbaio>
<re_irc>
<dirbaio> okay it was the middle one
<re_irc>
<adamgreig> I wonder if the functions should be extern(C) to allow a C implementation to provide them (eg an rtos) or consume them (??)
<re_irc>
<dirbaio> hmm I'd consider these fns to be "implementation details"
<re_irc>
<dirbaio> the only "public api" is the trait and "set_impl!()"
<re_irc>
<dirbaio> so if you want a CS impl that calls into C, you have write a bit of Rust glue using "set_impl!", instead of implementing the externs directly from C
<re_irc>
<adamgreig> I guess that's OK, and I feel like the other way around is pretty rare (in the worst case you could always write some rust glue that does expose an extern C fn)
<re_irc>
<dirbaio> yeah
rardiol has quit [Ping timeout: 252 seconds]
brazuca has quit [Quit: Client closed]
gsalazar has quit [Ping timeout: 268 seconds]
gsalazar has joined #rust-embedded
rardiol has joined #rust-embedded
<re_irc>
<adamgreig> dirbaio: just before you hit release, might be worth a quick check over the mutex stuff, I forgot to look when I was checking the rest last night
crabbedhaloablut has quit [Remote host closed the connection]
crabbedhaloablut has joined #rust-embedded
<cr1901>
James Munns: (hopefully the bridge is working today) https://github.com/jamesmunns/bbqueue#static-usage Re: "These halves can be sent to different threads or to an interrupt handler for thread safe SPSC usage", how do you Send safely to an interrupt handler without using RefCell?
<cr1901>
It would be different if prod/cons didn't have methods that take &mut self, but as the crate is, I can't visualize how to Send data to an interrupt handler (i.e. must be global/be Sync)
<cr1901>
err, ignore that second msg, I'm combining two separate thoughts into one
<re_irc>
<dirbaio> is "repr(transparent)" considered part of the "public API"? like, is it a "semver promise" that the transmute is allowed, therefore removing it is semver-breaking?
<re_irc>
<dirbaio> perhaps it'd be more conservative to _not_ have it then? we can always add it later, which wouldn't be a breaking change (?)
<re_irc>
<jannic> It's probably fine both ways, I can neither see any use case for the "repr(transparent)", nor a situation where we would want to remove it later. But I wondered why it was added in the first place - perhaps there actually is some use case I am missing?
<re_irc>
<dirbaio> yeah i'm in the same boat
<re_irc>
<dirbaio> slightly leaning towards removing it if it's considered a semver-promise that the transmute is allowed
crabbedhaloablut has quit [Remote host closed the connection]
crabbedhaloablut has joined #rust-embedded
<re_irc>
<adamgreig> dirbaio: nomicon says "This repr is only considered part of the public ABI of a type if either the single field is pub, or if its layout is documented in prose. Otherwise, the layout should not be relied upon by other crates."
<re_irc>
<dirbaio> ooh interesting
<re_irc>
<adamgreig> but yes, I think the technical use-case is strictly 1) transmutability 2) FFI safe
<re_irc>
<adamgreig> I added it because it also seems nice spiritually for wrapper types that should be exactly their contents, but I don't think I have a stronger technical argument if we don't plan on specifically guaranteeing transmutability
<agg>
cr1901: ugh, yes, you have fallen off again.... what the heck
<re_irc>
<dirbaio> leaving it in sounds good tom e then
<agg>
(dirbaio, cr1901 linked to that page in irc earlier this evening, I now see...)
<re_irc>
<dirbaio> * to me
<agg>
of course the bridge works fine for me...
<re_irc>
<dirbaio> ooh the bridge
<agg>
cr1901: could you try parting and rejoining the channel? i'll keep an eye on the logs...
<re_irc>
<dirbaio> anyway adamgreig do you think the mutex stuff is OK? shall I release? :D
<re_irc>
<adamgreig> dirbaio: this might be nice if there is a use case for getting the raw value out of the wrapper
<re_irc>
<adamgreig> ah I see renaming GH_6 is done, nice
dc740 has quit [Remote host closed the connection]
<re_irc>
<dirbaio> updating CS 0.2 -> 1.0 is breaking, so this'll need "atomic-polyfill" 0.2
<re_irc>
<dirbaio> and again in turn, updating AP 0.1 -> 0.2 is breaking too
<re_irc>
<dirbaio> should the new atomic-polyfill be 1.0 too? 🤔
<re_irc>
<adamgreig> 🤔
<re_irc>
<dirbaio> the only dep is CS1.0
<re_irc>
<adamgreig> it seems like it doesn't need to as much
<re_irc>
<dirbaio> and the public API is exactly the same as "core::sync::atomic", so it'll never ever change :D
<re_irc>
<adamgreig> until core::sync::atomic changes 🙈
<re_irc>
<adamgreig> but yea, maybe it can be 1.0 then
<re_irc>
<dirbaio> the only reason for breaking it is some blunder in how polyfilling is done
<re_irc>
<dirbaio> +design
<re_irc>
<dirbaio> (supplying unsound impls by default in CS was a blunder yup)
<re_irc>
<jannic> Regarding atomic-polyfill I wonder if it should provide separate atomic types that only provide load/store, not CAS. That would allow for more efficient implementations on platforms which provide native atomic types, but CAS needs a critical section.
<re_irc>
Not sure if the performance / code size advantage warrants the more complicated API.
<re_irc>
<dirbaio> oh, so
<re_irc>
<dirbaio> * yeah. it bothers me too that you have to eat the CS overhead even if you want only load/store
<re_irc>
<jannic> Crates which need atomic load/store without CAS could use the less powerful type and therefore allow for more efficient code.
<re_irc>
<dirbaio> so, it'd be an "AtomicLoadStoreU32" that only allows load/store
<re_irc>
<adamgreig> what platforms would need atomic-polyfill for those types? what would it even look like?
bitbangerror has quit [Ping timeout: 252 seconds]
<re_irc>
<dirbaio> so on thumbv6 it can compile to native atomics, only non-32-bit targets like msp430 would need CS
<re_irc>
<adamgreig> I guess you could implement AtomicLoadStoreU64 even on a cortex-m or something
<re_irc>
<adamgreig> native load/store on thumbv6, no such thing as a "native atomic" right
<re_irc>
<dirbaio> "native atomic load/store" :P
<re_irc>
<adamgreig> I wonder if there's a technical computer science term for a load-store-only atomic
<re_irc>
<dirbaio> but yeah, that
<re_irc>
<adamgreig> (i.e. not an exclusive load/store which is an arm thing on armv7-m etc)
<re_irc>
<adamgreig> I guess most crates would probably just use core::sync::atomic::AtomicU32 and only do load/store ops and that would work OK on a thumbv6 (and risc-v etc thanks to the new changes in LLVM), but then they'd find it didn't work on AVR or MSP430 or whatever and could swap to atomic_polyfill::AtomicLoadStoreU32 an
<re_irc>
<dirbaio> I hate the fact that "atomic_polyfill" needs to exist though 🤪
<re_irc>
<adamgreig> heh, ideally that's what compiler_builtin would do huh
<re_irc>
<dirbaio> yeah
<re_irc>
<dirbaio> rust has this "atomics should be either native or unavailable" thing though :(
<re_irc>
<dirbaio> there should be some flag you can enable to tell it "gimme the full "core::sync::atomic", even if you have to lower it to libcalls"
<re_irc>
<jannic> Another possible issue: atomic-polyfill emulates atomics by wrapping an UnsafeCell by a critical section. How does that guarantee proper ordering? critical-section explicitly states that it's the caller's responsibility to provide proper fences, which atomic-polyfill doesn't.
<re_irc>
<dirbaio> adamgreig: it's an alpha though, I'd wait for 1.0
<re_irc>
<dirbaio> > critical-section explicitly states that it's the caller's responsibility to provide proper fences
<re_irc>
wut, where?
<re_irc>
<jannic> Ah sorry, it's only on the unsafe "new" function.
<re_irc>
<jannic> But then, where are the fences in critical-section?
<re_irc>
<dirbaio> hmm it's not explicitly stated
<re_irc>
<dirbaio> but the critical-section impl should do the fencing, so that a release "happens-before" the next acquire
<re_irc>
<dirbaio> for example in the cortex-m impl, the fences are in "interrupt::{disable, enable}"
<re_irc>
<dirbaio> perhaps docs could be expanded, but I think "the impl must provide properly functioning critical sectioning" already implies fences
<re_irc>
<dirbaio> * critical-sectioning"
<re_irc>
<jannic> Ah ok, yes, just was too well hidden inside the assembly implementation of "__cpsid"/"__cpsie".
<re_irc>
<jannic> IIRC if one uses AtomicU32, the generated binary contains additional "dmb" instructions, which are not part of the "compiler_fence" used by "__cpsid"/"__cpsie". However that should be fine as "dmb" is not necessary for single-core processing. (And probably also for the rp2040 dual-core, as there seems to be no buffering involved in memory accesses on that MCU)
<re_irc>
<dirbaio> hmmm
<re_irc>
<dirbaio> if the "dmb" is needed, the CS impl would have to be the one emitting it
<re_irc>
<jannic> Yes, that's why I wonder if I should add those to to the "rp2040-hal" impl.
<re_irc>
<dirbaio> so it's ok for the load/store to be unsynced, the CS impl will sync it
<re_irc>
<dirbaio> perhaps check what the C sdk does for mutexes?
<re_irc>
<dirbaio> atomic-polyfill 0.1.9 released, with the soundness fix
<re_irc>
yanked 0.1.0-0.1.8
<re_irc>
atomic-polyfill 1.0.0 released, using CS1.0
<re_irc>
<jannic> dirbaio: good suggestion, but I'm too tired to read C code now. 😴
<re_irc>
<adamgreig> lol, on cortex-m3 and cortex-m4 dmb is always useless iirc
<re_irc>
<adamgreig> anyway cpsid is self synchronising, but i think cpsie is not? but that's not usually an issue
<re_irc>
<adamgreig> not sure about cm0+ in this context
<re_irc>
<adamgreig> ARM say "This simple implementation results in a reduced need for barriers in a number of situations when code is only intended to run on Cortex-M3 or Cortex-M4. All use of DMB is redundant due to the inherent ordering of all loads and stores on Cortex-M3 and Cortex-M4." lol
<re_irc>
<adamgreig> "all loads and stores always complete in program order"
<re_irc>
<adamgreig> ah, that is also true of cm0... how about cm0+
<re_irc>
<adamgreig> yes, also cm0+
<re_irc>
<adamgreig> they say "This simple implementation results in a reduced need for barriers in a number of situations when code is only intended to ever run on the Cortex-M0+. All use of DMB is redundant due to the inherent ordering of all loads and stores on the Cortex-M0+."
<re_irc>
<adamgreig> there is a good question of "what does a compiler fence even do" for cortex-m, maybe they should be replaced by full fences anyway
<re_irc>
<dirbaio> full fences emit DMB though don't they?
<re_irc>
<dirbaio> so compiler fence is like a way of saying "I want a full fence but I know my hardware doesn't need DMB"
<re_irc>
<adamgreig> welllllllllll
<re_irc>
<dirbaio> 🤣
<re_irc>
<adamgreig> I think the current understanding is that compiler_fence emits llvm "fence" instructions, which llvm say only affect atomics
<re_irc>
<adamgreig> i.e. it ensures atomic accesses won't be re-ordered around the fence, but not any other accesses
<re_irc>
<jannic> Doesn't the compiler fence just guarantee that the compiler doesn't reorder, and dmb guarantees that the hardware doesn't reorder. A full fence includes both.
<re_irc>
<adamgreig> that would be nice
<re_irc>
<adamgreig> (so e.g. if you did like "write to descriptor, compiler_fence, write descriptor address to mmio register", it's not actually guaranteed that the first write goes through before you send the address to the mmio)
<re_irc>
<jannic> If even Ralf Jung doesn't know, there's no hope...
<re_irc>
<adamgreig> DMB in ARMv6-M/ARMv7-M does "ensures that all explicit memory accesses that appear, in program order, before the DMB instruction are completed before any explicit memory accesses that appear, in program order, after the DMB instruction"
<re_irc>
<adamgreig> but I don't know that atomic::fence(Ordering::SeqCst) is guaranteed to emit a DMB
<re_irc>
<adamgreig> and cortex-m0/m0+/m3/m4 always complete all loads and stores in program order
<re_irc>
<adamgreig> I think dmb only does anything by the time you get to cortex-m7?
<re_irc>
<dirbaio> hm nrf-hal/embassy-nrf are full of "compiler_fence" for DMA, are these noop? 😂
<re_irc>
<adamgreig> (as an aside it seems like in practice compiler_fence usually has the effect of stopping normal memory accesses being re-ordered around it)
<re_irc>
<adamgreig> just... that's not what llvm _says_ it's for/does
<re_irc>
<dirbaio> for example to ensure the buffer writes are done before starting DMA
<re_irc>
<dirbaio> wtf
<re_irc>
<adamgreig> indeed
<re_irc>
<adamgreig> ARM DAI 0321A, "ARM Cortex™-M Programming Guide to Memory Barrier Instructions" is a great reference for "do i need a barrier here..." though it's silent on the matter of rust's compiler_fence
<re_irc>
<adamgreig> but yea, cortex-m's cpsie/cpsid functions have a compiler fence "to ensure nothing moves before/after" but I wonder if that's really correct at all
<re_irc>
<adamgreig> perhaps it should be an isb after the cpsie
<re_irc>
<adamgreig> the ARMv6/v7 thing is cursed because the arch spec would require a lot of barriers that are explicitly not required in cm0/cm0+/cm1/cm3/cm4 but might be in cm7...
<re_irc>
<dirbaio> adamgreig: or maybe memory clobbers in the "asm!"
<re_irc>
<dirbaio> like the "empty asm block with memory clobber" suggested fix for compiler fences 🤣
<re_irc>
<adamgreig> hah yea
<re_irc>
<adamgreig> but I don't really care about memory accesses in particular with cpsie/cpsid
<re_irc>
<dirbaio> well you want to prevent reordering memory accesses
<re_irc>
<dirbaio> oh you mean "isb" for preventing reordering _instructions_?? 👻
<re_irc>
<adamgreig> well not quite, isb would ensure any pending interrupt can be serviced right away before any instructions immediately following cpsie, e.g. an immediate cpsid
<re_irc>
<adamgreig> on cortex-m "cpsid, cpsie" will still run pending interrupts in the middle, but in armv6-m/v7-m generically you should have isb in the middle if you want that
<re_irc>
<adamgreig> arm say in general architecturally no barriers _required_ before or after cpsie/cpsid unless you do want to take pending interrupts before an immediate following cpsid, but in all cortex-m impls that happens anyway even without the isb
<re_irc>
<adamgreig> so i guess in conclusion i don't need any hardware barriers and just need to ensure rust doesn't reorder at compile time around the cpsid/cpsie
<re_irc>
<dirbaio> wow this is cursed
<re_irc>
<adamgreig> which you'd _hope_ the compiler fence would have done, lol
<re_irc>
<adamgreig> dirbaio: yea but this won't happen on hardware, only potentially by rust itself
<re_irc>
<dirbaio> anything I can do to help cortex-m 0.7.6? 👀
<re_irc>
<dirbaio> the "prepare release" PR?
<re_irc>
<adamgreig> PR's already up
<re_irc>
<dirbaio> ooh
<re_irc>
<adamgreig> yea
<re_irc>
<adamgreig> someone else on cortex-m just needs to check and r+ it, if it's ok then i'll publish