Updated aht20 driver to use thread safe api from idf>=5.4. (AEGHB-1062)

[jeetrohan]

Description

Updated the AHT20 driver.
Supports new thread safe I2C APIs introduced in idf=5.4.

Related

Testing

Tested using AHT20 latest revision with esp32 and exp32-s3 devkits.

---

Checklist

Before submitting a Pull Request, please ensure the following:

• 🚨 This PR does not introduce breaking changes.
• All CI checks (GH Actions) pass.
• Documentation is updated as needed.
• Tests are updated or added as necessary.
• Code is well-commented, especially in complex areas.
• Git history is clean — commits are squashed to the minimum necessary.

[CLAassistant]

All committers have signed the CLA.

[github-actions]

	Warnings
⚠️	Some issues found for the commit messages in this PR: the commit message " refactor(aht20): remove I2C clock Kconfig option and encapsulate sensor handle": summary looks empty type/action looks empty the commit message " docs(aht20): improve documentation and update changelog": summary looks empty type/action looks empty the commit message " feat(aht20): add Sensor Hub support, new test app, and example": body's lines must not be longer than 100 characters summary looks empty type/action looks empty the commit message " fix(aht20): fixed minor changes": summary looks empty type/action looks empty the commit message " refactor(aht20): update to use i2c_bus, add Kconfig options, bump to v1.1.0": summary looks empty type/action looks empty the commit message "megre (aht20): Merge branch 'e-update_aht20' into update_aht20": summary looks empty type/action looks empty Please fix these commit messages - here are some basic tips: follow Conventional Commits style correct format of commit message should be: <type/action>(<scope/component>): <summary>, for example fix(esp32): Fixed startup timeout issue allowed types are: change,ci,docs,feat,fix,refactor,remove,revert,test sufficiently descriptive message summary should be between 20 to 72 characters and start with upper case letter avoid Jira references in commit messages (unavailable/irrelevant for our customers) TIP: Install pre-commit hooks and run this check when committing (uses the Conventional Precommit Linter).
⚠️	Please consider squashing your 13 commits (simplifying branch history).

	Messages
📖	This PR seems to be quite large (total lines of code: 1053), you might consider splitting it into smaller PRs

👋 Hello jeetrohan, we appreciate your contribution to this project!

---

Click to see more instructions ...

This automated output is generated by the PR linter DangerJS, which checks if your Pull Request meets the project's requirements and helps you fix potential issues.

DangerJS is triggered with each push event to a Pull Request and modify the contents of this comment.

Please consider the following:
- Danger mainly focuses on the PR structure and formatting and can't understand the meaning behind your code or changes.
- Danger is not a substitute for human code reviews; it's still important to request a code review from your colleagues.
- Resolve all warnings (⚠️ ) before requesting a review from human reviewers - they will appreciate it.
- Addressing info messages (📖) is strongly recommended; they're less critical but valuable.
- To manually retry these Danger checks, please navigate to the Actions tab and re-run last Danger workflow.

Review and merge process you can expect ...

We do welcome contributions in the form of bug reports, feature requests and pull requests.

1. An internal issue has been created for the PR, we assign it to the relevant engineer.
2. They review the PR and either approve it or ask you for changes or clarifications.
3. Once the GitHub PR is approved we do the final review, collect approvals from core owners and make sure all the automated tests are passing.
- At this point we may do some adjustments to the proposed change, or extend it by adding tests or documentation.
4. If the change is approved and passes the tests it is merged into the default branch.

Generated by 🚫 dangerJS against 22cd170

[leeebo]

Hi @jeetrohan , thanks for your contribution!

[YanKE01]

Hi, @jeetrohan , thanks for your contribution! Before making a commit, you can install pre-commit to ensure that your code style is consistent with the repository.

pip install pre-commit
pre-commit install --allow-missing-config -t pre-commit -t commit-msg

[YanKE01]

Forcibly restricting the component in version 5.4 will make it difficult for many existing users of this component to accept. Perhaps the i2c_bus component can be used to manage the entire I2C, since i2c_bus will automatically switch to the esp_driver_i2c driver in version 5.3 and above.

[jeetrohan]

Hi @YanKE01 There are two reasons I had originally decided to use idf>=5.4.

1. The new I2C APIs introduced in this version are thread-safe, at least when multiple threads are not accessing the same device/sensor. Basically multiple devices can be accessed safely on same I2C bus, without contention from different threads.
   The new I2C APIs, if I remember correctly, introduced in idf-5.2 are incompatible with older ones(idf<5.2), so it should not affect much the users who are using the new APIs, as they are likely to update sooner than later.

2. I had assumed that using the Generic I2C driver APIs provide more flexibility to this driver, even when used without this repository (such as component registry, though I might be uninformed on this part, as component registry resolves all the dependencies).

However, considering only this repository, and taking into account your suggestion.. I've concluded that there are two feasible things that can be done...:

1. I simply do as have been suggested. Replace the driver to i2c_bus component. This will effectively strip the thread-safety for any users, not using idf>=5.4(A note can be placed in this regard in README section).
   The dependency in this case, If my understanding is correct, will change to idf>=4.3.

2. We release two versions of this driver. One with the i2c_bus component (v1.1.0) and other with esp_driver_i2c (v2.0.0) as the older i2c driver and its APIs are just a legacy support feature.
   A relevant note from idf 5.2.5 documentation (& still in idf.5.4.1) : "Please keep in mind that the legacy driver is now deprecated and will be removed in future."
   This was the reason to refer this as v2.0.0; to indicate that it is not meant to support legacy driver.

While you finalize the decision.. I'll prepare both the versions.

[YanKE01]

Hi, @jeetrohan , in the current repository, we are standardizing the use of i2c_bus to manage I2C-type sensors, so I believe using i2c_bus would be a better decision. Moreover, i2c_bus was designed with thread safety in mind from the beginning, so users don't need to worry about concurrency issues.

As for the AHT20, which is a commonly used component, its upgrade cannot simply switch to the latest version of the IDF (ESP-IDF), because we need to consider the versions being used by current users. If we force the use of the latest IDF, users who are still on versions earlier than IDF 5.4 will no longer be able to use the AHT20 component.

[jeetrohan]

Hi @YanKE01
I've updated the most recent submission to use the i2c_bus.
Yes, I understand the support for previous idf is still valid.

[YanKE01]

Memory leak checks have not been performed. It might be better to make modifications based on the original test_apps, since CI-related issues are also involved.

[jeetrohan]

I've updated this locally, waiting for your decision on the other recommendation.

[YanKE01]

It is recommended to update the version to 1.1.0. Additionally, one side of the parenthesis is missing.

[jeetrohan]

Fixed the version.

[YanKE01]

Suggested change

	#Features
	# Features

[jeetrohan]

I've updated this locally, waiting for your decision on the other recommendation.

[jeetrohan]

Hi, @jeetrohan , thanks for your contribution! Before making a commit, you can install pre-commit to ensure that your code style is consistent with the repository.

pip install pre-commit
pre-commit install --allow-missing-config -t pre-commit -t commit-msg

hi @YanKE01
I'm now having this error which is not letting me make commits and I'm unable to understand why (my idf_component file has version). Can you help me fix this:

Check copyright notices..................................................Passed
file contents sorter.................................(no files to check)Skipped
trim trailing whitespace.................................................Passed
fix end of files.........................................................Passed
check that executables have shebangs.................(no files to check)Skipped
check that scripts with shebangs are executable..........................Passed
mixed line ending........................................................Passed
fix double quoted strings............................(no files to check)Skipped
Do not use more than one slash in the branch name........................Passed
Do not use uppercase letters in the branch name..........................Passed
astyle formatter.........................................................Passed
Check File Permissions...............................(no files to check)Skipped
Validate executable-list.txt.............................................Passed

Update versions in readme................................................Failed

• hook id: release_versions
• exit code: 1

Traceback (most recent call last):
File "my-esp-iot-solution/tools/release_versions.py", line 29, in
components = get_components_versions(os.path.join(os.path.dirname(file), '../components'))
File "my-esp-iot-solution/tools/release_versions.py", line 25, in get_components_versions
component_version = data['version']
KeyError: 'version'

codespell................................................................Passed

[jeetrohan]

Hi @YanKE01
I've refactored all the recommendations you made regarding this driver.
I had to do a hard reset because of previous commits without hook. Apparently that was the reason I was unable to make commits after installing pre-commit hook and soft reset didn't helped.

If you still find anything that needs to be fixed, let me know.
I'll be releasing the aforementioned v2.0.0 as v1.. something in my component registry, so that if ever needed....

Also, I haven't merged this with Sensor Hub, because..... 🤐
Let's just say, I do feel like Sensor Hub can do more(yes, I did implement it to some extent, then stopped).

Thanks for pointing me out to use the i2c_bus component, I dislike it for my own reasons, but it was helpful in its own ways.
Also, is there a unified interface for SPI and I2C bus? I have tested a sensor (will be testing another probably soon enough) with both buses.
I wanted to contribute those too to this repository. If its not there then I'll simply make shift the things.

[jeetrohan]

Hi, @YanKE01 & @leeebo
Could you confirm me if the current submission is acceptable? or there is a need for further change?

[YanKE01]

Hi, @jeetrohan . Thank you very much for your contribution. The sensor_hub is still under development. Could you list some of the current issues and features you would like to see added? This will help us improve sensor_hub more effectively.

Additionally, the design principles of i2c_bus and spi_bus are the same, and both have taken the resource contention issues you mentioned into account. If you have any questions about i2c_bus or spi_bus, please let us know so we can further improve them.

[jeetrohan]

Hi, @jeetrohan . Thank you very much for your contribution. The sensor_hub is still under development. Could you list some of the current issues and features you would like to see added? This will help us improve sensor_hub more effectively.

Hi @YanKE01,
First TLDR : It is currently feature rich as it is (to a good extent, if not wholly) and I would like to contribute myself for many things to the implementation/framework itself.

This will become a very long rant if I will go on explaining everything. Even without details :

I'm not going to talk about features because, in my humble opinion, there are plenty of helpful features in current implementation. Additionally, no matter the amount of features, somebody can always come up with some niche but kind of essential feature.

What I'm going to talk about is, its structure or the framework itself.

1. In current implementation as it is:

• I remember seeing a small bug/resource management issue in the code, among many other things, which I'll fix and open PR for, myself. But I vaguely remember some other things too.
• There are few other things which I need to confirm by using it myself (I saw the code for whole thing and how it is connected and could cause issues but not certain at the moment).

2. Things I would like to contribute, myself:

