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 RtcI2c driver for ESP32-S3 #1694

Closed
wants to merge 1 commit into from
Closed

Conversation

Dominaezzz
Copy link
Collaborator

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).
  • I have used cargo xtask fmt-packages command to ensure that all changed code is formatted correctly.
  • My changes were added to the CHANGELOG.md in the proper section.
  • My changes are in accordance to the esp-rs API guidelines

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.

@jessebraham
Copy link
Member

jessebraham commented Jun 24, 2024

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.

@Dominaezzz
Copy link
Collaborator Author

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. 😅

@bjoernQ
Copy link
Contributor

bjoernQ commented Jul 1, 2024

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

@jessebraham
Copy link
Member

jessebraham commented Jul 2, 2024

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.

@JurajSadel
Copy link
Contributor

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.

@jessebraham
Copy link
Member

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.

@jessebraham jessebraham marked this pull request as draft July 23, 2024 18:50
@bugadani
Copy link
Contributor

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?

@Dominaezzz
Copy link
Collaborator Author

I don't care about the name at all but what's confusing about RtcI2c? That's what it's called in the TRM.

@bugadani
Copy link
Contributor

The whole "RTC" naming is confusing for the low power peripherals.

@Dominaezzz
Copy link
Collaborator Author

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. 🤷

@Dominaezzz
Copy link
Collaborator Author

I don't see myself getting back to this anytime soon.

@Dominaezzz Dominaezzz closed this Aug 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants