Updates for new I2C sensor SHT30

enelson1001 commented 3 years ago

Tested all 6 repeatability modes
Did minor cleanup DHT12.cpp
Just noticed CMakeLists.txt changed selected_test_project to be i2c_sht30_test so will immediately update and change back to starter_example. I am not sure how this happened?
The only other name I could think of for read_block was read_bytes.

enelson1001 commented 3 years ago

I2C Stretching

I did more research on i2c and clock stretching and i2c-bus.org has very good explanation. https://www.i2c-bus.org/clock-stretching/ So with clock stretching enabled on the SHT30 it pulls down the SCL line after ACKing to i2c_read request. The SHT30 will hold the SCL line low for a measurement duration (see table 4 and table 9). The different amounts of time depending upon which repeatability is chosen, so for Low Repeatability the worst case measurement time is 4.5ms, Medium Repeatability it is 6.5ms and High Repeatability it is 15.5ms (as shown in table 4).

ESP32 I2C Stretching

A clock stretching question was asked on https://github.com/espressif/esp-idf/issues/4173 So it looks like the esp32 supports clock stretching but the maximum timeout is 13ms.

What I did

So what I did was make a copy of your read function and called it read16 and implemented a 16bit slave register instead of an 8bit slave register. The read portion of the code failed. So next I read what the master i2c_timeout was set to and found it was set to a count of 8000. So then I changed the timeout to the maximum count of 1048575 (0xFFFFF) or approx. 13ms and then the new read16 worked. But it is not the best fit for the SHT30 because the High Repeatability with clock stretching (measurement duration) can be 15.5ms. I think I got lucky it worked on High Repeatability with clock stretching enabled because measurement time typically is 12.5ms and that is what I was seeing on my logic analyzer.

Proposed changes to I2CMasterDevice.cpp

Keep read_block(uint8_t address, core::util::FixedBufferBase& dest, bool end_with_nack = true);
To support 16bit slave register, implement read16 function that supports a 16 bit register address based on your read function.
To support clock stretching
- Implement set_master_i2c_timeout function that uses i2c_set_timeout(i2c_port_t i2c_num, int timeout)
- implement get_master_i2c_timeout function that uses i2c_get_timeout(i2c_port_t i2c_num, int *timeout)
  Proposed changes to SHT30
Update my default Repeatability repeatability_command to Repeatabilty::DisableMeasureHighRepeatabilityWithClockStretching so it is not using clock stretching and use read_block.
I will keep all 6 Repeatability modes in case someone would like to use for Medium/Low Repeatability with clock stretching and the new read16 function.
Changes to Test Program i2c_sht30_test.cpp
I guess the test program could be changed to use both read_block and read16 functions??
So what would you like to do

Let me know how you want to proceed.

PerMalmberg commented 3 years ago

Sorry for the late reply. Since longer clock stretching isn't supported in the esp not using that sounds like the way to go. Maybe put a note next to the enum explaining why it is off by default.

enelson1001 commented 3 years ago

Tested above with SHT30 and DHT12 Created conditions to produce error messages. They look like the following

E (36322) I2CMasterDevice: Error in - read16 - slave address: 0x44 - Operation timeout, bus busy

E (37383) I2CMasterDevice: Error in - read_block - slave address: 0x44 - Operation timeout, bus busy

enelson1001 commented 3 years ago

I tried changing res operations to &= or && but log_error was never called because the resultant of these operations was always ESP_OK because ESP_OK=0. So changed back |= and | so it looks like your original code was correct. I'm not sure but I think the command link stops execution if one of the commands does not equal ESP_OK otherwise if we have multiple failures and start ORing all the results we could have a weird value at the end.

I think I got all your requested updates. I'll updated contribution file once you say everything looks good - I guess I need to add #146 and #150

PerMalmberg commented 3 years ago

Oh, yeah. The joy of having OK = 0.... oh well. Sometime we might hcnage it to call() == ESP_OK instead.

I looks good otherwise.

enelson1001 commented 3 years ago

I've updated to latest master (PR #151) but now have problems running CI/.build_test.sh on local PC. Errors out when trying to build test programs.