• Current implementation, as I see it is a lot of good things put together with good intentions... but.. I don't see where it is going.. I mean I see it... but.. this is something which would require a more formal form of discussion (to you this is, not to me, I'm not the part of development team, just a contributor.)
• In addition to that.. there are some little things which I would like to add myself.
  However, this is the kind of work which makes more sense when I'm part of team. I don't know how you would evaluate my current work, but mine is : sloppy(my evaluation of my work I've submitted up until in any form to esp-idf). And that's because there's nothing associated in it for me. I would rather like to do sincere work.

Where is this coming from??
I have been using the esp-idf for quite sometime and there are little things, that keep feeling like why they are not there..? I understand there is a lot of workload on all the teams, because too many things are being pursued.

What I can do at the moment for certain:
I can definitely contribute 2 more sensor driver for the Hub (if acceptable). Both tested on hardware. (I'm not certain about contributing a few others, will need time. One of these is not the kind where a simple implementation of data sheet does it.)

Why the delay in some things?
Currently I'm trying to implement a few other things related to BLE (and also another RTOS).
Once I'm done, I'll update with more certainty.

[jeetrohan]

Additionally, the design principles of i2c_bus and spi_bus are the same, and both have taken the resource contention issues you mentioned into account. If you have any questions about i2c_bus or spi_bus, please let us know so we can further improve them.

I've not checked the code... but...
Since the i2c_bus internally itself uses the underlying esp_driver_i2c, would it not be a better idea to remove the resource lock used in i2c_bus for idf>=5.4.
It would be unnecessary to double lock the resource.. once in i2c_bus and then in esp_driver_i2c.
Unless.... the lock in i2c_bus guarantees thread safety even when the same resource/sensor on same bus is being accessed by two different threads. (my understanding of esp_driver_i2c from documentation is that it doesn't provides this access security.)

[jeetrohan]

Hi @YanKE01 ,
If you want me to add Sensor Hub support in this driver I can do that too... But I won't be testing it (for reasons I've mentioned in other reply.) and this is also the reason I dropped the idea of doing it.
According to documents and other sensors.. I need to just add a few more lines.. but from my perspective.. adding those lines is additional testing, and one more example, basically few hours.
So, if you're ok.. with having a few more lines added.. let me know.

And if there is any other thing to finalize this contribution, please let me know.
Thanks.

[YanKE01]

Additionally, the design principles of i2c_bus and spi_bus are the same, and both have taken the resource contention issues you mentioned into account. If you have any questions about i2c_bus or spi_bus, please let us know so we can further improve them.

I've not checked the code... but... Since the i2c_bus internally itself uses the underlying esp_driver_i2c, would it not be a better idea to remove the resource lock used in i2c_bus for idf>=5.4. It would be unnecessary to double lock the resource.. once in i2c_bus and then in esp_driver_i2c. Unless.... the lock in i2c_bus guarantees thread safety even when the same resource/sensor on same bus is being accessed by two different threads. (my understanding of esp_driver_i2c from documentation is that it doesn't provides this access security.)

You can see from the commit history of i2c_bus that it predates the current esp_driver_i2c driver by quite a bit. Moreover, the design philosophy of esp_driver_i2c shares similarities with i2c_bus, such as separating the bus and device, and ensuring thread safety.

As for your question about why esp_driver_i2c now provides thread-safe bus operation functions, yet i2c_bus still performs additional locking — the reason is that, at the time i2c_bus was made compatible with esp_driver_i2c, the latter did not yet guarantee thread-safe signaling for bus operations like it does now. You can refer to the documentation of esp_driver_i2c from version 5.3.3 to verify this.

ESP-IDF drivers are constantly being improved, but as a component, I believe compatibility should be the top priority — because you can't expect everyone to always stay updated with the latest IDF master branch.

[jeetrohan]

You can see from the commit history of i2c_bus that it predates the current esp_driver_i2c driver by quite a bit. Moreover, the design philosophy of esp_driver_i2c shares similarities with i2c_bus, such as separating the bus and device, and ensuring thread safety.

As for your question about why esp_driver_i2c now provides thread-safe bus operation functions, yet i2c_bus still performs additional locking — the reason is that, at the time i2c_bus was made compatible with esp_driver_i2c, the latter did not yet guarantee thread-safe signaling for bus operations like it does now. You can refer to the documentation of esp_driver_i2c from version 5.3.3 to verify this.

ESP-IDF drivers are constantly being improved, but as a component, I believe compatibility should be the top priority — because you can't expect everyone to always stay updated with the latest IDF master branch.

@YanKE01,
I apologize.. I suppose I didn't put it in the right way...
Allow me to put it more properly...

1. i2c_bus is meant to provide, yet another layer of abstraction, because currently the lifecycle of previous idf versions is going to exist until at least 25Jan2027 (esp-idf-5.3).

2. i2c_bus is meant to provide necessary features not available in older idf, and as such serves essential purpose.

3. It separates legacy driver from new driver by using this... #if ESP_IDF_VERSION >= ESP_IDF_VERSION_VAL(5, 3, 0).

4. My suggestion was... if possible.... the locking mechanism in i2c_bus should use something similar to avoid putting additional locking mechanism in esp-idf>=5.4 to avoid double locking of resources.....(as I said I haven't looked inside the code, but I'm assuming this should be possible. Though it may make it a bit stuffy/cluttered. Probably, also it will not make any significant difference in performance.. unless very time critical tasks are being performed, but at least will not add another layer during runtime).

5. But... the above suggestion has no value, if..... the lock in i2c_bus guarantees thread safety even when the same resource/sensor on same bus is being accessed by two different threads. (my understanding of esp_driver_i2c from documentation is that it doesn't provides this access security.)

This will ensure both compatibility with previous versions.. without adding another layer of locking mechanism on an existing one (provided it is same as the one in esp-idf=5.4).

Correction: In 4. above, it may still affect the performance under certain circumstances, even if the tasks are not time critical... but, that will depend on how these were implemented. And as I'm mentioning once again, I've not checked the exact implementations, so I can't say for sure.

[YanKE01]

Hi, @jeetrohan. I still recommend that you use i2c_bus to update the aht20 component, as it is compatible with different versions of the IDF and does not force users to switch to the latest IDF version. If you still prefer to use the esp_driver_i2c API, you can try submitting it to your own space in the component manager. You can refer to the relevant submission documentation.

[jeetrohan]

Hi, @jeetrohan. I still recommend that you use i2c_bus to update the aht20 component, as it is compatible with different versions of the IDF and does not force users to switch to the latest IDF version. If you still prefer to use the esp_driver_i2c API, you can try submitting it to your own space in the component manager. You can refer to the relevant submission documentation.

@YanKE01
I totally agree with your sentiment.

Please check the commit log from from last week's submission.

I've already updated this driver to use i2c_bus and I've informed you regarding the same.

[YanKE01]

• Replace the i2c interface with i2c_bus

[jeetrohan]

fixed.

[YanKE01]

If no new features of CMake are being used, I believe there's no need to make any changes.

Suggested change

	cmake_minimum_required(VERSION 3.16)
	cmake_minimum_required(VERSION 3.5)

[jeetrohan]

fixed.
But,
I had replaced the original files with my local files rather than editing (version 3.16 was used).
Please check the compilation after this change with corresponding CMake version.

[YanKE01]

Suggested change

	set(COMPONENTS main)
	set(EXTRA_COMPONENT_DIRS "$ENV{IDF_PATH}/tools/unit-test-app/components")

Please do not remove unit-test-app/components, otherwise the following issues will occur:

➜  test_apps git:(pr-503) idf.py set-target esp32s3
Adding "set-target"'s dependency "fullclean" to list of commands with default set of options.
Executing action: fullclean
Build directory '/home/yanke/project/esp-iot-solution/components/sensors/humiture/aht20/test_apps/build' not found. Nothing to clean.
Executing action: set-target
Set Target to: esp32s3, new sdkconfig will be created.
Running cmake in directory /home/yanke/project/esp-iot-solution/components/sensors/humiture/aht20/test_apps/build
Executing "cmake -G Ninja -DPYTHON_DEPS_CHECKED=1 -DPYTHON=/home/yanke/.espressif/python_env/idf5.5_py3.11_env/bin/python -DESP_PLATFORM=1 -DIDF_TARGET=esp32s3 -DCCACHE_ENABLE=0 /home/yanke/project/esp-iot-solution/components/sensors/humiture/aht20/test_apps"...
-- Found Git: /usr/bin/git (found version "2.34.1") 
-- Minimal build - OFF
-- The C compiler identification is GNU 14.2.0
-- The CXX compiler identification is GNU 14.2.0
-- The ASM compiler identification is GNU
-- Found assembler: /home/yanke/.espressif/tools/xtensa-esp-elf/esp-14.2.0_20241119/xtensa-esp-elf/bin/xtensa-esp32s3-elf-gcc
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: /home/yanke/.espressif/tools/xtensa-esp-elf/esp-14.2.0_20241119/xtensa-esp-elf/bin/xtensa-esp32s3-elf-gcc - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /home/yanke/.espressif/tools/xtensa-esp-elf/esp-14.2.0_20241119/xtensa-esp-elf/bin/xtensa-esp32s3-elf-g++ - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Building ESP-IDF components for target esp32s3
NOTICE: Dependencies lock doesn't exist, solving dependencies.
NOTICE: Using component placed at /home/yanke/project/esp-iot-solution/components/sensors/humiture/aht20 for dependency "aht20", specified in /home/yanke/project/esp-iot-solution/components/sensors/humiture/aht20/test_apps/main/idf_component.yml
......NOTICE: Updating lock file at /home/yanke/project/esp-iot-solution/components/sensors/humiture/aht20/test_apps/dependencies.lock
NOTICE: Processing 4 dependencies:
NOTICE: [1/4] aht20 (1.1.0) (/home/yanke/project/esp-iot-solution/components/sensors/humiture/aht20)
NOTICE: [2/4] espressif/cmake_utilities (0.5.3)
NOTICE: [3/4] i2c_bus (1.4.0) (/home/yanke/project/esp-iot-solution/components/i2c_bus)
NOTICE: [4/4] idf (5.5.0)
CMake Error at /home/yanke/esp/esp-idf/tools/cmake/build.cmake:328 (message):
  Failed to resolve component 'test_utils' required by component 'main':
  unknown name.
Call Stack (most recent call first):
  /home/yanke/esp/esp-idf/tools/cmake/build.cmake:377 (__build_resolve_and_add_req)
  /home/yanke/esp/esp-idf/tools/cmake/build.cmake:676 (__build_expand_requirements)
  /home/yanke/esp/esp-idf/tools/cmake/project.cmake:736 (idf_build_process)
  CMakeLists.txt:7 (project)


-- Configuring incomplete, errors occurred!
See also "/home/yanke/project/esp-iot-solution/components/sensors/humiture/aht20/test_apps/build/CMakeFiles/CMakeOutput.log".
HINT: The component 'test_utils' could not be found. This could be because: component name was misspelled, the component was not added to the build, the component has been moved to the IDF component manager, the component has been removed and refactored into some other component or the component may not be supported by the selected target.
Please look out for component in 'https://components.espressif.com' and add using 'idf.py add-dependency' command.
Refer to the migration guide for more details about moved components.
Refer to the build-system guide for more details about how components are found and included in the build.
cmake failed with exit code 1, output of the command is in the /home/yanke/project/esp-iot-solution/components/sensors/humiture/aht20/test_apps/build/log/idf_py_stderr_output_289434 and /home/yanke/project/esp-iot-solution/components/sensors/humiture/aht20/test_apps/build/log/idf_py_stdout_output_289434

[jeetrohan]

fixed.

[YanKE01]

In my opinion, no modifications are needed at this point.

Suggested change

	# aht20
	[](https://components.espressif.com/components/espressif/aht20)
	# Component: AHT20

[jeetrohan]

fixed.

[YanKE01]

I'm not sure what the indentation means here.

[jeetrohan]

fixed.

[YanKE01]

I'm not quite sure what the purpose of this function is. It's uncommon to put data inside a handle. In other words, compared to aht20_read_humidity, what makes this function necessary?

[jeetrohan]

@YanKE01
This... will be a long answer... and one without TLDR.

There are two questions here....

1. Why provide aht20_read_humiture() to read at once both temperature and humidity when there are separate APIs aht20_read_humidity() and aht20_read_temperature().
2. And even if implement it ... why the way it has been?

So the answer to 1. :
AHT20 datasheet indicate/directs that readings should be taken at the interval of minimum 2 seconds.
This means user should wait for at least 2 seconds in between calling aht20_read_humidity() and aht20_read_temperature(); as each of these function are equivalent of a single sensor data read.
However, it is somewhat inconvenient or at least not desired to have a delay between readings for associated parameters.. although unlikely that there will be much fluctuations in value, within 2 seconds. But still unnecessary when user wants to read both, because every read provides raw values for both.
If my memory serves correctly, some resources indicated that reading faster than 2 seconds may damage hardware, but I'm not sure how reliable this information is.

And answer to 2. :
I agree, its unconventional to put results in handle, at least in ESP ecosystem. But consider this just as a handy feature.. providing user with more convenient access to the data they are most likely to access with one thing that they will definitely need to access any call to sensor.
But why as a structure and not simply float variable? Again convenience to user in passing packed data.

Now what could be done.. :

• Completely remove aht20_read_humiture()
• Keep aht20_read_humiture(), but, instead of structure use simple float variables in handle.
• Keep aht20_read_humiture(), but return the structure instead of storing in handle.
• Keep the aht20_read_humiture() as it is.

[jeetrohan]

It's uncommon to put data inside a handle.

@YanKE01
There are 3 other reasons I can quote for this particular choice:

1. Security (too nascent stage to discuss).
2. Inter task communication.(self-explanatory)
3. Memory safety(goes out with deinit()).

When I make alterations to the existing structures/conventional approaches its after considering from multiple aspects.
However, allow me to make it easier to choose ...
Whether you would like to uphold the traditional choices, or add new features.. as long as they are not breaking the existing system or conflict with any possible future updates?

If you think this conflicts with current or any possible future updates.. let's just drop it.

[jeetrohan]

@YanKE01 is my response sufficient enough to explain the purpose?
or should I remove this function altogether?
Although, I've come to understand your question better when I delved again in Sensor_Hub implementation. This would appear as redundant.
However, the implementation of aht20 drive needs to be tweaked a bit in order to display its functionality in that case.

[YanKE01]

I'm not very certain myself. I feel like this is a feature I would hardly ever come across.

[jeetrohan]

@YanKE01 If you feel like the feature has no particular use I don't mind dropping it.. but.. please keep the following in consideration when making a decision.
even if nothing else..

AHT20 datasheet indicate/directs that readings should be taken at the interval of minimum 2 seconds.
This means user should wait for at least 2 seconds in between calling aht20_read_humidity() and aht20_read_temperature(); as each of these function are equivalent of a single sensor data read.
However, it is somewhat inconvenient or at least not desired to have a delay between readings for associated parameters.. although unlikely that there will be much fluctuations in value, within 2 seconds. But still unnecessary when user wants to read both, because every read provides raw values for both.

and....

If my memory serves correctly, some resources indicated that reading faster than 2 seconds may damage hardware, but I'm not sure how reliable this information is.

[jeetrohan]

I'm not very certain myself. I feel like this is a feature I would hardly ever come across.

@YanKE01 please also clarify if you're talking about ..
just this... aht20_read_humiture()
or
adding the results in handle or both?

[YanKE01]

Please make sure the function naming follows a consistent format throughout the code.

[jeetrohan]

fixed.

[YanKE01]

Since this is a Kconfig option, I think it's fine to leave it as is unless there's a strong reason to change it — users can always configure it to the IO they need.

[jeetrohan]

fixed.
I had just updated it with values I used on esp32.
Had replaced with local files during updated, rather than editing.

[YanKE01]

The usage of AHT20 is already clearly explained in the component's README. @leeebo , do you think it's still necessary to write an additional example?

[jeetrohan]

@YanKE01 and @leeebo ,
Sorry.. I didn't noticed the question was directed not at me, until I was about to comment..(edit: I mean post it)
But since.. I've..
consider this as my opinion or suggestion, I don't what it is, to both of you.

Of course, I will remove the example from my end, if that's the conclusion you two reach to.

Many, if not most, developers will agree with your sentiment; maybe I'm the odd one out.

Allow me to rephrase your question:
"There's a perfectly fine instruction manual so why the demo?"

I'll answer you as a typical layman user, in the most laymen language I can :

Note: Laymen in this context is a user who knows his way around C language, IDEs and has experience of working with micro-controllers but has zero interest in understanding the underlying framework used in esp32 ecosystem.

"I just want to use this sensor, to get some readings.. I don't want to bother to implement it myself.. and definitely not interested in how the readings are being collected by underlying framework... I just want to get the readings and use them for whatever insidious(humor, not serious) purposes I have.
I don't want to implement nothing (intentionally written instead of anything)."

In more simpler words.. Its for people used to Arduino, fire and forget.

[jeetrohan]

@leeebo I'm still waiting for your response. I will use that as guideline for the next two sensors I'll be contributing to this repository.(drivers already tested on hardware).

[YanKE01]

I noticed that in test_apps, the I2C bus handle is created via esp_driver_i2c rather than the i2c_bus framework. Then, this handle is directly passed to i2c_bus_device_create in aht20_create, which is not valid because the two handles do not point to the same internal data structure.

(1)     "AHT20 Sensor" [aht20][sensor]

Enter test for running.
1
Running AHT20 Sensor...
Requesting I2C bus handle
I2C bus handle acquired
I (1640) AHT20: adding aht20 as device to bus

E (1640) i2c_bus: /home/yanke/project/esp-iot-solution/components/i2c_bus/i2c_bus_v2.c:185 (i2c_bus_device_create):i2c_bus has not inited
E (1650) AHT20: aht20_create(290): unable to create device

E (1650) i2c_bus: /home/yanke/project/esp-iot-solution/components/i2c_bus/i2c_bus_v2.c:214 (i2c_bus_device_delete):Null Device Handle
MALLOC_CAP_8BIT: Before 388860 bytes free, After 388756 bytes free (delta -104)
MALLOC_CAP_32BIT: Before 388860 bytes free, After 388756 bytes free (delta -104)
./main/aht20_test_app.c:123:AHT20 Sensor:PASS
Test ran in 58ms

[jeetrohan]

I noticed that in test_apps, the I2C bus handle is created via esp_driver_i2c rather than the i2c_bus framework. Then, this handle is directly passed to i2c_bus_device_create in aht20_create, which is not valid because the two handles do not point to the same internal data structure.

(1)     "AHT20 Sensor" [aht20][sensor]

Enter test for running.
1
Running AHT20 Sensor...
Requesting I2C bus handle
I2C bus handle acquired
I (1640) AHT20: adding aht20 as device to bus

E (1640) i2c_bus: /home/yanke/project/esp-iot-solution/components/i2c_bus/i2c_bus_v2.c:185 (i2c_bus_device_create):i2c_bus has not inited
E (1650) AHT20: aht20_create(290): unable to create device

E (1650) i2c_bus: /home/yanke/project/esp-iot-solution/components/i2c_bus/i2c_bus_v2.c:214 (i2c_bus_device_delete):Null Device Handle
MALLOC_CAP_8BIT: Before 388860 bytes free, After 388756 bytes free (delta -104)
MALLOC_CAP_32BIT: Before 388860 bytes free, After 388756 bytes free (delta -104)
./main/aht20_test_app.c:123:AHT20 Sensor:PASS
Test ran in 58ms

@YanKE01
It was already fixed locally yesterday, when....
I noticed this while I was trying to add the Sensor Hub Functionality and also add a unit test for that... however, I'm facing a bit of problem with that.. would you like to help me with that?

or

can I just add the Sensor Hub relevant code and update it as it is, without the unit test?

[YanKE01]

I noticed that in test_apps, the I2C bus handle is created via esp_driver_i2c rather than the i2c_bus framework. Then, this handle is directly passed to i2c_bus_device_create in aht20_create, which is not valid because the two handles do not point to the same internal data structure.

(1)     "AHT20 Sensor" [aht20][sensor]

Enter test for running.
1
Running AHT20 Sensor...
Requesting I2C bus handle
I2C bus handle acquired
I (1640) AHT20: adding aht20 as device to bus

E (1640) i2c_bus: /home/yanke/project/esp-iot-solution/components/i2c_bus/i2c_bus_v2.c:185 (i2c_bus_device_create):i2c_bus has not inited
E (1650) AHT20: aht20_create(290): unable to create device

E (1650) i2c_bus: /home/yanke/project/esp-iot-solution/components/i2c_bus/i2c_bus_v2.c:214 (i2c_bus_device_delete):Null Device Handle
MALLOC_CAP_8BIT: Before 388860 bytes free, After 388756 bytes free (delta -104)
MALLOC_CAP_32BIT: Before 388860 bytes free, After 388756 bytes free (delta -104)
./main/aht20_test_app.c:123:AHT20 Sensor:PASS
Test ran in 58ms

@YanKE01 It was already fixed locally yesterday, when.... I noticed this while I was trying to add the Sensor Hub Functionality and also add a unit test for that... however, I'm facing a bit of problem with that.. would you like to help me with that?

or

can I just add the Sensor Hub relevant code and update it as it is, without the unit test?

What specific issues have you encountered?

[jeetrohan]

@YanKE01 It was already fixed locally yesterday, when.... I noticed this while I was trying to add the Sensor Hub Functionality and also add a unit test for that... however, I'm facing a bit of problem with that.. would you like to help me with that?
or
can I just add the Sensor Hub relevant code and update it as it is, without the unit test?

What specific issues have you encountered?

Rather than using sensor_hub, I decided to use the humiture functions.
& This is the issue:

And the reason, I'm trying to drop this by simply adding the required Sensor Hub files is that..
I'm working again on an incomplete MPU6050 driver that I submitted in other repository. I am adding new functionality to it which requires both refactoring and testing.

And I've another request, please mark the conversations as resolved regarding queries which you raised and believe are resolved.

[YanKE01]

@YanKE01 It was already fixed locally yesterday, when.... I noticed this while I was trying to add the Sensor Hub Functionality and also add a unit test for that... however, I'm facing a bit of problem with that.. would you like to help me with that?
or
can I just add the Sensor Hub relevant code and update it as it is, without the unit test?

What specific issues have you encountered?

Rather than using sensor_hub, I decided to use the humiture functions. & This is the issue:

I think you should add the dependency for sensor_hub; you can refer to the idf_component.yml of SHT3X:

version: "0.2.0"
description: I2C driver for SHT3X humiture sensor
url: https://github.com/espressif/esp-iot-solution/tree/master/components/sensors/humiture/sht3x
repository: https://github.com/espressif/esp-iot-solution.git
documentation: https://docs.espressif.com/projects/esp-iot-solution/en/latest/sensors/humiture.html
issues: https://github.com/espressif/esp-iot-solution/issues
dependencies:
  i2c_bus:
    public: true
    override_path: "../../../i2c_bus"
  sensor_hub:
    public: true
    override_path: "../../sensor_hub"
  idf: '>=4.4'
  cmake_utilities: "0.*"

[jeetrohan]

I think you should add the dependency for sensor_hub; you can refer to the idf_component.yml of SHT3X:

version: "0.2.0"
description: I2C driver for SHT3X humiture sensor
url: https://github.com/espressif/esp-iot-solution/tree/master/components/sensors/humiture/sht3x
repository: https://github.com/espressif/esp-iot-solution.git
documentation: https://docs.espressif.com/projects/esp-iot-solution/en/latest/sensors/humiture.html
issues: https://github.com/espressif/esp-iot-solution/issues
dependencies:
  i2c_bus:
    public: true
    override_path: "../../../i2c_bus"
  sensor_hub:
    public: true
    override_path: "../../sensor_hub"
  idf: '>=4.4'
  cmake_utilities: "0.*"

@YanKE01
It was already like that

ah20 idf._component.yml:

name: aht20

version: "1.1.0"

author: Rohan Jeet <[email protected]>

description: AHT20 Temperature and Humidity Sensor I2C Driver for ESP-IDF

issues: https://github.com/espressif/esp-iot-solution/issues

repository: git://github.com/espressif/esp-iot-solution.git

url: https://github.com/espressif/esp-iot-solution/tree/master/components/sensors/humiture/aht20

dependencies:

  i2c_bus:
    public: true
    override_path: "../../../i2c_bus"
  
  sensor_hub:
    public: true
    override_path: "../../sensor_hub"

  idf: ">=4.0"

I think the problem is here, aht20 CMakeLists.txt:

idf_component_register(SRCS "aht20.c"
                    INCLUDE_DIRS "include"
                    REQUIRES "sensor_hub")
                    

if(CONFIG_SENSOR_INCLUDED_HUMITURE)
    target_link_libraries(${COMPONENT_LIB} INTERFACE "-u humiture_aht20_init")
endif()

include(package_manager)
cu_pkg_define_version(${CMAKE_CURRENT_LIST_DIR})`


& in particular with this part:

`if(CONFIG_SENSOR_INCLUDED_HUMITURE)
    target_link_libraries(${COMPONENT_LIB} INTERFACE "-u humiture_aht20_init")
endif()

There are no proper instructions, what should be here:
target_link_libraries(${COMPONENT_LIB} INTERFACE "-u humiture_aht20_init")
in particular this:
-u humiture_aht20_init

and if you will read the error:
it mentions about idf_component_register in Cmakelists.txt file not idf_component.yml

This is test app CMakelists.txt:
idf_component_register(SRCS "aht20_Sensor_Hub_Humiture.c" INCLUDE_DIRS "." PRIV_REQUIRES unity test_utils aht20 sensor_hub)

& its idf_component.yml :
dependencies: aht20: version: "*" override_path: "../../../" sensor_hub: public: true override_path: "../../../../../sensor_hub"

[YanKE01]

I think you should add the dependency for sensor_hub; you can refer to the idf_component.yml of SHT3X:

version: "0.2.0"
description: I2C driver for SHT3X humiture sensor
url: https://github.com/espressif/esp-iot-solution/tree/master/components/sensors/humiture/sht3x
repository: https://github.com/espressif/esp-iot-solution.git
documentation: https://docs.espressif.com/projects/esp-iot-solution/en/latest/sensors/humiture.html
issues: https://github.com/espressif/esp-iot-solution/issues
dependencies:
  i2c_bus:
    public: true
    override_path: "../../../i2c_bus"
  sensor_hub:
    public: true
    override_path: "../../sensor_hub"
  idf: '>=4.4'
  cmake_utilities: "0.*"

@YanKE01 It was already like that

ah20 idf._component.yml:

name: aht20

version: "1.1.0"

author: Rohan Jeet <[email protected]>

description: AHT20 Temperature and Humidity Sensor I2C Driver for ESP-IDF

issues: https://github.com/espressif/esp-iot-solution/issues

repository: git://github.com/espressif/esp-iot-solution.git

url: https://github.com/espressif/esp-iot-solution/tree/master/components/sensors/humiture/aht20

dependencies:

  i2c_bus:
    public: true
    override_path: "../../../i2c_bus"
  
  sensor_hub:
    public: true
    override_path: "../../sensor_hub"

  idf: ">=4.0"

I think the problem is here, aht20 CMakeLists.txt:

idf_component_register(SRCS "aht20.c"
                    INCLUDE_DIRS "include"
                    REQUIRES "sensor_hub")
                    

if(CONFIG_SENSOR_INCLUDED_HUMITURE)
    target_link_libraries(${COMPONENT_LIB} INTERFACE "-u humiture_aht20_init")
endif()

include(package_manager)
cu_pkg_define_version(${CMAKE_CURRENT_LIST_DIR})`


& in particular with this part:

`if(CONFIG_SENSOR_INCLUDED_HUMITURE)
    target_link_libraries(${COMPONENT_LIB} INTERFACE "-u humiture_aht20_init")
endif()

There are no proper instructions, what should be here: target_link_libraries(${COMPONENT_LIB} INTERFACE "-u humiture_aht20_init") in particular this: -u humiture_aht20_init

and if you will read the error: it mentions about idf_component_register in Cmakelists.txt file not idf_component.yml

This is test app CMakelists.txt: idf_component_register(SRCS "aht20_Sensor_Hub_Humiture.c" INCLUDE_DIRS "." PRIV_REQUIRES unity test_utils aht20 sensor_hub)

& its idf_component.yml : dependencies: aht20: version: "*" override_path: "../../../" sensor_hub: public: true override_path: "../../../../../sensor_hub"

Perhaps support for the sensor hub can be added in a separate PR next time, as this PR already includes too many changes.

[jeetrohan]

I think you should add the dependency for sensor_hub; you can refer to the idf_component.yml of SHT3X:

version: "0.2.0"
description: I2C driver for SHT3X humiture sensor
url: https://github.com/espressif/esp-iot-solution/tree/master/components/sensors/humiture/sht3x
repository: https://github.com/espressif/esp-iot-solution.git
documentation: https://docs.espressif.com/projects/esp-iot-solution/en/latest/sensors/humiture.html
issues: https://github.com/espressif/esp-iot-solution/issues
dependencies:
  i2c_bus:
    public: true
    override_path: "../../../i2c_bus"
  sensor_hub:
    public: true
    override_path: "../../sensor_hub"
  idf: '>=4.4'
  cmake_utilities: "0.*"

@YanKE01 It was already like that
ah20 idf._component.yml:

name: aht20

version: "1.1.0"

author: Rohan Jeet <[email protected]>

description: AHT20 Temperature and Humidity Sensor I2C Driver for ESP-IDF

issues: https://github.com/espressif/esp-iot-solution/issues

repository: git://github.com/espressif/esp-iot-solution.git

url: https://github.com/espressif/esp-iot-solution/tree/master/components/sensors/humiture/aht20

dependencies:

  i2c_bus:
    public: true
    override_path: "../../../i2c_bus"
  
  sensor_hub:
    public: true
    override_path: "../../sensor_hub"

  idf: ">=4.0"

I think the problem is here, aht20 CMakeLists.txt:

idf_component_register(SRCS "aht20.c"
                    INCLUDE_DIRS "include"
                    REQUIRES "sensor_hub")
                    

if(CONFIG_SENSOR_INCLUDED_HUMITURE)
    target_link_libraries(${COMPONENT_LIB} INTERFACE "-u humiture_aht20_init")
endif()

include(package_manager)
cu_pkg_define_version(${CMAKE_CURRENT_LIST_DIR})`


& in particular with this part:

`if(CONFIG_SENSOR_INCLUDED_HUMITURE)
    target_link_libraries(${COMPONENT_LIB} INTERFACE "-u humiture_aht20_init")
endif()

There are no proper instructions, what should be here: target_link_libraries(${COMPONENT_LIB} INTERFACE "-u humiture_aht20_init") in particular this: -u humiture_aht20_init
and if you will read the error: it mentions about idf_component_register in Cmakelists.txt file not idf_component.yml
This is test app CMakelists.txt: idf_component_register(SRCS "aht20_Sensor_Hub_Humiture.c" INCLUDE_DIRS "." PRIV_REQUIRES unity test_utils aht20 sensor_hub)
& its idf_component.yml : dependencies: aht20: version: "*" override_path: "../../../" sensor_hub: public: true override_path: "../../../../../sensor_hub"

Perhaps support for the sensor hub can be added in a separate PR next time, as this PR already includes too many changes.

Thanks @YanKE01.
None of this was issue...
The problem was adding the header itself, only the sensor hub header should be used.
Fixed it all...
Added new example for Sensor Hub and a new test app using humiture, because I am facing problems with sensor hub based test app..
and I have no intentions of delving into that currently.
I'll be updating it very soon here and make a request for final review.

[jeetrohan]

Hi @YanKE01 ,
Final update (hopefully is here).
I suppose you can now officially call this as v2.0 or something? at least not v1.1.0.
And I don't know about you, but I'd like to Rest In Peace, now 😅 😅 .
(Actually not, since I will be doing the MPU6050 which is a real dread (no joke) and I've been avoiding for months.)

[jeetrohan]

Hi @YanKE01,
Please review the final submission.
And I added Sensor Hub support in this PR itself, because... 😅 lets just finish this...
AHT20 is a simple sensor with not much to do.
The next two submissions will require much of your attention.
Once this is closed I'll open a new issue regarding those, one after previous resolved.. .
I hope we can merge this soon.
Thanks.

PS: and while you are at it.. could you ask leeebo to respond to your previous query too(regarding examples)?

[jeetrohan]

Hi @YanKE01 and @leeebo
I've checked the documentation and retested the codes.
The driver works both as a standalone and also integrated into Sensor Hub.
There may be some possible typos or something like that but everything else is working or possible minor updates.
If any further changes are required, let me know.

I think we have spent more than a month on this simple driver and now I would be happy to see it integrated in the repository.
And also, I would like to move on to the next contribution, which requires discussion before I can open a PR.

Let me know if there are any remaining issues.

I've already provided a explanation to all the issues raised earlier in this PR, in context of standalone use of driver.
I could also provide reasoning for use in Sensor Hub for them, but that requires some changes, both in driver and Sensor Hub itself. Now this is something for long term.

So either I can drop all the particular APIs and refactors I've made to this driver.
Or
We can chalk it all down to future prospects and leave it for some one who might find them useful.
Thanks

PS:
@YanKE01 I haven't said it yet, but thank you for your support in this PR. It was a good experience for me in many ways.

[jeetrohan]

Hi @YanKE01 and @leeebo

I've refactored the code to original, i.e. now this PR only changes the I2C interface and corresponding parts in original driver.
No conflicts of any sort.
If this is also not sufficient, I can close this PR and open a new one with these changes only.

Its easier to solve problems when they are simple, but its more easier to ignore them when they're simple (context : the updates originally suggested in this PR). But I'm assuming there are better solutions, and this was one was not implemented to full extent, primarily to not cause any conflict with Sensor Hub as it is now.

[YanKE01]

Hi, @jeetrohan .Thank you very much for your contribution and for testing the sensor_hub. I have no additional comments after completing this review.

[jeetrohan]

Hi, @jeetrohan .Thank you very much for your contribution and for testing the sensor_hub. I have no additional comments after completing this review.

@YanKE01 is it now acceptable for the merge?

[YanKE01]

Hi, @jeetrohan .Thank you very much for your contribution and for testing the sensor_hub. I have no additional comments after completing this review.

@YanKE01 is it now acceptable for the merge?

We still need to wait for reviews from other relevant personnel. In the meantime, I think it would be good to squash the commit messages — there are too many at the moment.

[jeetrohan]

Hi, @jeetrohan .Thank you very much for your contribution and for testing the sensor_hub. I have no additional comments after completing this review.

@YanKE01 is it now acceptable for the merge?

We still need to wait for reviews from other relevant personnel. In the meantime, I think it would be good to squash the com2mit messages — there are too many at the moment.

Edit:
I think only two commit message have any relevance. I've removed any and all possibly conflicting changes completely.
The current submission is just a refactor of original AHT20 driver and now uses i2c_bus component.. nothing else.
these are :
57caca2
and
e9727a7

@YanKE01
I'll try to squash the commit messages (haven't done it before ever.) and keep only the relevant ones.
Please keep me posted.
Once we have done a few more drivers I'll demonstrate the solution I mentioned earlier, and why its needed, in code myself, but first we need to reach there.