Re-Introduce into_open_drain_output #51

kellerkindt · 2018-02-17T15:27:49Z

No description provided.

kellerkindt · 2018-02-17T15:55:26Z

OutputPin for $PXi<Output> is_low(&self) -> bool
OutputPin for $PXx<Output> is_low(&self) -> bool

need to read from idr instead from odr if MODE equals OpenDran to be able to use the OpenDrain mode properly (communicating with another chip via one line). ~~I dont know how to do this in the macro, maybe someone can show me how that?~~ see below

…rain Output Mode

…required

kellerkindt · 2018-02-17T20:18:12Z

Reading from IDR instead of ODR in OpenDrain mode allows one to implement protocols like OneWire using only the OutputPin trait (already implemented basic OneWire functionality that way in a local project).

kellerkindt · 2018-02-21T09:10:39Z

OneWire protocol implementation using an OpenDrain OutputPin. Crate also includes a ds18b20 (temperature sensor) implementation

@therealprof @japaric Shall I open a ticket for the merge request?

therealprof

LGTM

tib888 · 2018-03-26T10:22:16Z

What if into_open_drain_output would return not only an OutputPin, but also an InputPin? This would be more clear than the specialization of the is_low() in OpenDrain mode.
The only drawback I see is that both of the returned structs should be consumed by the next into_XXX call.

japaric

Thanks for the PR @kellerkindt. This basically looks good to me, but I had a question about is_low.

japaric · 2018-03-27T07:43:26Z

src/gpio.rs

                        // NOTE(unsafe) atomic write to a stateless register
                        unsafe { (*$GPIOX::ptr()).bsrr.write(|w| w.bits(1 << (16 + $i))) }
                    }
                }

+                /// In OpenDrain mode writing a high to the output causes


In OpenDrain mode writing a high to the output causes it to float.

So what happens if output (ODR) is set to low? Will this function do the right thing? Perhaps, this should check ODR first; if that's set to zero this returns true otherwise it returns the value stored in IDR?

Writing low causes it to be pulled down to low. So if one writes high, the state depends on whether there is a pull-up resistor connected and on how other devices react, but if set to low, the output is pulled down to low by the chip.

So what happens if output (ODR) is set to low? Will this function do the right thing?

It does. I am currently communicating with ten OneWire devices through this OpenDrain implementation.

Perhaps, this should check ODR first; if that's set to zero this returns true otherwise it returns the value stored in IDR?

That should always return the same as reading directly from IDR, since the chip is trying its best to pull the pin down. I can only imagine it to return a wrong value, if one broke the output driver. I think adding a check for ODR would only cause unnecessary runtime costs?

I think the HAL should work on every microcontroller architecture the same way.
I'm not sure that is_high(), is_low() is always available to read back output pins set state (so those could be removed from the Output trait).
I'm also not sure that an open drain out pin is always able to act the same time as input pin an read the voltage level.
On architectures, which support these, into_open_drain_output could return (OutputPin, InputPin) tuple based on the same pin, but maybe on other architectures you need to use 2 pins physically connected for the same purpose...
Unfortunately I can not tell examples for such a dumb architecture right now...
(If no one else does, then I'm perfectly fine with @kellerkindt's implementation.)

I think the HAL should work on every microcontroller architecture the same way.

This crate is solely about the stm32f103xx family?

I'm not sure that is_high(), is_low() is always available to read back output pins set state (so those could be removed from the Output trait).

The current OutputPin trait already requires that tho. If I am not mistaken, all chips of the stm32f103xx family support the open-drain mode? Reference manual

I'm also not sure that an open drain out pin is always able to act the same time as input pin an read the voltage level.

Looking at the Reference manual (Figure 15/16/17), this shouldn't be an issue?

On architectures, which support these, into_open_drain_output could return (OutputPin, InputPin) tuple based on the same pin,

Returning a tuple would require huge refactoring, since it must be guaranteed to consume the correct two values on into_... function calls, and thus I think its not in the scope of this pull request.
But I could see one being interested to also impl InputPin for $PXi<Output<OpenDrain>> - what do you think @japaric ?

but maybe on other architectures you need to use 2 pins physically connected for the same purpose...

For this crate, only interested in supporting the stm32f103xx family, this should be an issue?

(If no one else does, then I'm perfectly fine with @kellerkindt's implementation.)

☺️ 👍

