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

Add extended I2C HAL functions #3037

Merged
merged 20 commits into from
Sep 21, 2023
Merged

Add extended I2C HAL functions #3037

merged 20 commits into from
Sep 21, 2023

Conversation

agarof
Copy link
Contributor

@agarof agarof commented Sep 3, 2023

What's new

  • Add two functions furi_hal_i2c_tx_ext and furi_hal_i2c_rx_ext, giving the user more flexibility in writing I2C drivers by giving them more choices such as sending RESTART conditions and using multiple buffers in the same transaction.
  • Enable transfers larger than 255 bytes by automatically using the reload flag, The flag has been added to existing functions and shouldn't be an API breaking change.
  • Make the timeout for multi-transactions functions such as furi_hal_i2c_trx shared between all transactions instead of being duplicated.
  • Fix an issue where we wait for the timeout to expire when a device is not present on the bus in both the RX/TX functions and furi_hal_i2c_is_device_ready.
  • Add ten bit addresses support (untested).

I tried to keep the binary size increase to a minimum but this still adds about 300 bytes to the final binary (304 in debug, 296 in compact).

Verification

To check for basic functionality, simply boot the Flipper. Non working code will cause the communication with the battery control chips and the LED driver to fail, resulting in missing battery readouts and no screen backlight.
More advanced verification must be done using a logic analyzer.
As I have not opened my Flipper, I have only checked the behavior using the external bus.

Checklist (For Reviewer)

  • PR has description of feature/bug or link to Confluence/Jira task
  • Description contains actions to verify feature/bugfix
  • I've built this code, uploaded it to the device and verified feature/bugfix

Comment on lines +86 to +87
return ten_bit_address ? LL_I2C_GENERATE_RESTART_10BIT_WRITE :
LL_I2C_GENERATE_RESTART_7BIT_WRITE;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This could be simplified, since both values are equal, depending on how close to ST's LL you want to stick.

Copy link
Member

Choose a reason for hiding this comment

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

We'd rather hide anything related to ST LL inside.

Copy link
Member

@skotopes skotopes left a comment

Choose a reason for hiding this comment

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

Unit tests needed: can be gauge, charger or led driver.

firmware/targets/f7/furi_hal/furi_hal_i2c.c Outdated Show resolved Hide resolved
firmware/targets/furi_hal_include/furi_hal_i2c.h Outdated Show resolved Hide resolved
firmware/targets/furi_hal_include/furi_hal_i2c.h Outdated Show resolved Hide resolved
firmware/targets/furi_hal_include/furi_hal_i2c.h Outdated Show resolved Hide resolved
@skotopes skotopes marked this pull request as draft September 4, 2023 03:10
@skotopes
Copy link
Member

skotopes commented Sep 4, 2023

un draft when ready

@agarof
Copy link
Contributor Author

agarof commented Sep 5, 2023

I have fixed the functions and enums naming and have checked that the furi_hal (LED driver communication) and power (charger) tests pass.

@agarof agarof marked this pull request as ready for review September 5, 2023 00:16
@skotopes
Copy link
Member

skotopes commented Sep 6, 2023

I've pushed bunch of small edits: docs update, const qualifier usage cleanup, fix with incorrect argument pass. In general everything looks good.

We do need one last thing: unit tests, they must cover newly added features.
You can use any of existing flipper chips or some other popular one that we can buy and add to our unit tests stand.

@skotopes skotopes marked this pull request as draft September 6, 2023 06:17
@agarof
Copy link
Contributor Author

agarof commented Sep 11, 2023

I have added two unit tests, one testing the Restart and Pause / Resume features, the other testing the auto-reload feature.

The first uses the LED driver, same as the existing tests.
The second uses an I2C EEPROM connected to the external bus. It writes 512 bytes 16 at a time then reads them all at once, so the EEPROM needs to be at least 512 bytes large and have a 16 bytes page write buffer. Most I2C EEPROMs I have found, such as these two, match these characteristics and have the same addressing scheme as the one I have (I don't have a reference for it).

@agarof agarof marked this pull request as ready for review September 11, 2023 19:19
skotopes
skotopes previously approved these changes Sep 21, 2023
@skotopes skotopes merged commit f46018b into flipperdevices:dev Sep 21, 2023
9 checks passed
@skotopes
Copy link
Member

That was some great work, you did well ;-)

@hedger hedger added New Feature Contains an IMPLEMENTATION of a new feature Core+Services HAL, furi & core system services labels Oct 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Core+Services HAL, furi & core system services New Feature Contains an IMPLEMENTATION of a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants