stm32l4-multi: fix I2C timing calculation #600
Conversation
Summary of ChangesHello @jmaksymowicz, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the I2C driver for Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request significantly improves the I2C timing calculation by replacing a hardcoded and buggy implementation with a flexible and more robust one. The new code correctly calculates timing parameters for various I2C speeds and reference clock frequencies. The changes are well-structured, introducing a new API for speed configuration and encapsulating the complex calculations. I have one suggestion to improve the documentation of empirically-derived values for better long-term maintainability.
| /* Regardless of frequency, the low period seems to be 3 cycles too long and high period 2 cycles too long. | ||
| * This was measured empirically. */ | ||
| tSCLL = (tSCLL > (tDNF + 3)) ? (tSCLL - (tDNF + 3)) : 1; | ||
| tSCLH = (tSCLH > (tDNF + 2)) ? (tSCLH - (tDNF + 2)) : 1; |
There was a problem hiding this comment.
These empirical correction values are 'magic numbers' that can make the code harder to maintain. While the comment mentions they were measured empirically, adding more context would be very beneficial for future developers. Could you consider expanding the comment with details about the measurement setup (e.g., hardware, chip revision), or if this is a known silicon issue, a reference to an errata sheet or application note?
There was a problem hiding this comment.
I added a bigger comment closer to the top of the file so if there are any problems with I2C frequency hopefully the person fixing them will see it.
Calculate I2C timing variable at runtime based on reference clock and chosen bus speed. Add option to enable analog noise filter at compile time. Add API message for changing I2C speed. JIRA: RTOS-1176
jmaksymowicz
force-pushed
the
jmaksymowicz/stm32-i2c-fix
branch
from
55a3d89 to
24b9b75
Compare
TODO: Needs to be tested on STM32L4x6 platforms, so far only tested on STM32N6
Description
The previous code for setting I2C speed had multiple issues - the speed was hardcoded to I2C standard mode (100 kHz) and due to a mistake the calculation failed for reference clock frequency 64 MHz and higher. Additionally, the resulting I2C clock speed was not optimal and it wasn't clear how it was calculated.
This new code makes the speed calculation process flexible, allowing for any reference clock between 8 and 400 MHz. All standard I2C bus speeds are supported (100 kHz, 400 kHz and 1 MHz). The calculation now takes into account all relevant factors, so that optimal clock speeds can be achieved.
Because this code only uses integer operations there is less precision so that you can only achieve within about 5% of the optimal clock speed in the Fast Plus mode.
I added an option to enable the analog noise filter at compile time (off by default, but we may want to enable it in the future).
The user can now set the I2C clock speed through a new API message. I decided to give the user the ability to set I2C rise time compensation value. This is because the rise time depends on external factors such as pull-up resistor value and line capacitance, so it cannot be accounted for in the driver.
Motivation and Context
JIRA: RTOS-1176
Types of changes
How Has This Been Tested?
Checklist:
Special treatment