i2c_master: improve documentation

[JohSchneider]

while working on a new incarnation of an existing avr-keyboard, with now a chibios-based MCU i ran into compile-time issues for a driver that relies on the platform specific i2c_master implementation

it turned out that a "misaligned" signature in the chibios variant was the culprit...

these two patches re-align the "i2c_start" method with the documentation and the current avr implementation, by adding a "timeout" parameter

this PR possibly contributes a little to #8297

Description

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

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).

[sigprof]

This change is not really useful — it just increases the confusion, because the i2c_start() and i2c_stop() function provided by the AVR and ChibiOS drivers are completely different.

The AVR i2c_master driver provides the low-level I2C API:

• i2c_start(uint8_t address, uint16_t timeout) — transmit the START condition and the device address;
• i2c_write(uint8_t data, uint16_t timeout) — transmit a single data byte;
• i2c_read_ack(uint16_t timeout) — receive a single byte and reply with ACK;
• i2c_read_nack(uint16_t timeout) — receive a single byte and reply with NACK;
• i2c_stop() — transmit the STOP condition.

It also provides the high-level I2C API on top of that low-level API:

• i2c_transmit(uint8_t address, const uint8_t* data, uint16_t length, uint16_t timeout):
  transmit START condition, address with the WRITE direction, data bytes, STOP condition;
• i2c_receive(uint8_t address, uint8_t* data, uint16_t length, uint16_t timeout):
  transmit START condition, transmit address with the READ direction, receive data bytes (replying with ACK to all bytes except the last one), transmit STOP condition;
• i2c_writeReg(uint8_t devaddr, uint8_t regaddr, const uint8_t* data, uint16_t length, uint16_t timeout):
  transmit START condition, transmit address with the WRITE direction, transmit register address, transmit data bytes, transmit STOP condition;
• i2c_writeReg16(uint8_t devaddr, uint16_t regaddr, const uint8_t* data, uint16_t length, uint16_t timeout):
  the same as above with a 16-bit register address (big endian on the I2C bus);
• i2c_readReg(uint8_t devaddr, uint8_t regaddr, uint8_t* data, uint16_t length, uint16_t timeout):
  transmit START condition, transmit address with the WRITE direction, transmit register address, transmit START condition, transmit device address with the READ direction, receive data bytes (replying with ACK to all bytes except the last one), transmit STOP condition);
• i2c_readReg16(uint8_t devaddr, uint16_t regaddr, uint8_t* data, uint16_t length, uint16_t timeout):
  the same as above with a 16-bit register address (big endian on the I2C bus).

However, the ChibiOS I2C API does not provide any equivalent to the low-level I2C API — it provides only functions like i2cMasterReceiveTimeout() and i2cMasterTransmitTimeout(), which could be used to implement the high-level I2C API. Therefore the QMK i2c_master driver for ChibiOS implements only the high-level I2C API, and ideally it should not have the i2c_start() and i2c_stop() functions at all.

Unfortunately, somebody thought that it would be good to expose the i2cStart() and i2cStop() functions of the ChibiOS I2C API (which actually enable and disable the I2C controller and do not send anything over the I2C bus) under the i2c_start() and i2c_stop() names, despite the fact that functions with these names do completely different things on AVR. So now we are stuck with those names until someone finally pushes through a cleanup PR for this.

---

As for porting your keyboard, what you actually need to do is rewrite the code to use the high-level I2C API instead of the AVR-only low-level API — then your code should work on both AVR and ChibiOS. Any calls to i2c_start() or i2c_stop() would be dropped in the process, therefore the signature mismatch won't matter.

[JohSchneider]

thanks for the detailed feedback - wasn't aware of that kind of code legacy...

is rewrite the code to use the high-level I2C API instead

good idea! i'll do that

btw: from what i saw - going through the code and "adjusting" the few i2c_start(add_only) calls: there aren't that many users of the chibios version... so the "wrong-ish" chibios:i2c_start could be dropped relatively easily - thoughts on that?

also: revising the documentation to recommend/push users to stay with the higher level i2c-api instead of i2c_star/_stop?

[JohSchneider]

regarding documentation: how about something like this: e6ff49e ?

[github-actions]

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]

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]

Thank you for your contribution!
This pull request has been automatically closed because it has not had activity in the last 30 days. Please feel free to give a status update now, ping for review, or re-open when it's ready.
// [stale-action-closed]