micropython / micropython-esp32

Old port of MicroPython to the ESP32 -- new port is at https://github.com/micropython/micropython
MIT License
673 stars 216 forks source link

esp32/esp: read internal temperature sensor #192

Closed mogenson closed 6 years ago

mogenson commented 6 years ago

Add a function to the esp module to read the ESP32's internal temperature sensor. This uses temprature_sens_read()[1] from the librtc.a library linked from the ESP-IDF.

I stuffed this into the esp module because it doesn't seem significant enough to warrant a new class in the machine module. Additionally, it could be seen as a property of the ESP32 module, like flash size.

This pull request is primarily a means for me to learn how micropython interacts with the ESP-IDF. Let me know if anything has not been implemented properly.

[1] misspelling is present in the ESP-IDF

nickzoic commented 6 years ago

Great! I'll have a look as soon as possible ... I agree that this is probably an esp thing.

thinks unless most of the different platforms have something similar available in which case perhaps we should add a machine.get_internal_temperature to the common API.

nickzoic commented 6 years ago

stm32 port has this as pyb.ADC(...).read_core_temp()

I didn't notice any of the other platforms having similar functionality, and the ESP32 interface is different to this one, so not sure how relevant that is!

mogenson commented 6 years ago

If I understand correctly, the temperature sensor is read using the SAR ADC1. However, it's not listed as one of the ADC channels (like in the STM32), so it's not part of the ADC driver in the ESP-IDF.

I don't think the organization of micropython has to exactly mirror the ESP-IDF. It's probably a question of where a micropython user would look to find this peripheral.

There's also a hall effect sensor that's read using the ADC but not part of the ADC driver. When implemented, it's placement should be related to the placement of the temperature sensor.

Should these two sensors be methods of the machine.ADC class or go somewhere else?

tuupola commented 6 years ago

FWIW I think machine should only contain stuff which is available in all or most ports. Ie. ESP specific stuff should go to esp.

nickzoic commented 6 years ago

Yeah, I agree. I wanted to check if 'most' other ports had an equivalent, in which case we should standardize something under 'machine' but it looks like not so I think 'esp' is fine.

dpgeorge commented 6 years ago

@mogenson it looks like the temprature_sens_read() function is undocumented... is that right? What is the actual return value, is it in degrees Celcius?

Regarding the placement of the function: to maintain compatibility with the ESP8266 I think it would be best to create a new esp32 module for things very specific to the ESP32. For example there's already gpio_matrix_in() and gpio_matrix_out() which could be moved to an esp32 module.

mogenson commented 6 years ago

@dpgeorge temprature_sens_read() is undocumented. It appears to return raw ADC counts.

So far the temprature_sens_read() function and example routine in esp-idf/components/esp32/test/test_tsens.c are the only ways to access the built in temperature sensor. I was interested in this sensor because it's part of the ESP32 and doesn't require assembling any extra hardware.

nickzoic commented 6 years ago

Hmmm, I see what you mean, it's really truly undocumented in the sense that it isn't even in a header file.

Based on comments here https://github.com/espressif/esp-idf/issues/146 I think we should avoid using it as it isn't documented and is spelled wrong ... perhaps we'd be better off reproducing the test_tsens.c code or pleading with Espressif to implement and document this function for us!

Also that code seems to be in two bits: firstly powering up the sensor, then reading it out. Our API should probably reflect this.

dpgeorge commented 6 years ago

Based on comments here espressif/esp-idf#146 I think we should avoid using it as it isn't documented and is spelled wrong ... perhaps we'd be better off reproducing the test_tsens.c code or pleading with Espressif to implement and document this function for us!

Agreed.

@mogenson If you really want to use the sensor at this stage then it's possible to control the registers directly via the machine.mem32[] memory accessor (or uctypes module). You can write a little Python class to turn it on and read (and convert) a value, and that class should continue to work no matter what other changes there are to the API, either on the IDF level or the uPy level.

nickzoic commented 6 years ago

Hi Michael, thanks for your contribution though and yes, you're going about it the right way.

Hopefully the underlying API issues will be resolved soon and we can revisit this then (see https://github.com/micropython/micropython-esp32/pull/187 where it says "We have a task for adding an API for temperature sensor, which will likely be addressed for the next major version")