Make the library a Zephyr module

[b0661]

Introduction

As a lot of IoT applications (at least mine !-) are using the Zepyhr RTOS a Zephyr module would ease the library use.

Problem description

There is no Zephyr module support in the ThingSet device library code base.

Proposed change

Provide it as a Zephyr module.

Proposed change (Detailed)

Create a zephyr sub-directory to contain all the Zephyr specific module configuration and additional code that is necessary for Zephyr only. Add some libc functions to this code that the Zephyr minimal libc does (currently) not provide to make the library usable without newlib.

EDIT: Also add all available C++ unit test as C unit tests to the zephyr sub-directory. This assures C++ and C implementation is working properly.

EDIT: ~~As the library is also used by ESP-IDF based applications provide it as a ESP-IDF component too.~~ Lock CMakeLists.txt files for Zepyhr ~~and for ESP-IDF against each other to prevent each to use the wrong one.~~

Dependencies

Unknown

Concerns and Unresolved Questions

~~As I am currently not using ESP IDF the integration is untested. Kconfig ist also missing for ESP IDF.~~

Platform IO issues are expected. Only native-std is tested.

Alternatives

N/A

[b0661]

I found some quirks and bugs while implementing the unit tests. Now all C++ and C unit tests have status passed. The C unit tests are implemented using ztest as they are targeting Zephyr.

Here is the latest test coverage data.

The new unit test application zephyr/tests provides hints on how to create the test coverage data (see README.txt). Also it shows how to integrated the thingset library as a Zephyr module (see CMakeLists.txt).

[b0661]

Regarding unit tests: I don't think it's a good idea to duplicate the unit tests. In the end we have to decide for one way to run the tests.

You are right for the library core - it doesn't matter what test framework to use. I'm just not used to Unity and ztest works out of the box in the Zephyr build environment (like Unity in PlatformIO I guess). As I could easily transfer from Unity to ztest the differences may be more in some HW/ RTOS areas.

I will investigate whether with some macro mocking the unit tests may be executed using Unity or ztest. I think you prefer the Unity style assert macros?

[martinjaeger]

I will investigate whether with some macro mocking the unit tests may be executed using Unity or ztest. I think you prefer the Unity style assert macros?

Yes, I would prefer the Unity macros as they have more features and are more independent from Zephyr, as far as I understand. I will also have a look how both could be integrated.

BTW: I took the chance to rename the master branch to main before we merge the Zephyr module and link to it in our firmwares. The "old" master is still there to stay compatible with previous firmware that imported the library as a submodule.

[b0661]

Yes, I would prefer the Unity macros as they have more features and are more independent from Zephyr.

Commit 100b247 re-introduces the Unity macros. All tests are now using the Unity macros. There is an adaptation for ztest macros. I added a set of more complex ThingSeit specific macros to ease the writing of test functions and avoid duplication.

Regarding unit tests: I don't think it's a good idea to duplicate the unit tests. In the end we have to decide for one way to run the tests.

There is now only one test suite. The test applications may be either Unity based or zephyr based. The two test applications (test/main.cpp and zephyr/tests/src/main.c) are working properly.

[martinjaeger]

Thanks, this looks great. Also the new macros for less duplication are nice.

However, as the test file names have been renamed, PR #11 will be quite difficult to rebase. Also renaming files and making considerable changes in the same commit makes it very difficult to review the changes. I suggest to amend the last commit and keep the .cpp names (even though the code is plain C) for this PR. This allows me to quite easily rebase PR #11 after we merge this PR. After both PRs are in we can rename the files. What do you think?

Can you also please take out the ESP-IDF part as suggested above and rebase to main (where I added a commit to fix the CI)? Afterwards I will do another thorough review.

[b0661]

However, as the test file names have been renamed, PR #11 will be quite difficult to rebase. Also renaming files and making considerable changes in the same commit makes it very difficult to review the changes. I suggest to amend the last commit and keep the .cpp names (even though the code is plain C) for this PR. This allows me to quite easily rebase PR #11 after we merge this PR. After both PRs are in we can rename the files. What do you think?

I added two commits:

• 1/2: Does all the changes but keeps the file names
• 2/2: Just does the rename of the files.

In case you want to merge PR #11 you can rebase on 1/2 (59b56c7) and apply 2/2 ( d5eb6d6) after all your changes or do it within your changes. This way the branch stays workable and I hope you can better review commit 59b56c7.

Can you also please take out the ESP-IDF part as suggested above

Done.

rebase to main

Done

[martinjaeger]

Ok, thanks a lot for splitting the commit which helped a lot to do the review. I didn't find anything of concern, thanks again for the great work. The unit tests are much cleaner now.

Regarding PR #11: I'll have to update some of the tests anyway to use the new unified functions, so I'll just merge this PR including the file name changes now and then update the tests in the other PR during the rebase.

Could you quickly squash the last two commits into one again, as the first one would otherwise has wrong file names in the CMakeLists.txt so it would not compile during a git bisect? Afterwards I'll merge.

[martinjaeger]

@azeemshatp and @daniel-connectedenergy let me know if you have any concerns. The library is still compatible with PlatformIO as PlatformIO just compiles all files it finds under the src directory and ignores the CMakeLists.txt anyway. For Zephyr module integration via west.yml a few small changes are necessary, but I have already prepared them in the commit as mentioned above.

[b0661]

Could you quickly squash the last two commits into one again, as the first one would otherwise has wrong file names in the CMakeLists.txt so it would not compile during a git bisect?

Done.

Also fixed the sample application. Forgot about that before.

[daniel-connectedenergy]

@azeemshatp and @daniel-connectedenergy let me know if you have any concerns. The library is still compatible with PlatformIO as PlatformIO just compiles all files it finds under the src directory and ignores the CMakeLists.txt anyway. For Zephyr module integration via west.yml a few small changes are necessary, but I have already prepared them in the commit as mentioned above.

Thanks for the excellent work guys. I didn't have time to look at this thoroughly and I don't care at all about PlatformIO.