Skip to content
This repository has been archived by the owner on May 15, 2021. It is now read-only.

Fixes for onewire support #25

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Fixes for onewire support #25

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2c6f194
Implement InputPin for Output<OpenDrain> pins
valpackett Jan 1, 2020
fce3f4e
Use S0S1 for OpenDrain pins
valpackett Jan 1, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 32 additions & 2 deletions src/gpio.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,21 @@ macro_rules! gpio {
}
}

impl InputPin for $PXx<Output<OpenDrain>> {
type Error = Infallible;

#[inline(always)]
fn is_high(&self) -> Result<bool, Self::Error> {
self.is_low().map(|v| !v)
}

#[inline(always)]
fn is_low(&self) -> Result<bool, Self::Error> {
// NOTE(unsafe) atomic read with no side effects
Ok(unsafe { (*GPIO::ptr()).in_.read().bits() & (1 << self.i) == 0 })
}
}

$(
/// Pin
pub struct $PXi<MODE> {
Expand Down Expand Up @@ -159,7 +174,7 @@ macro_rules! gpio {
w.dir()
.input()
.drive()
.s0d1()
.s0s1()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit worried about changing the drive mode for existing users.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was s0d1 chosen? Did that work for anyone? Maybe it should be a separate thing, should OpenDrain/etc even touch it?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was s0d1 chosen?

It's needed for I2C operation IIRC.

Did that work for anyone?

Obviously.

Maybe it should be a separate thing, should OpenDrain/etc even touch it?

Well, the main theme here is Output. Using something labelled as an output for input is already questionable. How about a separate typestate for OneWire operation, something like Bidirectional?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the main theme here is Output

hmm, looking at the SDK

#define GPIO_PIN_CNF_DRIVE_S0S1 (0x00UL) /*!< Standard '0', Standard '1'. */
#define GPIO_PIN_CNF_DRIVE_H0S1 (0x01UL) /*!< High '0', Standard '1'. */
#define GPIO_PIN_CNF_DRIVE_S0H1 (0x02UL) /*!< Standard '0', High '1'. */
#define GPIO_PIN_CNF_DRIVE_H0H1 (0x03UL) /*!< High '0', High '1'. */
#define GPIO_PIN_CNF_DRIVE_D0S1 (0x04UL) /*!< Disconnected '0', Standard '1'. */
#define GPIO_PIN_CNF_DRIVE_D0H1 (0x05UL) /*!< Disconnected '0', High '1'. */
#define GPIO_PIN_CNF_DRIVE_S0D1 (0x06UL) /*!< Standard '0', Disconnected '1'. */
#define GPIO_PIN_CNF_DRIVE_H0D1 (0x07UL) /*!< High '0', Disconnected '1'. */

these things don't mention anything about "output" or "input"…

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's not specific to input vs. output but to the way you're using the pin. https://devzone.nordicsemi.com/f/nordic-q-a/66/gpio-drive-on-nrf51822

.pull()
.disabled()
.sense()
Expand Down Expand Up @@ -221,7 +236,7 @@ macro_rules! gpio {
w.dir()
.output()
.drive()
.s0d1()
.s0s1()
.pull()
.disabled()
.sense()
Expand Down Expand Up @@ -380,6 +395,21 @@ macro_rules! gpio {
Ok(unsafe { (*GPIO::ptr()).in_.read().bits() & (1 << $i) == 0 })
}
}

impl InputPin for $PXi<Output<OpenDrain>> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is most definitely wrong since all the surrounding code deals with Input<OpenDrain>.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, treating Output pins as InputPins is the whole point, that's what the onewire crate expects. This is how the stm32 hal works: japaric/stm32f103xx-hal#51 (I referenced that in the commit message)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is how the stm32 hal works: japaric/stm32f103xx-hal#51 (I referenced that in the commit message)

NB: That crate is obsolete... https://github.com/stm32-rs/stm32f1xx-hal is the new player in town.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, the discussion was in the old one, but the last commit in the new one does the same: stm32-rs/stm32f1xx-hal@fa26e97

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know, I happen to be one of the maintainers of that crate. However STM32F1 is "special" in a lot of regards.

type Error = Infallible;

#[inline(always)]
fn is_high(&self) -> Result<bool, Self::Error> {
self.is_low().map(|v| !v)
}

#[inline(always)]
fn is_low(&self) -> Result<bool, Self::Error> {
// NOTE(unsafe) atomic read with no side effects
Ok(unsafe { (*GPIO::ptr()).in_.read().bits() & (1 << $i) == 0 })
}
}
)+

impl<TYPE> $PXx<TYPE> {
Expand Down