InputPin should require &mut self just like the other traits.

It has required &self since 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 &self rules out implementations that

have mutable state (like in #546), or
require exclusive access to something, for example an spi/i2c bus for a gpio expander.

These are disadvantages. However, are there any advantages to requiring only &self? I don't think so. The main use of a &self InputPin 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 with RefCells.

Using &self forces 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 the RefCell.

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

Gpio expanders do not require &mut self, nor do the benefit from it, because they still need some form of shared access to the expander.

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 :rofl: and you do need the refcell anyway otherwise.

does this change the tradeoff though? &self is still an issue when you want mutable state, like in #546

(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 `Cell` with very little (maybe zero?) overhead.

&self hasn't been a problem for the past 6 years and it isn't really a problem now. I don't think it is a good idea to change it a few weeks before the release of 1.0 unless there is a very good reason.

&self did cause some problems, see #341

Also, changing to &mut self allowed doing #550 which I think is a good change. If is_set_high/low require &self but toggle requires &mut self do you do the blanket impl for & or &mut? :P

That only applies to StatefulOutputPin, right? I haven't thought about that much, I was mainly thinking about InputPin

it'd be inconsistent if is_high needs &self and is_set_high needs &mut self though.

Also, changing to &mut self allowed doing #550

Are you sure? It looks like it still works fine if is_set_high/low take &self

do you do the blanket impl for & or &mut?

It was never possible to impl StatefulOutputPin for & so the answer is obvious.

If I have a &hal::StatefulOutputPin I would expect that I can read the current state but not be able to change it. I would also expect that I can't use it as a full StatefulOutputPin, since that implies being able to change it.

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 &mut self doesn't actually solve the problem: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=4b253e3d171cfdbe82e07f0ced317f59

Do you have any practical argument in favor of &self?

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 &mut self: it makes it easier to do #546 , it makes it possible to implement an InputPin on top of an owned SPI/I2C bus. Sure they're rare/minor (#546 is a rare thing to need, and 1-pin i2c port expanders are rare too), but they're very concrete examples of "X is easier to write if is_high takes &mut self".

So, can you come up with any "X is easier to write if is_high takes &self" argument?

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 "&self vs &mut self" not as "read vs write", but as "can share, or requires exclusive access". There are implementations, even though they're rare, where is_high() does require exclusive access (to some piece of state in #546, or to an underlying SPi/I2C bus). So it makes sense to require &mut self to enable these implementations.

This is the reason std::io::Read, SpiBus::read(), I2c::read() also require &mut self even if they only do reads: because they do require exclusive access to some underlying thing.

As for https://github.com/rust-embedded/embedded-hal/issues/341, changing the trait to take &mut self doesn't actually solve the problem: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=4b253e3d171cfdbe82e07f0ced317f59

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 embedded-hal traits, so the issue wouldn't appear at all. HAL code has almost surely imported the traits.

Is this discussion settled enough to go ahead with the release tomorrow?

I'd say yes, the only argument in favor of &self brought up is "reads don't change state so should take &self". It was already discussed in Matrix before merging this PR and we decided to still merge because the practical concerns outweigh it. If no new arguments are being brought up we shouldn't continue discussing it in circles.

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 do

We don't have enough time to do a survey of all the existing uses of InputPin to find out how many people are currently sharing input pins or to think of all the possible future use cases, but it is certainly possible to imagine cases where it is useful:

Example

(error handling has been omitted for simplicity) ```rust struct Toggler { input: I, output: O, } impl Toggler { 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 `&mut self`

std::io::Read etc require &mut self because it doesn't just do a passive read, it actively removes data from the stream.

Reading an input pin does not remove data. You can call is_high as much as you want and it won't have any affect on future pin reads.

In this regard it is more similar to pwm::SetDutyCycle::max_duty_cycle(&self) -> u16 than SpiBus::read or I2c::read.

`&self` has been fine for the past 6 years

We shouldn't change it at the last minute unless there is a very very good reason to do so.

Reasons for `&mut self`

So far in this thread I have heard:

GPIO expanders won't need to use a RefCell anymore (they still do, except for hypothetical single pin gpio expanders)
Something like #546 isn't possible otherwise (it still is, it just requires using a Cell in this very rare case)
It fixes #341 (it doesn't, it makes it worse[^1])
It is required for #550 (it's not)

Conclusion

I believe the cons greatly outweigh the pros, and this should be reverted before 1.0.0.

At the very least, I would like it to be put to a vote.

[^1]: Before, the problem in #341 only affected StatefulOutputPin, now it also affects InputPin. InputPin::is_high(&mut self) takes priority over an inherent is_high(&self) method

At the very least, I would like it to be put to a vote.

@rust-embedded/hal, considering arguments from the discussion above, are you still OK with changing InputPin and StatefulOutputPin to require &mut self, or should we keep &self? (edit this post to vote)

@eldruin : [ ]
@MabezDev : [&mut self]
@ryankurte : [ ]
@therealprof: [&mut self]
@Dirbaio: [&mut self]

I have a bit of new thoughts too:

If we do &mut self, HALs can do impl InputPin for &Input, which also allows your Toggler example to work.

In most MCUs you can implement OutputPin atomically, so HALs could impl OutputPin for &Output too. You might want to do something like Toggler but for outputs: on edge on pin A, set pin X high, and on edge on pin B, set pin X low. Being able to share pin X would be helpful in this case.

For embedded-hal traits we just don't know whether the implementations natively can do sharing or not. Some can, some can't (like in #546). It makes sense for the traits to not assume sharing, which means taking &mut self. Implementations can opt-in to sharing by doing impl ... for &....

With &mut self, this is consistent between InputPin and OutputPin. If we do &self, the rules are completely inconsistent: sharing is forced in InputPin, forcing impls to use RefCell, while it's not in OutputPin, forcing users to use RefCell (if the HAL hasn't impl'd for &Output).

If we do &mut self, HALs can do impl InputPin for &Input, which also allows your Toggler example to work.

This is true, and if we go with the &mut self option we should encourage all HALs to do this. This isn't a perfect solution though because it doesn't allow drivers to create and use references to pins (a &impl InputPin is still useless).

Example

```rust struct ToggleShared { input: I, active_state: bool, } impl ToggleShared { fn new_toggler(&self, output: O) -> Toggler<'_, I, O> { Toggler { shared: self, output, } } } struct Toggler<'a, I: InputPin, O: StatefulOutputPin> { shared: &'a ToggleShared, 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() } } } ```

HALs could impl OutputPin for &Output too

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 OutputPin does (and should) take &mut self, it's not about what is easier to implement. This is a fundamental difference between InputPin and OutputPin.

For embedded-hal traits we just don't know whether the implementations natively can do sharing or not. Some can, some can't

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 :rofl: ) or if it was significantly harder to share the remaining implementations, I would have less of an objection to &mut self.

With &mut self, this is consistent between InputPin and OutputPin

As I said, InputPin and OutputPin are fundamentally different. They don't need to be "consistent" with each other any more than, for example, SetDutyCycle::max_duty_cycle (takes &self) and SetDutyCycle::set_duty_cycle (takes &mut self).

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 InputPin's methods &self as well as the argument that this was fine for 6 years and 2 weeks before 1.0 (which has been 3 years in the making) we have changed it. For me what would be decisive would be the situation around #341 and whether we agree that trivially sharing an InputPin is practically a reasonable thing to optimize for or not. (In reality, there is always access to a shared resource in the implementation, though).

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. I would prefer to postpone the release until after the next weekly meeting if the relevant people can make it and then decide.

This isn't a perfect solution though because it doesn't allow drivers to create and use references to pins (a &impl InputPin is still useless).

Yes, this is possible to do:

Drivers can do where T: OutputPin + Clone, which the user can satisfy by passing in T = &HalOutputPin. Or, drivers can do where &T: OutputPin, which the user can satisfy by passing in T = HalOutputPin.

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.

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

to acommodate the remaining 1%, and
for consistency with other traits.

Given this, we should require &mut self, also to accommodate the 1% of impls that can't share, and for consistency. "only 1% of impls can't share" is not an argument in favor of &self.

Making the traits fallible doesn't hurt drivers, they can still opt-in to requiring infallibility with where T: OutputPin<Error=Infallible>. Similarly, with &mut self drivers can still opt-in to sharing with where &T: OutputPin or where T: OutputPin+Clone.

So, the &mut self change makes life better for the 1% of impls that can't share, with no negative costs for drivers.

As I said, InputPin and OutputPin are fundamentally different. They don't need to be "consistent" with each other any more than, for example, SetDutyCycle::max_duty_cycle (takes &self) and SetDutyCycle::set_duty_cycle (takes &mut self).

No, they're not. 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... So, for maximum interoperability, we shouldn't force the implementation to allow sharing.

SetDutyCycle::max_duty_cycle is the different one. That one is guaranteed to not do any "action" on the hardware, it can just return a field stored in self. There's no possible implementation where it requires &mut self.

"reading" a value from the hardware (InputPin) is very different from reading a value from RAM (max_duty_cycle()).

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 &mut self, personally.

To quote the migration guide:

The main difference betewen embedded-hal 0.2 and 1.0 is the project is now focused on a single goal: traits for writing drivers that work on any HAL.

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. &mut self gives us flexibility down the line.

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 ~~infallible and fallible~~ cloneable and non-clonable pin at the same time, without duplicating code or whether we might be inadvertently splitting the ecosystem.

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 &self and a GPIO expander was used, what happens if, in the driver, the input pin is shared between threads? The implementation would need some kind of interior mutability, but because it might be shared across threads the HAL would have to use a Mutex. If the driver is aware of the mutability and knows it won't be split across threads, the driver can optimize slightly and just use a RefCell. This is quite a contrived example, I know, but it is the sort of weird thing that you might end up needing to do down the line. It gives more control to the driver because we're not hiding what the underlying implementation is doing at the HAL level.

I would also want to know how a driver can support both the infallible and fallible pin at the same time, without duplicating code or whether we might be inadvertently splitting the ecosystem.

Drivers by default support both infallible and fallible pins, by doing the standard where T: InputPin and propagating T::Error errors up. This works for fallible pins, but also for infallible pins out of the box: the impl just sets T::Error = Infallible and the compiler knows to optimize out the error paths because Infallible is uninhabited.

Then, drivers can optionally require infallible pins with where T: InputPin<Error = Infallible>.

Same for shareability: if a driver does where T: InputPin, it works with both shareable and nonshareable pins. Then, a driver can require shareability with where T: InputPin + Clone.

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.

I think its important that the driver is aware of the mutability. For instance, if input stayed as &self and a GPIO expander was used, what happens if, in the driver, the input pin is shared between threads? The implementation would need some kind of interior mutability, but because it might be shared across threads the HAL would have to use a Mutex.

This is a very good point. For impls that can't natively share, &self forces them to choose whether to use RefCell or Mutex. (or to duplicate the code two times, with both RefCell and Mutex).

If we do &mut self, the impl is not forced to choose. This is better in all scenarios:

If the driver doesn't need sharing, great! We saved the cost of one RefCell/Mutex.
If the driver does need sharing, it can either:
- Decide to use RefCell or Mutex itself, in which case it can accept non-shareable pins. The driver is likely in a better position than the HAL to know whether sharing across threads is needed or not.
- Decide to only accept shareable pins, with where T: InputPin + Clone. Note this delegates the choice to the user. which is the best positioned to choose. They get a non-shareable pin from the HAL, they adapt it into a shareable pin with RefCell or Mutex, they give it to the driver.
  - I imagine if this becomes a common problem we could add RefCellSharedPin, CriticalSectionSharedPin, MutexSharedPin etc adapters, like the ones we have in embedded-hal-bus for sharing I2C. I really don't think this'll be a problem though, this'd only be needed if the impl can't natively share (1% rare) and the driver requires sharing (0.1% rare, let me remind you we still haven't found a real-world driver that does this), so this combination would be like 0.001% rare.

Also note drivers can decide whether to require shareable across threads or not:

Single thread: where T: InputPin + Clone. -- this works with both RefCell and Mutex, so it doesn't split the ecosystem.
Across threads: where T: InputPin + Clone + Send. -- requires mutex

I would prefer to postpone the release until after the next weekly meeting if the relevant people can make it and then decide.

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 &mut self makes sense to represent ownership of a pin for reading or writing (or hypothetically changing configurations / modes). as other folks have mentioned executing a read may change the state or require some processing internally, which we either hide with interior mutability or expose as with other HAL components.

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?

Then, drivers can optionally require infallible pins with where T: InputPin.

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.

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 &mut self is the way forward, too.

@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 :grimacing: . 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 :rofl: )

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.

If we get consensus I'd say we should go ahead with the release now.

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 what

This 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 RefCell and why that is a suitable tradeoff to make for these rare cases.

But then I discovered a powerful fact: every single InputPin impl can be shared for free, without using a RefCell. And I can prove it.

Imagine two traits, SharedInputPin which takes &self, and UnsharedInputPin, which takes &mut self. Now consider this zero-cost adapter which can turn any UnsharedInputPin into a SharedInputPin:

impl

(Error handling omitted for simplicity) ```rust pub struct SharingAdapter { inner: UnsafeCell, } impl SharedInputPin for SharingAdapter 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 } } ```

playground

To be clear, I'm not saying we should have two traits, or that we need to provide the SharingAdapter ourselves. The very rare "natively unshareabe" implementations don't have to use this exact adapter, they can use whatever they want. I am just using it to prove beyond any doubt that all implementations have the option to be shareable easily and for free. There are no impls that can't be shared, there are no impls that are too slow/expensive to share, there are no impls that are too hard to share.

You may notice that SharingAdapter is !Sync, which means it can only be used on one thread at a time, it can't shared across threads. This is perfectly normal, as I will explain in the footnote about sharing across threads.

`&mut self` is less flexible where it matters

Several people have said or implied that &mut self is strictly more flexable than &self. In fact, the opposite is true.

&mut self gives more "flexibility" to implementers, which they do not need (as shown in the previous section), at the expense of removing flexibility from drivers and users. For 100% of all possible use cases, &mut self is strictly less flexible.

We should not "optimize" for implementations that do not and will not exist, at the expense of real users, drivers, and implementations.

Conclusion

I believe that interoperability and "traits for writing drivers that work on any HAL" are important. When you combine that with these facts...

All drivers/users can use shareable input pins.
Some drivers/users must use shareable input pins.
As shown above, all impls (not just 99% but all 100%) can be shareable.

...the only logical conclusion is "all impls must be shareable".

I am more convinced than ever that &self is the correct choice.

Footnotes

Expand me!

### Requiring `impl InputPin for &Input` is inconvenient and bad for interoperability

Expand me!

The workaround where all shareable impls (which I claim is *all* impls) are required to include `impl InputPin for &Input` is an inconvenience for every impl. More importantly, we have to assume that some impls will simply forget to do this, which is bad for interoperability. That is exactly the type of needless fragmentation that we are trying to avoid in 1.0.0

--- ### Normal multi-pin GPIO expander pins are always "natively" shareable, `&mut self` is *never* useful

Expand me!

I think there may still be some confusion about this so I want to reiterate that all "real" gpio expanders (ones that have more than one pin) are already part of the 99.9% that support sharing "natively". Any gpio expander impl will look roughly like this: ```rust 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, pin: u8, } ``` You can read the pin with `&ExpanderPin` just fine, `&mut ExpanderPin` does not give you *anything* useful that you didn't already have with a shared reference.

--- ### `RefCell` vs `Mutex` for GPIO expanders

Expand me!

No matter what, the expander impl will need to choose between something like `RefCell` or `Mutex`, or provide an implementation for both. With `Mutex`, you can share different pins across threads, and you can also share the same pin across threads. With `RefCell`, you cannot share different pins across threads, and you also cannot share the same pin across threads, but you can still share pins (same and different) on a single thread. This is all true regardless of whether `InputPin` takes `&self` or `&mut self`. The only difference with `&mut self` is that it also requires the extra `impl InputPin for &ExpanderPin` in order to enable the full functionality.

--- ### Sharing across threads

Expand me!

All impls can be shared on a single thread, but not all can be shared across threads. This is totally normal and perfectly consistent with our other traits, none of them require `Send` or `Sync`. If a driver needs to share a pin across threads, it should require `InputPin + Sync` (and also `'static` if the thread isn't scoped). This allows the driver to `Send` shared references across threads. But most of the time, the driver won't need to do this, or even think about it. For example, in my [previous Toggler example](https://github.com/rust-embedded/embedded-hal/pull/547#issuecomment-1870907409), the compiler will automatically decide that a `Toggler` is `Send` (and therefore can be moved to another thread) if and only if the input is `Sync` and the output is `Send`. This isn't anything new, it's just the way `Send`/`Sync` works. When a user wants to use a driver that requires `InputPin + Sync`, they must choose an impl that is `Sync` (for example a mutex based gpio expander impl instead of a refcell based impl) or wrap the impl in an mutex-based adapter to make it `Sync`. Again, this is nothing new.

--- ### Shareability vs fallibility

Expand me!

Several people have compared shareability with fallibility, but fallibility is very different. It's not about making it "easier" to implement the traits over fallible interfaces, it's a matter of making it possible to sanely implement the traits at all. There is simply no good way to turn a fallible impl into an infallible one, the only options are to panic or fail silently, which are both not good. In contrast, it is *always* possible to turn an "unshareable" impl into a shareable one, as demonstrated above.

--- ### It really *is* the same as `SetDutyCycle::max_duty_cycle`

Expand me!

When I started writing this, I thought I was just being overly pedantic, but by the end I think I stumbled upon an important point. Dirbaio said: > SetDutyCycle::max_duty_cycle is the different one. That one is guaranteed to not do any "action" on the hardware, it can just return a field stored in self. There's no possible implementation where it requires &mut self. This is not actually true. For example [in embassy-stm32](https://github.com/embassy-rs/embassy/blob/47a94f22aa040f195a6265ff6950187647e1fe48/embassy-stm32/src/timer/simple_pwm.rs#L166C29-L166C29) (which doesn't support the pwm trait yet, but the implementation will likely be the same), it *does* touch the hardware by doing an MMIO read of the ARR register. Furthermore, a driver for an external pwm chip may decide to save ram by not caching a copy of the max duty cycle, and instead read it over the spi/i2c bus each time. Of course it would need to use some form of interior mutability to get `&mut` access to the bus. (If it's a multi-channel chip, it will already have interior mutability.) That is a lot of effort to save just two bytes of ram, but my point is that it's a perfectly valid implementation. Neither of these violate any "guarantee" to not touch hardware, because such a "guarantee" doesn't exist. Conceptually, the `max_duty_cycle` method is "look but don't touch", but it can be implemented however you want, as long as it follows those semantics. This is actually very similar to `StatefulOutputPin`, where `is_set_{low,high}` can be reasonably implemented by reading an MMIO register, communicating over a bus, or simply by returning a cached value from ram. Regardless of implementation, the semantics are still "look but don't touch". The same thing also applies to `InputPin` except it can't reasonably return a cached value. (Unless the impl kept track of how old the value was, then it could just update it whenever it is too stale :thinking: )

--- ### More details about how `&mut self` makes #341 slightly worse, not better

Expand me!

eldruin said: > For me what would be decisive would be the situation around #341 and \[...\] [playground](https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=7bd546833b3bef0c4cc9b7d99e2d7864) | |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](https://github.com/rust-embedded/embedded-hal/pull/547#issuecomment-1870907409)

SharingAdapter is unsound. See playground, run with Tools -> MIRI.

error: Undefined Behavior: not granting access to tag <3099> because that would remove [Unique for <3065>] which is strongly protected because it is an argument of call 1204
  --> src/main.rs:25:38
   |
25 |         let inner: &mut T = unsafe { &mut *self.inner.get() };
   |                                      ^^^^^^^^^^^^^^^^^^^^^^ not granting access to tag <3099> because that would remove [Unique for <3065>] which is strongly protected because it is an argument of call 1204
   |
   = help: this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental
   = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information
help: <3099> was created by a SharedReadWrite retag at offsets [0x0..0x10]
  --> src/main.rs:25:44
   |
25 |         let inner: &mut T = unsafe { &mut *self.inner.get() };
   |                                            ^^^^^^^^^^^^^^^^
help: <3065> is this argument
  --> src/main.rs:42:25
   |
42 |     fn unshared_is_high(&mut self) -> bool {
   |                         ^^^^^^^^^
   = note: BACKTRACE (of the first span):
   = note: inside `<SharingAdapter<EvilPin> as SharedInputPin>::shared_is_high` at src/main.rs:25:38: 25:60
note: inside `<EvilPin as UnsharedInputPin>::unshared_is_high`
  --> src/main.rs:46:13
   |
46 |             self.z.get().unwrap().shared_is_high();
   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: inside `<SharingAdapter<EvilPin> as SharedInputPin>::shared_is_high`
  --> src/main.rs:27:19
   |
27 |         let res = inner.unshared_is_high();
   |                   ^^^^^^^^^^^^^^^^^^^^^^^^
note: inside `main`
  --> src/main.rs:66:5
   |
66 |     shared.shared_is_high();
   |     ^^^^^^^^^^^^^^^^^^^^^^^

note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace

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 unshared_is_high() from an arbitrary implementation, which is essentially arbitrary code.

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)

The workaround where all shareable impls (which I claim is all impls) are required to include impl InputPin for &Input is an inconvenience for every impl.

Yes, I agree with this :(

More importantly, we have to assume that some impls will simply forget to do this, which is bad for interoperability. That is exactly the type of needless fragmentation that we are trying to avoid in 1.0.0

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.

Normal multi-pin GPIO expander pins are always "natively" shareable, &mut self is never useful

True! they already need refcell/mutex.

All impls can be shared on a single thread, but not all can be shared across threads. This is totally normal and perfectly consistent with our other traits, none of them require Send or Sync. If a driver needs to share a pin across threads, it should require InputPin + Sync (and also 'static if the thread isn't scoped). This allows the driver to Send shared references across threads.

The problem is &self forces the impl to include a RefCell, so it won't be shareable across threads and the user can do nothing about it.

With &mut self, the user or the driver can choose to use a Mutex instead of a RefCell, allowing sharing across threads.

RefCell vs Mutex for GPIO expanders. No matter what, the expander impl will need to choose between something like RefCell or Mutex, or provide an implementation for both.

Yes, the impl having to choose is bad (as I stated above). With &mut self it only affects expanders, there's nothing we can do about it. With &self it affects any impl that wants to have some kind of mutable/exclusive access to something such as #546 .

This is not actually true. For example in embassy-stm32 (which doesn't support the pwm trait yet, but the implementation will likely be the same), it does touch the hardware by doing an MMIO read of the ARR register. Furthermore, a driver for an external pwm chip may decide to save ram by not caching a copy of the max duty cycle, and instead read it over the spi/i2c bus each time.

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 get_max_duty vs is_high(). The max duty value will never "spontaneously" change, it will only change in response to a configuration change (like changing frequency or prescaler or mode) which is always explicitly done by the driver. So it is always possible for a driver to cache it. While with is_high() the method has to return by definition the current value of the pin at this instant, so it must be read from the hardware at every call, it's impossible to cache.

More details about how &mut self makes https://github.com/rust-embedded/embedded-hal/issues/341 slightly worse, not better

I think the autoderef issue is very minor. It only happens when all of these are true:

Non-generic code (using HAL types directly).
Code has the InputPin trait imported.
Code uses a &mut HalPin. Not a HalPin, not a &HalPin, not a StructThatContainsHalPin, not a &StructThatContainsHalPin, not a &mut StructThatContainsHalPin.

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 embedded-hal traits.

Also, the change from &self to &mut self makes the issue disappear in other places (#341 happened before, doesn't happen now), so I wouldn't say it makes the problem worse, it just "moves it around".

SharingAdapter is unsound.

Wow, you're right. I didn't consider that an impl could do such tricky things with Rc<Cell> cycles. This is my own personal Leakpocalypse! :sob:

The problem is &self forces the impl to include a RefCell, so it won't be shareable across threads and the user can do nothing about it.

That's not true. Even if the impl has to contain a Cell/RefCell/UnsafeCell (which is very rare), it is still Send, so you can make it Sync by simply wrapping the whole thing in a mutex.

The problem with multi-pin expanders is that they contain a &RefCell, which is !Send. I believe that this is the only case where an impl needs to have duplicate impls for RefCell vs Mutex. As discussed, &mut self doesn't help at all here.

Minor stuff, expand me!

> For i2c/spi PWMs it really should be implemented by caching it in RAM, as reading it over i2c/spi would be quite slow. 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. > There's also a fundamental difference in `get_max_duty` vs `is_high()`. The max duty value will never "spontaneously" change Would you agree that in this regard, `get_max_duty` and `is_set_high` are fundamentally similar, while `is_high` is fundamentally a bit different from both? > with `is_high()` the method has to return *by definition* the *current* value of the pin at this instant By this definition, #546 is *not* a proper `InputPin` impl. I do kinda unironically believe this, but I still think it is useful as a workaround for buggy drivers that don't do proper debouncing on their own. I wrote about this in a previous draft, but I ended up removing it. > Also, the change from `&self` to `&mut self` makes the issue disappear in other places (#341 happened before, doesn't happen now) 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 &self vs &mut self. I will also explore what it takes to make these impls work with &self. (Spoiler alert: even in these super rare cases, you usually don't need a full RefCell!) For the past several days, I have been trying to think of every possible impl like this, and I have only been able to come up with these three (let me know if you can think of more!):

A debouncing adapter like #546
A gpio "expander" with only one pin
Something silly, like an impl that just returns "Hello world!" in morse code

I already wrote most of this, but I removed it because I thought that SharingAdapter (RIP, you will be missed :sob: ) made the section unnecessary. Don't worry, I saved a copy.

That's not true. Even if the impl has to contain a Cell/RefCell/UnsafeCell (which is very rare), it is still Send, so you can make it Sync by simply wrapping the whole thing in a mutex.

but then you're dealing with Mutex<HalPin> which does not impl InputPin anymore. You have make a newtype and impl InputPin on it, which is annoying.

Would you agree that in this regard, get_max_duty and is_set_high are fundamentally similar, while is_high is fundamentally a bit different from both?

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 is_set_high to return the updated value. In that case, you'd also need to read it fresh from the hardware, you can't cache it, like with is_high.

By this definition, https://github.com/rust-embedded/embedded-hal/issues/546 is not a proper InputPin impl. I do kinda unironically believe this, but I still think it is useful as a workaround for buggy drivers that don't do proper debouncing on their own. I wrote about this in a previous draft, but I ended up removing it.

This is just trait contract lawyering. it's perfectly fine to make an InputPin that represents a "logical pin" with some extra processing to debounce, same as you'd debounce in hardware with a capacitor and resistor. It's also completely orthogonal to my original argument, which was that you can't cache is_high.

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.

The code linked in #341. 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.

In my next post, I plan to discuss the very rare impls that would be in any way different with &self vs &mut self. I will also explore what it takes to make these impls work with &self

I think it would be more productive to focus on drivers using the traits, not on impls.

Changing from &self to &mut self adds freedom to impls, and removes freedom from drivers. If we look at impls, the answer is always going to be "&mut self is better". We might discuss how bad is &self for impls, or how rare they are, but that can only tip the balance from "&mut self is better" to "&mut self is slightly better", at best.

Drivers are what matters, because drivers are what can get hurt by changing to &mut self. If you want to tip the balance to "&self is better", you should show a real-world driver where &mut self makes InputPin harder to use. It should be a real driver for a real device, not something artificial like Toggler. Ideally something from awesome-embedded-rust.

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 &mut self is slightly better, for drivers it's a tie because it seems it impacts no drivers".

EDIT: to phrase this differently, hopefully it's clearer. I do agree with you that the downside of &self for impls is very small, there's no need to discuss it further. However, the downsides are still nonzero (hopefully you agree?), so to tip the balance we should find upsides of &self for drivers, and AFAICT there aren't any for real-world drivers.

but then you're dealing with Mutex<HalPin> which does not impl InputPin anymore. You have make a newtype and impl InputPin on it, which is annoying.

Your original point here was that with &mut self the user/driver could choose to wrap the pin in a Mutex or a RefCell, depending on what they need, but that with &self they don't have that option. My point is that this is incorrect. With &self they do still have the option of wrapping the pin in a Mutex. Yes, it's annoying, but no more so than with &mut self.

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.

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"

&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.

I do agree with you that the downside of &self for impls is very small, there's no need to discuss it further. However, the downsides are still nonzero (hopefully you agree?)

What actually are the real-world downsides in your opinion? If it is just that #546 requires a Cell, I would say it is basically zero.

to tip the balance we should find upsides of &self for drivers

keypad is a real-world example of a crate that would be negatively affected by &mut self. Currently, it stores each output pin in it's own RefCell, but the inputs don't require RefCells. I'm not sure if the where for<'a> &'a T: InputPin/impl InputPin for &Input trick would even work here, due to the use of &dyn InputPin. The "solution" would probably just be to add a lot more RefCells.

keypad is written in a bit of an odd way, you can do it with a single refcell. &mut self doesn't increase the refcell count.

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 &mut self.

rust-embedded / embedded-hal

gpio: require `&mut self` in `InputPin` and `StatefulOutputPin`. #547

546 is a very unusual use case, and even then it can just use a `Cell` with very little (maybe zero?) overhead.

Sharing an InputPin is a reasonable thing to do

Philosophically, reading the state of a pin shouldn't require `&mut self`

`&self` has been fine for the past 6 years

Reasons for `&mut self`

Conclusion

Every InputPin implementation can be shared, for free, no matter what

`&mut self` is less flexible where it matters

Conclusion

Footnotes

rust-embedded / embedded-hal

gpio: require `&mut self` in `InputPin` and `StatefulOutputPin`. #547

546 is a very unusual use case, and even then it can just use a Cell with very little (maybe zero?) overhead.

Sharing an InputPin is a reasonable thing to do

Philosophically, reading the state of a pin shouldn't require &mut self

&self has been fine for the past 6 years

Reasons for &mut self

Conclusion

Every InputPin implementation can be shared, for free, no matter what

&mut self is less flexible where it matters

Conclusion

Footnotes

546 is a very unusual use case, and even then it can just use a `Cell` with very little (maybe zero?) overhead.

Philosophically, reading the state of a pin shouldn't require `&mut self`

`&self` has been fine for the past 6 years

Reasons for `&mut self`

`&mut self` is less flexible where it matters