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

Support for inverting UART lines #1703

Closed
wants to merge 1 commit into from

Conversation

SergioGasquez
Copy link
Member

Thank you for your contribution!

We appreciate the time and effort you've put into this pull request.
To help us review it efficiently, please ensure you've gone through the following checklist:

Submission Checklist 📝

  • I have updated existing examples or added new ones (if applicable).
  • I have used cargo xtask fmt-packages command to ensure that all changed code is formatted correctly.
  • My changes were added to the CHANGELOG.md in the proper section.
  • My changes are in accordance to the esp-rs API guidelines

Extra:

Pull Request Details 📖

Description

Adds support for inverting UART signals. See #1216

Testing

    let inverted_signals = InvertedSignals {
        rx: true,
        tx: true,
        rts: false,
        cts: false,
    };

    let mut uart1 = UartTx::new(peripherals.UART1, &clocks, None, io.pins.gpio5).unwrap();
    uart1.set_inverted_signals(inverted_signals);
    let mut uart0 = UartRx::new(peripherals.UART0, &clocks, None, io.pins.gpio4).unwrap();
    uart0.set_inverted_signals(inverted_signals);

    loop {
        uart1.write_bytes(&[0x42]).ok();
        delay.delay_millis(250);

        let read = block!(uart0.read_byte());

        match read {
            Ok(read) => println!("Read 0x{:02x}", read),
            Err(err) => println!("Error {:?}", err),
        }
    }
  • On H2, using rx: true and tx: true, it works fine (it reads 0x42), and if we use rx: false and tx: true it reads what I expect (0x2f)
  • On C3, using rx: true and tx: true, it works fine (it reads 0x42), but if we use rx: false and tx: true it also reads (0x42) which I think its wrong.
    let inverted_signals = InvertedSignals {
        rx: true,
        tx: true,
        rts: false,
        cts: false,
    };
    let mut uart = Uart::new(peripherals.UART1, &clocks, io.pins.gpio4, io.pins.gpio5).unwrap();
    uart.set_inverted_signals(inverted_signals);
    loop {
        uart.write_byte(0x42).ok();
        let read = block!(uart.read_byte());
        match read {
            Ok(read) => println!("Read 0x{:02x}", read),
            Err(err) => println!("Error {:?}", err),
        }

        delay.delay_millis(250);
    }

When running this example on H2, I also read 0x2f (the inverted 0x42) even when using rx: true and tx: true. Which is wrong.

My guess is that it has to do with sync_regs() as the order of calling:

        self.rx.set_inverted_signals(inverted_signals);
        self.tx.set_inverted_signals(inverted_signals);

affects the results. I also did some other test of changing the order in which we set the register and sync them, but no luck so far.

Other things/approaches I tried:

  • Tried to invert the signals using connect_peripheral_to_output_with_options instead of connect_peripheral_to_output and setting the invert value to true, but when doing this, the TX would just not send anything.
  • Tried to make InvertSignals struct part of the Config and update the inverted signals in the constructor (you would need to use new_with_config(). But ESP-IDF does not make inverted signals part of the conifg and has some public methods to set them, so I discarded this approach.

@SergioGasquez SergioGasquez linked an issue Jun 21, 2024 that may be closed by this pull request
@bjoernQ
Copy link
Contributor

bjoernQ commented Jun 24, 2024

Just wondering why this is needed?

Something like this already works

#[entry]
fn main() -> ! {
    let peripherals = Peripherals::take();
    let system = SystemControl::new(peripherals.SYSTEM);
    let clocks = ClockControl::boot_defaults(system.clock_control).freeze();

    let io = Io::new(peripherals.GPIO, peripherals.IO_MUX);
    let a = esp_hal::gpio::any_pin::AnyPin::new_inverted(io.pins.gpio4);
    let b = esp_hal::gpio::any_pin::AnyPin::new_inverted(io.pins.gpio5);

    let mut serial1 = Uart::new(peripherals.UART1, &clocks, a, b).unwrap();

    let delay = Delay::new(&clocks);

    println!("Start");
    loop {
        serial1.write_byte(0x42).ok();
        let read = block!(serial1.read_byte());

        match read {
            Ok(read) => println!("Read 0x{:02x}", read),
            Err(err) => println!("Error {:?}", err),
        }

        delay.delay_millis(250);
    }
}

I think a few peripherals have addtional support for inverting a signal at the peripheral level but I'm wondering if we need two mechanisms to do the same thing? Or maybe it's not the same thing?

@SergioGasquez
Copy link
Member Author

I think a few peripherals have addtional support for inverting a signal at the peripheral level but I'm wondering if we need two mechanisms to do the same thing?

I would agree with you, if we already have a way to achieve it, it's not worth to add this.

Or maybe it's not the same thing?

The end result is the same, but looks like there are 2 ways of achieving it, by modifying the UART registers or by inverting the pins.

Btw, I tried your suggested method and it works well, both for H2 and S3.

@jessebraham
Copy link
Member

Yeah, perhaps just using the existing method and documenting the behaviour is the correct path forward.

@SergioGasquez
Copy link
Member Author

Ill close this PR as we decided to use this method which is documented in #1726

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

Successfully merging this pull request may close these issues.

Uart: polarity inversion
3 participants