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

Missing support for internal temperature sensor #333

Open
GamerBene19 opened this issue Nov 8, 2023 · 21 comments · May be fixed by #337
Open

Missing support for internal temperature sensor #333

GamerBene19 opened this issue Nov 8, 2023 · 21 comments · May be fixed by #337

Comments

@GamerBene19
Copy link

Various ESP32 models have an integrated temperature sensor that is supported by esp-idf.
It would be nice to see support for that sensor built into this crate.

I would like to try implement it - what steps need to be taken for such an implementation? Is there documentation for that process? Is this crate even the right place or does it need to be implemented in esp-idf-sys instead?

Links

@Vollbrecht
Copy link
Collaborator

This repo is the right place to implement a safe wrapper around the raw C bindings.

To roughly outline what would need to happen.

  • To automatically get the C bindgen generated the header temperatur_sensor.h needs to be added in esp-idf-sys here . Make sure when adding it that it is only included for the correct esp variants + checkout if the header-file itself e.g the driver is the same for esp-idf 4.4.6 and 5.1.1 /( our current supported versions) and depending on add it.
  • Write a safe wrapper driver in this repo that depends on the bindgens you get now from esp-idf-sys
  • profit

To dev it you just can generate a new project with the template make a fork of both this and the -sys repo and use

[patch.crates-io]
esp-idf-sys = {path=<path to your local repo clone>} #or use a remote git repo  
esp-idf-hal =  {path=<path to your local repo clone>}

PR's are welcome if you have any question check out our matrix channel

@GamerBene19
Copy link
Author

Thanks for the quick reply, @Vollbrecht. I've tried to follow the steps you provided, but I am somewhat stuck. What I did so far is:

  1. Clone esp-idf-hal, esp-idf-sys and esp-idf (last one for good measure)
    Note: I realize that I still have to fork, I wanted to keep it simple for the setup and will simply change origin url afterwards.
  2. Create new project temp-sensor-driver from esp-idf-template with cargo generate
  3. Add
    [patch.crates-io]
    esp-idf-sys = { path="../esp-idf-sys" }
    esp-idf-hal = { path="../esp-idf-hal" }
    
    into Cargo.toml (of temp-sensor-driver)
  4. cargo build in temp-sensor-driver which succeedes and uses my local clones.
  5. Add
    #if defined(CONFIG_IDF_TARGET_ESP32)
    #include "driver/temperatur_sensor.h"
    #endif
    
    into eps-idf-sys/src/include/esp-idf/bindings.h (I realize that I might have to tweak that if condition; I kept it simple for now)

Now I am somewhat stuck, because I'm not sure if (and how) I need to create/update the bindings from esp-idf-sys.

  • I tried cargo build in esp-idf-sys and esp-idf-hal, but that fails with Error: Unsupported target 'x86_64-unknown-linux-gnu'.
  • Building thermal-sensor-driver works, so perhaps bindings are generated automatically? If so I don't know how to use them. I assume I have to import them from esp-idf-sys (currently using use esp_idf_sys::*;), but I don't know if/how to access the C-Functions or what they are called in the bindings.
  • I've also tried to look at other "drivers" (sorry if that is not the correct term) already implemented in esp-idf-hal, but I wasn't able to find any that seem to use bindings. Are there any examples I can use as a "guide"?

Perhaps you can explain the process in a little more detail and answer my questions and/or confirm if my assumptions about the process are correct.

Thanks again!

@Vollbrecht
Copy link
Collaborator

if you are using cargo generate for your project, the default behavior is that it clones and installes esp-idf all by itself in a project local dir called .embuild. so thats already covert.

esp-idf-sys generates all the bindings and export them directly. They are several ways to get the complete output, you could

  • directly open the generated bindgens.rs file somewhere located in target/xtensa../build/esp-idf-sys.../out/bindgens.rs,
  • with rust-analyzer just "go to definition" on any esp-idf-sys function
  • just simply run cargo doc --open and click on esp-idf-sys and you get all the function bindings, you can use the search option.

With this you can verify know that the functions defined in the esp-idf function header are now available.

@GamerBene19
Copy link
Author

GamerBene19 commented Nov 9, 2023

esp-idf-sys generates all the bindings and export them directly.

When are they generated? What triggers their generation? (As mentioned above I can't run cargo build in esp-idf-sys)

directly open the generated bindgens.rs file somewhere located in target/xtensa../build/esp-idf-sys.../out/bindgens.rs,

Does not exist in my esp-idf-sys directory. Edit: Nvmd my bad, is present in my thermal-sensor-driver project and reflects what is generated from the doc command (which makes sense)

with rust-analyzer just "go to definition" on any esp-idf-sys function

How can I find such functions? I tried to do what you mentioned (in VSCode) on various functions in other files but none of them "resolve" to esp-idf-sys.

just simply run cargo doc --open and click on esp-idf-sys and you get all the function bindings, you can use the search option.

When I open the documentation, the only thing I see in regards to the temperature sensor are constants like soc_periph_temperature_sensor_clk_src_t... which are also present when I remove the included header from esp-idf-sys.

From what I can see I suspect that the bindings are not generated. How can I generate them (manually)?

@Vollbrecht
Copy link
Collaborator

Vollbrecht commented Nov 9, 2023

the buildstep of esp-idf-sys generates them, (that's why you find them in the build-artifacts as i described it) inside your normal binary project that you created and if you use the crates-io patch it will build esp-idf-sys alongside with your local changes.
you run cargo doc --open inside your project dir not the esp-idf-sys dir, ( there is a way to directly build esp-idf-sys or the other crates for testing but this should be simpler) as you don't need to pass 300 parameters, if you are curios you can look into our CI.
What i described all assumes that you work from the generated cargo project

@Vollbrecht
Copy link
Collaborator

Vollbrecht commented Nov 9, 2023

To make it clear if your include worked you would for example see the struct temperature_sensor_config_t defined in temperatur_sensor.h as esp_idf_sys::temperature_sensor_config_t . In this case you just can click on it in vscode and follow it into its definition

@GamerBene19
Copy link
Author

Alright, thanks for the clarification. Sounds like my include was wrong then. I'll try again and report back. Sorry for beeing such a noob and thanks for the patience.

@GamerBene19
Copy link
Author

Quick update before I go to bed: I got a temperature out of the ESP successfully. I'll try to create a PR in the next few days.

@GamerBene19
Copy link
Author

GamerBene19 commented Nov 12, 2023

As mentioned previously I got a temperature reading. I used this (very basic PoC) code to accomplish that:

use std::{thread::sleep, time::Duration};

use esp_idf_sys::{
    soc_periph_temperature_sensor_clk_src_t_TEMPERATURE_SENSOR_CLK_SRC_DEFAULT,
    temperature_sensor_config_t, temperature_sensor_enable, temperature_sensor_get_celsius,
    temperature_sensor_handle_t, temperature_sensor_install,
};

fn main() {
    // It is necessary to call this function once. Otherwise some patches to the runtime
    // implemented by esp-idf-sys might not link properly. See https://github.com/esp-rs/esp-idf-template/issues/71
    esp_idf_svc::sys::link_patches();

    // Bind the log crate to the ESP Logging facilities
    esp_idf_svc::log::EspLogger::initialize_default();
    let mut temp_sensor: temperature_sensor_handle_t = std::ptr::null_mut();
    let temp_sensor_config = temperature_sensor_config_t {
        range_min: 10,
        range_max: 50,
        clk_src: soc_periph_temperature_sensor_clk_src_t_TEMPERATURE_SENSOR_CLK_SRC_DEFAULT,
    };
    unsafe {
        temperature_sensor_install(&temp_sensor_config, &mut temp_sensor);
        temperature_sensor_enable(temp_sensor);
    }
    let mut tsens_value: f32 = 0.0;
    loop {
        unsafe {
            temperature_sensor_get_celsius(temp_sensor, &mut tsens_value);
        }
        log::info!("{}", tsens_value);
        sleep(Duration::from_millis(1000));
    }
}

I now wanted to "move" the code from my main method in my temp-sensor-driver project into esp-idf-hal, but I am facing some more problems. Currently my code (in esp-idf-hal) looks like this:

use esp_idf_sys::soc_periph_temperature_sensor_clk_src_t;
pub type Config = config::Config;

/// Temperature Sensor configuration
pub mod config {
    #[derive(Copy, Clone)]
    pub struct Config {
        // TODO: check int size
        pub range_min: i32,
        pub range_max: i32,
        pub clk_src: soc_periph_temperature_sensor_clk_src_t,
    }
    impl Default for Config {
        fn default() -> Self {
            Config {
                range_min: -10,
                range_max: 80,
                clk_src: soc_periph_temperature_sensor_clk_src_t_TEMPERATURE_SENSOR_CLK_SRC_DEFAULT,
            }
        }
    }

    impl Config {
        pub fn new() -> Self {
            Default::default()
        }
    }
}

and I'm trying to use the following in my main method:

use esp_idf_hal::temperature_sensor::Config;
...
fn main() {
    ...
    let config = Config::new();
}

The problem is that compilation fails because the import from esp_idf_sys cannot be found. I've added the path override you mentioned above to the esp-idf-hal project too and also tried using use crate sys::*;, but that didn't work.

I'm now wondering how I can use the types from the bindings in esp-idf-hal and if I that is even the right approach. I don't know if what I'm writing for esp-idf-hal should "merely" be a wrapper with - perhaps - some nicer names for the generated bindings (which is what I currently think), or if I need to do "more".

In addition I'm also wondering

  • How would I handle the case where there are different version of the driver (you mention to check if it has changed compared to previous versions)?
  • How to handle different boards? I realize, that I need to add the necessary if defined... to bindings.h, but while investigating I found out that some models seem to use different clock sources for the temerature sensor (see upstream here and here)

But I think those questions can be answered later, when a first draft for a PR exists.

@Vollbrecht
Copy link
Collaborator

Regarding the imports:
Since the latest minor releases we changed how a binary can interact with esp-idf-sys/hal/svc. Previously the bin crate depended on all 3 crates. With our current minor release we re-export the sys & hal crates. If a user uses only esp-idf-hal. sys is re-exported as esp-idf-hal::sys:: , and for esp-idf-svc hal and sys are re-exported as esp-idf-svc::hal and esp-idf-svc::sys,

  • So first make sure that you try to do this the same way in your testing.
  • Second make sure that you not unintentionally got multiple versions of esp-idf-sys pulled into your binary crate. You can check this with cargo tree -f "{p} {f}" | grep esp-idf-sys

As you see - often - we implement our own types for our public API, and not re-export the original C types. This is then accompanied with impl From to convert our types into the C types and vice versa if necessary.

You mention a good point with regards to "wrapper api" , while we essentially wrapping many of the underlying C drivers, our main goal is to be a good rust api and not just a thin safe wrapper. So we avoid to leak to much of the underlying C api if the user only directly uses hal/svc. That way we can support multiple esp-idf versions ( currently v4.4.6 and 5.1.1) with one api. But if something only exist for example in esp-idf 5 and onwards we can hide a complete api behind this if its necessary.

A second goal is also to not use to much generics, but this should not be a problem here.

A third goal is - as embedded lib developers - to limit our self by trying to avoid allocation heavy stuff and try to make parts work in no_std environments ( even if the complete project could not be used in a no_std env)

At compile time you got the information handled downward with compiler flags, so you can determine if the current api is compiled against esp-idf 5.1 or 4.6. Also you got information regarding what type of esp is used. Check out some of the other drivers and you find examples quickly

@GamerBene19
Copy link
Author

GamerBene19 commented Nov 12, 2023

Regarding the imports:
Since the latest minor releases we changed how a binary can interact with esp-idf-sys/hal/svc. Previously the bin crate depended on all 3 crates. With our current minor release we re-export the sys & hal crates. If a user uses only esp-idf-hal. sys is re-exported as esp-idf-hal::sys:: , and for esp-idf-svc hal and sys are re-exported as esp-idf-svc::hal and esp-idf-svc::sys

So If I understand you correctly I should use esp-idf-svc::hal/sys in temp-sensor-driver, and esp-idf-sys in esp-idf-hal. I am doing just that with

use esp_idf_sys::soc_periph_temperature_sensor_clk_src_t;

but the import does not resolve.
Just to avoid confusion: the soc_periph_temperature_sensor_clk_src_t type is part of the bindings generated by esp-idf-sys when building temp-sensor-driver and I'm trying to use it in esp-idf-hal.

As far as I can tell the same approach works for the other "drivers" (e.g. for timer_alarm_t_TIMER_ALARM_DIS which just uses use esp_idf_sys::*;)

Any ideas what I could be doing wrong and how I could debug it?

Thanks also for the clarifications regarding my additional questions - I'll keep them in mind when writing my code.

Thanks again for your help!

@Vollbrecht
Copy link
Collaborator

Vollbrecht commented Nov 12, 2023

Still make sure you don't pull the multiple versions of esp-idf-sys. To make your life a little simpler, here is how you can compile esp-idf-hal directly

 ESP_IDF_VERSION="v5.1.1" MCU="esp32c3" cargo build --target riscv32imc-esp-espidf --features=nightly,criticalsection,embassy-sync,embedded-hal-async -Zbuild-std=std,panic_abort -Zbuild-std-features=panic_immediate_abort

you just adjust the idf version, mcu, flags and target to your testcase. For your local testing you can add a .cargo/config.toml, so you dont need to to run this long command every-time inside esp-idf-hal, similar to your bin crate but we don't want to have this in a potential PR from you. With this you also could run cargo tree inside the esp-idf-hal dir.

Edit: you also might need to set the correct rust toolchain, so maybe need to add +esp if its not the default one

@GamerBene19
Copy link
Author

GamerBene19 commented Nov 12, 2023

Still make sure you don't pull the multiple versions of esp-idf-sys.

Sorry, forget to mention. Output of the cargo tree command was/is

│   ├── esp-idf-hal v0.42.5 (/path/to/parent/esp-idf-hal) alloc,atomic-waker,binstart,critical-section,embassy-sync,esp-idf-sys,native,std
│   │   ├── esp-idf-sys v0.33.7 (/path/to/parent/esp-idf-sys) binstart,native,std

as far as I can tell this is as expected. esp-idf-hal depends on esp-idf-sys and since there is only one line with esp-idf-sys at the front the package is only pulled once - correct me if I am wrong.

I also tried your command (I had to use +nightly for -Z arguments, critical-section (instead of criticalsection) and add RUSTFLAGS="--cfg espidf_time64" since I got type issues otherwise), but it results in a same/similar error

cannot find type `soc_periph_temperature_sensor_clk_src_t` in this scope

Interestingly I also get

unused imports: `soc_periph_temperature_sensor_clk_src_t_TEMPERATURE_SENSOR_CLK_SRC_DEFAULT`, `soc_periph_temperature_sensor_clk_src_t`

The types do exist in the bindings at esp-idf-hal/target/riscv32imc-esp-espidf/debug/build/esp-idf-sys-.../out/bindings.rs tough.

Any ideas?

@Vollbrecht
Copy link
Collaborator

Is this an rust-analyzer error or this error comes up when you try to compile it?

@Vollbrecht
Copy link
Collaborator

Vollbrecht commented Nov 12, 2023

when working with local repos and git repos also always try to use cargo update in between and sometimes making a clean run also can help :D Are all changes commited inside esp-idf-sys you made?

@GamerBene19
Copy link
Author

Is this an rust-analyzer error or this error comes up when you try to compile it?

Errors come up when trying to build with cargo build in temp-sensor-driver and with your command above in esp-idf-hal

when working with local repos and git repos also always try to use cargo update in between and sometimes making a clean run also can help

I did execute cargo clean before (almost) every build. And I just did cargo clean && cargo update && cargo build which still runs into the same issue (both in temp-sensor-driver and esp-idf-hal).

Are all changes commited inside esp-idf-sys you made?

No, they were not. I committed them, but the issue persists. I also suspect committing does not make a difference since the bindings were correctly generated.

@Vollbrecht
Copy link
Collaborator

Vollbrecht commented Nov 12, 2023

you are currently developing it for the esp32 right? so with my command you are building it for the esp32c3, and this mcu don't have this driver generated inside esp-idf-sys because it does not have this driver / internal hardware ! So make sure it works for the esp32 and generate fine there, and also you can build it for othere targets. (You need to gate the imports to the targets where they exist !! ) So esp32 is xtensa-esp32-espidfas a target

@GamerBene19
Copy link
Author

you are currently developing it for the esp32 right?

No, ESP32C3 (atm; long term goal is for it to work on all esp32's that have the internal temp-sensor). Sorry, should've probably clarified earlier.

You need to gate the imports to the targets where they exist !!

I don't quite understand what I would have to do here - can you give an example?

@Vollbrecht
Copy link
Collaborator

Vollbrecht commented Nov 12, 2023

Yeah sorry to make it clear. If you are generating the bindings only for a special target ->

#if defined(CONFIG_IDF_TARGET_ESP32)
#include "driver/temperatur_sensor.h"
#endif

you than only can use any statements regarding this api only behind similar gatex expression, So if you are using use esp-idf-sys::foo_bla it needs to be gated with #(cfg(esp32)) inside esp-idf-hal, at every place that reference this special import. The gating all depends on how you define the defined import in esp-idf-sys. It needs to reflect that

@Vollbrecht
Copy link
Collaborator

Even if its not currently able to build you can create a draft PR, so i can see what you are doing more clearly. 😸

@GamerBene19
Copy link
Author

Even if its not currently able to build you can create a draft PR, so i can see what you are doing more clearly. 😸

Sure, I'll quickly do that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Todo
Development

Successfully merging a pull request may close this issue.

2 participants