Building esp32 test projects -----------------------------------------
/src/CI/container_scripts/prepare_idf.sh: line 3: .: filename argument required
.: usage: . filename [arguments]
/esp/esp-idf /src
Git describe tags -----------------------------------------------------
v4.4-dev-4-g73db14240
/src
#######################################
Compiling project access_point
#######################################

-- Compiling for ESP
-- Found Git: /usr/bin/git (found version "2.27.0") 
-- IDF_TARGET not set, using default target: esp32
-- The C compiler identification is unknown
-- The CXX compiler identification is unknown
-- The ASM compiler identification is unknown
-- Found assembler: xtensa-esp32-elf-gcc
CMake Error at /esp/esp-idf/tools/cmake/project.cmake:308 (__project):
  The CMAKE_C_COMPILER:

    xtensa-esp32-elf-gcc

  is not a full path and was not found in the PATH.

  Tell CMake where to find the compiler by setting either the environment
  variable "CC" or the CMake cache entry CMAKE_C_COMPILER to the full path to
  the compiler, or to the compiler name if it is in the PATH.
Call Stack (most recent call first):
  CMakeLists.txt:74 (project)

CMake Error at /esp/esp-idf/tools/cmake/project.cmake:308 (__project):
  The CMAKE_CXX_COMPILER:

    xtensa-esp32-elf-g++

  is not a full path and was not found in the PATH.

  Tell CMake where to find the compiler by setting either the environment
  variable "CXX" or the CMake cache entry CMAKE_CXX_COMPILER to the full path
  to the compiler, or to the compiler name if it is in the PATH.
Call Stack (most recent call first):
  CMakeLists.txt:74 (project)

CMake Error at /esp/esp-idf/tools/cmake/project.cmake:308 (__project):
  The CMAKE_ASM_COMPILER:

    xtensa-esp32-elf-gcc

  is not a full path and was not found in the PATH.

  Tell CMake where to find the compiler by setting either the environment
  variable "ASM" or the CMake cache entry CMAKE_ASM_COMPILER to the full path
  to the compiler, or to the compiler name if it is in the PATH.
Call Stack (most recent call first):
  CMakeLists.txt:74 (project)

-- Warning: Did not find file Compiler/-ASM
-- Configuring incomplete, errors occurred!
See also "/src/build/CMakeFiles/CMakeOutput.log".
See also "/src/build/CMakeFiles/CMakeError.log".

PerMalmberg commented 3 years ago

/src/CI/container_scripts/prepare_idf.sh: line 3: .: filename argument required must be this line . $IDF_TOOLS_EXPORT_CMD so that variable must be empty for you for some reason.

I can't figure out why and I'm sure I ran the tests from latest master after merging that PR.

enelson1001 commented 3 years ago

I ended up changing . $IDF_TOOLS_EXPORT_CMD to this -----> . $IDF_PATH/export.sh and it worked for me. Are you OK with me submitting this change?

PerMalmberg commented 3 years ago

I ended up changing . $IDF_TOOLS_EXPORT_CMD to this -----> . $IDF_PATH/export.sh and it worked for me. Are you OK with me submitting this change?

Let's hear if @peterus has some input as to why IDF_TOOLS_EXPORT_CMD would be empty.

peterus commented 3 years ago

it looks like @enelson1001 is not using the newest docker image. please delete the local docker image with this commands: docker image rm permalmberg/smooth and run the build command again.

@PerMalmberg maybe we should add some kind of version information (as the docker image tag) to the docker image. This can/should be based on the lastest idf version tag. using latest is not always the best as you can see in this example.

PerMalmberg commented 3 years ago

it looks like @enelson1001 is not using the newest docker image. please delete the local docker image with this commands: docker image rm permalmberg/smooth and run the build command again.

@PerMalmberg maybe we should add some kind of version information (as the docker image tag) to the docker image. This can/should be based on the lastest idf version tag. using latest is not always the best as you can see in this example.

Yes, that's a good idea.

enelson1001 commented 3 years ago

deleting the local docker image did the trick and build_test.sh ran with no error.

PerMalmberg / Smooth