-
Notifications
You must be signed in to change notification settings - Fork 194
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 RtcI2c driver for ESP32-S3 #1694
Conversation
Thanks for the PR! I took a quick glance and things look quite nice, however I will do a proper review some time this week. One small thing, and this is very optional (you've contributed plenty already!), it would be nice to see doc comments on any public types/functions. I'm happy to do this in a subsequent PR if you do not feel like it though, not a big deal at all. We have used the LIS3DH accelerometer for other I2C examples, which each member of our team should have access to. I can throw together a quick example using this sensor when I get around to reviewing/testing this PR. |
I was going to come back to add public docs but I've been occupied with the DMA (and other) stuff. For future PRs I'll add documentation but I'll leave this one to you if you don't mind. 😅 |
I think also everyone owns a BMP180 and an SSD1306 based LCD display. For BMP180 we already have a very similar example |
Sorry took me a bit longer to get to this than expected. I updated your example to reflect our existing example for the BMP180 (forgot I had this chip, should have suggested it initially), however I am currently unable to get it working. It keeps failing with |
I tried to run the example with BMP180 and I have Timeout error as well. With logic analyzer I can see START and STOP bit at least. |
I'm converting this to a draft just to help me keep track of its status (my brain is too full 😁) until we can confirm its working, please feel free to convert it back to a proper PR whenever you're ready. |
I would just like to do a bit of bikeshedding on the name. As with the RtcCntl, do we want to give the driver a somewhat less confusing name? |
I don't care about the name at all but what's confusing about |
The whole "RTC" naming is confusing for the low power peripherals. |
Fair enough. |
I don't see myself getting back to this anytime soon. |
Thank you for your contribution!
We appreciate the time and effort you've put into this pull request.
To help us review it efficiently, please ensure you've gone through the following checklist:
Submission Checklist 📝
cargo xtask fmt-packages
command to ensure that all changed code is formatted correctly.CHANGELOG.md
in the proper section.Extra:
Pull Request Details 📖
Added a RtcI2c driver for the ESP32-S3's main CPU.
In a follow up PR I'll more/less copy the code to the lp-hal for the ULP core to use.
ESP32-S2 support could be added but I don't have one to test with.
Description
See #1030 .
My main blocker is I need to write an example someone on the team can run... Anyone have any register based I2C devices? 😅
I've got these:
Testing
I've ran the code (before I refactored it for this PR) with an ESP32-S3 + I2C 16x2 LCD, from both the main core and ulp core.