gpio: require &mut self in InputPin and StatefulOutputPin.
#547
Conversation
|
Gpio expanders do not require struct ExpanderPin<'a, SPI: SpiDevice> {
// this needs to be a `&RefCell` or `&Mutex` etc because
// it is shared with all the other pins from the expander
device: &'a RefCell<SPI>,
pin: u8,
}
impl<'a, SPI: SpiDevice> ExpanderPin<'a, SPI> {
fn is_high(&self) -> bool {
// we need mut access to the device to send and receive data
let device = self.device.borrow_mut();
todo!()
}
} |
|
that's true! it could own the bus if there was only 1 pin in the device, but then it wouldn't really be a gpio expander 🤣 and you do need the refcell anyway otherwise. does this change the tradeoff though? |
|
(Sorry, I was going to bring this up at yesterday's meeting, but I was busy) I think it does change the tradeoff. #546 is a very unusual use case, and even then it can just use a
|
|
That only applies to |
|
it'd be inconsistent if |
Are you sure? It looks like it still works fine if
It was never possible to impl If I have a I don't fully understand the autoderef resolution problem yet, I'll need to look in to that more |
|
I opened #553 to show that it is possible to revert this without reverting #550 As for #341, changing the trait to take |
|
Do you have any practical argument in favor of As discussed earlier, the only argument in favor I've seen so far is that "is_high is a read, and reads should take &self because they don't mutate anything". This argument is a bit theoretical/philosophical though, not practical. There are practical arguments in favor of So, can you come up with any "X is easier to write if We found none in the WG meeting where we discussed this. Maybe you can? If we can't find any, I don't think we should revert. |
|
Also IMO it makes sense to think of " This is the reason |
Yes, this is a bit unfortunate. We can never eliminate the problem, we can only "move it around". I do think the version of the problem in #341 was a bit worse, because it shows up in HAL code, while the one on your playground shows up in user code. User code using the HAL crate directly wouldn't import the |
|
Is this discussion settled enough to go ahead with the release tomorrow? |
|
I'd say yes, the only argument in favor of |
|
To summarize my opinion, there are three main reasons that I think this should be reverted: (There is a bit of new information here, be sure to read the footnote) Sharing an InputPin is a reasonable thing to doWe don't have enough time to do a survey of all the existing uses of Example(error handling has been omitted for simplicity) struct Toggler<I: InputPin, O: StatefulOutputPin> {
input: I,
output: O,
}
impl<I: InputPin, O: StatefulOutputPin> Toggler<I, O> {
fn toggle_if_high(&mut self) {
if self.input.is_high() {
self.output.toggle()
}
}
}
fn main() {
let (input, output1, output2) = todo!();
let mut toggler1 = Toggler {
input: &input,
output: output1,
};
let mut toggler2 = Toggler {
input: &input,
output: output2,
};
for i in 0..100 {
if i % 3 == 0 {
toggler1.toggle_if_high()
}
if i % 5 == 0 {
toggler2.toggle_if_high()
}
sleep();
}
}Philosophically, reading the state of a pin shouldn't require
|
@rust-embedded/hal, considering arguments from the discussion above, are you still OK with changing
|
|
I have a bit of new thoughts too: If we do In most MCUs you can implement OutputPin atomically, so HALs could For With |
This is true, and if we go with the Examplestruct ToggleShared<I: InputPin> {
input: I,
active_state: bool,
}
impl<I: InputPin> ToggleShared<I> {
fn new_toggler<O: StatefulOutputPin>(&self, output: O) -> Toggler<'_, I, O> {
Toggler {
shared: self,
output,
}
}
}
struct Toggler<'a, I: InputPin, O: StatefulOutputPin> {
shared: &'a ToggleShared<I>,
output: O,
}
impl<'a, I: InputPin, O: StatefulOutputPin> Toggler<'a, I, O> {
fn toggle_if_active(&mut self) {
// Note: it is not possible to get `&mut I` here
let input: &I = &self.shared.input;
if input.is_high() == self.shared.active_state {
self.output.toggle()
}
}
}
fn main() {
let (input, output1, output2) = get_io();
let toggle_shared = ToggleShared {
input,
active_state: true,
};
let mut toggler1 = toggle_shared.new_toggler(output1);
let mut toggler2 = toggle_shared.new_toggler(output2);
for i in 0..50 {
if i % 3 == 0 {
toggler1.toggle_if_active()
}
if i % 5 == 0 {
toggler2.toggle_if_active()
}
}
}
We should explicitly discourage this. Drivers are written under the assumption that output pins held by the driver won't change unexpectedly. A real-world example of this is display-interface-parallel-gpio This is why
I'm sorry to nit-pick the phrasing here, but "some can, some can't" makes it sound like it is about 50-50. In reality, 99.9% of implementers support sharing "natively" (every MCU, every real gpio expander, linux, etc) and the remaining 0.1% can still trivially allow sharing. I only point this out because I think it makes a big difference. If it was 50-50 (or even 75-25, but let's not haggle 🤣 ) or if it was significantly harder to share the remaining implementations, I would have less of an objection to
As I said, |
|
I am not sure what best here and I think I was too quick to approve this a couple of weeks ago. I agree with the philosophical reasons to keep I understand the disappointment but let's not hastily decide which way to take on release day. There have been many changes in the recent weeks and days. |
Yes, this is possible to do: Drivers can do
We're targeting interoperability with the 1.0 traits. We've already made concessions in the trait design for "1% situations". One example is fallibility. 99% of GPIO impls are infallible, yet we still decided to make the traits fallible
Given this, we should require Making the traits fallible doesn't hurt drivers, they can still opt-in to requiring infallibility with So, the
No, they're not. Both
"reading" a value from the hardware ( |
|
I've been thinking about this for the past few days. These are some excellent points, so thank you @GrantM11235 for bringing them up. I am, however, leaning towards keeping To quote the migration guide:
With the 1.0 release, we should ensure that any driver, written now, or in 15 years (if we get this right, it's entirely feasible we stay 1.x forever) can be expressed using the traits. We already make this exception for GPIO fallibility, even though 99% of GPIO operations are infallible. We don't know the future and we should be prepared for that. I won't re-make @Dirbaio's points around drivers opting into immutable impls, but I will say that we need to document this so HAL implementors know about these specialized impls. It is also probably a good idea to work through an example and use it to see how it actually feels to use. A playground example is probably fine. I would also want to know how a driver can support both the Finally, when it comes to the 1%, whether that be a GPIO Expander or something else, I think its important that the driver is aware of the mutability. For instance, if input stayed as |
Drivers by default support both infallible and fallible pins, by doing the standard Then, drivers can optionally require infallible pins with Same for shareability: if a driver does So we're not at risk of splitting the ecosystem, the default is drivers work with any HAL. Only if the driver author chooses to explicitly add more bounds it gets restricted.
This is a very good point. For impls that can't natively share, If we do
Also note drivers can decide whether to require shareable across threads or not:
|
We aren't schedule to have a meeting on Tues 2nd, so the next scheduled meeting is the 9th - but maybe it's good to have the discussion in the thread here anyway and see if a conclusion can be reached? Or just have an unscheduled HAL team chat on Matrix if you don't want to wait until the 9th. |
|
i think multiple readers would also have to make assumptions about the pin configuration (input pins are not unconditionally sharable, can expect different pulls etc.), which might as well require explicitly opting-in to shared use. it also means input and output pin handling is more consistent, and we should be able to provide primitives to help with sharing input and/or output pins as we do with busses?
while a useful trick for applications i don't think we should encourage this at the driver level as it would dramatically reduce driver / hal compatibility. |
I agree, I just wanted to point out it's possible, to highlight the parallel with requiring shareable pins. There's not much reason to require infallible pins (other than "I'm lazy to do error handling") but there is for requiring shareable pins. |
|
I agree with the points made and do think that |
|
@therealprof @ryankurte (and @eldruin ?) would you like to officially vote by editing this comment? If we get consensus I'd say we should go ahead with the release now. I would prefer not to wait for a WG meeting seeing there's not one until Tue Jan 9th. |
|
Wow, that's a lot of comments 😬 . I already started drafting a response, but I want to take some time to make it as clear and concise as I can (you're welcome 🤣 ) For the record, I don't think the fast-paced, short-format matrix chat would make this any easier, but feel free to ping me there anyway if you want me to (try to) give you a quick answer about something. I'll try to be at the next meeting, whenever it is.
I saw this just before I was about to post this message. I would really like a chance to finish my response, there are still a lot of questions, comments, and misconceptions that I think I can clear up. I'll try to post my response ASAP. |
|
Okay, here it is. I have two new-ish points that I think are very compelling, plus a ton of extra junk that I moved to the footnote section. Every InputPin implementation can be shared, for free, no matter whatThis was originally a massive 1000 word section where I described all the impls that "can't be shared natively": debouncing adapters like #546, gpio expanders with only one pin, and even pointless impls like one that just says "Hello world" in morse code. It went into great detail about how they can be made shareable with a But then I discovered a powerful fact: every single InputPin impl can be shared for free, without using a Imagine two traits, impl(Error handling omitted for simplicity) pub struct SharingAdapter<T> {
inner: UnsafeCell<T>,
}
impl<T> SharedInputPin for SharingAdapter<T> where T: UnsharedInputPin {
fn shared_is_high(&self) -> bool {
// Safety: This is safe because `Self: !Sync`,
// and it also can't be called reentrantly.
// The mutable borrow is guaranteed to end before
// the next time `shared_is_high` is called.
let inner: &mut T = unsafe { &mut *self.inner.get()};
let res = inner.unshared_is_high();
// Ensure the mutable borrow ends here.
let _ = inner;
res
}
}To be clear, I'm not saying we should have two traits, or that we need to provide the You may notice that
|
| InputPin | StatefulOutputPin | |
|---|---|---|
&self |
good | bad |
&mut self |
bad | bad |
As I said, &mut self makes this worse, not better. I don't personally think this needs to be a major deciding factor, but it is clearly a point in favor of &self, not the other way around.
Miscellaneous responses to ryankurte's comment that didn't fit in any other section
Expand me!
i think &mut self makes sense to represent ownership of a pin for reading or [...]
I'm not aware of any existing HALs, gpio expander impls, etc that require &mut self to read a pin, even with their non-trait methods, so I disagree that this restriction "makes sense".
multiple readers would also have to make assumptions about the pin configuration
This is not a problem at all with &self, unless the HAL does something silly like fn change_config(&self). (An inherent method like that should take &mut self instead.) If a driver is holding an impl InputPin (or &impl InputPin) it should be able to assume that nothing else will be able to change the config. This is consistent with rust's general shared XOR mutable philosophy.
we should be able to provide primitives to help with sharing [...] output pins
(emphasis mine)
We should not do this, for the same reason HALs should not opt-in to sharing outputs, as I mentioned in the second part of this previous comment
|
You can only substitute RefCell with UnsafeCell when you have 100% control of the code so you can prove you won't get called reentrantly. You're calling So, all impls can be shareable, but with RefCell, which is not zero-cost. (an individual impl such as the one in #546 (ie not a generic adapter) can choose use UnsafeCell instead of RefCell, if it's written carefully to ensure it doesn't call arbitrary code, but I don't think it's something we should encourage)
Yes, I agree with this :(
it's not a "fundamental" interoperability issue (unlike something like the unconstrained associated types, where it's unfixable if we don't change the traits). If a HAL forgets the impl, they can always add it later when someone complains, in a minor release even.
True! they already need refcell/mutex.
The problem is With
Yes, the impl having to choose is bad (as I stated above). With
it's implemented like that for convenience, because doing a register read is cheap. It could easily be implemented by caching it in RAM instead. For i2c/spi PWMs it really should be implemented by caching it in RAM, as reading it over i2c/spi would be quite slow. There's also a fundamental difference in
I think the autoderef issue is very minor. It only happens when all of these are true:
With the 1.0 focus on "generic drivers" instead of "standardizing HAL APIs", there's little reason code using the concrete HAL types should import the Also, the change from |
Wow, you're right. I didn't consider that an impl could do such tricky things with
That's not true. Even if the impl has to contain a The problem with multi-pin expanders is that they contain a Minor stuff, expand me!
It's a classic time/space tradeoff. My point is that it is perfectly valid and in some super-niche cases it is more optimal.
Would you agree that in this regard,
By this definition, #546 is not a proper
Do you have an example of this? I haven't been able to find any cases on my own. I agree that this isn't really a big problem, I'm mostly just curious. In my next post, I plan to discuss the very rare impls that would be in any way different with
I already wrote most of this, but I removed it because I thought that |
but then you're dealing with
Kind of. Some MCUs allow setting up the hardware to autonomously change the pin's output value. For example with nRF's PPI + GPIOTE you can make a GPIO pin go high when a timer expires. With those, you'd want
This is just trait contract lawyering. it's perfectly fine to make an
The code linked in #341. IIRC I didn't actually test changing |
I think it would be more productive to focus on drivers using the traits, not on impls. Changing from Drivers are what matters, because drivers are what can get hurt by changing to I'm bringing this up because I haven't been able to find any impacted driver. and when we discussed this on Matrix no one else knew of any either. So in my eyes the current state of the discussion is "for impls EDIT: to phrase this differently, hopefully it's clearer. I do agree with you that the downside of |
Your original point here was that with
I did test it, it didn't fix it.
What actually are the real-world downsides in your opinion? If it is just that #546 requires a
|
|
use core::cell::RefCell;
use embedded_hal_1::digital::{ErrorType, InputPin, OutputPin};
pub struct Keypad<O: OutputPin, I: InputPin, const NCOLS: usize, const NROWS: usize> {
inner: RefCell<KeypadInner<O, I, NCOLS, NROWS>>,
}
struct KeypadInner<O: OutputPin, I: InputPin, const NCOLS: usize, const NROWS: usize> {
cols: [O; NCOLS],
rows: [I; NROWS],
}
pub struct KeypadInput<'a, O: OutputPin, I: InputPin, const NCOLS: usize, const NROWS: usize> {
inner: &'a RefCell<KeypadInner<O, I, NCOLS, NROWS>>,
row: usize,
col: usize,
}
impl<'a, O: OutputPin, I: InputPin, const NCOLS: usize, const NROWS: usize> ErrorType for KeypadInput<'a, O, I, NCOLS, NROWS> {
type Error = core::convert::Infallible;
}
impl<'a, O: OutputPin, I: InputPin, const NCOLS: usize, const NROWS: usize> InputPin for KeypadInput<'a, O, I, NCOLS, NROWS> {
fn is_high(&mut self) -> Result<bool, Self::Error> {
Ok(!self.is_low()?)
}
fn is_low(&mut self) -> Result<bool, Self::Error> {
let inner = &mut *self.inner.borrow_mut();
let row = &mut inner.rows[self.row];
let col = &mut inner.cols[self.col];
// using unwrap for demo purposes.
col.set_low().unwrap();
let out = row.is_low().unwrap();
col.set_high().unwrap();
Ok(out)
}
} |
|
Discussed in today's WG meeting, we have HAL team consensus to go with |
InputPin should require
&mut selfjust like the other traits.It has required
&selfsince forever (#36), but I can't find any discussion on why or the pros and cons. If I had to guess, I'd say the historical answer is probably "inputs are read only, therefore can be shared safely, therefore &".However, using
&selfrules out implementations thatThese are disadvantages. However, are there any advantages to requiring only
&self? I don't think so. The main use of a&selfInputPin would be a driver that somehow splits itself in "parts" where each part needs access to the same pin. However it's unlikely that the only shared thing is an InputPin, it's likely they also need sharing something that requires&mut, like an OutputPin, or UART/SPI/I2C. Even with this change, drivers can still share InputPins withRefCells.Using
&selfforces the trait implementation to use a RefCell, which means all uses of it pay the overhead. If we require&mut, only drivers that actually need sharing pay the cost of theRefCell.This doesn't hurt usability of HAL-dependent code, HAL crates can still provide a HAL-specific
fn is_high(&self)method that takes&self, just like many offer infallible methods now.Also, a big reason of making GPIO traits fallible is to support port expanders, but these need
&mut self. We're doing only "half" the changes to support these, it's a bit incoherent/inconsistent.Fixes #546