Hardware abstracion is about to use the same driver implementation on any microcontroller, which is valid target for rust. OutputPin trait is declared in hal::digital::OutputPin, which is not STM specific. that is why I'm not sure that is_high(), is_low() should be part of OutputPin...
But impl InputPin for $PXi<Output<OpenDrain>> is not a bad idea on platforms which support it.

…ain>>

kellerkindt · 2018-03-29T18:36:26Z

Soooo? 😁 @tib888

tib888 · 2018-03-29T20:51:47Z

@kellerkindt
I'd rather keep every OutputPin implementation the same, so the toggle could work as expected.
but to avoid the name conflicts between the two is_low implementation, I would change the name to is_set_low on OutputPin trait, like this:
`
impl OutputPin for $PXi<Output> {
fn is_set_high(&self) -> bool {
!self.is_set_low()
}

                fn is_set_low(&self) -> bool {
                    // NOTE(unsafe) atomic read with no side effects
                    unsafe { (*$GPIOX::ptr()).odr.read().bits() & (1 << $i) == 0 }
                }

                fn set_high(&mut self) {
                    // NOTE(unsafe) atomic write to a stateless register
                    unsafe { (*$GPIOX::ptr()).bsrr.write(|w| w.bits(1 << $i)) }
                }

                fn set_low(&mut self) {
                    // NOTE(unsafe) atomic write to a stateless register
                    unsafe { (*$GPIOX::ptr()).bsrr.write(|w| w.bits(1 << (16 + $i))) }
                }
            }

                impl<MODE> $PXi<Output<MODE>> {
                /// Toggles the output of the pin
                pub fn toggle(&mut self) {
                    if self.is_set_low() {
                        self.set_high()
                    } else {
                        self.set_low()
                    }
                }
            }

                impl InputPin for $PXi<Output<OpenDrain>> {
                fn is_high(&self) -> bool {
                    !self.is_low()
                }

                fn is_low(&self) -> bool {
                    // NOTE(unsafe) atomic read with no side effects
                    unsafe { (*$GPIOX::ptr()).idr.read().bits() & (1 << $i) == 0 }
                }
            }

`
But I haven't tested this yet...

kellerkindt · 2018-03-29T20:58:07Z

This isn't possible without modifying the OutputPin trait

kellerkindt · 2018-03-29T21:02:43Z

The more I look at the additional InputPin implementation (which is redundant and causes ambiguity regarding the is_low and is_high functions) the less I like it.
Maybe reverting da288c3?
What do you think @japaric, what needs to be done that this can be merged?

tib888 · 2018-03-29T21:41:31Z

I recommend modifying the names in OutputPin, because is_low has differnt meaning on OutputPin: it just reads back the previously set state, not the true level on the pin.
@japaric please make a wise decision.
(I have forked embeded-hal and tested this approach with my onewire implementation. See the example here. )

…_drain_output for GPIO is not implemented yet follow: japaric/stm32f103xx-hal#51

@japaric

…t<OpenDrain>>" This reverts commit da288c3. The longer I look at it, the less comfortable I feel with that implementation. The change did add no new functionality but only tried to make one thing more clear: that the pin returned by into_open_drain_output is also an InputPin. But what it also introduces was ambiguity. One cannot call is_low() is_high() on the returned pin, since InputPin and OutputPin overlap. One needs to call InputPin::is_low(&pin) or OutputPin::is_low(&pin), which raises the next question: Do these two functions behave differently? Should they? Currently they dont. My gut says they shouldn't behave differently, it would make things even more ambiguous. Since OutputPin already has is_low() and is_high(), I see no need anymore to also implement InputPin since the functions in OutputPin seem to be supposed to provide what would be gained by implementing InputPin - phew, what a sentence. Also I think, since @japaric kinda approved the state before this commit, one shouldn't make a change with such an impact afterwards. (And since a revert can be reverted easily, nothing is lost @tib888).

kellerkindt · 2018-04-04T22:11:14Z

I recommend modifying the names in OutputPin, because is_low has differnt meaning on OutputPin: it just reads back the previously set state, not the true level on the pin.

Since in every other method, the state should be as set by set_low / set_high. Otherwise a physical error is present (damaged output driver or short circuit - which would probably either destroy the output driver or cause the controller to be powered off).
In OpenDrain mode, the state doesn't depend solely on whether one called set_low / set_high, but on the connected periphery. See also my concerns raised in 86842b3.

