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

Updates for new I2C sensor SHT30 #150

Merged
merged 10 commits into from
Apr 10, 2021

Conversation

enelson1001
Copy link
Contributor

  • 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.

Copy link
Owner

@PerMalmberg PerMalmberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, so it wasn't possible to combine the existing write/read functions to achieve the desired functionality. If you say that's the case then I'll take your word for it.

Let me know if you're happy with this as-is and we'll get it merged.

@enelson1001
Copy link
Contributor Author

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 espressif/esp-idf#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

  1. Keep read_block(uint8_t address, core::util::FixedBufferBase<uint8_t>& dest, bool end_with_nack = true);
  2. To support 16bit slave register, implement read16 function that supports a 16 bit register address based on your read function.
  3. 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

  1. Update my default Repeatability repeatability_command to Repeatabilty::DisableMeasureHighRepeatabilityWithClockStretching so it is not using clock stretching and use read_block.
  2. 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

  1. 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
Copy link
Owner

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
Copy link
Contributor Author

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

Copy link
Owner

@PerMalmberg PerMalmberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also don't forget to update the contribution file.

lib/smooth/application/io/i2c/SHT30.cpp Outdated Show resolved Hide resolved
lib/smooth/application/io/i2c/SHT30.cpp Outdated Show resolved Hide resolved
lib/smooth/application/io/i2c/SHT30.cpp Outdated Show resolved Hide resolved
lib/smooth/core/io/i2c/I2CMasterDevice.cpp Outdated Show resolved Hide resolved
lib/smooth/core/io/i2c/I2CMasterDevice.cpp Show resolved Hide resolved
lib/smooth/include/smooth/core/io/i2c/I2CMasterDevice.h Outdated Show resolved Hide resolved
lib/smooth/include/smooth/core/io/i2c/I2CMasterDevice.h Outdated Show resolved Hide resolved
lib/smooth/include/smooth/core/io/i2c/I2CMasterDevice.h Outdated Show resolved Hide resolved
lib/smooth/include/smooth/core/io/i2c/I2CMasterDevice.h Outdated Show resolved Hide resolved
lib/smooth/include/smooth/core/io/i2c/I2CMasterDevice.h Outdated Show resolved Hide resolved
@enelson1001
Copy link
Contributor Author

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
Copy link
Owner

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

I looks good otherwise.

lib/smooth/core/io/i2c/I2CMasterDevice.cpp Outdated Show resolved Hide resolved
lib/smooth/core/io/i2c/I2CMasterDevice.cpp Outdated Show resolved Hide resolved
@enelson1001
Copy link
Contributor Author

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
Copy link
Owner

PerMalmberg commented Apr 6, 2021

/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
Copy link
Contributor Author

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
Copy link
Owner

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
Copy link
Contributor

peterus commented Apr 9, 2021

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
Copy link
Owner

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
Copy link
Contributor Author

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

@PerMalmberg PerMalmberg merged commit 1a9ed5f into PerMalmberg:master Apr 10, 2021
@enelson1001 enelson1001 deleted the new-sensor-sht30 branch April 10, 2021 15:44
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.

3 participants