zboss: Add ZBOSS library #166

maciekfabia · 2020-05-05T22:13:17Z

Add ZBOSS Zigbee stack version 3.3

zboss/CMakeLists.txt

tejlmand · 2020-05-06T06:59:24Z

zboss/CMakeLists.txt

+elseif(CONFIG_ZBOSS_LIBRARY_SOURCES)
+  # Read ZOI platform repository path from west manifest file.
+  execute_process(
+    COMMAND
+    ${WEST} list -f {posixpath} manifest
+    OUTPUT_VARIABLE ZOI_PLATFORM_SOURCE_DIR
+    )
+  # Trim a trailing whitespace.
+  string(STRIP ${ZOI_PLATFORM_SOURCE_DIR} ZOI_PLATFORM_SOURCE_DIR)
+
+  # Include ZBOSS build CMake file.
+  if(EXISTS "${ZOI_PLATFORM_SOURCE_DIR}/CMakeLists.txt")
+    include("${ZOI_PLATFORM_SOURCE_DIR}/CMakeLists.txt")
+  else()
+    message( FATAL_ERROR "Unable to find ZOI platform repository. Please checkout repositories from ZOI manifest file to recover. Exiting." )
+  endif()


A couple of comments to this:

Does users of NCS in general have access to the ZBoss source code ?
if not, then I would like to get this removed from code provided in NCS.

Remove the west list here, west is an optional tool, instead the ZOI_PLATFORM_SOURCE should be a Zephyr module and Zephyr will add_subdirectory() on the folder provided in <ZOI_PLATFORM_SOURCE>/zephyr/module.yml, and that code could do inclusion, as well as provide additional KConfig settings.
Also it allows for the usage of: ZEPHYR_ZOI_MODULE_DIR or whatever name the module is given in the manifest file.

See more: https://docs.zephyrproject.org/latest/guides/modules.html

@tejlmand Only if they are members of ZBOSS Open Initative. What we'd like to achieve is to have the possibility to use the same NCS source for either library or source ZBOSS builds. This is simpler for users and simpler for us (maintenance, CI, etc.).

I'll let @tomchy comment on the modules feature but as far as I remember we didn't see an elegant way to pull in the ZBOSS sources conditionally, i.e., give the user option to use ZBOSS source and its Kconfig when available. Do you think what you propose will help?

@tejlmand as you may anticipated, we did not find the solution to make both - conditional Kconfg and CMakeLists.txt includes. The Zephyr modules feature allows us to do that (the ZOI main manifest repository may be a module), we have missed that.

This leaves us with two remaining issues:

We still have to provide Kconfig options, that were available during stack compilation, so the common set of headers will select correct ZBOSS defines (please take a look at the {nrfxlib_zboss}/include/osif/zb_vendor.h file). This is the reason behind using visible if ZBOSS_LIBRARY_SOURCES instead of a simple if inside Kconfig.sources file. My proposition to tackle this issue is to export relevant Kconfig defines to a separate file as an artifact of ZBOSS build command and add the following line inside nrfxlib to pull it in:

toolchain_cc_imacros(${CMAKE_CURRENT_SOURCE_DIR}/include/osif/libzboss_config.h)

We still have to conditionally remove CMakeLists commands under if (CONFIG_ZBOSS_LIBRARY_PRECOMPILED). If we go by the path of removing all ZBOSS-source-related things from nrfxlib, the Kconfig file becomes empty and we need to add something to conditionally exclude lines from CMakeLists.txt, without Kconfig options. My proposition here is to use the path of the module as the switch:

if ((NOT DEFINED ZEPHYR_PLATFORM_NCS_MODULE_DIR) OR (CONFIG_ZBOSS_LIBRARY_PRECOMPILED))

Additional note: using the Kconfig inside both nrfxlib and defining ZOI module will make the configuration counter-intuitive, with the following structure:

