Stable driver analysis: I2C master

[MabezDev]

As part of #2491, which has more details on driver analysis.

esp-hal API-GUIDELINE omissions

• The asynchronous driver implementation must also expose the blocking methods (except for interrupt related functions).
  • Blocking and Async methods have the same name and they are implemented exclusively for their respective modes.
• cfg gated defmt derives and impls are added to new structs and enums.
  • I2c doesn't derive anything - but should we even make driver structs debug-printable?
  • Operation enum as well
• API documentation must be provided for every new driver and API.
  • Documentation is lacking in general.
• Private details should not leak into the public API
  • Depends whether we consider State, Info and Instance private. They are public and not hidden. Info exposes the RegisterBlock and system::Peripheral enum - which is necessary if we consider Info part of a public, low-level API. The resolution of this point should also be reflected in the API-GUIDELINES under Driver implementation, as a clarification.
• Prefer compile-time checks over runtime checks where possible, prefer a fallible API over panics.
  • There are a number of unwraps. In set_interrupt_handler it's a common pattern and the function is infallible, but configure_clock should instead return errors.
  • Some panic!s listed (just to revisit if we can improve here): https://github.com/search?q=repo%3Aesp-rs%2Fesp-hal++panic%21+path%3Aesp-hal%2Fsrc%2Fi2c%2Fmaster%2Fmod.rs&type=code
• If your driver provides a way to listen for interrupts
  • It does not, but it should, so I'm marking this as an omission.
• Every line of code is a liability. Take some time to see if your implementation can be simplified before opening a PR.
  • The hardware versions are seemingly more similar than the implementation suggests, we may want to explore this.
• Generally, follow common "good practices" and idiomatic Rust style
  • We have commented-out code, at least one FIXME and TODO in the codebase.
• Prefer cfg_if! over multiple exclusive #[cfg] attributes.
  • We are good here, although multiple versions of the same function exist which violates this rule. The solution, however, is not cfg_if in that case.
• Modules should have the following documentation format
  • Missing link to TRM
  • The documentation is generally uninformative and should be reworked.
• Use rustdoc syntax for linking to other documentation
  • We have a raw markdown link pointing to embedded-hal
• Drivers must have a Drop implementation resetting the peripheral to idle state.
  • Is it an exception? For now, I can't see any reason why not to do a Drop implementation for this. IDF also has some code for this.

---

Rust API guideline omissions

• Naming:
  • C-CASE: Type parameters: DM

    In UpperCamelCase, acronyms and contractions of compound words count as one word: use Uuid rather than UUID, Usize rather than USize or Stdin rather than StdIn

  • C-COMMON-TRAITS ("to be implemented" traits are listed)
    • Error, ConfigError: Display, Eq, Hash˛
    • Operation: Display, Debug, PartialEq, Eq, Hash.
    • Config: Display, Eq, Hash
    • I2c: Debug, Display, defmt::Format, Eq
    • Info, State: ?
  • C-CONV-TRAITS: into_async and into_blocking suggests From might make sense
  • C-SEND-SYNC: review interrupt footgunness
  • C-GOOD_ERR:
    • Error types should always implement the [std::error::Error](https://doc.rust-lang.org/std/error/trait.Error.html) trait -std +core
  • Macros: instance! should be hidden.
  • Documentation: C-METADATA and C-RELNOTES don't apply, others are in violation
  • C-CONV-SPECIFIC: should we add .degrade() to peripheral singletons?
  • C-INTERMEDIATE
    • Should write, read, write_read return the number of transmitted/received bytes on error?
    • Should transaction return the number of successfully completed operations?
  • C-GENERIC: write, read, write_read should accept impl AsRef<[u8]> and AsMut
  • C-CUSTOM-TYPE: these may be okay but we should be mindful of future changes (device-specific timeout options, 10-bit addresses)
    • timeout field in Config is an Option
    • address parameter in write, read, write_read and transaction is a u8
  • C-VALIDATE
    • write, read, write_read and transaction do not validate the address
  • C-DEBUG: Operation, I2c does not implement Debug
    • Info , State: to be decided (but I'll better mention it here)
  • C-FAILURE: Function docs include error, panic, and safety considerations.
    • We're good in terms of safetys, but not # Errors and # Panics
  • C-STRUCT-PRIVATE: Structs have private fields
    • To be discussed: Do we really need ALL fields in Info or State to be public?

---

Hardware feature omissions

• I2C: support 10-bit addressing #2507 - 10bit I2C support is missing
• I2C: direct RAM access is not supported (? idf only implements for slave mode where it is actually useful)
• I2C: low level transaction builder #2598
• I2C: support driving SCL with SDA high (slave reset) #2599
• I2C: allow listening for interrupts #2600
• Incomplete configuration
  • I2C: missing clock source configuration #2601
  • I2C: more granular timeout configuration on later chips #2602
  • I2C: understand and implement forced direct SDA/SCL output #2603
  • I2C: allow disabling bus arbitration #2604
  • I2C: allow configuring bit order #2605
  • I2C: allow configuring clock sampling level #2606
  • I2C: allow configuring pin timings #2607
  • I2C: allow configuring pin filters #2608

Mode/features of the driver that are lacking

• I2C_CHUNK_SIZE needs to be clarified - all devices have 32x8bit RAM
• The different per-device implementations can probably be consolidated a bit

Misc

• Just a nitpick, but we use macro_rules! instance in I2C driver, but macro_rules! impl_instance in the UART one for the same action. It's probably worth unifying the names across esp-hal for this (if present/applicable somewhere else)
• We do not distinguish NACK sources (address/data) which embedded-hal supports. Adding this would change the Error type, most likely renaming the AckCheckFailed variant.
• Housekeeping:
  • TODOs:
    • https://github.com/esp-rs/esp-hal/blob/main/esp-hal/src/i2c/master/mod.rs#L256
    • https://github.com/esp-rs/esp-hal/blob/main/esp-hal/src/i2c/master/mod.rs#L448
  • FIXMEs:
    • https://github.com/esp-rs/esp-hal/blob/main/esp-hal/src/i2c/master/mod.rs#L991
    • https://github.com/esp-rs/esp-hal/blob/main/esp-hal/src/i2c/master/mod.rs#L1076-L1089
    • https://github.com/esp-rs/esp-hal/blob/main/esp-hal/src/i2c/master/mod.rs#L1389-L1402 and https://github.com/esp-rs/esp-hal/blob/main/esp-hal/src/i2c/master/mod.rs#L1424-L1428

---