The only point I see in your argumentation is, that OutputPin::is_*(&pin) would always return the previously set state, while InputPin::is_*(&pin) reads back from the input. But as mentioned in 86842b3, I prefer to hear @japaric opinion first. (also in da288c3 was the removal of the specialization feature and default implementation missing, therefore incomplete)

tib888 · 2018-04-05T12:14:11Z

@kellerkindt, please find my proposal here, if that will be accepted it will solve this nicely.
Until then I'm fine with your solution.

…nto patch-3

kellerkindt · 2018-06-08T22:20:51Z

So, I updated this pull request to the latest changes. The specialization-feature is no longer required and I had to update panic-semihosting to 0.3.0 and panic-itm to 0.2.0 because of your (@japaric ) recent changes.
~~The examples also no longer work because of those changes, they need to reorientate to the minimal example of cortex-m-quickstart - but that is not within the scope of this pull request.~~ I meant those but there are also these that seem fine to me...

Is this pull request now ready to be merged? If not, what further changes need to be made?

…nto patch-3

kellerkindt · 2018-07-08T15:03:31Z

Since #86 has been merged, there is no need to update the dependencies by this PR anymore.

This is getting a bit frustrating with no replies in months...
So, please, tell me, what is blocking this PR? What am I missing?

@tib888 @ilya-epifanov @therealprof @japaric

tib8 · 2018-07-09T07:20:30Z

@kellerkindt, I reintroduced into_open_drain_output on my branch too.
It is a nice fit since the embedded-hal improved the OutputPin trait.

kellerkindt · 2018-07-10T12:53:57Z

@tib8 Yeah, I updated this PR beginning of June to fit the the changes of the new OutputPin trait.
Your implementation is pretty similar to this PR - but this PR implements the InputPin also for $PXx<Output> in addition to $PXi<Output>.

tib8 · 2018-07-10T14:09:50Z

I would not do that!
I think StatefulOutputPin implementation of $PXi is enough.

kellerkindt · 2018-07-10T21:06:28Z

The doc of StatefulOutputPin says "Output pin that can read its output state". Correct me if I am wrong, but that says that it can read back wether it was once set to high or low, right?
The InputPin is reading the actual state, which you seem to be aware of, since you also implemented InputPin for $PXi<Output<OpenDrain>> on your branch.

The existing OutputPin and InputPin implementations are implemented for $PXi as well as for $PXx, so I am sticking to that pattern.

tib8 · 2018-07-11T10:30:26Z

I implemented it only for $PXi<Output> because it makes sense.
But in other cases it is too bad if the is_high() != is_set_high() (it means the pin is sorted)...
Anyway, rethinking the big picture, your solution is fine: lets implement InputPin whenever that pin input register is valid.

therealprof

LGTM

Re-Introduce into_open_drain_output

0fcf8c3

kellerkindt added 3 commits February 17, 2018 20:52

Use specialization feature to read from IDR instead from ODR in OpenD…

3892909

…rain Output Mode

Add description why a different implementation for OpenDrain mode is …

3e2476a

…required

Remove leading empty lines in gpio.rs

bf84d53

Increase max speed top 50MHz

7e0c324

ilya-epifanov approved these changes Feb 21, 2018

View reviewed changes

kellerkindt added 2 commits March 18, 2018 19:38

Merge branch 'master' into patch-3

04225b5

Fix compile error, somehow this got eaten away by the merge?

42b0961

kellerkindt mentioned this pull request Mar 18, 2018

Re-Introduce into_open_drain_output #63

Open

therealprof reviewed Mar 18, 2018

View reviewed changes

japaric approved these changes Mar 27, 2018

View reviewed changes

Implement InputPin for $PXx<Output<OpenDrain>> and $PXi<Output<OpenDr…

da288c3

…ain>>

tib888 mentioned this pull request Apr 3, 2018

[Discussion] digital marker traits, such as digital::StronglyDriven, digital::OpenDrain, digital::OpenCollector, etc. rust-embedded/embedded-hal#49

Open

MabezDev added a commit to MWatch/kernel that referenced this pull request Apr 4, 2018

Unable to test the new I2C implementation because into_alternate_open…

4ea3827

…_drain_output for GPIO is not implemented yet follow: japaric/stm32f103xx-hal#51

kellerkindt added 2 commits June 9, 2018 00:10

Merge branch 'master' of https://github.com/japaric/stm32f103xx-hal i…

35c2261

