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

cpu/esp32: use ESP-IDF i2c HAL for ESP32 periph/i2c #18279

Merged
merged 2 commits into from
Jul 17, 2022

Conversation

gschorcht
Copy link
Contributor

@gschorcht gschorcht commented Jun 28, 2022

Contribution description

This PR is a split-off from PR #17841 and provides the following changes:

  • ESP-IDF i2c HAL and ESP-IDF interrupt HAL are used in hardware implementation (module esp_i2c_hw).
  • In software implementation, which is used by default, fixed delay values are replaced by calculated delays measured in CPU cycles. The advantage is that for each ESP SoC only a clock calibration offset has to be specified. The delay measured in CPU cycles are then derived from current CPU frequency for the given bus speed. The disadvantage is that the calculated delays are not as precise as the predefined fixed delays before.

This PR depends on PR #18270.

Testing procedure

  1. Green CI (compilation of hardware implementation is covered by board esp32-ci).
  2. Compile and test the hardware implementation with
    USEMODULE='esp_i2c_hw esp_log_startup' BOARD=esp32-wroom-32 make -j8 -C tests/periph_i2c flash term
    
  3. Compile and test the software implementation with
    USEMODULE='esp_log_startup' BOARD=esp32-wroom-32 make -j8 -C tests/periph_i2c flash term
    

Issues/PRs references

Split-off from PR #17841
Depends on PR #18270

@github-actions github-actions bot added Area: boards Area: Board ports Area: cpu Area: CPU/MCU ports Area: pkg Area: External package ports Platform: ESP Platform: This PR/issue effects ESP-based platforms labels Jun 28, 2022
@gschorcht gschorcht added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation State: waiting for other PR State: The PR requires another PR to be merged first labels Jun 28, 2022
@gschorcht gschorcht requested a review from benpicco June 28, 2022 19:01
@gschorcht gschorcht changed the title Cpu/esp32/periph hal esp32 i2c cpu/esp32/periph hal esp32 i2c Jun 28, 2022
@gschorcht gschorcht changed the title cpu/esp32/periph hal esp32 i2c cpu/esp32: use ESP-IDF i2c HAL for ESP32 periph/i2c Jun 28, 2022
@gschorcht gschorcht force-pushed the cpu/esp32/periph_hal_esp32_i2c branch from 6a428a3 to e064403 Compare July 15, 2022 14:12
@gschorcht gschorcht added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed State: waiting for other PR State: The PR requires another PR to be merged first labels Jul 15, 2022
Comment on lines +353 to +358
uint8_t module; /**< I2C module identifier */
i2c_speed_t speed; /**< I2C bus speed */
gpio_t scl; /**< GPIO used as SCL pin */
gpio_t sda; /**< GPIO used as SDA pin */
bool scl_pullup; /**< Pullup enabled for SCL pin */
bool sda_pullup; /**< Pullup enabled for SDA pin */
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
uint8_t module; /**< I2C module identifier */
i2c_speed_t speed; /**< I2C bus speed */
gpio_t scl; /**< GPIO used as SCL pin */
gpio_t sda; /**< GPIO used as SDA pin */
bool scl_pullup; /**< Pullup enabled for SCL pin */
bool sda_pullup; /**< Pullup enabled for SDA pin */
gpio_t scl; /**< GPIO used as SCL pin */
gpio_t sda; /**< GPIO used as SDA pin */
uint8_t module; /**< I2C module identifier */
i2c_speed_t speed; /**< I2C bus speed */
bool scl_pullup; /**< Pullup enabled for SCL pin */
bool sda_pullup; /**< Pullup enabled for SDA pin */

struct packing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems that the reordering does not save any byte. Should I change it anyway?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm I had hoped this might save a few bytes of ROM, but it's no big deal either way

cpu/esp32/periph/i2c_hw.c Outdated Show resolved Hide resolved
cpu/esp32/periph/i2c_hw.c Outdated Show resolved Hide resolved
cpu/esp32/periph/i2c_hw.c Outdated Show resolved Hide resolved
cpu/esp32/periph/i2c_hw.c Outdated Show resolved Hide resolved
@benpicco
Copy link
Contributor

Please squash

Fixed delay values are replaced by calculated delays measured in CPU cycles in I2C software implementation. The advantage is that for each ESP SoC only a clock calibration offset has to be specified. The delay measured in CPU cycles are then then derived from current CPU frequency for the given bus speed. The disadvantage is that the calculated delays are not as precise as the predefined fixed delays.
@gschorcht gschorcht force-pushed the cpu/esp32/periph_hal_esp32_i2c branch from 9e757f8 to 06aa8f2 Compare July 17, 2022 16:48
@benpicco benpicco enabled auto-merge July 17, 2022 17:01
@benpicco benpicco merged commit 365fbed into RIOT-OS:master Jul 17, 2022
@gschorcht
Copy link
Contributor Author

@benpicco Thanks for reviewing and merging

@gschorcht gschorcht deleted the cpu/esp32/periph_hal_esp32_i2c branch July 18, 2022 04:50
@gschorcht
Copy link
Contributor Author

gschorcht commented Jul 18, 2022

@benpicco This was the last PR that had to be merged to continue the work. Many thanks again for the quick review and merge of all these split-offs from PR #17841, I really appreciate it. Thanks to your reviews we were able to merge them all into the release, so we should have a consistent state of the APIs used. The only remaining PR #18260 is completely independent of the other changes in PR #17841 and can be merged later, even after the changes in PR #17842 and in conjunction with PR #17844.

@chrysn chrysn added this to the Release 2022.07 milestone Aug 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: boards Area: Board ports Area: cpu Area: CPU/MCU ports Area: pkg Area: External package ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: ESP Platform: This PR/issue effects ESP-based platforms Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants