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 temperature sensor header for ESP32C3 #259

Merged
merged 1 commit into from
Nov 17, 2023
Merged

Add temperature sensor header for ESP32C3 #259

merged 1 commit into from
Nov 17, 2023

Conversation

GamerBene19
Copy link
Contributor

Prerequisite for getting the internal temperature sensor in various ESP32 chips working.

Also see this issue over in esp-idf-hal: esp-rs/esp-idf-hal#333

@GamerBene19
Copy link
Contributor Author

@Vollbrecht FYI

@Vollbrecht
Copy link
Collaborator

lets make this work here so we can merge this, one idea you could test is if we can use the SOC_TEMP_SENSOR_SUPPORTED define from here to only import the headers, otherwise we may should go the same route with setting it for all but the esp32 with a negation

@GamerBene19
Copy link
Contributor Author

lets make this work here so we can merge this, one idea you could test is if we can use the SOC_TEMP_SENSOR_SUPPORTED define from here to only import the headers, otherwise we may should go the same route with setting it for all but the esp32 with a negation

Seems like SOC_TEMP_SENSOR_SUPPORTED does work. Do you have a preference for one vs. the other approach?

@Vollbrecht
Copy link
Collaborator

lets make this work here so we can merge this, one idea you could test is if we can use the SOC_TEMP_SENSOR_SUPPORTED define from here to only import the headers, otherwise we may should go the same route with setting it for all but the esp32 with a negation

Seems like SOC_TEMP_SENSOR_SUPPORTED does work. Do you have a preference for one vs. the other approach?

well if it works i think we should go with it, because than we don't need to keep track of it since its handled upstream .

@GamerBene19
Copy link
Contributor Author

GamerBene19 commented Nov 15, 2023

lets make this work here so we can merge this, one idea you could test is if we can use the SOC_TEMP_SENSOR_SUPPORTED define from here to only import the headers, otherwise we may should go the same route with setting it for all but the esp32 with a negation

Seems like SOC_TEMP_SENSOR_SUPPORTED does work. Do you have a preference for one vs. the other approach?

At least it did so locally (i tested esp32c3 against esp32). I tried to look at the looks to figure out the problem, but they mention

fatal: No names found, cannot describe anything.

and

error: patch failed: components/app_update/esp_app_desc.c:11

and I have not touched anything related to that so I'm wondering if the issue is not with my changes perhaps?

@Vollbrecht
Copy link
Collaborator

we need to distinguish between esp-idf versions v4 and v5. The driver in its current form we exporting here is only available in v5, look here, they had a driver specific for each target in v4 but it is not that capable compered to the newer on for example here is the header for the c3. So you see they changed the directory structure for the drivers. For now we should import this driver if esp-idf-version > 4 && SOC_TEMP_SENSOR_SUPPORTED

Only includes the header for driver implemented in esp-idf v5+
At the time of writing following ESP variants are supported by the driver ESP32{S2,S3,C2,C3,C6,H2}
@ivmarkov
Copy link
Collaborator

@GamerBene19 @Vollbrecht Is there anything remaining to do here? Still marked as "Draft"?

@Vollbrecht
Copy link
Collaborator

The hal driver is not finished but this one can be merged under the constraint that the SOC_ flags gets passed through from the build-system,

@GamerBene19
Copy link
Contributor Author

under the constraint that the SOC_ flags gets passed through from the build-system,

Wouldn't the tests have failed if that wasn't the case? If not how could one verify that?

@ivmarkov
Copy link
Collaborator

under the constraint that the SOC_ flags gets passed through from the build-system,

Wouldn't the tests have failed if that wasn't the case? If not how could one verify that?

@GamerBene19 can you mark this PR as ready so that I can merge? Thanks.

@ivmarkov ivmarkov marked this pull request as ready for review November 17, 2023 16:08
@ivmarkov
Copy link
Collaborator

Button was right in front of me.

@ivmarkov ivmarkov merged commit 22c5059 into esp-rs:master Nov 17, 2023
4 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.

3 participants