…nto patch-3

Remove specialization no longer required since it is no longer required

7810509

Merge branch 'master' of https://github.com/japaric/stm32f103xx-hal i…

0cf6f85

…nto patch-3

kellerkindt mentioned this pull request Oct 12, 2018

Reintroduced pull_down_input, pull_up_input for GPIOs #99

Merged

therealprof approved these changes Oct 12, 2018

View reviewed changes

therealprof merged commit 5118b93 into japaric:master Oct 12, 2018

valpackett mentioned this pull request Jan 1, 2020

Fixes for onewire support nrf-rs/nrf51-hal#25

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Re-Introduce into_open_drain_output #51

Re-Introduce into_open_drain_output #51

kellerkindt commented Feb 17, 2018

kellerkindt commented Feb 17, 2018 •

edited

Loading

kellerkindt commented Feb 17, 2018 •

edited

Loading

kellerkindt commented Feb 21, 2018 •

edited

Loading

therealprof left a comment

tib888 commented Mar 26, 2018

japaric left a comment

japaric Mar 27, 2018

kellerkindt Mar 27, 2018 •

edited

Loading

tib888 Mar 27, 2018 •

edited

Loading

kellerkindt Mar 29, 2018

tib888 Mar 29, 2018

kellerkindt commented Mar 29, 2018

tib888 commented Mar 29, 2018

kellerkindt commented Mar 29, 2018

kellerkindt commented Mar 29, 2018

tib888 commented Mar 29, 2018 •

edited

Loading

kellerkindt commented Apr 4, 2018 •

edited

Loading

tib888 commented Apr 5, 2018

kellerkindt commented Jun 8, 2018 •

edited

Loading

kellerkindt commented Jul 8, 2018 •

edited

Loading

tib8 commented Jul 9, 2018

kellerkindt commented Jul 10, 2018

tib8 commented Jul 10, 2018 •

edited

Loading

kellerkindt commented Jul 10, 2018 •

edited

Loading

tib8 commented Jul 11, 2018

therealprof left a comment

Re-Introduce into_open_drain_output #51

Re-Introduce into_open_drain_output #51

Conversation

kellerkindt commented Feb 17, 2018

kellerkindt commented Feb 17, 2018 • edited Loading

kellerkindt commented Feb 17, 2018 • edited Loading

kellerkindt commented Feb 21, 2018 • edited Loading

therealprof left a comment

Choose a reason for hiding this comment

tib888 commented Mar 26, 2018

japaric left a comment

Choose a reason for hiding this comment

japaric Mar 27, 2018

Choose a reason for hiding this comment

kellerkindt Mar 27, 2018 • edited Loading

Choose a reason for hiding this comment

tib888 Mar 27, 2018 • edited Loading

Choose a reason for hiding this comment

kellerkindt Mar 29, 2018

Choose a reason for hiding this comment

tib888 Mar 29, 2018

Choose a reason for hiding this comment

kellerkindt commented Mar 29, 2018

tib888 commented Mar 29, 2018

kellerkindt commented Mar 29, 2018

kellerkindt commented Mar 29, 2018

tib888 commented Mar 29, 2018 • edited Loading

kellerkindt commented Apr 4, 2018 • edited Loading

tib888 commented Apr 5, 2018

kellerkindt commented Jun 8, 2018 • edited Loading

kellerkindt commented Jul 8, 2018 • edited Loading

tib8 commented Jul 9, 2018

kellerkindt commented Jul 10, 2018

tib8 commented Jul 10, 2018 • edited Loading

kellerkindt commented Jul 10, 2018 • edited Loading

tib8 commented Jul 11, 2018

therealprof left a comment

Choose a reason for hiding this comment

kellerkindt commented Feb 17, 2018 •

edited

Loading

kellerkindt commented Feb 17, 2018 •

edited

Loading

kellerkindt commented Feb 21, 2018 •

edited

Loading

kellerkindt Mar 27, 2018 •

edited

Loading

tib888 Mar 27, 2018 •

edited

Loading

tib888 commented Mar 29, 2018 •

edited

Loading

kellerkindt commented Apr 4, 2018 •

edited

Loading

kellerkindt commented Jun 8, 2018 •

edited

Loading

kellerkindt commented Jul 8, 2018 •

edited

Loading

tib8 commented Jul 10, 2018 •

edited

Loading

kellerkindt commented Jul 10, 2018 •

edited

Loading