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

feat: add an I2C controller abstraction #421

Merged

Conversation

ROMemories
Copy link
Collaborator

@ROMemories ROMemories commented Sep 17, 2024

Description

This PR provides an abstraction layer for I2C support, in controller mode.
It focuses on providing non-DMA-enabled drivers, which still provide an async interface, with timeouts—e.g., to avoid indefinitely blocking when the target is not connected.
The drivers implement embedded_hal_async::i2c::I2c, and can thus easily be used by any device driver relying on that trait.

It provides ways to either select standard frequencies (100 kHz or 400 kHz) which all MCUs are expected to support, or to request the highest available frequency within a range, resolved at compile-time using highest_freq_in.

Issues/PRs references

Depends on #422

Open Questions

  • Should Frequency be renamed to Bitrate?
  • Should we attempt to make the test more generic, so it doesn't rely on a specific sensor?

Limitations

DMA-enabled drivers are out-of-scope of this PR as the number of DMA channels is limited on some architectures and non-DMA-enabled drivers are therefore useful when no channels are left. Moreover, setting up DMA incurs some overhead, and is thus not relevant for small operations as it is the case with sensors, which was the initial motivation for adding support for I2C.

Future work

  • Provide DMA-enabled drivers in a dma module inside i2c::controller.

Change checklist

  • I have cleaned up my commit history and squashed fixup commits.
  • I have followed the Coding Conventions.
  • I have performed a self-review of my own code.
  • I have made corresponding changes to the documentation.

@ROMemories ROMemories added the arch Architecture support label Sep 17, 2024
@ROMemories ROMemories marked this pull request as ready for review September 18, 2024 07:10
@ROMemories
Copy link
Collaborator Author

CI lint and cargo-test jobs are currently failing because the i2c modules expect some laze context cfg flags to be set on some architectures, which could be done for clippy with RUSTFLAGS='--cfg context="esp32c6"' for instance.

@kaspar030 Should we try to leverage laze for running clippy or is it fine to pass only the required cfg flags using that environment variable? If the latter, it's not obvious to me that rs-clippy-check allows to do that as easily as passing CLI arguments.

@kaspar030
Copy link
Collaborator

CI lint and cargo-test jobs are currently failing because the i2c modules expect some laze context cfg flags to be set on some architectures, which could be done for clippy with RUSTFLAGS='--cfg context="esp32c6"' for instance.

@kaspar030 Should we try to leverage laze for running clippy or is it fine to pass only the required cfg flags using that environment variable? If the latter, it's not obvious to me that rs-clippy-check allows to do that as easily as passing CLI arguments.

Using laze would be ideal but seems like more work. I think we might nudge rs-clippy-check into the right direction by adding a step before each arch-specific clippy call setting RUSTFLAGS, e.g.,

- run: echo "RUSTFLAGS='--cfg context=\"esp32c6\"'" >> $GITHUB_ENV
- name: clippy for ESP32                                                                                                                               
  uses: clechasseur/rs-clippy-check@v3                                                                                                                 
  with:                                                                                                                                                
      args:  ...

Could you try that, also if it works two times in a row (later entry in GITHUB_ENV overrides previous)?

@ROMemories ROMemories force-pushed the feat/i2c-controller-abstraction branch from 0a8f9d8 to 6d54881 Compare September 20, 2024 11:19
@kaspar030
Copy link
Collaborator

LGTM (minor last CI fixes)!

@ROMemories
Copy link
Collaborator Author

Could you try that, also if it works two times in a row (later entry in GITHUB_ENV overrides previous)?

I had to remove one of the quotes; the following works:

- run: echo 'RUSTFLAGS=--cfg context="esp32c6"' >> $GITHUB_ENV

@kaspar030
Copy link
Collaborator

LGTM, please squash!

@ROMemories ROMemories force-pushed the feat/i2c-controller-abstraction branch from 84dc9c3 to db9ea57 Compare September 20, 2024 13:17
@ROMemories ROMemories added this pull request to the merge queue Sep 20, 2024
Merged via the queue into future-proof-iot:main with commit 47b90cb Sep 20, 2024
26 checks passed
@ROMemories ROMemories deleted the feat/i2c-controller-abstraction branch September 20, 2024 13:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arch Architecture support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants