Skip to content
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

The serial::Rx and uart::Rx splitting causes copypaste for high orger API #630

Closed
qwerty19106 opened this issue May 7, 2023 · 2 comments

Comments

@qwerty19106
Copy link
Contributor

@burrbull
The serial and uart was splitted in #608.

I read bytes by DMA until IDLE interrupt was received. I create support struct to reduce copypaste:

pub struct RxDmaIdle<STREAM, const CHANNEL: u8, USART, const BUFFER_SIZE: usize>
where
    STREAM: Stream,
    USART: Instance,
    Rx<USART>: PeriAddress<MemSize = u8>,
{
    transfer: Transfer<STREAM, CHANNEL, Rx<USART>, PeripheralToMemory, RxBufferObject<BUFFER_SIZE>>,
    pool: &'static ObjectPool<Vec<u8, BUFFER_SIZE>>,
}

impl<STREAM, const CHANNEL: u8, USART, const BUFFER_SIZE: usize>
    RxDmaIdle<STREAM, CHANNEL, USART, BUFFER_SIZE>
where
    STREAM: Stream,
    ChannelX<CHANNEL>: Channel,
    USART: Instance,
    Rx<USART>: PeriAddress<MemSize = u8> + RxISR + DMASet<STREAM, CHANNEL, PeripheralToMemory>,
    ...
{
	pub fn new(
        mut rx: Rx<USART>,
        stream: STREAM,
		...)  -> Self {
			rx.unlisten();
        	rx.listen_idle();	

			...
		}

	pub fn uart_interrupt_impl(
        &mut self,
    ) -> Result<RxBufferObject<BUFFER_SIZE>, RxPoolExhaustedError> {
		...
	}
}

But after #608 I should duplicate it for serial::Rx and uart::Rx because the unlisten() and listen_idle() is methods of Rx, not trait.

It would be logical to have one struct Rx for USART and UART, some common traits with common/different implementation and different traits with different implementation.
The Tx have the same problem.

Are there really important reasons to split serial::Rx and uart::Rx?

Full RxDmaIdle code ```Rust use embedded_dma::WriteBuffer; use heapless::{pool::object_simple::*, Vec}; use stm32f4xx_hal::{ dma::{ config::DmaConfig, traits::{Channel, DMASet, PeriAddress, Stream}, ChannelX, PeripheralToMemory, Transfer, }, serial::{Instance, Rx, RxISR}, };

use crate::{RxBufferObject, RxPoolExhaustedError};

pub struct RxDmaIdle<STREAM, const CHANNEL: u8, USART, const BUFFER_SIZE: usize>
where
STREAM: Stream,
USART: Instance,
Rx: PeriAddress<MemSize = u8>,
{
transfer: Transfer<STREAM, CHANNEL, Rx, PeripheralToMemory, RxBufferObject<BUFFER_SIZE>>,
pool: &'static ObjectPool<Vec<u8, BUFFER_SIZE>>,
}

impl<STREAM, const CHANNEL: u8, USART, const BUFFER_SIZE: usize>
RxDmaIdle<STREAM, CHANNEL, USART, BUFFER_SIZE>
where
STREAM: Stream,
ChannelX: Channel,
USART: Instance,
Rx: PeriAddress<MemSize = u8> + RxISR + DMASet<STREAM, CHANNEL, PeripheralToMemory>,
Object<Vec<u8, BUFFER_SIZE>>: WriteBuffer<Word = <Rx as PeriAddress>::MemSize>,
{
pub fn new(
mut rx: Rx,
stream: STREAM,
pool: &'static ObjectPool<Vec<u8, BUFFER_SIZE>>,
start: bool,
) -> Self {
rx.unlisten();
rx.listen_idle();

    let buffer = pool.request().unwrap();

    let mut transfer = Transfer::init_peripheral_to_memory(
        stream,
        rx,
        buffer,
        None,
        DmaConfig::default().memory_increment(true),
    );

    if start {
        transfer.start(|_| {});
    }

    Self { transfer, pool }
}

pub fn uart_interrupt_impl(
    &mut self,
) -> Result<RxBufferObject<BUFFER_SIZE>, RxPoolExhaustedError> {
    assert!(self.transfer.is_idle());

    self.transfer.clear_idle_interrupt();

    if let Some(next_buffer) = self.pool.request() {
        let number_of_transfers = STREAM::get_number_of_transfers() as usize;
        let bytes_count = BUFFER_SIZE - number_of_transfers;

        let (mut buffer, _) = self.transfer.next_transfer(next_buffer).unwrap();

        unsafe {
            buffer.set_len(bytes_count);
        }

        Ok(buffer)
    } else {
        Err(RxPoolExhaustedError)
    }
}

}


</details>
@burrbull
Copy link
Member

burrbull commented May 7, 2023

It would be logical to have one struct Rx for USART and UART, some common traits with common/different implementation and different traits with different implementation.

I could not find how to leave these structures common between UART & USART with different RegisterBlocks.
If you have such solution I'm happy to look at it.
#608 is prep step for advanced possibilities different between UART and USART. For example #466. Although it is still blocked by stm32-rs release.
Technically the problem is missing specialization in Rust. When I try to do impl<U: I1> for Rx<U> { fn foo() } and impl<U: I2> for Rx<U> { fn foo() } Rust says about conflicting implementations. https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=5b2fb49cdad7745139d6fdd1a00669c7

About your idea about Listen trait. It is good to have such universal trait between different peripherals. Not only serial.

bors bot added a commit that referenced this issue May 11, 2023
636: Join Serial for USART and UART again. Make inner traits with different implementation for USART and UART. r=burrbull a=qwerty19106

cc #630
`@burrbull` 

## Main idea
```rust
#[cfg(feature = "uart4")]
pub(crate) use crate::pac::uart4::RegisterBlock as RegisterBlockUart;
pub(crate) use crate::pac::usart1::RegisterBlock as RegisterBlockUsart;

