<barnabyw[m]>
usually what James said is true, and it’s not so far off in this case, but with the additional complication that there are two crates that you’d have to fork/send PRs to in order to get async working
<holo[m]>
barnabyw[m]: it sounds like lot of work and more knowledge needed which allredy have
<holo[m]>
will stick in that case with my solution it was working too
<barnabyw[m]>
personally, unless I was particularly invested in making an async library available for this sensor, I’d just make an async implementation of whatever logic was required for my specific project
<barnabyw[m]>
yeah, it could be a good learning experience if you felt like jumping into it, but probably not a good use of time if you have other goals. definitely worth filing issues on both of those other crates to implement async support though!
<barnabyw[m]>
the write_command_u8 and _u16 functions seem completely unnecessary, not sure what they‘re doing there 🤔
<barnabyw[m]>
pretty cool that sensirion have open source rust libraries for their sensors at all, though
<holo[m]>
<barnabyw[m]> "yeah, it could be a good..." <- im not feeling i have enough knowledge for now to make such task, today i was learning how to make modules/libs in Rust and i still need to look for examples everything what i want to do. Basically writing my own lib based on what i learned last time looks easier for me than working on somone else code 🙂
<holo[m]>
at least for such simple sensor
notgull has joined #rust-embedded
Socke has quit [Ping timeout: 260 seconds]
Socke has joined #rust-embedded
emerent has quit [Ping timeout: 260 seconds]
emerent has joined #rust-embedded
notgull has quit [Ping timeout: 255 seconds]
IlPalazzo-ojiisa has quit [Remote host closed the connection]
Shell has quit [Quit: ZNC 1.8.2+deb3.1 - https://znc.in]
Shell has joined #rust-embedded
jr-oss has quit [Ping timeout: 268 seconds]
cr1901 has quit [Ping timeout: 245 seconds]
cr1901 has joined #rust-embedded
notgull has joined #rust-embedded
notgull has quit [Ping timeout: 245 seconds]
jr-oss has joined #rust-embedded
Guest7282 has left #rust-embedded [Error from remote client]
mpiechotka[m] has joined #rust-embedded
<mpiechotka[m]>
2. Am I reading correctly that postcard enum serializations is extention compatible. I.e. if I have an enum E { ... } is serializer and enum E { ..., Var(...) } in deserializer than deserializer can deserialize.
<mpiechotka[m]>
1. Am I reading correctly that postcard struct serialization is prefix compatible? I.e. if I have a struct S { ... } in deserializer and struct S { ..., field: F } in serializer than deserializer can deserialize as long as it ignores 'junk' at the end (i.e. it's a topmost struct)
<JamesMunns[m]>
<mpiechotka[m]> "1. Am I reading correctly that..." <- > <@mpiechotka:matrix.org> 1. Am I reading correctly that postcard struct serialization is prefix compatible? I.e. if I have a struct S { ... } in deserializer and struct S { ..., field: F } in serializer than deserializer can deserialize as long as it ignores 'junk' at the end (i.e. it's a topmost struct)
<JamesMunns[m]>
> 2. Am I reading correctly that postcard enum serializations is extention compatible. I.e. if I have an enum E { ... } is serializer and enum E { ..., Var(...) } in deserializer than deserializer can deserialize.
<JamesMunns[m]>
I'm not 100% sure I follow, but I think yes, but ONLY at the top level. If the struct or enum is inside another struct or enum, it will no longer work.
<mpiechotka[m]>
JamesMunns[m]: Sorry. I try to clarify.
<mpiechotka[m]>
* to clarify and rewrote it several times.
<JamesMunns[m]>
I also just woke up and haven't had coffee yet, so it might be me :)
<mpiechotka[m]>
JamesMunns[m]: Why enum not work? All descriminant would be known so it should handle it properly?
<JamesMunns[m]>
Enum should work, though if you hit an unknown descriminant on deser, the whole deser will fail
<mpiechotka[m]>
JamesMunns[m]: Yeah. This is expected. In one case I try to read board information (extending enums in which case unknown board is failure condition anyway).
<mpiechotka[m]>
In struct case I try to parse fw update metadata and there serializer is by definition newer.
<JamesMunns[m]>
but yeah, in general you CAN append struct fields in the "top layer", or append enum variants, and it should be safe, but definitely test it out
<AdamHott[m]>
Wondering if someone could help me understand what I ordered. Did I order 5 PCB boards without assembly, the prototypes? And then I ordered 5 more boards with assembly?
<JamesMunns[m]>
Nah, that's just what JLCs system looks like
<AdamHott[m]>
Ah ok cool
<JamesMunns[m]>
you have one order of 5x boards, and you have a second order of 5x assembly
<JamesMunns[m]>
(that happens to be assembled on the 5x boards in the first line item)
<AdamHott[m]>
Ah ok, so what I'll receive is 5 boards?
<JamesMunns[m]>
yep, 5 boards with parts on the,.
<JamesMunns[m]>
s/the,./them./
<AdamHott[m]>
ok cool, that's what I wanted!
<AdamHott[m]>
It was like 2 am in the morning when I finalized the BOM and order, I was so tired....glad to see it seems to be working out!
M9names[m] has quit [Quit: Idle timeout reached: 172800s]
jannic[m] has quit [Quit: Idle timeout reached: 172800s]
FreeKill[m] has quit [Quit: Idle timeout reached: 172800s]
<AdamHott[m]>
Is this a good flux pen for soldering onto PCB?
<AdamHott[m]>
Description: Flux TK83 is a highly efficient solution for welding, composed of a combination of alcohol, resin and organic activators. This flux, which follows ISO 9454 type 1134 / J-STD-004 ROL0 specifications, is specially developed for manual tinning, with the aim of restoring the solderability of electronic... (full message at <https://catircservices.org/_matrix/media/v3/download/catircservices.org/SysXCilWKbkajDmqPtUqMmmw>)
<AdamHott[m]>
I'm worried about the maximum application temperature of 280 degrees Celsius, my soldering iron is hotter than this.
<AdamHott[m]>
My soldering iron has a maximum temperature of 410 degrees Celcius.
<JamesMunns[m]>
You might find a better forum that reviews flux temp, tho I'm unsure if the "application temp" means "how hot while soldering" and not "how hot when you put it on the board"
<JamesMunns[m]>
like your board will probably be room temp ish when you APPLY the flux, then you solder after that.
<AdamHott[m]>
ah ok, I'll research this further, thanks James!
sjm42[m] has joined #rust-embedded
<sjm42[m]>
410 wat lol
<sjm42[m]>
* 410°C wat lol
<sjm42[m]>
Yeah this is going fast offtopic, but I just have one link/hint about this particular subject.
<JamesMunns[m]>
I'm not sure why you `lol`'d at 410C as a soldering temp, then linked a soldering iron that has a max temp of 450C, or why you linked a soldering iron at all in response to "is this a good flux to use"?
<sjm42[m]>
Yeah and about flux: it's normal to solder at about 320-330°C with old school lead solder, and something like 360-380°C with unleaded and AFAIK normal flux works just fine.
<sjm42[m]>
The "application temperature" must mean the max temperature at the moment the flux is _applied_ just _before_ actual soldering. I cannot interpret it in any other way.
<sjm42[m]>
Recently I purchased some presumably high quality flux from digi-key and now I have to check out what do they actually say about temperatures...
<barnabyw[m]>
yeah I’d assume the application temperature is the temperature at which the fuzzy pen tip part is rated
<sjm42[m]>
Yeah it could be read as "at higher temperatures than this the actual flux will burn out and provide no more help..."
<GrantM11235[m]>
Should we put the gpio thing at the end in case it runs long?
eldruin[m] has joined #rust-embedded
<eldruin[m]>
we should put it at the beginning in case we can bring it to an end :P
<adamgreig[m]>
haha, yea, I don't think there's too much else that's urgent anyway
<adamgreig[m]>
ideally I need to leave a few minutes before we finish but we'll see how it goes :P
<dirbaio[m]>
yeah it's the most impactful thing to discuss right now, better do it first
<adamgreig[m]>
ok, let's begin, sorry we're a bit late!
<adamgreig[m]>
welcome back everyone, hope you all had a good couple weeks without meetings!
<adamgreig[m]>
two quick announcements from me, first is to welcome sirhcel to the working group and the linux team! 🎉
<adamgreig[m]>
and second is the advisory James Munns wrote for the cortex-m miscompilation with opt-level=z, which is still present in stable rust, https://github.com/rust-embedded/cortex-m/discussions/503 (with thanks to Peter Hansen for finding yet another outrageous bug)
<diondokter[m]>
Also wanted to mention this PR is in motion which should unlock some more float math functions for cortex-m if I understood it correctly: https://github.com/rust-lang/rust/pull/118645
<adamgreig[m]>
sweet, yes, looks good!
<dirbaio[m]>
also, Rust 1.75 is now out, with async-fn in traits support in stable, finally
<GrantM11235[m]>
Here is the summary of my point of view: It was originally done based on an incorrect assumption, and no one has been able to think of any real justifications (IMHO) for it since then. There are however some real downsides (nothing that can't be worked around, but still)
<GrantM11235[m]>
dirbaio: (By the way, your suggestion for the `keypad` crate is good. If I was the maintainer, I would use that instead.)
<adamgreig[m]>
there's been pretty extensive discussion both ways on the github issue, are there specific points that people want to discuss further?
<eldruin[m]>
> IIRC I didn't actually test changing InputPin to &mut self, but from looking through the code it seemed to me that it would fix it, which is why I listed it as a solution in #341.
<eldruin[m]>
I did test it, it didn't fix it.If we look at impls, the answer is always going to be "&mut self is better"
<eldruin[m]>
&mut self is worse for virtually all impls because it requires them to do impl InputPin for &Input. This alone is enough to outweigh any supposed benefits, IMO.
<GrantM11235[m]>
adamgreig[m]: That is my view also
<adamgreig[m]>
it's not quite like the gpio return type where _some_ impls might need to return an error, so we had to add error return: having &self doesn't stop _some_ impls using interior mutability
<thejpster[m]>
What did people do in eh 0.2? Was this a problem before for any implementation?
<GrantM11235[m]>
it has been &self since the begining
<adamgreig[m]>
right, makes sense. and the idea is that &mut is a bit better for some rare impls, and &self is a lot easier for drivers, but what's the balance?
<adamgreig[m]>
well, perhaps that's too strong and we don't have many good examples of how &self is that much better for drivers
<GrantM11235[m]>
Also, I think there are a limited number of possible impls and an unlimited number of possible drivers
<adamgreig[m]>
obviously it's easier for them, but perhaps in practice that's immaterial because other things are exclusive or they don't need shared access
<adamgreig[m]>
how so? there's an unlimited number of HALs and of chips you might make a HAL for, surely
jr-oss has quit [Ping timeout: 252 seconds]
<GrantM11235[m]>
adamgreig[m]: Yeah, that's probably not the best choice of words. What I mean is that impls are limited by the actual hardware that they support, but drivers can be anything you imagine. Does that make more sense, I'm not sure 😅
jannic[m] has joined #rust-embedded
<jannic[m]>
Is interior mutability always practical, or are there situations where it would be much too expensive (eg. no atomics, critical section is expensive etc.)?
<adamgreig[m]>
it's maybe fair to say that in practice there are more driver crates than implementation crates
<GrantM11235[m]>
jannic[m]: I think it is always practical. If you would have been ok with `&mut self`, that means you don't care about sharing, which means you definatly don't care about sharing across threads. therefor you don't need to worry about `Sync` and you can just use a `Cell`/`RefCell`
<adamgreig[m]>
dirbaio: are there any other surviving arguments either way, like consistency with other e-h traits?
<dirbaio[m]>
as soon as you do nontrivial mutable state you're going to need a RefCell
<adamgreig[m]>
otherwise it kinda comes down to the strength of one niche example vs zero examples
<thejpster[m]>
oh damn, filtering input pins.
<thejpster[m]>
(or button debouncers)
<dirbaio[m]>
IMO after the discussion I don't think HALs should be bothered with `impl InputPin for &Pin` nor `Clone`.
<GrantM11235[m]>
dirbaio[m]: Do you have any real world examples of that?
<jannic[m]>
GrantM11235[m]: I don't understand that. If you have something that can not be shared, and implementing locking is too expensive, you can still use `&mut` because that guarantees unique access.
<dirbaio[m]>
GrantM11235[m]: for example you want a heapless::HistoryBuffer to store the InputPin history for filtering/debouncing. That's going to need `&mut self`.
exark has quit [Quit: quit]
<thejpster[m]>
also "pins" that implement edge detection and poll as high exactly once.
<adamgreig[m]>
that wouldn't really be a good InputPin impl :P
<dirbaio[m]>
dirbaio[m]: because drivers never need to share InputPin ONLY, when they share inputpins they also share other stuff so they need RefCell anyway.
<dirbaio[m]>
so I think our sharing story is inevitably ending up with RefCells
<dirbaio[m]>
same as SPI and I2C sharing
exark has joined #rust-embedded
<thejpster[m]>
honestly I'd expect most drivers to take ownership of the pin anyway.
<GrantM11235[m]>
I think that sharing an input pin is a reasonable thing to do. I haven't found any publicly available examples of it, since there isn't a lot of public end user code, but I suspect that people are doing it
<cr1901>
This change doesn't affect (improve/worsen) SPI/I2C sharing ergonomics, correct?
<thejpster[m]>
and it seems fairly easy to construct some fake cloneable pin attached to some global atomic bool, so if you did have like four drivers that all wanted to own the same pin, so can just hand out fake pins and poll the real pin and use that to update the bool they all look at.
<GrantM11235[m]>
thejpster[m]: They take `impl InputPin`, which was previously satisfied by `&Input` due to the blanket impl
jr-oss has joined #rust-embedded
<GrantM11235[m]>
dirbaio[m]: But the `RefCell` is totally unneccesary for every known impl
<eldruin[m]>
cr1901: no, it's unrelated
<adamgreig[m]>
GrantM11235[m]: but it's necessary if you have anything in addition to the InputPin to share, which almost all (?) drivers do, right?
<thejpster[m]>
ok, to avoid walking in circles, what's the process for getting out of this?
<adamgreig[m]>
like if you are also sharing an SPI bus, or an output pin, or anything else from e-h, or some other state
<eldruin[m]>
* it's unrelated, except for bitbanging implementations :P
<thejpster[m]>
who gets to make the decision, how will they make it and when will they make it by?
<adamgreig[m]>
thejpster: discussion, followed by hal team vote
<dirbaio[m]>
GrantM11235[m]: I just showed an example of an impl that needs RefCell: an impl containing Vec or HistoryBuffer for debouncing.
<adamgreig[m]>
if contentious enough, hal team can have an rfc vote
<GrantM11235[m]>
adamgreig[m]: I think it is reasonable to share only an InputPin, for example an emergency stop button or something
<adamgreig[m]>
but I don't think it's outside of our normal scope of discussions and process for coming to a conclusion, we don't need to do anything special, the point now is to discuss it in detail
<dirbaio[m]>
GrantM11235[m]: No driver in the wild today does that.
<adamgreig[m]>
GrantM11235[m]: but what do you do if the estop is pushed? the action you take probably needs something that needed exclusive access?
<dirbaio[m]>
and they can still do it with a RefCell.
<thejpster[m]>
and you can trivially simulate that with an atomic bool
<adamgreig[m]>
the atomic bool doesn't help you share the InputPin if it requires &mut
<GrantM11235[m]>
dirbaio[m]: It might not be a driver, it could be end user code that isn't public
<adamgreig[m]>
for better or for worse the focus of e-h is on drivers and hals, the idea being that end user code can use concrete hal types (which might then not need &mut)
<adamgreig[m]>
if you want to write end user code that's generic over a HAL, then you're "writing a driver", I guess?
<GrantM11235[m]>
This is a big change at the last minute, and the only justification is that it might make a hypothetical adapter slightly more efficient
<jannic[m]>
thejpster[m]: Am I right that nobody has any example where one of the two choices would be blocking issue? It's all about preferences and what would be nicer or easier? Or asked in a different way: Would anyone be deeply disappointed with one of the two choices?
<jannic[m]>
If so, what about a vote to settle the issue? I don't think democratic decisions are necessarily best for technical questions, but in this case, I think the two choices are so similar that it just doesn't matter much.
<thejpster[m]>
I don't think debouncing is hypothetical.
<jannic[m]>
* would be a blocking issue?
<adamgreig[m]>
jannic: the HAL team get to vote on it, it's their decision
<dirbaio[m]>
IMO the risk for this change is low, because you can always workaround it with a RefCell
<dirbaio[m]>
it essentially determines where you end up needing the refcell
<adamgreig[m]>
GrantM11235[m]: what do you think might be different if we'd had this change since an earlier e-h alpha?
<GrantM11235[m]>
thejpster[m]: The real debouncer doesn't need a `RefCell`, it only needs a `Cell`, which is basically free
<adamgreig[m]>
we'd have more confidence that we'd discovered any surprising sharp edges, at least
<cr1901>
I'm probably okay with changing this if the documentation makes it Very Damn Clear why it takes &mut self now, and why it didn't take &mut self until near release
<dirbaio[m]>
very dumb impls of a debouncer can be impl'd with just a Cell, anything nontrivial will need RefCell.
Farooq has joined #rust-embedded
<Farooq>
Is it true that programming bare metal an MCU is easier than one for MPU/regular processor?
<GrantM11235[m]>
It doesn't even make sense to impl InputPin for a debouncer like that anyway imo
<dirbaio[m]>
> Both InputPin and OutputPin are a "handle" to a piece of hardware, where calling methods on it does some "action" on the hardware (to write pin value, or to read pin value). We don't know how this "action" is implemented, therefore we don't know whether it can support sharing or not. It might not because it has to mutate some state (#546), it might have to do some I2C transfer...
<therealprof[m]>
@farooqkz:mozilla.org @k900:0upti.me Mind holding off the discussion for a bit?
<barnabyw[m]>
(or in a thread)
<GrantM11235[m]>
But you never need exclusive access the read a pin
<dirbaio[m]>
* I don't agree with the "it's a read therefore it should be `&self`" argument at all
<dirbaio[m]>
you do if the impl has to update a Vec for debouncing
<cr1901>
dirbaio[m]: Yes, I have a speil about "mut" should've been called "excl". Just to reiterate, can there be docs added to at least explain "why it's &mut self"?
<cr1901>
What is obvious to you might not be obvious to others
<jannic[m]>
<adamgreig[m]> "jannic: the HAL team get to vote..." <- I didn't mean to question if the HAL team has the final decision. To the contrary: I think all facts and opinions were stated, and it's not likely that further discussion will improve the results. So why not settle it with a vote? By the HAL team.
<dirbaio[m]>
cr1901: IMO the reason it needs `&mut self` is the same reason OutputPin or I2c needs `&mut self`.
<GrantM11235[m]>
dirbaio[m]: No you don't, if this very rare impl just uses a `RefCell`
<AdamHott[m]>
Hey all I have to drop, i learned a lot tonight for real, thanks!
<cr1901>
Why _does_ i2c need &mut self?
<dirbaio[m]>
GrantM11235[m]: you do need exclusive access. the RefCell is how you force getting exclusive access if the trait required `&self`
<dirbaio[m]>
but it's still exclusive access
<GrantM11235[m]>
dirbaio[m]: That isn't the same because an I2C operation actually "changes" something
<cr1901>
It could've been &self IMO. it would've made _my_ life easier by avoiding RefCells :)
<dirbaio[m]>
GrantM11235[m]: updating the debouncer state is also a "change"
<GrantM11235[m]>
dirbaio[m]: Yes, but that is an implementation detail, the user doesn't need to know about it
<adamgreig[m]>
but it's not a conceptual change to the pin, I think is GrantM11235's point
<adamgreig[m]>
still I think the point is that we're treating &mut not as "mutable access" but as "exclusive access", and in that sense the idea is that you have exclusive access to the input pin, and if you want to share it you use some sharing adaptor (refcell)
<dirbaio[m]>
an ADC read is also not a conceptual change
<dirbaio[m]>
yet the 0.2 Adc trait requires `&mut self`.
<therealprof[m]>
Exclusive access makes a ton of sense to me, whether it's actually required or not is a different matter.
<dirbaio[m]>
* `&mut self` and `&mut Pin`.
<adamgreig[m]>
the adc seems like a nice example of how a "read" might still need `&mut`
<therealprof[m]>
The mutability aspect is indeed only a minor detail.
<GrantM11235[m]>
Is InputPin for input pins, or is it also for debouncers? No acual input pin needs exclusive access to be read
<adamgreig[m]>
you could argue that the adc has to perform a conversion while the inputpin just returns its state though :P
<GrantM11235[m]>
s//`/, s//`/, s/acual/actual/
danielb[m] has joined #rust-embedded
<danielb[m]>
adamgreig[m]: but an InputPin is just a 1 bit ADC 👀
<dirbaio[m]>
adamgreig[m]: but it's an implementation detail, you can do it just fine with a RefCell!
<GrantM11235[m]>
You can impl InputPin for a debouncer if that makes your life easier, but I would argue that that isn't what the trait is for
<adamgreig[m]>
sure. call it an AnalogueInputPin and I agree, same deal
<therealprof[m]>
Fetching the state of an input pin *can* change the actual physical state. That's nothing you want to automatically be free for all.
<GrantM11235[m]>
Do we currently have an ADC trait?
<dirbaio[m]>
but if we had one in 1.0, it should take `&mut self` IMO
<dirbaio[m]>
and if Adc takes `&mut self`, so should InputPin
<GrantM11235[m]>
dirbaio[m]: I'm not sure about that
<dirbaio[m]>
they are the same
<adamgreig[m]>
we're running low on time, and I think the points have all been pretty fairly heard now
<dirbaio[m]>
they're a "handle" to a piece of hardware, where you do some "operation" on it to read something (digital high/low, or ADC reading)
<cr1901>
(This is why ehal should've started at 1.0.0. We've spent literal years treating 1.0.0 as a point-of-no-return, allergic to even the _notion_ of major > 1, rather than iterating with confidence/breaking releases when we screw up.)
<dirbaio[m]>
and we don't know whether the impl can support sharing or not
<JamesMunns[m]>
(also just throwing this out there wrt "last minute changes": the whole reason for waiting so long to cut a 1.0 release was to catch things like this, if we couldn't change something we caught, then what was the point of waiting?)
<JamesMunns[m]>
Aka, yes, I agree with cr1901
<cr1901>
JamesMunns[m]: There was no point in waiting
<GrantM11235[m]>
The old adc trait took (&mut adc, &mut pin). We could do the same thing for gpio if we wanted, which would mean that expanders can be impld without a refcell. But that would be less ergonomic, so I think its a bad idea
<adamgreig[m]>
I don't think we would ever have been able to release lots of breaking releases
<cr1901>
All the beta versions are de-facto incompat
<dirbaio[m]>
the consistent thing is to require exclusive access always by default, then layer shared access on top with RefCell.
<cr1901>
breaking releases
<dirbaio[m]>
like we're already doing with I2C and SPI
<dirbaio[m]>
there's no reason InputPin should be special
<GrantM11235[m]>
dirbaio[m]: All "real" impls support sharing
<dirbaio[m]>
no reason for ADC to be special either
<cr1901>
adamgreig[m]: We already have had years of breaking releases w/ the betas (cargo shouldn't be treating them as compat, but cargo is horrifically understaffed)
<therealprof[m]>
I think the only person on the HAL team who raised a voice in favour of reverting back to `&self` was @eldruin:matrix.org.
<dirbaio[m]>
GrantM11235[m]: all "real" drivers don't need sharing, or already have a RefCell because they need to share stuff other than InputPin
<therealprof[m]>
It would be great to figure out whether we can address their concerns.
<adamgreig[m]>
we're out of time for this meeting, and I think we've given the topic plenty of discussion between here and the GH issue now, without anything materially new coming up
<adamgreig[m]>
I really appreciate your persistence and advocacy GrantM11235
<eldruin[m]>
indeed
<adamgreig[m]>
but I think it's probably now down to where the hal team wants to draw the balance between the pros and cons of each option
<JamesMunns[m]>
ryankurte and eldruin are the two open votes ATM
<adamgreig[m]>
I don't think either way is so ruinous that it clearly can't work, at least
<GrantM11235[m]>
I also want to point out that this change affects non-trait impls too
<cr1901>
adamgreig[m]: Neither do I, I'm just sick of not being able to press "release". It's not like I have "Go fever"
<GrantM11235[m]>
It doesn't *need* to, but it does in practice. It encourages hal authors to use `&mut self` for their inherent methods which don't need exclusive access
<dirbaio[m]>
GrantM11235[m]: for the autoderef issue?
<dirbaio[m]>
or because HAL authors will copy from the trait by inertia?
<eldruin[m]>
while I agree with the philosophy for `&self` and I do not like adding such a change so close to the release, I would vote to keep `&mut self`.
<eldruin[m]>
With the condition that the documentation for the change be expanded a bit in the migration guide.
<dirbaio[m]>
what would you document?
<eldruin[m]>
I really hope not to disappoint GrantM11235, though. You have showed great skill and made for a great technical discussion
<dirbaio[m]>
beyond "you can add a refcell to make your code compile"?
<eldruin[m]>
dirbaio[m]: it's always been more than that
<GrantM11235[m]>
dirbaio[m]: It should explain why you need to add a refcell where there wasn't one before, and why it is worth it overall
<cr1901>
[14:51:43] <cr1901> What is obvious to you might not be obvious to others
<dirbaio[m]>
IMO the pros/cons of `&self` vs `&mut self` is out of scope of the migration guide, the user is interested just in "how to upgrade my code"
<adamgreig[m]>
they might be interested in "why must I upgrade my code" too
<dirbaio[m]>
then they can read the discussion in the PR
<cr1901>
That's easy for you to say when you're championing the change
<eldruin[m]>
there is plenty of reasoning in the migration guide
mabez[m] has joined #rust-embedded
<mabez[m]>
Perhaps a link back to the discussion issue and leave it there then?
<GrantM11235[m]>
dirbaio[m]: That is a massive thead, and half the information turned out to be false
<dirbaio[m]>
eldruin[m]: and I think it's too much reasoning! I cut it back a bit when I updated it, not sure if you saw
<eldruin[m]>
😱
<dirbaio[m]>
only a bit! :D
<dirbaio[m]>
I do think some justification is needed for drastic changes such as "trait X is completely gone"
<dirbaio[m]>
because the answer is "you can't upgrade your code, watch these issues instead"
<adamgreig[m]>
(that's all for the meeting, thank you very much everyone and welcome back!)
<dirbaio[m]>
for the `&mut self` change it's just "if you're an impl change the signature, if you're a driver that need sharing add a RefCelL"
<eldruin[m]>
well, I am not asking for a thesis there, but more than one sentence might be in order
<dirbaio[m]>
* for the `&mut self` change it's just "if you're an impl change the signature, if you're a driver that need sharing add a RefCell", it's much more trivial
<dirbaio[m]>
👍️
<dirbaio[m]>
will do then
<dirbaio[m]>
"Methods for `InputPin` and `StatefulOutputPin` now take `&mut self` to allow for mutable state in impls. If you're a driver blahblah"?
<dirbaio[m]>
* driver blahblah, if you're an impl blahblah"?
<dirbaio[m]>
so after this, are we cleared for release? any other concerns?
<dirbaio[m]>
I'll update the migration guide and the blog post right now
<eldruin[m]>
dirbaio[m]: any improvements GrantM11235 ?
<eldruin[m]>
dirbaio[m]: we are indeed.
<GrantM11235[m]>
Do we want to tell HALs to keep using `&self` in their non-trait methods if they support it?
* cr1901
would
<GrantM11235[m]>
How about `impl InputPin for &Input`?
<dirbaio[m]>
GrantM11235[m]: we haven't found any driver that would benefit from this, so I would say no to this
<dirbaio[m]>
even though I'm the one that originally proposed it
<GrantM11235[m]>
That's not about drivers, its about end user code (which is generally not public)
<dirbaio[m]>
i'd say the "recommended" way of sharing stuff is RefCells.
<dirbaio[m]>
and about end-user code, they wouldn't be using the embedded-hal traits anyway, they'd use the HAL inherent methods
<dirbaio[m]>
* sharing stuff *in drivers* is RefCells.
<GrantM11235[m]>
The end user would be using multiple drivers with the same pin
<dirbaio[m]>
how common is that?
<GrantM11235[m]>
Idk, it's generally not public
<dirbaio[m]>
IMO we should recommend nothing for now
Ralph[m] has joined #rust-embedded
<Ralph[m]>
(but i also have written little embedded rust end-user code...)
<Ralph[m]>
from my experience in the enterprise IT world using this abstraction layer is very beneficial (in this case e.g. to easily port software to new hardware, or from bare-metal to running on linux (with `embedded-hal-linux`) or another (RT)OS, etc.
<Ralph[m]>
i'm an end user who writes end-user code against e-h whenever possible. setup happens specific to the HAL (for obvious reasons), the rest hopefully is written against e-h
<dirbaio[m]>
do you have an example of real-world code that relies on `&self` for InputPin and doesn't have a RefCell already because it's sharing other stuff?
<dirbaio[m]>
if you don't, then you're not affected by the change to `&mut self`.
<dirbaio[m]>
`&mut self` doesn't make the issue worse
<dirbaio[m]>
the issue is caused by mixing
<dirbaio[m]>
if you do all `&mut self` it works equally well as before
<GrantM11235[m]>
It wasn't mixed before, so I think it is fair to say it made the issue worse
<dirbaio[m]>
if as a HAL author you choose to have inherent `&self` methods you do have to mix now
<dirbaio[m]>
but it's just adding `(&*self)`
<dirbaio[m]>
(fwiw when upgrading to rc3 I changed inherent methods to `&mut self` but I think I'll change them back to `&self`)
<dirbaio[m]>
* when upgrading Embassy to rc3
huszty[m] has quit [Quit: Idle timeout reached: 172800s]
<cr1901>
why _does_ i2c traits take &mut anyway?
<dirbaio[m]>
same reason, it's a "handle" to a piece of hardware, doing stuff with it sometimes requires exclusive access to something
<dirbaio[m]>
like a registerblock
<dirbaio[m]>
just that for i2c the "sometimes" is "almost always", while for InputPin it's "almost never"
<cr1901>
But registerblocks have pointers, pointers allow non-excl mutation (as long as it's data-race free)
<dirbaio[m]>
PACs today require exclusive access to the singletons
<dirbaio[m]>
on Linux you'd require exclusive access to a file descriptor
<dirbaio[m]>
etcetc
<dirbaio[m]>
in both cases `&mut` avoids RefCells if you don't need sharing
<dirbaio[m]>
just that on i2c it's "often", in InputPin it's "rarely"
<cr1901>
I can do a PAC write without requiring &mut self thanks to the write/read proxies
<GrantM11235[m]>
dirbaio[m]: The HAL could just steal the pac
<dirbaio[m]>
(I do think PACs should not have singletons, therefore not require exclusive access, but that's a whole other story)
<dirbaio[m]>
even if the PAC doesn't require exclusive access, you still "loglcally" require exclusive access to the i2c peripheral, because even if each individual register access is atomic the whole i2c operation isn't
<cr1901>
Okay, that's fair. But to address your point re: PACs, the singleton members of a PAC do not require excl access. They're structs of pointers
<dirbaio[m]>
svd2rust PACs do require exclusive access
<dirbaio[m]>
or not because it's `&RegisterBlock`??? 🤔
<dirbaio[m]>
now I'm confused
<cr1901>
I'm almost positive that PACs use interior mutability via the unsound VolatileCell to do writes to I/O addrs
<dirbaio[m]>
true, you can still do register access with a `&I2C3`
<cr1901>
(And thus they should become *mut)
<GrantM11235[m]>
<dirbaio[m]> "on Linux you'd require exclusive..." <- You don't need exclusive access to read from or write to a file descriptor
<dirbaio[m]>
you still "logically" do if one "operation" is multiple reads/writes/ioctls
<cr1901>
Yes, that's a fair argument
<dirbaio[m]>
same as multiple register accesses in bare metal
<dirbaio[m]>
(I dunno if this is the case for linux i2c/spi tho)
<GrantM11235[m]>
Yes, but that can still be guaranteed if you do it within a single method call and the struct is !Sync
<dirbaio[m]>
nope
<dirbaio[m]>
same reason your SharingAdapter is unsound
<dirbaio[m]>
you can have reentrancy even in a single thread
<dirbaio[m]>
even with !Sync
<cr1901>
via recursion :)
<GrantM11235[m]>
That is only a problem if you have a malicious impl, and in that case the worst case is just a bug, not UB
<dirbaio[m]>
if the i2c trait requires `&self`, if the user gets to run arbitrary code in the middle of an i2c operation, it can call into the i2c again
<dirbaio[m]>
even if the impl is not malicious, there's lots of ways of accidentally allowing running arbitrary code
<dirbaio[m]>
the obvious one is a callback
<dirbaio[m]>
but also Drop impls, panic handler, alloc handler
<GrantM11235[m]>
They can't run anything in the middle of an i2c operation, only the i2c impl can run things. As long as the i2c impl doesn't call arbitrary code somehow, it's not a problem
<dirbaio[m]>
it's quite easy to accidentally run arbitrary code
<dirbaio[m]>
also with async, you can get suspended in the middle of an operation by design, at which point other code can run
<dirbaio[m]>
it'd be weird to make blocoking i2c take `&self` and async i2c take `&mut self`.
<dirbaio[m]>
s/blocoking/blocking/
<cr1901>
let's ignore async for now. In a blocking impl, I don't see how &self vs &mut self makes a difference
<GrantM11235[m]>
yeah, async need exclusive access
<cr1901>
if the impl is blocking and !Sync, no other code can run while i2c is running
<dirbaio[m]>
with blocking, you could do `&self` if you're reeeeaaaally careful to not call arbitrary code
<cr1901>
how would &self save you
<dirbaio[m]>
as I said, it's quite easy to call arbitrary code
<cr1901>
err &mut self*
<dirbaio[m]>
`&mut self` prevents all reentrancy
<cr1901>
Where do you inject a callback into the I2C trait?
<dirbaio[m]>
add a "timeout" functionality for it, it'll call out into an arbitrary "time driver"
<dirbaio[m]>
make it generic on a word size, the user can pass a Word impl that has Drop that runs arbitrary code
<dirbaio[m]>
ifi you use Vec in the impl, you'll call into the alloc handler which is again arbitrary code
<cr1901>
Oh so basically Generics Ruin Everything again :)
<dirbaio[m]>
it's just too much of a footgin
<dirbaio[m]>
s/footgin/footgun/
<GrantM11235[m]>
These are just ways that the user can shoot themself in the foot if they try hard enough, but it will only cause bugs, not UB
<dirbaio[m]>
yes, it's *possible* to impl an i2c with `&self` correctly
<dirbaio[m]>
but it'd be very annoying to put that footgun in the hands of every single HAL author
<dirbaio[m]>
so we do `&mut self` and require RefCells for sharing
<GrantM11235[m]>
If you pass a timeout that messes with the i2c bus, that's on you
<dirbaio[m]>
¯\_(ツ)_/¯
<cr1901>
Also, it's not clear how &mut self can _not_ call an arbitrary alloc failure handler that calls the i2c impl again
<dirbaio[m]>
you can't get another `&mut self` pointer without unsafe
<dirbaio[m]>
for example, if the i2c was behind a RefCell, trying to borrow the refcell again will panic
<GrantM11235[m]>
(For the record, I am NOT arguing that we should change the i2c traits, this is all purely theoretical)
<cr1901>
Oh right... these aren't "free functions"
<cr1901>
you can't just call them anywhere
<GrantM11235[m]>
I think that a more compelling reason for &mut self in i2c is that it means nothing else will mess with your device while you are holding it
<cr1901>
The quickest way to throw out all guarantees in Rust is to depend on non-local data. I have C brain :'D
<GrantM11235[m]>
GrantM11235[m]: Actually, that doesn't work because you can pass any address you want
<cr1901>
(A "free function that writes to I2C" would depend on non-local data not in a higher stack frame)
<GrantM11235[m]>
But it does work better for spi. If you are holding an impl SpiDevice, you can be reasonably confident that no one else will be sending commands to that device
<cr1901>
>that doesn't work because you can pass any address you want <-- why does this matter? The xfer will be done before the trait method returns.
<cr1901>
You're free to send another addr after it possibly if your device need not multiple xfers
<GrantM11235[m]>
We can't actually prevent that with the current i2c trait, but we can with the SpiDevice trait
<GrantM11235[m]>
We could prevent that if the i2c traits and sharing looked more like spi, where an I2cDevice is a shared I2cBus plus an address
<GrantM11235[m]>
Well, I guess that just moves the problem up a level, since it would still be possible to create identical I2cDevices by passing the same address
<dirbaio[m]>
I think it's cleaner for HAL authors to have it in the mig guide
<GrantM11235[m]>
I think it would be good to follow up in #341 anyway, to explain that it wasn't actually fixed, but that we decided it's not a big deal
<dirbaio[m]>
HAL authors most likely will run into it when implementing the traits based on inherent methods, giving a code snippet of how to do that is the most straightforward imo
<dirbaio[m]>
HAL users shouldn't run into it because they wouldn't use the traits, and driver authors shouldn't either because they only use the traits
<GrantM11235[m]>
I think that section can be shortened to just "You can work around the issue by replacing `self.is_high()` with `(&*self).is_high()`"
<dirbaio[m]>
why? a code snippet is the clearest way to show where/when you'd need the `&*self
<dirbaio[m]>
* the `&*self`
<GrantM11235[m]>
The code block has a lot of unnecesary boilerplate
<dirbaio[m]>
lots of people won't even immediately know what "inherent method" means
<dirbaio[m]>
with the snippet it's super clear
notgull has joined #rust-embedded
<dirbaio[m]>
"oh I add non-trait methods with `&self`, then implement the trait methods based on thse"
<dirbaio[m]>
s/thse/these/
<dirbaio[m]>
you need the boilerplate to show that
<GrantM11235[m]>
The "how" and "why" is important, but I think it should go in #341. For the migration guide you just need enough information so that people can identify that the problem is happening, and tell them how to fix it
<dirbaio[m]>
it's not just for fixing the autoderef
<dirbaio[m]>
it's for recommending HAL authors to add inherent methods that take `&self` and are infallible
<GrantM11235[m]>
So you could just say "There is a small issue (link) where the compiler uses the trait methods when you want to use non-trait methods from the hal. If that happens, do this"
<cr1901>
GrantM11235[m]: The _that_ in "actually, that doesn't work" refers to "using &mut self"?
<dirbaio[m]>
mabez therealprof eldruin ryankurte can I have a r+ in the release PR? 🙏
<cr1901>
Your code snippet is using & shared ref (&shared_bus impls i2c traits, so deref coercion doesn't kick in?)
<GrantM11235[m]>
Sorry, I was talking about a different potential issue about having two drivers at once for the same chip. That is unrelated to the deref coercion problem
<GrantM11235[m]>
dirbaio: I also don't think you need to put the keypad example in the migration guide. That only really applies to that one crate
<dirbaio[m]>
it's an example of how to share inputpins
<dirbaio[m]>
the ONLY example of a real-world driver that does that
<dirbaio[m]>
* does that we found
<dirbaio[m]>
if you have a better example, please send a PR
<dirbaio[m]>
* that does InputPin sharing that, * that we found
sirhcel[m] has joined #rust-embedded
<sirhcel[m]>
<adamgreig[m]> "two quick announcements from me,..." <- Hello! I'm late to the party today. 👋
Guest7282 has joined #rust-embedded
<cr1901>
dirbaio[m]: I agree w/ your remarks that "RefCell should be used more for sharing", and this isn't on you, but I've heard a bunch of "RefCell bad" rhetoric over the years. To the point that it feels like an alternate cell could replace many uses without the runtime penalty
<GrantM11235[m]>
How about just
<GrantM11235[m]>
>**For driver authors**: We believe that almost all drivers don't need to share input pins, so you should be able to upgrade without any changes.
<GrantM11235[m]>
If you do need to share input pins, the recommended solution is wrapping them with a `RefCell`.
* cr1901
still needs to read the GostCell paper
<cr1901>
GhostCell*
<dirbaio[m]>
I wanted an example to make it clear that you can share muliple things with a single RefCell
<GrantM11235[m]>
lmao, I also been meaning to read the ghostcell paper. I have had the tab open forever
<dirbaio[m]>
and therefore the `&mut self` change is likely to not need extra refcells
<dirbaio[m]>
if it just said "wrap the inputpin with a RefCell" someone might get the impression you need one per pin
<dirbaio[m]>
* extra refcells, if you're already sharing other stuff
<dirbaio[m]>
like the keypad crate
<cr1901>
What docs are you updating? migration guide?
<GrantM11235[m]>
dirbaio[m]: hmm, maybe remove the refcell sentance completely?
<cr1901>
I like the RefCell sentence, tbh
<dirbaio[m]>
we do need to give a solution for migrating traits that need sharing
<dirbaio[m]>
it's the migration guide
<dirbaio[m]>
s/traits/drivers/
<GrantM11235[m]>
If we truly believe that no one is sharing input pins, why are we explaining how to migrate?
<dirbaio[m]>
you guys told me to document this in the migration guide
<dirbaio[m]>
¯\_(ツ)_/¯
<GrantM11235[m]>
well, keypad needs to do something, but I think it would be more effective to just tell them directly if they are the only affect person
<dirbaio[m]>
now you're telling not to document it
<GrantM11235[m]>
sorry 😭
<dirbaio[m]>
I hadn't deemed this important enough to include it in the mig guide in the first place
<GrantM11235[m]>
There definitely should be something in the guide, I just think it should be as short as possible
<cr1901>
GrantM11235[m]: _I_ asked for this in the guide
<cr1901>
or at least some form of documentation
<cr1901>
that makes Very Damn Clear what's going on*
<dirbaio[m]>
the migration guide is not part of the crates.io release, we can edit it any time
<dirbaio[m]>
if you feel it can be improved, please send PRs
<cr1901>
I like it, no complaints
<dirbaio[m]>
I think what's there now is good enough to go ahead with the release
<cr1901>
(finally, no complaints from me about something)
<GrantM11235[m]>
Do we need to mention it in the actual doc? I think probably not
<dirbaio[m]>
GrantM11235[m]: yeah imo it's only relevant if you come from 0.2
<GrantM11235[m]>
Maybe "If you do need to share an input pin, you are probably sharing other things too, just put the input pin inside the refcell/mutex that you already have"
<Ralph[m]>
dirbaio[m]: would it make sense to link to the migration guide in the changelog for v1.0? it's also already linked in the main README, but i think for the changelog it's nice to have a reference there as well
<GrantM11235[m]>
and maybe add "Fore users: If you are sharing a pin in your application, you can use the `&self` methods from your hal, or use a refcell/mutex"
<GrantM11235[m]>
s/Fore/For/
<GrantM11235[m]>
I guess it doesn't really matter. The migration guide is already big enough that most people aren't going to read it beginning to end, so everyone else can just skip that section. In my head I was thinking about it more like a release announcement that should only have the minimum relevant info
M9names[m] has joined #rust-embedded
<M9names[m]>
<Leonardvdj[m]> "Was I accidentally mentioned..." <- Everyone gets tagged for weekly meetings
<GrantM11235[m]>
>For future reference, #547 didn't fix this, and it now affects `InputPin` as well as `StatefulOutputPin`.
<GrantM11235[m]>
>We decided that this is okay because the problem is rare and the solution is easy: replace `self.is_set_high()` with `(*self).is_set_high()`
<dirbaio[m]>
sure, go ahead!
ragarnoy[m] has joined #rust-embedded
<ragarnoy[m]>
anyone knows a tool that flashes an image on a board that's connected to a remote computer ?
<Ralph[m]>
would be interesting to see (some of) their code and hear more about what they implemented in rust (if anyone from airhart is here in the chat and would want to elaborate that'd be great! 👼)
<ragarnoy[m]>
> would be interesting to see (some of) their code and hear more about what they implemented in rust (if anyone from airhart is here in the chat and would want to elaborate that'd be great! 👼)
<ragarnoy[m]>
seems a bit weird since DAL-A rust isn't possible yet
<dirbaio[m]>
reverted embassy to use `&self` for inherent methods. works fine other than the `*self` workaround
<Ralph[m]>
> how does `StatefulOutputPin::is_set_high` respond if it hasn't been set so far? since the e-h docs don't mention it i presume that this might be implementation-specific?
<DanielakaCyReVol>
and yes passing that via CLI is extremely painful with escaping escape characters and quotes and serialized data, so I am thankful that we have a Makefile :D