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

Docstrings for GPIO/PinDriver #473

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Changes from 1 commit
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
35 changes: 35 additions & 0 deletions src/gpio.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,39 @@
//! GPIO and pin configuration
//!
//! Interface for the input/output pins.
//!
//! `Gpio1` through `Gpio39` represent the *physical* pins of the ESP chip.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Gpio39 -> GpioXX

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also maybe "... represent the physical pins or in other words - the pins peripherals of the ESP chip".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Espressif's documentation uses the "physical pin" nomenclature, so I believe it's important to keep it. I'll rephrase this part.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I never said to remove the physical notion, but to add to it that we mean a peripheral, which is the notion we use throughout the hal.

//!
//! *Logical* pins (compatible with `embedded_hal::digital::InputPin`/`OutputPin`)
//! are implemented through `PinDriver`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe add: "... just like other embedded_hal traits are implemented on their corresponding *Driver structs and not on the peripherals themselves"

Copy link
Collaborator

Choose a reason for hiding this comment

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

You can also sprinkle a mention of the embedded_hal_async pin traits as these are also supported by PinDriver.

//!
//! The ESP architecture has a I/O multiplexer, which means that (almost) any
//! physical pin can be used for any logical function (i.e. GPIO, I2C, SPI, ADC, etc).
//! Even though it's possible to use a pin for several functions at once, this
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is that really possible? Even if it is possible, it is not possible with the safe API of esp-idf-hal for sure, as the pin peripheral (what you call above "physical" pins) can only be used by one driver at a time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From what I'm reading at https://hackaday.io/page/21183-hacking-esp32-multiplexing-i2c , it seems that it's possible to use the same pin peripheral ("physical") as the CLK pin in two different I2C interfaces at once.

Which means one of the I2C buses is working as expected, and the other one is receiving all ones (or all 0xFFs) on the SDA pin, which is innocuous enough.

Copy link
Contributor Author

@IvanSanchez IvanSanchez Aug 20, 2024

Choose a reason for hiding this comment

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

See also the notes on pages 51 and 53 of https://www.espressif.com/sites/default/files/documentation/esp32_technical_reference_manual_en.pdf#iomuxgpio :

One input pad can be connected to multiple input_signals.

and

The output signal from a single peripheral can be sent to multiple pads simultaneously.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Interesting. How would you do that with the ESP IDF C API?

//! should be avoided. This is particularly important with the pins used for the
//! SPI RAM and the SPI Flash.
//!
//! # Examples
//!
//! Create a logical input/output pin on physical pin 2
//! ```
//! use esp_idf_hal::peripherals::Peripherals;
//! use esp_idf_hal::gpio:PinDriver;
//! use esp_idf_hal::gpio:Level;
//!
//! let physical_pin_2 = Peripherals::take().unwrap().pins.gpio2;
//!
//! // Set pin to input/output and open drain
//! let logical_pin_2 = PinDriver::input_output_od().unwrap();
//!
//! // Set pin to high
//! logical_pin_2.set_level(Level::High);
//!
//! // The logical pin implements some embedded_hal traits, so it can
//! // be used in crates that rely on those traits, e.g.:
//! use one_wire_bus::OneWire;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you come up with a different example from one_wire_bus? The reason why esp-idf-hal got a onewire driver just recently is to avoid the generic busy-looping one_wire_bus & similar drivers, as they simply do not work reliably. Under ESP IDF at least. So it might be weird to suggest in an example an API which we would otherwise advise against using.

//! let bus = OneWire::new(logical_pin_2).unwrap();
//! ```

use core::marker::PhantomData;

Expand Down
Loading