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

I2C driver cleanup #21273

Merged
merged 2 commits into from
Jan 17, 2024
Merged

I2C driver cleanup #21273

merged 2 commits into from
Jan 17, 2024

Conversation

infinityis
Copy link
Contributor

QMK's ARM and AVR I2C drivers currently expose i2c_start() and i2c_stop() which are lower level functions used in the I2C protocol. These functions should not be called outside of the driver because their misuse can introduce errors to I2C bus communications. This main focus of this PR is to remove those functions from the ARM and AVR I2C driver interface.

Description

  • Keyboard code (and other code) which previously called i2c_start() and i2c_stop() has been rewritten to use the applicable I2C functions instead.
  • i2c_write(), i2c_read_ack(), i2c_read_nack() have been eliminated from the AVR I2C driver interface for the same reason. These were only exposed in the AVR I2C driver (without documentation); they did not exist in the ARM driver interface.
  • i2c_transmit_P() has been added to the AVR driver, and is now used by the legacy OLED driver
  • i2c_ping_address() has been added to the AVR and ARM drivers (as a weakly defined function to compensate for the limitations of the ARM implementation)
  • Keyboards previously defining I2C_ADDR_WRITE and I2C_ADDR_READ have had those definitions removed in favor of keeping only I2C_ADDR

I was only able to test on one keyboard with an OLED (EVO70) and it worked with no issues, which indicates that i2c_transmit_P has been tested. All the affected keyboards successfully compiled for me, but I did not have hardware to test them on.

List of affected keyboards:

  • hotdox
  • ergodox_ez
  • gboards/ergotaco
  • gboards/georgi
  • gboards/gergo
  • gboards/gergoplex
  • bajjak
  • system76/launch_1
  • dc01/left
  • handwired/frenchdev
  • handwired/pterodactyl
  • kagizaraya/chidori
  • ymdk/sp64
  • nek_type_a
  • ingrained
  • ferris/0_2
  • 3w6/rev2

There are a few keyboards that defined their own lower-level I2C functions. Because they don't use the QMK drivers, they were outside of the scope for this PR, but they are good candidates for future improvement (to retool them to use QMK's I2C drivers instead). These keyboards are:

  • lfkeyboards
  • input_club/k_type
  • fc980c
  • fc660c
  • woodkeys/meira

Types of Changes

  • Core
  • Bugfix
  • New feature
  • Enhancement/optimization
  • Keyboard (addition or update)
  • Keymap/layout/userspace (addition or update)
  • Documentation

Issues Fixed or Closed by This PR

#19997 (comment)
*

Checklist

  • My code follows the code style of this project: C, Python
  • I have read the PR Checklist document and have made the appropriate changes.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • I have tested the changes and verified that they work and don't break anything (as well as I can manage).

@tzarc
Copy link
Member

tzarc commented Jun 17, 2023

No code review as yet, but tested against #19997 and I2C OLEDs still work fine (with appropriate no-ops for comms start/stop).

Copy link
Contributor

@sigprof sigprof left a comment

Choose a reason for hiding this comment

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

Partial review (did not really look at all that expander code that should not exist in so many copies scattered around).

keyboards/dc01/left/matrix.c Outdated Show resolved Hide resolved
keyboards/ergodox_ez/led_i2c.c Show resolved Hide resolved
keyboards/ergodox_ez/led_i2c.c Outdated Show resolved Hide resolved
@sigprof
Copy link
Contributor

sigprof commented Jun 17, 2023

Also keyboards/handwired/d48/ds1307.c should have the i2c_stop(); call removed to make it compile again.

@tzarc tzarc requested review from tzarc and a team October 29, 2023 10:16
Copy link

Thank you for your contribution!
This pull request has been automatically marked as stale because it has not had activity in the last 45 days. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, or re-open when it's ready.
For maintainers: Please label with bug, awaiting review, breaking_change, in progress, or on hold to prevent the issue from being re-flagged.

@github-actions github-actions bot added the stale Issues or pull requests that have become inactive without resolution. label Dec 24, 2023
@KarlK90 KarlK90 added awaiting review and removed stale Issues or pull requests that have become inactive without resolution. labels Dec 24, 2023
Copy link
Member

@KarlK90 KarlK90 left a comment

Choose a reason for hiding this comment

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

@infinityis thanks for tackling this long standing inconsistency in i2c driver api, IMHO this is the right way forward.

I've tested the changes with a cirque trackpad and RP2040 mcu and everything works just like before. There are some minor remarks but from my side this is good to go. There are some conflicts with the develop branch due to the removal of user spaces from main repo that would need to be resolved though.

drivers/painter/comms/qp_comms_i2c.c Show resolved Hide resolved
drivers/painter/comms/qp_comms_i2c.c Outdated Show resolved Hide resolved
platforms/avr/drivers/i2c_master.c Outdated Show resolved Hide resolved
platforms/avr/drivers/i2c_master.c Outdated Show resolved Hide resolved
@KarlK90
Copy link
Member

KarlK90 commented Jan 16, 2024

@infinityis #22905 got merged before your PR, could you rebase once more?

@KarlK90 KarlK90 merged commit e9bd7d7 into qmk:develop Jan 17, 2024
3 of 5 checks passed
nuess0r pushed a commit to nuess0r/qmk_firmware that referenced this pull request Sep 8, 2024
* remove i2c_start and i2c_stop from i2c drivers

* remove static i2c_address variable from chibios i2c driver
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants