Skip to content
This repository has been archived by the owner on Feb 13, 2019. It is now read-only.

Re-Introduce into_open_drain_output #51

Merged
merged 12 commits into from
Oct 12, 2018
Merged

Re-Introduce into_open_drain_output #51

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
0fcf8c3
Re-Introduce into_open_drain_output
kellerkindt Feb 17, 2018
3892909
Use specialization feature to read from IDR instead from ODR in OpenD…
kellerkindt Feb 17, 2018
3e2476a
Add description why a different implementation for OpenDrain mode is …
kellerkindt Feb 17, 2018
bf84d53
Remove leading empty lines in gpio.rs
kellerkindt Feb 17, 2018
7e0c324
Increase max speed top 50MHz
kellerkindt Feb 21, 2018
04225b5
Merge branch 'master' into patch-3
kellerkindt Mar 18, 2018
42b0961
Fix compile error, somehow this got eaten away by the merge?
kellerkindt Mar 18, 2018
da288c3
Implement InputPin for $PXx<Output<OpenDrain>> and $PXi<Output<OpenDr…
kellerkindt Mar 29, 2018
86842b3
Revert "Implement InputPin for $PXx<Output<OpenDrain>> and $PXi<Outpu…
kellerkindt Apr 4, 2018
35c2261
Merge branch 'master' of https://github.com/japaric/stm32f103xx-hal i…
kellerkindt Jun 8, 2018
7810509
Remove specialization no longer required since it is no longer required
kellerkindt Jun 8, 2018
0cf6f85
Merge branch 'master' of https://github.com/japaric/stm32f103xx-hal i…
kellerkindt Jul 8, 2018
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
81 changes: 53 additions & 28 deletions src/gpio.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ macro_rules! gpio {
use rcc::APB2;
use super::{
Alternate, Floating, GpioExt, Input,
// OpenDrain,
OpenDrain,
Output,
// PullDown, PullUp,
PushPull,
Expand Down Expand Up @@ -123,27 +123,39 @@ macro_rules! gpio {
_mode: PhantomData<MODE>,
}


impl<MODE> OutputPin for $PXx<Output<MODE>> {
fn is_high(&self) -> bool {
default fn is_high(&self) -> bool {
!self.is_low()
}

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

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

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

/// In OpenDrain mode writing a high to the output causes
/// it to float. Therefore whether the pin is high or low
/// depends on other periphery connected to that pin. Therefore
/// one needs to read from IDR instead from ODR in this mode.
impl OutputPin for $PXx<Output<OpenDrain>> {
fn is_low(&self) -> bool {
// NOTE(unsafe) atomic read with no side effects
unsafe { (*$GPIOX::ptr()).idr.read().bits() & (1 << self.i) == 0 }
}
}

$(
/// Pin
pub struct $PXi<MODE> {
Expand Down Expand Up @@ -237,27 +249,29 @@ macro_rules! gpio {
// $PXi { _mode: PhantomData }
// }

// /// Configures the pin to operate as an open drain output pin
// pub fn into_open_drain_output(
// self,
// moder: &mut MODER,
// otyper: &mut OTYPER,
// ) -> $PXi<Output<OpenDrain>> {
// let offset = 2 * $i;

// // general purpose output mode
// let mode = 0b01;
// moder.moder().modify(|r, w| unsafe {
// w.bits((r.bits() & !(0b11 << offset)) | (mode << offset))
// });
/// Configures the pin to operate as an open drain output pin
/// The Output implementation for [Output#is_low(&self)] and
/// [Output#is_high(&self)] read from the IDR instead from the ODR
/// since a written high does not need to mean the pin is actually high
pub fn into_open_drain_output(
self,
cr: &mut $CR,
) -> $PXi<Output<OpenDrain>> {
let offset = (4 * $i) % 32;
// General purpose output open-drain
let cnf = 0b01;
// Open-Drain Output mode, max speed 50 MHz
let mode = 0b11;
let bits = (cnf << 2) | mode;

// // open drain output
// otyper
// .otyper()
// .modify(|r, w| unsafe { w.bits(r.bits() | (0b1 << $i)) });
cr
.cr()
.modify(|r, w| unsafe {
w.bits((r.bits() & !(0b1111 << offset)) | (bits << offset))
});

// $PXi { _mode: PhantomData }
// }
$PXi { _mode: PhantomData }
}

/// Configures the pin to operate as an push pull output pin
pub fn into_push_pull_output(
Expand Down Expand Up @@ -295,25 +309,36 @@ macro_rules! gpio {
}

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

fn is_low(&self) -> bool {
default fn is_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) {
default 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) {
default fn set_low(&mut self) {
// 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
Copy link
Owner

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@kellerkindt kellerkindt Mar 27, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link

@tib888 tib888 Mar 27, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

☺️ 👍

Copy link

Choose a reason for hiding this comment

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

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.

/// it to float. Therefore whether the pin is high or low
/// depends on other periphery connected to that pin. Therefore
/// one needs to read from IDR instead from ODR in this mode.
impl OutputPin for $PXi<Output<OpenDrain>> {
fn is_low(&self) -> bool {
// NOTE(unsafe) atomic read with no side effects
unsafe { (*$GPIOX::ptr()).idr.read().bits() & (1 << $i) == 0 }
}
}
)+
}
}
Expand Down
1 change: 1 addition & 0 deletions src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
//!
//! [examples]: examples/index.html

#![feature(specialization)]
#![feature(unsize)]
#![feature(never_type)]
#![no_std]
Expand Down