Internal temperature sensor #478

lu-zero · 2024-09-08T11:01:36Z

Rebase of #337

Code is very rudimentary and still suspect to change. Notably, error handling is not implemented yet.

ivmarkov · 2024-09-08T11:45:53Z

Thanks a lot!

However, the CI fails on a number of chips, you need to look into what is wrong there.
I also have a few comments specific to the PR, will follow up shortly with those.

ivmarkov

Most important feedback is w.r.t. the fact that this driver does not take any peripheral, because it does not even define/model the internal temperature sensor peripheral.

Since every other driver in esp-idf-hal takes a peripheral, we should do this here as well. Lok in the other drivers as to how to model a peripheral, it is not complex at all, and there are macros that do this.

src/temp_sensor.rs

ivmarkov · 2024-09-08T11:49:46Z

src/temp_sensor.rs

+#[cfg(not(any(esp32)))]
+#[derive(Copy, Clone)]
+/// Rust wrapper for `temperature_sensor_config_t`
+pub struct TemperatureSensorConfig {


Please also provide a const new() constructor function so that folks can use a const TemperatureSensorConfig. Default can then delegate to this new constructor

Please move the TemperatureSensorConfig to its own mini config mod, as we do everywhere and rename to just config. You should then also do pub type TemperatureSensorConfig = config::Config

Done in 76289a1 and 01ce494

ivmarkov · 2024-09-08T11:50:26Z

src/temp_sensor.rs

+// -- TemperatureSensorConfig --
+
+#[cfg(not(any(esp32)))]
+#[derive(Copy, Clone)]


Remove the Copy derivation

Add a Debug derivation and why not PartialEq, Eq and Hash

ivmarkov · 2024-09-08T11:50:45Z

src/temp_sensor.rs

+
+// -- TemperatureSensorClockSource --
+
+#[derive(Copy, Clone)]


Add Debug, Eq, PartialEq, Hash

ivmarkov · 2024-09-08T11:52:16Z

src/temp_sensor.rs

+
+#[cfg(not(any(esp32)))]
+impl TemperatureSensorDriver {
+    pub fn new(config: TemperatureSensorConfig) -> Result<Self, EspError> {


Take the config by & as we do everywhere else. You might need to change your From trait impl after that.

Done(ish) in 4d34e0e

See #337 (comment) for further discussion.

ivmarkov · 2024-09-08T11:52:49Z

src/temp_sensor.rs

+}
+
+#[cfg(not(any(esp32)))]
+impl TemperatureSensorDriver {


This is the first driver that does not take any peripheral.

So here's the question: what happens if I try to instantiate the driver twice?

from the documentation:

In the temperature sensor driver, we do not add any protection to ensure the thread safety, because typically this driver is only supposed to be used in one task. If you have to use this driver in different tasks, please add extra locks to protect it.

What I'm asking for does not have anything to do with thread (un)safety.
Thread unsafety is correctly modeled already by:

the driver taking &mut refs everywhere

the driver containing a raw pointer (ptr) which makes it !Sync

(Before I forget: the driver however needs an unsafe impl Send for TemperatureSensorDriver {} because currently it is also !Send due to the above mentioned ptr. However, there is nothing that stops the driver from being Send so we should mark it as such.)

What I'm talking about is - consider this:

let driver1 = TemperatureSensorDriver::new()?; let driver2 = TemperatureSensorDriver::new()?; // Will likely fail with a runtime error because the C driver is already installed // there is a deeper problem here however - what if you try to call `TemperatureSensorDriver::new` // from TWO separate threads, and thus the two calls race each other? // If the C call `temperature_sensor_install` is itself thread-safe, that would work and one of these calls will fail, // while the other will succeed; however if it is not thread-safe, you might end up with memory corruption or even worse things

The fact the second driver instantiation fails is OK because it does not make sense to have multiple drivers for a single peripheral. However, esp-idf-hal goes one step further than that: every driver takes a peripheral. This way - even if you wanted to instantiate the driver twice, you cannot. At compile time. Including from multiple threads, as only one of the thread can "own" the peripheral at any point in time. I suggest (again) to take a look at the other drivers as to how to model a peripheral.

Btw, and if you are struggling with the "peripheral" thing:

You only need to copy-paste this line in your temp_sensor module. But instead of HallSensor of course use TemperatureSensor for the peripheral name.

Then, you just need to add the TemperatureSensor as a temperature_sensor member in the Peripherals structure, and then change the TemperatureSensorDriver::new to take the peripheral, using the impl Peripheral<P = TemperatureSensor> + 'd magic you can copy from everywhere (this allows the driver to take the peripheral either by move, or by &mut).

Update: you'll also need a _p: PhantomData<&'d mut ()> in the driver and the driver should become TemperatureSensorDriver<'d> (i.e. lifetimed by 'd). If you don't know what this is - don't worry - just copy-paste it from elsewhere. I'll help if you have compilation issues.

Also, what do you think about renaming TemperatureSensor* everywhere to just TempSensor*?

TemperatureSensor is a bit too long

Module is already named temp_sensor

(The peripheral instance in the Peripherals struct can then also be named temp_sensor.)

Also, what do you think about renaming TemperatureSensor* everywhere to just TempSensor*?

Done in 85f6a7c

ivmarkov · 2024-09-08T11:53:00Z

src/temp_sensor.rs

+        Ok(val)
+    }
+
+    pub fn get_fahrenheit(&self) -> Result<f32, EspError> {


Do we really need this?

Personally I would not, but I just did the rebase so I'd wait for the original author. @GamerBene19 do you have time to complete it?

I don't follow. :)
Do you plan to finish this PR, or you just rebased and plan to leave it as-is, leaving it in open state? If the latter, it would likely just rot as the original PR did, as nobody else might have the time to complete it for you.

Do we really need this?

(Also replying for kelvin below)
Since I "passed through" the naming of the implementation from espressif (which contains "_celcius") I thought it wouldn't hurt to have these functions available to users of the library for convenience.
If that's not a thing you do in this project we can leave it out. I don't have any strong preference in either direction.

do you have time to complete it?

See #337 (comment): TL;DR Yes, but I likely need some assistance.

Sure and I'll assist. As I posted in the other comment - it does not need a huge amount of work anyway.

Peripheral implemented in cd015dc.

ivmarkov · 2024-09-08T11:53:22Z

src/temp_sensor.rs

+        Ok((celsius * 1.8) + 32.0)
+    }
+
+    pub fn get_kelvin(&self) -> Result<f32, EspError> {


And especially this? This is utility code that probably does not belong in the driver. User can do the math themselves.

lu-zero · 2024-09-08T13:21:19Z

Thanks a lot!

However, the CI fails on a number of chips, you need to look into what is wrong there. I also have a few comments specific to the PR, will follow up shortly with those.

It looks like esp32c2/esp32c6/esp32h2 support this feature.

ivmarkov · 2024-09-08T13:50:57Z

Thanks a lot!
However, the CI fails on a number of chips, you need to look into what is wrong there. I also have a few comments specific to the PR, will follow up shortly with those.

It looks like esp32c2/esp32c6/esp32h2 support this feature.

Fair enough. Looks like the sensor is supported on everything else, but the original esp32.
In order not to have this repeating #[cfg(not(esp32))] everywhere, you can just put it above the temp_sensor module, in lib.rs.

You'll still have to repeat it unfortunately inside peripherals.rs once you implement a temp sensor peripheral, but at least the repetition inside the temp_sensor module would be avoided.

ivmarkov · 2024-09-08T13:52:24Z

Thanks a lot!
However, the CI fails on a number of chips, you need to look into what is wrong there. I also have a few comments specific to the PR, will follow up shortly with those.

It looks like esp32c2/esp32c6/esp32h2 support this feature.

Fair enough. Looks like the sensor is supported on everything else, but the original esp32. In order not to have this repeating #[cfg(not(esp32))] everywhere, you can just put it above the temp_sensor module, in lib.rs.

You'll still have to repeat it unfortunately inside peripherals.rs once you implement a temp sensor peripheral, but at least the repetition inside the temp_sensor module would be avoided.

Also, regardless whether the temp sensor is supported or not on some MCUs, the CI should not fail. In fact, the #[cfg()] we are talking about is precisely there to make sure that the temp sensor code is "compiled out" on MCUs that don't have this peripheral, so no matter what, the CI should succeed.

lu-zero · 2024-09-09T14:16:44Z

Let's move back to #337

GamerBene19 added 5 commits September 8, 2024 12:55

Add first version of temp sensor driver

249a5a3

Code is very rudimentary and still suspect to change. Notably, error handling is not implemented yet.

Change cfg attribute to not any

e3d432b

Rename TemperatureSensor to TemperatureSensorDriver

b43c74d

Switch to core::ptr

363798c

Add error handling

Loading
Loading status checks…

7274337

ivmarkov requested changes Sep 8, 2024

View reviewed changes

lu-zero force-pushed the internal-temperature-sensor branch from d1953e2 to 83a1e66 Compare September 8, 2024 14:32

lu-zero added 3 commits September 8, 2024 21:20

Build the internal temperature sensor only on supported targets

2826429

Fix the derives in TemperatureSensorConfig, TemperatureSensorClockSource

a6a67bb

Implement From<TemperatureSensorConfig> for temperature_sensor_config_t

Loading
Loading status checks…

189ff77

lu-zero force-pushed the internal-temperature-sensor branch from 83a1e66 to 189ff77 Compare September 8, 2024 19:20

GamerBene19 mentioned this pull request Sep 9, 2024

Add support for ESP internal temperature sensor #337

Merged

lu-zero closed this Sep 9, 2024

lu-zero deleted the internal-temperature-sensor branch September 9, 2024 14:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Internal temperature sensor #478

Internal temperature sensor #478

lu-zero commented Sep 8, 2024

ivmarkov commented Sep 8, 2024

ivmarkov left a comment

ivmarkov Sep 8, 2024

GamerBene19 Sep 9, 2024

ivmarkov Sep 8, 2024

ivmarkov Sep 8, 2024

ivmarkov Sep 8, 2024

GamerBene19 Sep 9, 2024

ivmarkov Sep 8, 2024

lu-zero Sep 8, 2024 •

edited

Loading

ivmarkov Sep 8, 2024

ivmarkov Sep 8, 2024 •

edited

Loading

GamerBene19 Sep 9, 2024

ivmarkov Sep 8, 2024

lu-zero Sep 8, 2024

ivmarkov Sep 8, 2024

GamerBene19 Sep 8, 2024 •

edited

Loading

ivmarkov Sep 8, 2024

GamerBene19 Sep 9, 2024

ivmarkov Sep 8, 2024

lu-zero commented Sep 8, 2024

ivmarkov commented Sep 8, 2024

ivmarkov commented Sep 8, 2024

lu-zero commented Sep 9, 2024

Internal temperature sensor #478

Internal temperature sensor #478

Conversation

lu-zero commented Sep 8, 2024

ivmarkov commented Sep 8, 2024

ivmarkov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lu-zero Sep 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivmarkov Sep 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GamerBene19 Sep 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lu-zero commented Sep 8, 2024

ivmarkov commented Sep 8, 2024

ivmarkov commented Sep 8, 2024

lu-zero commented Sep 9, 2024

lu-zero Sep 8, 2024 •

edited

Loading

ivmarkov Sep 8, 2024 •

edited

Loading

GamerBene19 Sep 8, 2024 •

edited

Loading