Add support for ESP internal temperature sensor

[GamerBene19]

What the title says. Currently very much WIP. PR created to help give feedback.

Closes #333

[GamerBene19]

@Vollbrecht FYI

[Vollbrecht]

i added some comments, piece by piece we will get there.

[Vollbrecht]

The interesting point is when you start implement the callback functions provided by the driver. It should allow you to setup a callback on certain temperature condition. In our crate terminology we call it a subscribe, if you want to peak in other drivers how similar things are handled.

[Vollbrecht]

since it seams to work with the flags we are now using in esp-idf-sys. We could also change it here. Rustcompiler give us the flag as esp_idf_soc_temp_sensor_supported.

[Vollbrecht]

If you are unsure how to proceed now to get the rest of the exposed api integrated, we can wrap it up here and i can integrate the rest ontop of it, or if you feel advantageous we can go deeper getting the callbacks to work with the api.
I am not sure whats your background so i leave it up to you.

[GamerBene19]

since it seams to work with the flags we are now using in esp-idf-sys. We could also change it here. Rustcompiler give us the flag as esp_idf_soc_temp_sensor_supported.

Would that then be #[cfg(esp_idf_soc_temp_sensor_supported)]?

If you are unsure how to proceed now to get the rest of the exposed api integrated, we can wrap it up here and i can integrate the rest ontop of it, or if you feel advantageous we can go deeper getting the callbacks to work with the api. I am not sure whats your background so i leave it up to you.

If you don't mind me asking questions and it perhaps taking a bit longer then if you would do it yourself, I would like to try it. If I find out it's too much work we can still only merge my basic support in the future.

[jwhitlark]

Is there any further progress on this? I’m having problems with a chip overheating and would like to quantify the problem better. Usable basic support would be helpful, if other work is paused. I’d be happy to help out, although I’m new enough to this stuff I don’t know how much use I’d be.

[lu-zero]

Since I cannot update the PR myself I opened a rebase of it.

[GamerBene19]

Is there any further progress on this? I’m having problems with a chip overheating and would like to quantify the problem better. Usable basic support would be helpful, if other work is paused. I’d be happy to help out, although I’m new enough to this stuff I don’t know how much use I’d be.

Sorry for the late reply, I forgot to answer back then.

As from my side: I might find some time to work on this in the next few weeks, but I likely need some time to get back into it (which repo of esp-rs does what, what is still missing, ...) and some more help (see question above). If someone else wants to do it instead, feel free :)

I'm pinging @ivmarkov too since he was asking about the state of the PR that @lu-zero opened.

[lu-zero]

I spent a bit of time to try to get it to at least pass the CI, I guess the remaining bits is to wire in the Peripheral and figure out if there are remaining failures.

[ivmarkov]

I spent a bit of time to try to get it to at least pass the CI, I guess the remaining bits is to wire in the Peripheral and figure out if there are remaining failures.

I'm pretty sure we can finish the driver still today or so. It is so simple, that it needs an extra couple of hours at most. And I'll assist of course. :) What I can't help with is putting my own time to test it, but I'm hopeful you can do that.

[GamerBene19]

I'm pretty sure we can finish the driver still today or so.

I'm back at home and have a few hours free time this evening. I'll try to address (some of) the remaining issues.

@lu-zero Do you mind if I cherry-pick your commits into my branch/this PR?

[GamerBene19]

since it seams to work with the flags we are now using in esp-idf-sys. We could also change it here. Rustcompiler give us the flag as esp_idf_soc_temp_sensor_supported.

Would that then be #[cfg(esp_idf_soc_temp_sensor_supported)]?

Can (and should) we use this flag instead of #[cfg(all(not(esp32)))]?

[GamerBene19]

@ivmarkov Regarding the peripheral, I've

• added crate::impl_peripheral!(TempSensor); in temp_sensor.rs
• added

  #[cfg(all(not(esp32), esp_idf_version_major = "5"))]
  pub temp_sensor: temp_sensor::TempSensor,
  

  to Peripherals struct
  and

  #[cfg(all(not(esp32), esp_idf_version_major = "5"))]
  temp_sensor: temp_sensor::TempSensor::new(),
  

  to it's impl
• Adjusted the function signature of the driver constructor to be

  impl<'d> TempSensorDriver {
  pub fn new(
      config: &TempSensorConfig,
      temp_periph: impl Peripheral<P = TempSensor> + 'd,
      _p: PhantomData<&'d mut ()>,
  ) -> Result<Self, EspError>
  

What do I do with temp_periph now? Do I even need to use it, or can I simply prefix it with _?

[ivmarkov]

@ivmarkov Regarding the peripheral, I've

• added crate::impl_peripheral!(TempSensor); in temp_sensor.rs

• added

  #[cfg(all(not(esp32), esp_idf_version_major = "5"))]
  pub temp_sensor: temp_sensor::TempSensor,
  

  to Peripherals struct
  and

  #[cfg(all(not(esp32), esp_idf_version_major = "5"))]
  temp_sensor: temp_sensor::TempSensor::new(),
  

  to it's impl

• Adjusted the function signature of the driver constructor to be

  impl<'d> TempSensorDriver {
  pub fn new(
      config: &TempSensorConfig,
      temp_periph: impl Peripheral<P = TempSensor> + 'd,
      _p: PhantomData<&'d mut ()>,
  ) -> Result<Self, EspError>
  

What do I do with temp_periph now? Do I even need to use it, or can I simply prefix it with _?

It should be instead:

impl<'d> TempSensorDriver<'d> {
pub fn new(
    config: &TempSensorConfig,
    _sensor: impl Peripheral<P = TempSensor> + 'd,
) -> Result<Self, EspError>

Notice:

• That the driver is now lifetimed by 'd. And it should be

   pub struct TempSensorDriver<'d> {
       ptr: ...
       _p: PhantomData<&'d mut ()>,
   }

• The removal of _p from the constructor function. The _p is just a zero-sized trick so that the driver "remembers" that it might get the peripheral by &'d mut (i.e. the peripheral - when passed by &mut stays mutably borrowed until the driver is dropped)

Also, you need to initialize the _p in TempSensorDriver simply with ... PhantomData. No need to pass the PhantomData instance on constructor, as already mentioned above. That's an internal driver detail/trick.

[ivmarkov]

Also - and if it is not clear - the 'p time becomes simply 'static if you - rather than passing the peripheral by &mut pass it by moving, i.e. TempSensorDriver::new(peripherals.temp_sensor). The type of this expression is TempSensorDriver<'static>, so the 'p lifetime does not hurt the "straightforward" use case. Of course, with the "straightforward" use case, you can no longer re-use the peripheral once the driver is dropped. That's why the "other" use case exists, where you pass the peripheral by &mut...

[ivmarkov]

@GamerBene19 @lu-zero Once you feel this is ready (should be, once you address the non-panicing behavior of the dtor), please un-draft it so that we can see if it builds OK.

Post-merge I might implement the usual subscribe / subscribe_nonstatic dance as well as its more ergonomic wait async variant, so that folks don't have to constantly poll the sensor in a loop so as to detect when the device starts to over-heat...

[GamerBene19]

since it seams to work with the flags we are now using in esp-idf-sys. We could also change it here. Rustcompiler give us the flag as esp_idf_soc_temp_sensor_supported.

Would that then be #[cfg(esp_idf_soc_temp_sensor_supported)]?

Can (and should) we use this flag instead of #[cfg(all(not(esp32)))]?

@ivmarkov Do you have an opinion here?

[ivmarkov]

@GamerBene19 Oh one more thing:

unsafe impl<'d> Send for TempSensorDriver<'d> {}

This is necessary so that you can create the driver in one thread and then send it to another (as long as the periph is not mutably borrowed, but owned by the driver). Sending the driver should be completely safe (even if the driver is not thread-safe) so why not?

[ivmarkov]

since it seams to work with the flags we are now using in esp-idf-sys. We could also change it here. Rustcompiler give us the flag as esp_idf_soc_temp_sensor_supported.

Would that then be #[cfg(esp_idf_soc_temp_sensor_supported)]?

Can (and should) we use this flag instead of #[cfg(all(not(esp32)))]?

@ivmarkov Do you have an opinion here?

First of all a small nit: #[cfg(all(not(esp32)))] should be just #[cfg(not(esp32))] or else Clippy might kick-in.
With that said, either the above ^^^ or #[cfg(esp_idf_soc_temp_sensor_supported)] would work, up to you. Though I'm surprised that the soc flags are exposed. We are only supposed to support CONFIG_ ESP IDF entries (and only boolean ones) as Rust #[cfg]s; not sure how the _SOC_ stuff sneaked in, but yes, why not given that it is in there?

[ivmarkov]

@GamerBene19 @lu-zero I guess from your side this is ready for merge?

[lu-zero]

I'm happy with what is done and since I have access to this branch I can close my PR in favor of this one.

[GamerBene19]

@GamerBene19 @lu-zero I guess from your side this is ready for merge?

Unfortunately I currently don't have a functioning ESP with which I could test this code. I think it should work given that there were no "real" changes to the logic, but I think it makes sense to test it before merging.

Also I'd like to try using #[cfg(esp_idf_soc_temp_sensor_supported)] as it is more future proof, I've just pushed a commit to try it out.

[ivmarkov]

@GamerBene19 @lu-zero I guess from your side this is ready for merge?

Unfortunately I currently don't have a functioning ESP with which I could test this code. I think it should work given that there were no "real" changes to the logic, but I think it makes sense to test it before merging.

Also I'd like to try using #[cfg(esp_idf_soc_temp_sensor_supported)] as it is more future proof, I've just pushed a commit to try it out.

@lu-zero Can you check the PR on an MCU?

[lu-zero]

Checked and seems working at least on the c3 and c6, added an example while at it. Shall we keep or remove the get_kelvin and get_fahrenheit?

[lu-zero]

Not sure why I cannot really push on the branch but here is the example used https://github.com/GamerBene19/esp-idf-hal/pull/1

[ivmarkov]

Checked and seems working at least on the c3 and c6, added an example while at it. Shall we keep or remove the get_kelvin and get_fahrenheit?

Ah, keep them - not so important.

Can't see the example in the PR though?

[GamerBene19]

Not sure why I cannot really push on the branch

As far as I can tell you should be able to, you have collaborator access and I don't see an option to explicitly grant you write permissions.

Can't see the example in the PR though?

It's here now

[ivmarkov]

Not sure why I cannot really push on the branch

As far as I can tell you should be able to, you have collaborator access and I don't see an option to explicitly grant you write permissions.

Can't see the example in the PR though?

It's here now

Ah, CI failed. Look at the other examples as to how to build the example only for certain mcus.

[lu-zero]

Github does not let me push to that branch... @GamerBene19 pick the amended commit from lu-zero@9ce159f

[lu-zero]

Sorry, pick the patch again, forgot to move the use inside the main.

[ivmarkov]

Thank you both! Merging.