Zephyr Kernel Configuration | + Modules. ---> | | | + ZBOSS stack configuration. ---> | | | + Nordic nrfxlib --> | | | | | + ZBOSS stack configuration. ---> | | | + Nordic nRF Connect ---> | | | + Enable Zigbee stack --->

This is the main reason, why I'm voting for making the nrfxlib's Kconfig file empty if the source-related options are removed from nrfxlib.

What do you think about my proposals?

Regarding:

We still have to provide Kconfig options, that were available during stack compilation, so the common set of headers will select correct ZBOSS defines (please take a look at the {nrfxlib_zboss}/include/osif/zb_vendor.h file). This is the reason behind using visible if ZBOSS_LIBRARY_SOURCES instead of a simple if inside Kconfig.sources file. My proposition to tackle this issue is to export relevant Kconfig defines to a separate file as an artifact of ZBOSS build command and add the following line inside nrfxlib to pull it in:

This appears very similar to a mbedTLS config file.
You could generated this file in same fashion as is done for mbedTLS, based on the Kconfig settings when you compile from source, and provide a static conf file that is part of the nrfxlib.
(i'm under the impression that this header file is static for users using the precompiled lib, correct ?)

https://github.com/NordicPlayground/nrfxlib/blob/master/nrf_security/cmake/kconfig_mbedtls_configure.cmake#L476-L479

Thank you for your proposal, this looks like an elegant solution for us.

At some point in time, we will need at least two dimensional diversity of ZBOSS libraries:

Debug/trace-enabled and release/size-optimized

Single role-optimized and multi role

The file itself is bound to the library variant, but the variant depends on the application's configuration.

As agreed, I will try to come up with some kind of cmake's config_file template and add the output to this PR. The possibility to reduce the zb_vendor.h file is definitely a good optimization, but I am not sure if we can fit it in the timeline.

Kconfig pruned from source-code related options, zb_vendor.h left as-is, libzboss_config.h generated by the config_file command.

tejlmand · 2020-05-06T07:02:00Z

zboss/Kconfig

+# Add a menu option to switch between compiled libraries and source code.
+choice
+	prompt "ZBOSS library type"
+	default ZBOSS_LIBRARY_PRECOMPILED
+
+config ZBOSS_LIBRARY_PRECOMPILED
+	bool "Libraries"
+	help
+	  Use ZBOSS compiled libraries while building Zigbee application
+
+config ZBOSS_LIBRARY_SOURCES
+	bool "Source code"
+	help
+	  Use ZBOSS source code while building Zigbee application
+
+endchoice


This should be adjusted based on the outcome of: #166 (comment)

Moved to NCS platform repository.

tejlmand · 2020-05-06T07:02:37Z

zboss/Kconfig.sources

+# This file defines options, that are used to set configurable entries of zb_vendor.h and zb_config.h platform files.
+# It will be used only if ZBOSS source code is available.


Should be part of a Zephyr module.

Moved to NCS platform repository.

tejlmand · 2020-05-06T07:05:17Z

zboss/Kconfig.sources

+# It will be used only if ZBOSS source code is available.
+
+menu "ZBOSS features"
+	visible if ZBOSS_LIBRARY_SOURCES


This only remove visibility in limited view in Kconfig.
If user toggles show-all in menuconfig, then those settings can still be viewed.

The reasoning behind this is explained in: #166 (comment)

Changed to regular if in NCS platform repository.

tomchy

I will prepare some patches to remove source code-related options and improve the config-generation process.
Please fix minor, coding style issues.

zboss/CMakeLists.txt

tomchy · 2020-05-06T17:52:16Z

zboss/CMakeLists.txt

+elseif(CONFIG_ZBOSS_LIBRARY_SOURCES)
+  # Read ZOI platform repository path from west manifest file.
+  execute_process(
+    COMMAND
+    ${WEST} list -f {posixpath} manifest
+    OUTPUT_VARIABLE ZOI_PLATFORM_SOURCE_DIR
+    )
+  # Trim a trailing whitespace.
+  string(STRIP ${ZOI_PLATFORM_SOURCE_DIR} ZOI_PLATFORM_SOURCE_DIR)
+
+  # Include ZBOSS build CMake file.
+  if(EXISTS "${ZOI_PLATFORM_SOURCE_DIR}/CMakeLists.txt")
+    include("${ZOI_PLATFORM_SOURCE_DIR}/CMakeLists.txt")
+  else()
+    message( FATAL_ERROR "Unable to find ZOI platform repository. Please checkout repositories from ZOI manifest file to recover. Exiting." )
+  endif()


Thank you for your proposal, this looks like an elegant solution for us.

At some point in time, we will need at least two dimensional diversity of ZBOSS libraries:

Debug/trace-enabled and release/size-optimized

Single role-optimized and multi role

The file itself is bound to the library variant, but the variant depends on the application's configuration.

As agreed, I will try to come up with some kind of cmake's config_file template and add the output to this PR. The possibility to reduce the zb_vendor.h file is definitely a good optimization, but I am not sure if we can fit it in the timeline.

zboss/Kconfig

NordicBuilder · 2020-05-11T19:18:18Z

All checks are passing now.

Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages.

CLAassistant · 2020-05-12T10:20:20Z

All committers have signed the CLA.

tejlmand

Much better.
Thanks for the update.

Still a couple of small improvements to do.

tejlmand · 2020-05-14T08:51:51Z

zboss/CMakeLists.txt

+zephyr_include_directories(include/addons)
+
+# Source files
+zephyr_sources(src/zb_error/zb_error_to_string.c)


Are there any reason why those files are going to libzephyr.a ?

I think there should be a zephyr_library() call, and then all additional include directories, compile definitions, etc. should be using zephyr_libary_x() instead, unless there are special needs to get the flag used by all Zephyr libraries.

The reason is quite simple:
The zephyr_library_* extension is mentioned and documented only in the CMake source code (at least - I cannot find it elsewhere). This makes it hard to find, understand and use. The only examples you may find is to go through other modules and try to find something similar to your needs. It would be great if you can point me to the tutorial on how to use zephyr_library_* extensions.
On the other hand - if you try to use "raw" CMake commands (fall-back to what you can find on the Internet), it will fail to link to the default zephyr target.

This should not be a surprise that, becasue zephyr_sources/include_directories/link_libraries functions are easiest to use, they are the first choice for a developer with small experience in Zephyr's extensions to CMake files.

From what I see - our project needs to define at least three libraries. I hope that this approach will meet your requirements.

Current proposal can be viewed here:
https://github.com/maciekfabia/nrfxlib/pull/2/files

The reason is quite simple:
The zephyr_library_* extension is mentioned and documented only in the CMake source code (at least - I cannot find it elsewhere). This makes it hard to find, understand and use. The only examples you may find is to go through other modules and try to find something similar to your needs. It would be great if you can point me to the tutorial on how to use zephyr_library_* extensions.

Agree, and I actually raised this 2 years ago: zephyrproject-rtos/zephyr#9947
seems it was closed (resolved) without being properly resolved.

Reopened now.

On the other hand - if you try to use "raw" CMake commands (fall-back to what you can find on the Internet), it will fail to link to the default zephyr target.

I agree, which is why I raised this almost 2 years ago:
zephyrproject-rtos/zephyr#8439

but until that is resolved, we have to stick to zephyr cmake wrappers.

Current proposal can be viewed here:
https://github.com/maciekfabia/nrfxlib/pull/2/files

Good direction 👍
Placed some comments in there.

zboss/CMakeLists.txt

tejlmand · 2020-05-14T10:56:54Z

zboss/CMakeLists.txt

+
+  if (CONFIG_ZIGBEE_ROLE_COORDINATOR OR CONFIG_ZIGBEE_ROLE_ROUTER)
+    zephyr_link_libraries(${ZBOSS_LIB_PATH}/libzboss.a)
+    toolchain_cc_imacros(${CMAKE_CURRENT_SOURCE_DIR}/include/osif/libzboss_config.h)


Why do we want this as a imacro getting included everywhere ?
Should only be included by those that needs it ?
As this is config settings, would it not be possible to set a conf file in the zboss headers, similar to how mbedTLs does things.

The imacro will cause this to be used for all C-files, which should really be for really global things like the autoconf.h

Well - it replicates exactly what is happening when you have source code with the corresponding Kconfig file, which brings us to a "unified" approach for all build types.

In precompiled builds, those defines are needed by:

Zigbe subsys

Zigbee applications (those with CONFIG_ZIGBEE == y)

Zigbee API test cases (those with CONFIG_ZIGBEE == y)

Additionally, this header must not be included if the ZBOSS source code is available.

If you look at those cases, you should see that the "imacro" approach is the simplest and shortest one you can imagine.

If we need to fix this, than yes, this should be pulled in via the default #include <zboss_api.h> directive. For example:

// file: zb_vendor.h #include "libzboss_config_generic.h"

and - pulled in in a way, that it gets removed in source code builds:

// file: libzboss_config_generic.h.template #cmakedefine LIBZBOSS_CONFIG_FILE #ifdef LIBZBOSS_CONFIG_FILE #include "${LIBZBOSS_CONFIG_FILE}" #endif

What do you think about this approach?

If you look at those cases, you should see that the "imacro" approach is the simplest and shortest one you can imagine.

The shortest way is not always the safest ;)
This works when using gcc, but there are other toolchains out there which doesn't support imacros.
As well as the previous comment, (and strongest reason), there is no reason to expand this header when compiling Zephyr kernel.

What do you think about this approach?

Good direction, but I think can be reduced and still achieve same goal.
Placed some suggestions:
maciekfabia#2 (review)

Add ZBOSS library providing a Zigbee stack Signed-off-by: Maciej Fabia <maciej.fabia@nordicsemi.no>

Add owner for ZBOSS Zigbee stack Signed-off-by: Maciej Fabia <maciej.fabia@nordicsemi.no>

tomchy

All changes proposed and discussed in the child-PR are now merged in.
Internal CI for the alternative, source code -based build was successful passed a simple test suite (Build 38).

@tejlmand could you check if all requested changes are made and comments were addressed?

@rlubos @carlescufi This PR has been significantly improved over the last week, could you take a look at it? It is a bit scary if you look at the number of files added, but most of them are imported from external project, so there is not much to go through 🙂

rlubos · 2020-05-18T11:30:27Z

@tomchy I trust @tejlmand in terms of CMake/Kconfig integration since he has way more knowledge about the build system than I do. IMO once he approves, we could merge this PR (since fw-nrfconnect-nrf` counterpart taht updates the manifest file is also green).

tejlmand

lgtm

wbober · 2020-05-19T07:35:01Z

@carlescufi Can we merge this please? It would unblock a lot of tasks for us.

rlubos · 2020-05-19T08:43:49Z

@wbober Let's wait until nrfconnect/sdk-nrf#2208 review is finished, so we won't delay the manifest update. There shouldn't be any further delays here, I'll merge as soon as we're done in 2208.

maciekfabia requested review from carlescufi and tejlmand as code owners May 5, 2020 22:13

maciekfabia mentioned this pull request May 5, 2020

zigbee: Add zigbee support nrfconnect/sdk-nrf#2208

Merged

tejlmand requested a review from tomchy May 6, 2020 06:19

tejlmand requested changes May 6, 2020

View reviewed changes

rlubos self-requested a review May 6, 2020 08:48

tomchy requested changes May 6, 2020

View reviewed changes

tejlmand reviewed May 6, 2020

View reviewed changes

zboss/Kconfig Outdated Show resolved Hide resolved

maciekfabia force-pushed the add_zboss_library branch from 5fd10f3 to 3dfbb19 Compare May 11, 2020 19:10

maciekfabia force-pushed the add_zboss_library branch from 3dfbb19 to a487b57 Compare May 11, 2020 19:42

tomchy approved these changes May 11, 2020

View reviewed changes

maciekfabia force-pushed the add_zboss_library branch 2 times, most recently from 1e288d2 to d63024d Compare May 13, 2020 12:59

tejlmand requested changes May 14, 2020

View reviewed changes

maciekfabia force-pushed the add_zboss_library branch from d63024d to 6e26bcf Compare May 17, 2020 22:01

maciekfabia added 2 commits May 18, 2020 10:48

zboss: Add the ZBOSS library

15ec8d5

Add ZBOSS library providing a Zigbee stack Signed-off-by: Maciej Fabia <maciej.fabia@nordicsemi.no>

codeowners: Add owner for ZBOSS library

923d027

Add owner for ZBOSS Zigbee stack Signed-off-by: Maciej Fabia <maciej.fabia@nordicsemi.no>

maciekfabia force-pushed the add_zboss_library branch from 6e26bcf to 923d027 Compare May 18, 2020 08:49

tomchy approved these changes May 18, 2020

View reviewed changes

tejlmand approved these changes May 18, 2020

View reviewed changes

rlubos approved these changes May 19, 2020

View reviewed changes

tejlmand merged commit 0b0049b into nrfconnect:master May 19, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

zboss: Add ZBOSS library #166

zboss: Add ZBOSS library #166

maciekfabia commented May 5, 2020

tejlmand May 6, 2020

wbober May 6, 2020

tomchy May 6, 2020 •

edited

Loading

tejlmand May 6, 2020 •

edited

Loading

tomchy May 6, 2020

tomchy May 11, 2020

tejlmand May 6, 2020 •

edited

Loading

tomchy May 11, 2020

tejlmand May 6, 2020

tomchy May 11, 2020

tejlmand May 6, 2020

tomchy May 6, 2020

tomchy May 11, 2020

tomchy left a comment

tomchy May 6, 2020

NordicBuilder commented May 11, 2020 •

edited

Loading

CLAassistant commented May 12, 2020 •

edited

Loading

tejlmand left a comment

tejlmand May 14, 2020

tomchy May 14, 2020

tomchy May 14, 2020

tejlmand May 15, 2020

tejlmand May 15, 2020

tejlmand May 14, 2020

tomchy May 14, 2020

tejlmand May 15, 2020

tomchy left a comment •

edited

Loading

rlubos commented May 18, 2020

tejlmand left a comment

wbober commented May 19, 2020

rlubos commented May 19, 2020

		# This file defines options, that are used to set configurable entries of zb_vendor.h and zb_config.h platform files.
		# It will be used only if ZBOSS source code is available.

zboss: Add ZBOSS library #166

zboss: Add ZBOSS library #166

Conversation

maciekfabia commented May 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomchy May 6, 2020 • edited Loading

Choose a reason for hiding this comment

tejlmand May 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tejlmand May 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomchy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NordicBuilder commented May 11, 2020 • edited Loading

CLAassistant commented May 12, 2020 • edited Loading

tejlmand left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomchy left a comment • edited Loading

Choose a reason for hiding this comment

rlubos commented May 18, 2020

tejlmand left a comment

Choose a reason for hiding this comment

wbober commented May 19, 2020

rlubos commented May 19, 2020

tomchy May 6, 2020 •

edited

Loading

tejlmand May 6, 2020 •

edited

Loading

tejlmand May 6, 2020 •

edited

Loading

NordicBuilder commented May 11, 2020 •

edited

Loading

CLAassistant commented May 12, 2020 •

edited

Loading

tomchy left a comment •

edited

Loading