Skip to content

Add a tx only and a rx only serial instance #34

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Jan 11, 2019
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/).

- Support for stm32f0x1 line - @jessebraham
- Support for HSE as a system clocksource (#25 - breaking change) - @zklapow
- Add ability to use a Tx/Rx only serial instance - @david-sawatzke

### Changed

Expand Down
226 changes: 160 additions & 66 deletions src/serial.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,8 @@ use embedded_hal::prelude::*;

use crate::{gpio::*, rcc::Rcc, time::Bps};

use core::marker::PhantomData;

/// Serial error
#[derive(Debug)]
pub enum Error {
Expand Down Expand Up @@ -208,50 +210,91 @@ pub struct Serial<USART, TXPIN, RXPIN> {
pins: (TXPIN, RXPIN),
}

// Common register
type SerialRegisterBlock = crate::stm32::usart1::RegisterBlock;

/// Serial receiver
pub struct Rx<USART> {
// This is ok, because the USART types only contains PhantomData
usart: *const USART,
usart: *const SerialRegisterBlock,
_instance: PhantomData<USART>,
}

// NOTE(unsafe) Required to allow protected shared access in handlers
unsafe impl<USART> Send for Rx<USART> {}

/// Serial transmitter
pub struct Tx<USART> {
// This is ok, because the USART types only contains PhantomData
usart: *const USART,
usart: *const SerialRegisterBlock,
_instance: PhantomData<USART>,
}

// NOTE(unsafe) Required to allow protected shared access in handlers
unsafe impl<USART> Send for Tx<USART> {}

macro_rules! usart {
($($USART:ident: ($usart:ident, $usartXen:ident, $apbenr:ident),)+) => {
($($USART:ident: ($usart:ident, $usarttx:ident, $usartrx:ident, $usartXen:ident, $apbenr:ident),)+) => {
$(
use crate::stm32::$USART;
impl<TXPIN, RXPIN> Serial<$USART, TXPIN, RXPIN> {
impl<TXPIN, RXPIN> Serial<$USART, TXPIN, RXPIN>
where
TXPIN: TxPin<$USART>,
RXPIN: RxPin<$USART>,
{
/// Creates a new serial instance
pub fn $usart(usart: $USART, pins: (TXPIN, RXPIN), baud_rate: Bps, rcc: &mut Rcc) -> Self
where
TXPIN: TxPin<$USART>,
RXPIN: RxPin<$USART>,
{
let mut serial = Serial { usart, pins };
serial.configure(baud_rate, rcc);
// Enable transmission and receiving
serial.usart.cr1.modify(|_, w| w.te().set_bit().re().set_bit().ue().set_bit());
serial
}
}

impl<TXPIN> Serial<$USART, TXPIN, ()>
where
TXPIN: TxPin<$USART>,
{
/// Creates a new tx-only serial instance
pub fn $usarttx(usart: $USART, txpin: TXPIN, baud_rate: Bps, rcc: &mut Rcc) -> Self
{
let rxpin = ();
let mut serial = Serial { usart, pins: (txpin, rxpin) };
serial.configure(baud_rate, rcc);
// Enable transmission
serial.usart.cr1.modify(|_, w| w.te().set_bit().ue().set_bit());
serial
Copy link
Member

Choose a reason for hiding this comment

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

Why not have the Tx only constructor just return a Tx<USART>?

Copy link
Member Author

Choose a reason for hiding this comment

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

I want to enable listen and potential future things for Rx/Tx only usarts. Returning a Tx would also make release pretty hard to do.

Copy link
Member

Choose a reason for hiding this comment

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

To be fair currently if you split() an interface you already lose the ability to release, so maybe Tx and Rx need to keep hold of their pins and implement release each. Also I could see claim for being able to call listen/unlisten on Tx and Rx instances, or even just make the listening methods on some global type, given they are atomic and not bound to a given object.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's why stm32-rs/stm32f4xx-hal#49 was done. As a side effect, this is also accomplished by this pull request (an separate object sounds really annoying)

Copy link
Member

Choose a reason for hiding this comment

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

I don't see how that fixes split usarts from being unable to release their pins?

Copy link
Member

Choose a reason for hiding this comment

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

I suppose I tend to see splitting usarts as a useful and important feature as it allows them to be partially shared into interrupt handlers in a sensible way without lock contention.

Copy link
Member

Choose a reason for hiding this comment

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

@HarkonenBade I'm not quite sure about the usefulness of splitting, to me that is mostly a historical thing rather than a practicality. It would make most sense if Rx and Tx were mostly used independently but I really haven't seen that being used anywhere in the wild. In fact I often have to share Rx/Tx together with my interrupt handlers to make use of them (unless I only need one of them).

Maybe it would make sense to actually implement the serial traits directly on the serial port handle to allow direct use of a bi- or unidirectional serial channel with release capability but also keep the possibility to split in case a serial port is meant to be used in two separate places. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

That sounds reasonable to me, also I'm unsure over the use of generating temporary Tx/Rx instances to impl the Read/Write traits, it would seem more appropriate to move the implementations out to free functions that take a *const USART argument.

Copy link
Member Author

Choose a reason for hiding this comment

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

Moved it out in separate functions with RegisterBlock, I could remove the USART parameter from Tx & Rx type, not sure if I should.

Copy link
Member

Choose a reason for hiding this comment

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

I'd leave it there, its potentially relevant to distinguish Tx and Rx objects from different USART types.

}
}

impl<RXPIN> Serial<$USART, (), RXPIN>
where
RXPIN: RxPin<$USART>,
{
/// Creates a new tx-only serial instance
pub fn $usartrx(usart: $USART, rxpin: RXPIN, baud_rate: Bps, rcc: &mut Rcc) -> Self
{
let txpin = ();
let mut serial = Serial { usart, pins: (txpin, rxpin) };
serial.configure(baud_rate, rcc);
// Enable receiving
serial.usart.cr1.modify(|_, w| w.re().set_bit().ue().set_bit());
serial
Copy link
Member

Choose a reason for hiding this comment

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

As above but for Rx

}
}

impl<TXPIN, RXPIN> Serial<$USART, TXPIN, RXPIN> {
fn configure(&mut self, baud_rate: Bps, rcc: &mut Rcc) {
// Enable clock for USART
rcc.regs.$apbenr.modify(|_, w| w.$usartXen().set_bit());

// Calculate correct baudrate divisor on the fly
let brr = rcc.clocks.pclk().0 / baud_rate.0;
usart.brr.write(|w| unsafe { w.bits(brr) });
self.usart.brr.write(|w| unsafe { w.bits(brr) });

// Reset other registers to disable advanced USART features
usart.cr2.reset();
usart.cr3.reset();

// Enable transmission and receiving
usart.cr1.modify(|_, w| w.te().set_bit().re().set_bit().ue().set_bit());

Serial { usart, pins }
self.usart.cr2.reset();
self.usart.cr3.reset();
}

/// Starts listening for an interrupt event
Expand Down Expand Up @@ -289,7 +332,7 @@ macro_rules! usart {
}

usart! {
USART1: (usart1, usart1en, apb2enr),
USART1: (usart1, usart1tx, usart1rx, usart1en, apb2enr),
}
#[cfg(any(
feature = "stm32f030x8",
Expand All @@ -302,7 +345,7 @@ usart! {
feature = "stm32f091",
))]
usart! {
USART2: (usart2, usart2en, apb1enr),
USART2: (usart2, usart2tx, usart2rx,usart2en, apb1enr),
}
#[cfg(any(
feature = "stm32f030xc",
Expand All @@ -312,19 +355,15 @@ usart! {
feature = "stm32f091",
))]
usart! {
USART3: (usart3, usart3en, apb1enr),
USART4: (usart4, usart4en, apb1enr),
USART3: (usart3, usart3tx, usart3rx,usart3en, apb1enr),
USART4: (usart4, usart4tx, usart4rx,usart4en, apb1enr),
}
#[cfg(any(feature = "stm32f030xc", feature = "stm32f091"))]
usart! {
USART5: (usart5, usart5en, apb1enr),
USART6: (usart6, usart6en, apb2enr),
USART5: (usart5, usart5tx, usart5rx,usart5en, apb1enr),
USART6: (usart6, usart6tx, usart6rx,usart6en, apb2enr),
}

// It's s needed for the impls, but rustc doesn't recognize that
#[allow(dead_code)]
type SerialRegisterBlock = crate::stm32::usart1::RegisterBlock;

impl<USART> embedded_hal::serial::Read<u8> for Rx<USART>
where
USART: Deref<Target = SerialRegisterBlock>,
Expand All @@ -333,23 +372,20 @@ where

/// Tries to read a byte from the uart
fn read(&mut self) -> nb::Result<u8, Error> {
// NOTE(unsafe) atomic read with no side effects
let isr = unsafe { (*self.usart).isr.read() };

Err(if isr.pe().bit_is_set() {
nb::Error::Other(Error::Parity)
} else if isr.fe().bit_is_set() {
nb::Error::Other(Error::Framing)
} else if isr.nf().bit_is_set() {
nb::Error::Other(Error::Noise)
} else if isr.ore().bit_is_set() {
nb::Error::Other(Error::Overrun)
} else if isr.rxne().bit_is_set() {
// NOTE(read_volatile) see `write_volatile` below
return Ok(unsafe { ptr::read_volatile(&(*self.usart).rdr as *const _ as *const _) });
} else {
nb::Error::WouldBlock
})
read(self.usart)
}
}

impl<USART, TXPIN, RXPIN> embedded_hal::serial::Read<u8> for Serial<USART, TXPIN, RXPIN>
where
USART: Deref<Target = SerialRegisterBlock>,
RXPIN: RxPin<USART>,
{
type Error = Error;

/// Tries to read a byte from the uart
fn read(&mut self) -> nb::Result<u8, Error> {
read(&*self.usart)
}
}

Expand All @@ -361,30 +397,32 @@ where

/// Ensures that none of the previously written words are still buffered
fn flush(&mut self) -> nb::Result<(), Self::Error> {
// NOTE(unsafe) atomic read with no side effects
let isr = unsafe { (*self.usart).isr.read() };

if isr.tc().bit_is_set() {
Ok(())
} else {
Err(nb::Error::WouldBlock)
}
flush(self.usart)
}

/// Tries to write a byte to the uart
/// Fails if the transmit buffer is full
fn write(&mut self, byte: u8) -> nb::Result<(), Self::Error> {
write(self.usart, byte)
}
}

impl<USART, TXPIN, RXPIN> embedded_hal::serial::Write<u8> for Serial<USART, TXPIN, RXPIN>
where
USART: Deref<Target = SerialRegisterBlock>,
TXPIN: TxPin<USART>,
{
type Error = void::Void;

/// Ensures that none of the previously written words are still buffered
fn flush(&mut self) -> nb::Result<(), Self::Error> {
flush(&*self.usart)
}

/// Tries to write a byte to the uart
/// Fails if the transmit buffer is full
fn write(&mut self, byte: u8) -> nb::Result<(), Self::Error> {
// NOTE(unsafe) atomic read with no side effects
let isr = unsafe { (*self.usart).isr.read() };

if isr.txe().bit_is_set() {
// NOTE(unsafe) atomic write to stateless register
// NOTE(write_volatile) 8-bit write that's not possible through the svd2rust API
unsafe { ptr::write_volatile(&(*self.usart).tdr as *const _ as *mut _, byte) }
Ok(())
} else {
Err(nb::Error::WouldBlock)
}
write(&*self.usart, byte)
}
}

Expand All @@ -394,16 +432,23 @@ where
{
/// Splits the UART Peripheral in a Tx and an Rx part
/// This is required for sending/receiving
pub fn split(self) -> (Tx<USART>, Rx<USART>) {
pub fn split(self) -> (Tx<USART>, Rx<USART>)
where
TXPIN: TxPin<USART>,
RXPIN: RxPin<USART>,
{
(
Tx {
usart: &self.usart as *const _,
usart: &*self.usart,
_instance: PhantomData,
},
Rx {
usart: &self.usart as *const _,
usart: &*self.usart,
_instance: PhantomData,
},
)
}

pub fn release(self) -> (USART, (TXPIN, RXPIN)) {
(self.usart, self.pins)
}
Expand All @@ -420,3 +465,52 @@ where
Ok(())
}
}

/// Ensures that none of the previously written words are still buffered
fn flush(usart: *const SerialRegisterBlock) -> nb::Result<(), void::Void> {
// NOTE(unsafe) atomic read with no side effects
let isr = unsafe { (*usart).isr.read() };

if isr.tc().bit_is_set() {
Ok(())
} else {
Err(nb::Error::WouldBlock)
}
}

/// Tries to write a byte to the uart
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// Tries to write a byte to the uart
/// Tries to write a byte to the UART

/// Fails if the transmit buffer is full
fn write(usart: *const SerialRegisterBlock, byte: u8) -> nb::Result<(), void::Void> {
// NOTE(unsafe) atomic read with no side effects
let isr = unsafe { (*usart).isr.read() };

if isr.txe().bit_is_set() {
// NOTE(unsafe) atomic write to stateless register
// NOTE(write_volatile) 8-bit write that's not possible through the svd2rust API
unsafe { ptr::write_volatile(&(*usart).tdr as *const _ as *mut _, byte) }
Ok(())
} else {
Err(nb::Error::WouldBlock)
}
}

/// Tries to read a byte from the uart
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// Tries to read a byte from the uart
/// Tries to read a byte from the UART

fn read(usart: *const SerialRegisterBlock) -> nb::Result<u8, Error> {
// NOTE(unsafe) atomic read with no side effects
let isr = unsafe { (*usart).isr.read() };

Err(if isr.pe().bit_is_set() {
nb::Error::Other(Error::Parity)
} else if isr.fe().bit_is_set() {
nb::Error::Other(Error::Framing)
} else if isr.nf().bit_is_set() {
nb::Error::Other(Error::Noise)
} else if isr.ore().bit_is_set() {
nb::Error::Other(Error::Overrun)
} else if isr.rxne().bit_is_set() {
// NOTE(read_volatile) see `write_volatile` below
return Ok(unsafe { ptr::read_volatile(&(*usart).rdr as *const _ as *const _) });
} else {
nb::Error::WouldBlock
})
}