pub trait RegisterBlockImpl {
    fn new<UART: Instance<RegisterBlock = Self>, WORD>(
        uart: UART,
        pins: (impl Into<UART::Tx<PushPull>>, impl Into<UART::Rx<PushPull>>),
        config: impl Into<config::Config>,
        clocks: &Clocks,
    ) -> Result<Serial<UART, WORD>, config::InvalidConfig>;

    fn read_u16(&self) -> nb::Result<u16, Error>;
    fn write_u16(&self, word: u16) -> nb::Result<(), Error>;

    fn read_u8(&self) -> nb::Result<u8, Error> {
        // Delegate to u16 version, then truncate to 8 bits
        self.read_u16().map(|word16| word16 as u8)
    }

    fn write_u8(&self, word: u8) -> nb::Result<(), Error> {
        // Delegate to u16 version
        self.write_u16(u16::from(word))
    }

    fn flush(&self) -> nb::Result<(), Error>;

    fn bwrite_all_u8(&self, buffer: &[u8]) -> Result<(), Error> {
        for &b in buffer {
            nb::block!(self.write_u8(b))?;
        }
        Ok(())
    }

    fn bwrite_all_u16(&self, buffer: &[u16]) -> Result<(), Error> {
        for &b in buffer {
            nb::block!(self.write_u16(b))?;
        }
        Ok(())
    }

    fn bflush(&self) -> Result<(), Error> {
        nb::block!(self.flush())
    }

    // RxISR
    fn is_idle(&self) -> bool;
    fn is_rx_not_empty(&self) -> bool;
    fn clear_idle_interrupt(&self);

    // TxISR
    fn is_tx_empty(&self) -> bool;

    // RxListen
    fn listen_rxne(&self);
    fn unlisten_rxne(&self);
    fn listen_idle(&self);
    fn unlisten_idle(&self);

    // TxListen
    fn listen_txe(&self);
    fn unlisten_txe(&self);

    // Listen
    fn listen(&self, event: Event);
    fn unlisten(&self, event: Event);

    // PeriAddress
    fn peri_address(&self) -> u32;
}

macro_rules! uartCommon {
    ($RegisterBlock:ty) => {
        impl RegisterBlockImpl for $RegisterBlock {
			...
		}
    }
}

uartCommon! { RegisterBlockUsart }

#[cfg(feature = "uart4")]
uartCommon! { RegisterBlockUart }
```
Note that `RegisterBlockImpl` not exports from `stm32f4xx-hal`.

## Changes in public API
- add `type RegisterBlock;` to `Instance`
```rust 
pub trait Instance: crate::Sealed + rcc::Enable + rcc::Reset + rcc::BusClock + CommonPins {
    type RegisterBlock;

    #[doc(hidden)]
    fn ptr() -> *const Self::RegisterBlock;
    #[doc(hidden)]
    fn set_stopbits(&self, bits: config::StopBits);
}
```
- remove `uart::{Serial, Rx, Tx}`
- add `RxListen`, `TxListen`, `Listen` traits
```rust
/// Trait for listening [`Rx`] interrupt events.
pub trait RxListen {
    /// Start listening for an rx not empty interrupt event
    ///
    /// Note, you will also have to enable the corresponding interrupt
    /// in the NVIC to start receiving events.
    fn listen(&mut self);

    /// Stop listening for the rx not empty interrupt event
    fn unlisten(&mut self);

    /// Start listening for a line idle interrupt event
    ///
    /// Note, you will also have to enable the corresponding interrupt
    /// in the NVIC to start receiving events.
    fn listen_idle(&mut self);

    /// Stop listening for the line idle interrupt event
    fn unlisten_idle(&mut self);
}

/// Trait for listening [`Tx`] interrupt event.
pub trait TxListen {
    /// Start listening for a tx empty interrupt event
    ///
    /// Note, you will also have to enable the corresponding interrupt
    /// in the NVIC to start receiving events.
    fn listen(&mut self);

    /// Stop listening for the tx empty interrupt event
    fn unlisten(&mut self);
}

/// Trait for listening [`Serial`] interrupt events.
pub trait Listen {
    /// Starts listening for an interrupt event
    ///
    /// Note, you will also have to enable the corresponding interrupt
    /// in the NVIC to start receiving events.
    fn listen(&mut self, event: Event);

    /// Stop listening for an interrupt event
    fn unlisten(&mut self, event: Event);
}
```
- relax `Serial.split` and `Serial.release` trait bounds to `UART: CommonPins`
- relax `Rx.join` and `Tx.join` trait bounds to `UART: CommonPins`

## Questions
- Why `PeriAddress` and `DMASet` implemented for `Rx<UART, u8>`, not `Rx<UART, WORD>`? And Tx too.

## P.S.
I have tried use `#![feature(specialization)]` and `#![feature(min_specialization)]` and failed miserably.
The `min_specialization` not does not cover this case, the `specialization` cause ICE.

Besides I think that current [specialization RFC](https://github.com/rust-lang/rfcs/blob/master/text/1210-impl-specialization.md)  not suitable for our case at all, because we have `impl InstanceUsart` and `impl InstanceUart` with the same "specialization", but not trait bounds inheritance. 

Co-authored-by: Роман Кривенков <qwerty19106@gmail.com>
@qwerty19106
Copy link
Contributor Author

Fixed on #636

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants