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

feat: add support for internal temperature sensor (tsens) for esp32c6 and esp32c3 #2875

Merged
merged 5 commits into from
Jan 7, 2025

Conversation

davoclavo
Copy link
Contributor

@davoclavo davoclavo commented Jan 1, 2025

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.
  • I have added necessary changes to user code to the Migration Guide.
  • My changes are in accordance to the esp-rs API guidelines

Extra:

Pull Request Details 📖

Description

Added basic support for internal temperature sensor for esp32c6 and esp32c3. Starts paving the path to fix #2116

Next steps:

  • Add temperature ranges to the peripheral config
  • Add interrupt handlers
  • Support more chipsets

Testing

I created an example that measures internal chip temperature every second and prints it.

Any feedback is more than welcome, I'm very happy to start contributing to this amazing project!

@davoclavo davoclavo force-pushed the feat/add_temp_sensor_support branch 4 times, most recently from 99cf766 to d8eed8c Compare January 1, 2025 17:39
@davoclavo davoclavo changed the title feat: add support for internal temperature sensor (tsens) for esp32c6 feat: add support for internal temperature sensor (tsens) for esp32c6 and esp32c3 Jan 1, 2025
@davoclavo davoclavo force-pushed the feat/add_temp_sensor_support branch from d8eed8c to 9e46483 Compare January 1, 2025 18:32
esp-hal/src/tsens.rs Outdated Show resolved Hide resolved
@davoclavo davoclavo force-pushed the feat/add_temp_sensor_support branch 2 times, most recently from 0d6b2e7 to 5f033bf Compare January 2, 2025 02:18
esp-hal/src/tsens.rs Show resolved Hide resolved
esp-hal/src/tsens.rs Show resolved Hide resolved
esp-hal/src/tsens.rs Show resolved Hide resolved
esp-hal/src/tsens.rs Outdated Show resolved Hide resolved
esp-hal/src/tsens.rs Show resolved Hide resolved
examples/src/bin/temperature_sensor.rs Outdated Show resolved Hide resolved
@MabezDev
Copy link
Member

MabezDev commented Jan 3, 2025

Thanks for the PR! Overall this a nice PR, and pretty close already - just a few bits to iron out.

@davoclavo davoclavo force-pushed the feat/add_temp_sensor_support branch 4 times, most recently from 4f36b6b to 0f0bd2e Compare January 3, 2025 23:23
@davoclavo
Copy link
Contributor Author

davoclavo commented Jan 3, 2025

Thanks a lot for the feedback. I have addressed all the suggested changes/improvements mentioned and rebased with the latest changes.

About adding a HIL test: I tried to add a deterministic test to ensure the sensor is properly configured and working. Thought about making a test that tries to heat up the CPU and test there was an increase in temperature, but couldn't reliably heat it up in my experiments.

esp-hal/src/lib.rs Outdated Show resolved Hide resolved
@davoclavo davoclavo force-pushed the feat/add_temp_sensor_support branch from 0f0bd2e to ad3e44b Compare January 6, 2025 19:11
Copy link
Member

@jessebraham jessebraham left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM!

Copy link
Member

@MabezDev MabezDev left a comment

Choose a reason for hiding this comment

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

Thanks!

@MabezDev MabezDev added this pull request to the merge queue Jan 7, 2025
@davoclavo
Copy link
Contributor Author

Thanks a lot for the feedback! will keep it in mind, and looking forward to keep contributing :)

Merged via the queue into esp-rs:main with commit 5a64d9b Jan 7, 2025
28 checks passed
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.

Is it possible to add the internal temperature sensor support?
6 participants