-
Notifications
You must be signed in to change notification settings - Fork 13
Fixes for onewire support #25
base: master
Are you sure you want to change the base?
Conversation
Required for One-Wire. ref: japaric/stm32f103xx-hal#51
Required for One-Wire, this is what various C examples use: e.g.: https://gist.github.com/pabigot/46502004045eeca86612#file-main-c-L159
167083b
to
fce3f4e
Compare
@@ -159,7 +174,7 @@ macro_rules! gpio { | |||
w.dir() | |||
.input() | |||
.drive() | |||
.s0d1() | |||
.s0s1() |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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"…
There was a problem hiding this comment.
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
@@ -380,6 +395,21 @@ macro_rules! gpio { | |||
Ok(unsafe { (*GPIO::ptr()).in_.read().bits() & (1 << $i) == 0 }) | |||
} | |||
} | |||
|
|||
impl InputPin for $PXi<Output<OpenDrain>> { |
There was a problem hiding this comment.
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>
.
There was a problem hiding this comment.
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 InputPin
s 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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
This was required to get onewire working.