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

esp32/modesp32.c: Add mcu_temperature() function for C3/S2/S3 devices. #13276

Merged
merged 2 commits into from
May 7, 2024

Conversation

ricksorensen
Copy link
Contributor

For ESP32C3/S2/S3 IDFv5 exposes new internal temperature API which is different than base ESP32, IDFv4.

Thanks to @robert-hh for cleaner code and testing sensor capability in these devices.

See discussion #10443

@DvdGiessen
Copy link
Sponsor Contributor

DvdGiessen commented Jan 8, 2024

Looks good to me. Tested it on custom S3 and C3 boards and seems to work well in my brief test.

@ricksorensen
Copy link
Contributor Author

Thanks for checking

Copy link
Contributor

@projectgus projectgus left a comment

Choose a reason for hiding this comment

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

This looks very useful, thanks @ricksorensen !

@@ -18,6 +18,9 @@ working with this board it may be useful to get an overview of the microcontroll
general.rst
tutorial/index.rst

Note that there are several varieties of ESP32- ESP32, ESP32C3, ESP32S2, ESP32S3 supported by MicroPython,
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a nitpick but please use the same chip naming schema as Espressif (and elsewhere in the docs), i.e. ESP32-C3, ESP32-S2, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will update soon. Just need to get my courage up for trying to update pull request correctly...

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know if this is useful, but jim has some tips here: https://github.com/jimmo/git-and-micropython?tab=readme-ov-file#i-want-to-make-changes-to-the-commits-my-pr--branch

If all else fails, making a new commit and pushing it to this branch is fine - we can squash the commits down before merging.

ports/esp32/modesp32.c Show resolved Hide resolved
docs/esp32/quickref.rst Show resolved Hide resolved
@projectgus
Copy link
Contributor

This is an automated heads-up that we've just merged a Pull Request
that removes the STATIC macro from MicroPython's C API.

See #13763

A search suggests this PR might apply the STATIC macro to some C code. If it
does, then next time you rebase the PR (or merge from master) then you should
please replace all the STATIC keywords with static.

Although this is an automated message, feel free to @-reply to me directly if
you have any questions about this.

@Rumidom
Copy link

Rumidom commented Mar 25, 2024

esp32.raw_temperature() seems to be missing on esp32 S3 Firmware with Support for Octal-SPIRAM

v1.22.2 (2024-02-22) .uf2

>>> import esp32
>>> esp32.raw_temperature()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: 'module' object has no attribute 'raw_temperature'
>>> help(esp32)
object <module 'esp32'> is of type module
  __name__ -- esp32
  wake_on_touch -- <function>
  wake_on_ext0 -- <function>
  wake_on_ext1 -- <function>
  wake_on_ulp -- <function>
  gpio_deep_sleep_hold -- <function>
  idf_heap_info -- <function>
  NVS -- <class 'NVS'>
  Partition -- <class 'Partition'>
  RMT -- <class 'RMT'>
  ULP -- <class 'ULP'>
  WAKEUP_ALL_LOW -- False
  WAKEUP_ANY_HIGH -- True
  HEAP_DATA -- 4
  HEAP_EXEC -- 1

@ricksorensen
Copy link
Contributor Author

@Rumidom I believe all the newer architectures (C2, S2, S3 .. C6?) use the newer IDF 5.x libraries. This PR is an attempt to include them but has not been integrated yet. Part of that is because I have not completely figured out how to fix all the errors, and part is fitting in with the micropython development process.
This PR should give a new temperature function (returning deg celsius) instead of the older ESP32 raw_temperature.
The current code has raw_temperature() functioned only if CONFIG_IDF_TARGET_ESP32 is defined - which is not for the S2/S3/C3/C6 variants.

@robert-hh
Copy link
Contributor

The error may be caused by your habit or merging the master branch into your PR branch, instead for rebasing you branch onto the master. I do not know how to fix that. Maybe it's easier to create a new branch from the actual master and then cherry-pick the commit that make up you PR from the previous branch. Like, when you checked out the new branch:

git checkout master
git checkout -b esp32_temp_new
git cherry-pick

And then re-edit esp32/modesp.c to change STATIC into static. The above mentioned has is 798fb80, but I do not know if it's the same on your PC.

@dpgeorge dpgeorge added this to the release-1.23.0 milestone Apr 30, 2024
Signed-off-by: Rick Sorensen <rick.sorensen@gmail.com>
For ESP32C3/S2/S3 IDFv5 exposes new internal temperature API which is
different to the base ESP32, IDFv4.

Thanks to @robert-hh for cleaner code and testing sensor capability in
these devices.

See discussion micropython#10443.

Signed-off-by: Rick Sorensen <rick.sorensen@gmail.com>
@dpgeorge dpgeorge merged commit 63c30a2 into micropython:master May 7, 2024
9 checks passed
@ricksorensen ricksorensen deleted the esp32_temp branch May 17, 2024 21:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants