PCNT implementation for v4 esp-idf api (will work for on v5 with v4 api)

[liebman]

V4 api only (v5 will be a later PR)

[Vollbrecht]

some under qualified comments 😄

[ivmarkov]

Will try to review over the weekend. In the process of releasing esp-idf-hal 0.40 in the meantime.

[liebman]

Take your time. I'm mostly AFK until the 27th.

[ivmarkov]

I already reviewed 4.4 driver. Next one coming over the weekend.

[ivmarkov]

Actually, if you can address the feedback for the V4 driver first. We can review the V5 one after that.

[liebman]

@ivmarkov should I remove the pcnt4 feature option?

[ivmarkov]

@ivmarkov should I remove the pcnt4 feature option?

Yes please. We might re-introduce it once we have the V5 driver as well.

[ivmarkov]

Why do we need to provide the config after the driver has been created? Is this having any real use case?

[ivmarkov]

@ivmarkov should I remove the pcnt4 feature option?

Yes please. We might re-introduce it once we have the V5 driver as well.

Wait. You still have the V5 version lurking around in pulse_cnt.rs. I need to review it again. Or you need to remove it temporarily / make it dead code. And btw - can you rename the module to something a bit more meaningful, as in pcntv5.rs? Who can tell ATM what is the difference between pcnt.rs and pulse_cnt.rs?

[ivmarkov]

@liebman For the V5 driver:

• Why do we still have the unsafe channel_config method there?
• We should name it PcntDriver, just like we named in in the V4 case
• The fact that it does not take a pcnt peripheral - as the V4 version does - is IMO a disadvantage, not an advantage. But let me think about this a bit. In any case, it should at least take the pins in its new method, just like the V4 driver

[liebman]

@ivmarkov should I remove the pcnt4 feature option?

Yes please. We might re-introduce it once we have the V5 driver as well.

Wait. You still have the V5 version lurking around in pulse_cnt.rs. I need to review it again. Or you need to remove it temporarily / make it dead code. And btw - can you rename the module to something a bit more meaningful, as in pcntv5.rs? Who can tell ATM what is the difference between pcnt.rs and pulse_cnt.rs?

I'll remove it and make a new PR after we're done with v4 - without a change in esp-idf-sys it won't be available.

[liebman]

@liebman For the V5 driver:

• Why do we still have the unsafe channel_config method there?
• We should name it PcntDriver, just like we named in in the V4 case
• The fact that it does not take a pcnt peripheral - as the V4 version does - is IMO a disadvantage, not an advantage. But let me think about this a bit. In any case, it should at least take the pins in its new method, just like the V4 driver

yea - removing v5 for a later PR. I also do not like the new interface as it's misleading in a few ways, in particular the channel allocation (will there be more than 2 in future hardware?) and watch points (current hardware has only one that the user can set, the others (limit and zero) can't be changed (future hardware?).

[liebman]

@ivmarkov we can attempt the workflow again. I've run clippy on the following targets:

• xtensa-esp32-espidf
• xtensa-esp32s2-espidf
• xtensa-esp32s3-espidf
• riscv32imc-esp-espidf

and rerun fmt.

[liebman]

@ivmarkov ready for another workflow attempt

[liebman]

@ivmarkov the compile error was coming from esp-idf-svc (something with esp-now on idf v5) so I removed the use of the log in the pcnt example and esp-idf-svc from dev-dependencies.

[ivmarkov]

@liebman Can you fix the remaining errors (perhaps replace error! with panic! in the example) so I can finally merge this?

[liebman]

I tried

#[cfg(not(all(not(feature = "riscv-ulp-hal"), any(esp32, esp32s2, esp32s3))))]
fn main() {
    panic!("pcnt peripheral not supported on this device!");
}

but that gives a link error (bug?)

  = note: [ldproxy] Running ldproxy
          Error: Linker /Users/chris.l/.espressif/tools/riscv32-esp-elf/esp-2021r2-patch5-8.4.0/riscv32-esp-elf/bin/riscv32-esp-elf-gcc failed: exit status: 1
          STDERR OUTPUT:
          /Users/chris.l/.espressif/tools/riscv32-esp-elf/esp-2021r2-patch5-8.4.0/riscv32-esp-elf/bin/../lib/gcc/riscv32-esp-elf/8.4.0/../../../../riscv32-esp-elf/bin/ld: esp-idf/freertos/libfreertos.a(port_common.c.obj): in function `main_task':
          /Users/chris.l/.espressif/esp-idf/v4.4.3/components/freertos/port/port_common.c:129: undefined reference to `app_main'
          collect2: error: ld returned 1 exit status
          
          
          Stack backtrace:
             0: backtrace::backtrace::trace
             1: anyhow::backtrace::capture::Backtrace::capture
             2: anyhow::error::<impl anyhow::Error>::msg
             3: ldproxy::main
             4: std::sys_common::backtrace::__rust_begin_short_backtrace
             5: std::rt::lang_start::{{closure}}
             6: std::rt::lang_start_internal
             7: _main
          
  = note: some `extern` functions couldn't be found; some native libraries may need to be installed or have their path specified
  = note: use the `-l` flag to specify native libraries to link
  = note: use the `cargo:rustc-link-lib` directive to specify the native libraries to link with Cargo (see https://doc.rust-lang.org/cargo/reference/build-scripts.html#cargorustc-link-libkindname)

I'm going with this I think (compiles fine)

#[cfg(not(all(not(feature = "riscv-ulp-hal"), any(esp32, esp32s2, esp32s3))))]
fn main() {
    use esp_idf_hal::delay::FreeRtos;
    println!("pcnt peripheral not supported on this device!");
    loop {
        FreeRtos::delay_ms(100u32);
    }
}