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

STM32: Fix I2C repeated start by converting to IT mode #4169

Merged
merged 3 commits into from
Feb 18, 2021
Merged

STM32: Fix I2C repeated start by converting to IT mode #4169

merged 3 commits into from
Feb 18, 2021

Conversation

hierophect
Copy link
Collaborator

The existing implementation of I2C does not currently recognize requests for a repeated start condition, which caused reads to fail on some chips. Since only the "IT" interrupt-driven versions of the ST HAL support this feature, fixing this issue also required implementing the usual infrastructure for ST interrupts, matching other modules like UART and PulseIn.

@tannewt I didn't really touch the base implementation here, but if you think we'd benefit from moving regular I2C operations to the interrupt mode (non-blocking) I can add in the requisite timeouts/keyboard interrupt stuff.

Resolves #3736

@dhalbert
Copy link
Collaborator

dhalbert commented Feb 10, 2021

The single build failure is the stm/boards/espruino_pico ja build, which is overflowing by over 2k bytes, which is odd, because I would have expected previous PR's to cause this.

jepler
jepler previously approved these changes Feb 11, 2021
Copy link
Member

@jepler jepler left a comment

Choose a reason for hiding this comment

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

If you gotta, you gotta. No testing performed. I initially wondered if i2c_assign_irq needed a "free irq" counterpart, but now I see it's just determining the preassigned IRQ for the peripheral... I'll stick on an Approve but I don't know what's going on with that failed build .. doesn't make much sense

@tannewt
Copy link
Member

tannewt commented Feb 11, 2021

I'm fine with whatever you want to do. Please stick to one reviewer. You need to sort out the build issue before we can merge.

@hierophect
Copy link
Collaborator Author

@tannewt I added @siddacious since it was originally his issue, and he might have wanted to test.

@hierophect
Copy link
Collaborator Author

@jepler quick ping for your review again, this should be good to go.

Copy link
Member

@jepler jepler left a comment

Choose a reason for hiding this comment

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

change to save storage seems fine to me. though, rgbmatrix and sharpdisplay both become less useful without framebufferio so consider disabling them as well in a future commit.

@dhalbert
Copy link
Collaborator

We can find something else to remove instead. We were not able to understand why the ja build suddenly seemed so much larger.

@hierophect
Copy link
Collaborator Author

@jepler @dhalbert I really wouldn't worry about it - the STM32F401xD line are very tiny chips that we only see used on the Espruino Pico. It'd be a poor pick for any form of display work anyway.

@hierophect hierophect merged commit 0ecb24c into adafruit:main Feb 18, 2021
@hierophect hierophect deleted the stm32-i2cstart branch February 18, 2021 20:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feather STM32F405 Does not properly handle I2C repeated start
4 participants