esp-hal
esp-hal copied to clipboard
esp-rs
Add RtcI2c driver for ESP32-S3
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 📝
- [ ] I have updated existing examples or added new ones (if applicable).
- [x] I have used
cargo xtask fmt-packagescommand to ensure that all changed code is formatted correctly. - [x] My changes were added to the
CHANGELOG.mdin the proper section. - [x] My changes are in accordance to the esp-rs API guidelines
Extra:
- [x] I have read the CONTRIBUTING.md guide and followed its instructions.
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? :sweat_smile:
I've got these:
- https://wiki.seeedstudio.com/Grove-LCD_RGB_Backlight/
- https://shop.m5stack.com/products/i-o-hub-1-to-6-expansion-unit-stm32f0
- https://shop.m5stack.com/products/extend-i-o-unit-2-stm32f0
- https://shop.m5stack.com/products/i2c-hub-1-to-6-expansion-unit-pca9548apw
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.
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.
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.
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. :sweat_smile:
We have used the LIS3DH accelerometer for other I2C examples, which each member of our team should have access to.
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 Error::TimeOut, I will spend some time debugging and try to find the source of this error.
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 RtcI2c? That's what it's called in the TRM.
Fair enough. I'm just going to note that the RTC_I2C peripheral found on the S2/S3 and the LP_I2C peripheral on the C6 (and maybe P4), are very different pieces of hardware and shouldn't be called the same thing. Or maybe call it the same thing but accept that the API will be different depending on the selected chip. 🤷