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

Adds embedded_hal::blocking::i2c::Transactional #229

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

jmarcelomb
Copy link

Hello,

I was trying to use an ESP32-C3 with the module crate pn532.
I didn't test yet, but I think it is the implementation.

Thank you

@Vollbrecht
Copy link
Collaborator

Vollbrecht commented Apr 1, 2023

Ty for the PR. Seems good so far, if you have the chance to test it with your sensor that would be great.
Tracking Issue: rust-lang/cargo#197

@Vollbrecht
Copy link
Collaborator

@jmmb13 can you fix the clippy warnings

@ivmarkov
Copy link
Collaborator

@Vollbrecht Is this still relevant?

@Vollbrecht
Copy link
Collaborator

@Vollbrecht Is this still relevant?

yes, if the original author will not fix the CI i can impl it if you want.

@ivmarkov
Copy link
Collaborator

Let's do it, but no rush - it will be merged after this weekend's release anyway.

@jmarcelomb
Copy link
Author

I'm sorry for not answering, I was off. I added ";" that I think it is required to don't exit in the first operation.

@Vollbrecht
Copy link
Collaborator

Vollbrecht commented May 13, 2023

I'm sorry for not answering, I was off. I added ";" that I think it is required to don't exit in the first operation.

dont forget to run cargo fmt / cargo clippy to make sure ci dont fail

@jmarcelomb
Copy link
Author

jmarcelomb commented May 13, 2023

I'm sorry for not answering, I was off. I added ";" that I think it is required to don't exit in the first operation.

dont forget to run cargo fmt / cargo clippy to make sure ci dont fail

How do I run clippy?

$ cargo clippy
   Compiling esp-idf-sys v0.33.0
error: failed to run custom build command for `esp-idf-sys v0.33.0`

Caused by:
  process didn't exit successfully: `/home/jmmb/Code/esp-idf-hal/target/debug/build/esp-idf-sys-dea13f6084fc101b/build-script-build` (exit status: 1)
  --- stdout
  cargo:rerun-if-env-changed=ESP_IDF_TOOLS_INSTALL_DIR
  cargo:rerun-if-env-changed=ESP_IDF_SDKCONFIG
  cargo:rerun-if-env-changed=ESP_IDF_SDKCONFIG_DEFAULTS
  cargo:rerun-if-env-changed=MCU
  cargo:rerun-if-env-changed=ESP_IDF_SYS_ROOT_CRATE
  cargo:rerun-if-env-changed=ESP_IDF_VERSION
  cargo:rerun-if-env-changed=ESP_IDF_REPOSITORY
  cargo:rerun-if-env-changed=ESP_IDF_CMAKE_GENERATOR
  cargo:rerun-if-env-changed=IDF_PATH
  cargo:rerun-if-env-changed=EXTRA-COMPONENTS
  cargo:rerun-if-env-changed=ESP_IDF_COMPONENTS

  --- stderr
  Build configuration: BuildConfig {
      esp_idf_tools_install_dir: None,
      esp_idf_sdkconfig: None,
      esp_idf_sdkconfig_defaults: Some(
          [
              ".github/configs/sdkconfig.defaults",
          ],
      ),
      mcu: None,
      native: NativeConfig {
          esp_idf_version: None,
          esp_idf_repository: None,
          esp_idf_cmake_generator: None,
          idf_path: None,
          extra_components: [],
          esp_idf_components: None,
      },
      esp_idf_sys_root_crate: None,
  }
  Error: Unsupported target 'x86_64-unknown-linux-gnu'

@Vollbrecht
Copy link
Collaborator

Vollbrecht commented May 13, 2023

you can run the build commands like we do it in the CI : for example for an riscv target run cargo clippy --no-deps --target riscv32imc-esp-espidf -Zbuild-std=std,panic_abort -Zbuild-std-features=panic_immediate_abort -- -Dwarnings you can check all the build commands here : check Build | Fmt Check & Build | Clippy https://github.com/esp-rs/esp-idf-hal/actions/runs/4968078029/jobs/8890463099. the jobs on the left side are for all the different targets

@jmarcelomb
Copy link
Author

I ran it doesn't complain.
Rust 1.69.0 and esp-idf v4.4.0

@Vollbrecht
Copy link
Collaborator

@ivmarkov @MabezDev can you start the workflow

@jmarcelomb
Copy link
Author

jmarcelomb commented May 21, 2023

I'm sorry, I was executing the cargo clippy command on the master branch, my bad. Now it is fixed :)

@ivmarkov @MabezDev, now you can start the workflow

@ivmarkov
Copy link
Collaborator

ivmarkov commented Jun 11, 2023

@Vollbrecht The PR passes fmt and clippy. Are there any other outstanding work, or shall I merge?

@Vollbrecht
Copy link
Collaborator

Vollbrecht commented Jun 12, 2023

@Vollbrecht The PR passes fmt and clippy. Are there any other outstanding work, or shall I merge?

@ivmarkov i think we are not ready yet.
@jmmb13 i skimmed the documentation again for embedded-hal transaction. the way you propose to implement it is i think not compatible to the way it is described here. Calling the driver methods read/write in itself is a closed transaction with a separate start / stop bit send. Transactional needs to be implemented in a way so that the stop condition is only written once at the end. But each read / write will send it so you cant just use this methods.

@jmmb13 do you also know of a library that uses the e-hal exec method, so we can test against something when we implement it? your initial msg mentions pn532

@teamplayer3
Copy link
Contributor

I can impl this trait in #397 for the legacy version of the driver.

@Vollbrecht
Copy link
Collaborator

I can impl this trait in #397 for the legacy version of the driver.

feel free to do so if you want!

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

Successfully merging this pull request may close these issues.

4 participants