Add support for ESP internal temperature sensor #337
Conversation
|
@Vollbrecht FYI |
| @@ -0,0 +1,38 @@ | |||
| #[cfg(esp32c3)] | |||
| use esp_idf_sys::{ | |||
There was a problem hiding this comment.
when you import a crate its not automatically scoped to all the child modules inside the file
src/temp_sensor.rs
Outdated
| /// Temperature Sensor configuration | ||
| #[cfg(esp32c3)] | ||
| pub mod config { | ||
| #[derive(Copy, Clone)] |
There was a problem hiding this comment.
you need to import here with either use super::*; from the parent or the better way directly move the above use esp-idf-sys inside the module so you don't need extra super
There was a problem hiding this comment.
I see. That was the missing information. Thanks!
Code is very rudimentary and still suspect to change. Notably, error handling is not implemented yet.
src/temp_sensor.rs
Outdated
| // -- TemperatureSensorClockSource -- | ||
|
|
||
| #[derive(Copy, Clone)] | ||
| #[cfg(any(esp32s2, esp32s3, esp32c2, esp32c3, esp32c6, esp32h2))] |
There was a problem hiding this comment.
to make the #[cfg(any(esp32s2, esp32s3, esp32c2, esp32c3, esp32c6, esp32h2))] not so verbose i think its currently best to list the "not any" - so the original esp32 and so forth, because this list will potentially only grow and the chips that not support it will more likely stay the same
src/temp_sensor.rs
Outdated
| // -- TemperatureSensor -- | ||
|
|
||
| #[cfg(any(esp32s2, esp32s3, esp32c2, esp32c3, esp32c6, esp32h2))] | ||
| pub struct TemperatureSensor { |
There was a problem hiding this comment.
to have an consistent naming schema across the modules, the main struct that impl all the methonds and do the stuff has a Driver in the name - PinDriver, SpiDriver , TemperatureSensorDriver, its more verbose but consistend
src/temp_sensor.rs
Outdated
| #[cfg(any(esp32s2, esp32s3, esp32c2, esp32c3, esp32c6, esp32h2))] | ||
| impl TemperatureSensor { | ||
| pub fn new(config: TemperatureSensorConfig) -> Self { | ||
| let mut sensor = std::ptr::null_mut(); |
There was a problem hiding this comment.
std::ptr::null_mut has a counterpart -> core::ptr::null_mut, we do that for the reasons i stated in the issue
Cargo.toml
Outdated
| @@ -33,6 +33,9 @@ panic_handler = ["esp-idf-sys/panic_handler"] | |||
| binstart = ["esp-idf-sys/binstart"] | |||
| libstart = ["esp-idf-sys/libstart"] | |||
|
|
|||
| [patch.crates-io] | |||
| esp-idf-sys = { path="../esp-idf-sys" } | |||
There was a problem hiding this comment.
you can now directly reference your branch of this on github from your PR on esp-idf-sys with git = and branch= , this will give the ci the chance to run correctly
|
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. |
src/temp_sensor.rs
Outdated
| pub fn new(config: TemperatureSensorConfig) -> Self { | ||
| let mut sensor = core::ptr::null_mut(); | ||
| unsafe { | ||
| temperature_sensor_install(&config.into(), &mut sensor); |
There was a problem hiding this comment.
don,'t eat the errors you get back from the underlying C api calls, every Error should be propagated. You can use a `Result<(),EspError> on the return's of all the method calls where its needed
There was a problem hiding this comment.
for error converting we have the esp! macro
There was a problem hiding this comment.
Done in 2b0694d. The only thing I'm still uncertain about is how to handle errors that occur in drop, since drop has to return (). Perhaps you have a good idea.
There was a problem hiding this comment.
yeah you don't have many chances in a drop. We either eat the error, unwrap it or panic if we get one. The underlying driver probably still sends something out over Error println. Out of my head in most cases we eat the error in the othere drivers here.
src/temp_sensor.rs
Outdated
| impl Drop for TemperatureSensorDriver { | ||
| fn drop(&mut self) { | ||
| unsafe { | ||
| self.disable(); |
There was a problem hiding this comment.
self.disable is not an unsafe call - try to scope unsafe to the minimal part where it is originated from
|
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 |
|
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. |
Would that then be
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. |
What the title says. Currently very much WIP. PR created to help give feedback.
Closes #333