zigbee: Add zigbee support #2208

maciekfabia · 2020-05-05T23:54:40Z

Add Zigbee support with accompanying samples.
It uses ZBOSS stack that has a separate pull request:
nrfconnect/sdk-nrfxlib#166

NordicBuilder · 2020-05-05T23:57:35Z

All checks are passing now.

checkpatch (informational only, not a failure)

No additional types will be considered - file 'scripts/checkpatch/typedefsfile': No such file or directory
-:591: WARNING:NEW_TYPEDEFS: do not add new typedefs
#591: FILE: samples/zigbee/light_bulb/src/main.c:106:
+typedef struct {

-:908: WARNING:MULTILINE_DEREFERENCE: Avoid multiple line dereference - prefer 'device_cb_param->cb_param.set_attr_value_param.cluster_id'
#908: FILE: samples/zigbee/light_bulb/src/main.c:423:
+		cluster_id = device_cb_param->cb_param.
+			     set_attr_value_param.cluster_id;

-:910: WARNING:MULTILINE_DEREFERENCE: Avoid multiple line dereference - prefer 'device_cb_param->cb_param.set_attr_value_param.attr_id'
#910: FILE: samples/zigbee/light_bulb/src/main.c:425:
+		attr_id = device_cb_param->cb_param.
+			  set_attr_value_param.attr_id;

-:923: WARNING:MULTILINE_DEREFERENCE: Avoid multiple line dereference - prefer 'device_cb_param->cb_param.set_attr_value_param.values.data16'
#923: FILE: samples/zigbee/light_bulb/src/main.c:438:
+			u16_t value = device_cb_param->cb_param.
+				      set_attr_value_param.values.data16;

-:1372: WARNING:LONG_LINE_STRING: line over 80 characters
#1372: FILE: samples/zigbee/network_coordinator/src/main.c:136:
+			LOG_ERR("Failed to initialize Zigbee stack using NVRAM data (status: %d)",

-:1380: WARNING:LONG_LINE_STRING: line over 80 characters
#1380: FILE: samples/zigbee/network_coordinator/src/main.c:144:
+				LOG_INF("Allow pre-Zigbee 3.0 devices to join the network");

-:1941: WARNING:LONG_LINE_STRING: line over 80 characters
#1941: FILE: subsys/zigbee/common/zigbee_helpers.c:203:
+			LOG_INF("Production configuration is not present or invalid (status: %d)",

-:1988: WARNING:LONG_LINE_STRING: line over 80 characters
#1988: FILE: subsys/zigbee/common/zigbee_helpers.c:250:
+			LOG_ERR("Failed to initialize Zigbee stack (status: %d)",

-:2021: WARNING:LONG_LINE_STRING: line over 80 characters
#2021: FILE: subsys/zigbee/common/zigbee_helpers.c:283:
+			LOG_INF("Joined network successfully on reboot signal (Extended PAN ID: %s, PAN ID: 0x%04hx)",

-:2026: WARNING:LONG_LINE_STRING: line over 80 characters
#2026: FILE: subsys/zigbee/common/zigbee_helpers.c:288:
+				LOG_INF("Unable to join the network, start network steering");

-:2029: WARNING:LONG_LINE_STRING: line over 80 characters
#2029: FILE: subsys/zigbee/common/zigbee_helpers.c:291:
+				LOG_ERR("Failed to initialize Zigbee stack using NVRAM data (status: %d)",

-:2061: WARNING:LONG_LINE_STRING: line over 80 characters
#2061: FILE: subsys/zigbee/common/zigbee_helpers.c:323:
+			LOG_INF("Joined network successfully (Extended PAN ID: %s, PAN ID: 0x%04hx)",

-:2072: WARNING:LONG_LINE_STRING: line over 80 characters
#2072: FILE: subsys/zigbee/common/zigbee_helpers.c:334:
+				LOG_INF("Network steering was not successful (status: %d)",

-:2076: WARNING:LONG_LINE_STRING: line over 80 characters
#2076: FILE: subsys/zigbee/common/zigbee_helpers.c:338:
+				LOG_INF("Network steering failed on Zigbee coordinator (status: %d)",

-:2115: WARNING:LONG_LINE_STRING: line over 80 characters
#2115: FILE: subsys/zigbee/common/zigbee_helpers.c:377:
+			LOG_INF("Network formed successfully, start network steering (Extended PAN ID: %s, PAN ID: 0x%04hx)",

-:2215: WARNING:LONG_LINE_STRING: line over 80 characters
#2215: FILE: subsys/zigbee/common/zigbee_helpers.c:477:
+		LOG_INF("Device update received (short: 0x%04hx, long: %s, status: %d)",

-:2331: WARNING:LONG_LINE_STRING: line over 80 characters
#2331: FILE: subsys/zigbee/common/zigbee_helpers.c:593:
+			LOG_ERR("No free buffer available, skipping conflict resolving this time.");

-:2408: WARNING:LONG_LINE_STRING: line over 80 characters
#2408: FILE: subsys/zigbee/common/zigbee_helpers.c:670:
+			LOG_INF("Network rejoin procedure stopped as %sscheduled.",

-:2518: WARNING:LONG_LINE_STRING: line over 80 characters
#2518: FILE: subsys/zigbee/common/zigbee_helpers.c:780:
+			LOG_INF("Network rejoin procedure stopped as %sscheduled.",

-:2703: WARNING:NEW_TYPEDEFS: do not add new typedefs
#2703: FILE: subsys/zigbee/common/zigbee_helpers.h:117:
+typedef enum {

-:2851: WARNING:NEW_TYPEDEFS: do not add new typedefs
#2851: FILE: subsys/zigbee/common/zigbee_logger_eprxzcl.c:45:
+typedef struct {

-:2907: WARNING:MACRO_WITH_FLOW_CONTROL: Macros with flow control statements should be avoided
#2907: FILE: subsys/zigbee/common/zigbee_logger_eprxzcl.c:101:
+#define PRV_ADVANCE_CURR_LOG_MESSAGE_PTR(s)			\
+	do {							\
+		if ((s) < 0) {					\
+			LOG_INST_ERR(				\
+				logger.inst,			\
+				"Received ZCL command but encoding error occurred during log producing"); \
+			return ZB_FALSE;			\
+		}						\
+		log_message_curr += (s);			\
+		if (log_message_curr >= log_message_end) {	\
+			LOG_INST_ERR(				\
+				logger.inst,			\
+				"Received ZCL command but produced log is too long"); \
+			return ZB_FALSE;			\
+		}						\
+	} while (0)

-:2912: WARNING:LONG_LINE: line over 80 characters
#2912: FILE: subsys/zigbee/common/zigbee_logger_eprxzcl.c:106:
+				"Received ZCL command but encoding error occurred during log producing"); \

-:2919: WARNING:LONG_LINE: line over 80 characters
#2919: FILE: subsys/zigbee/common/zigbee_logger_eprxzcl.c:113:
+				"Received ZCL command but produced log is too long"); \

-:3626: WARNING:NEW_TYPEDEFS: do not add new typedefs
#3626: FILE: subsys/zigbee/osif/zb_nrf_platform.c:21:
+typedef enum {

-:3636: WARNING:NEW_TYPEDEFS: do not add new typedefs
#3636: FILE: subsys/zigbee/osif/zb_nrf_platform.c:31:
+typedef struct {

-:4063: WARNING:NEW_TYPEDEFS: do not add new typedefs
#4063: FILE: subsys/zigbee/osif/zb_nrf_platform.h:13:
+typedef enum {

-:4276: WARNING:NEW_TYPEDEFS: do not add new typedefs
#4276: FILE: subsys/zigbee/osif/zb_nrf_timer.c:17:
+typedef struct {

-:4441: WARNING:NEW_TYPEDEFS: do not add new typedefs
#4441: FILE: subsys/zigbee/osif/zb_nrf_transceiver.c:25:
+typedef struct nrf_802154_rx_frame {

-:5802: WARNING:NEW_TYPEDEFS: do not add new typedefs
#5802: FILE: tests/subsys/zigbee/osif/timer/stubs/zb_types.h:11:
+typedef char               zb_char_t;

-:5804: WARNING:NEW_TYPEDEFS: do not add new typedefs
#5804: FILE: tests/subsys/zigbee/osif/timer/stubs/zb_types.h:13:
+typedef unsigned char      zb_uchar_t;

-:5806: WARNING:NEW_TYPEDEFS: do not add new typedefs
#5806: FILE: tests/subsys/zigbee/osif/timer/stubs/zb_types.h:15:
+typedef unsigned char      zb_uint8_t;

-:5808: WARNING:NEW_TYPEDEFS: do not add new typedefs
#5808: FILE: tests/subsys/zigbee/osif/timer/stubs/zb_types.h:17:
+typedef signed char        zb_int8_t;

-:5810: WARNING:NEW_TYPEDEFS: do not add new typedefs
#5810: FILE: tests/subsys/zigbee/osif/timer/stubs/zb_types.h:19:
+typedef unsigned short     zb_uint16_t;

-:5812: WARNING:NEW_TYPEDEFS: do not add new typedefs
#5812: FILE: tests/subsys/zigbee/osif/timer/stubs/zb_types.h:21:
+typedef signed short       zb_int16_t;

-:5814: WARNING:NEW_TYPEDEFS: do not add new typedefs
#5814: FILE: tests/subsys/zigbee/osif/timer/stubs/zb_types.h:23:
+typedef void               zb_void_t;

-:5818: WARNING:NEW_TYPEDEFS: do not add new typedefs
#5818: FILE: tests/subsys/zigbee/osif/timer/stubs/zb_types.h:27:
+typedef enum zb_bool_e zb_bool_t;

-:5819: WARNING:NEW_TYPEDEFS: do not add new typedefs
#5819: FILE: tests/subsys/zigbee/osif/timer/stubs/zb_types.h:28:
+typedef unsigned int       zb_uint32_t;

-:5821: WARNING:NEW_TYPEDEFS: do not add new typedefs
#5821: FILE: tests/subsys/zigbee/osif/timer/stubs/zb_types.h:30:
+typedef zb_uint32_t zb_time_t;

-:5827: WARNING:NEW_TYPEDEFS: do not add new typedefs
#5827: FILE: tests/subsys/zigbee/osif/timer/stubs/zb_types.h:36:
+typedef zb_uint8_t zb_64bit_addr_t[8];

-:5832: WARNING:NEW_TYPEDEFS: do not add new typedefs
#5832: FILE: tests/subsys/zigbee/osif/timer/stubs/zb_types.h:41:
+typedef zb_64bit_addr_t zb_ieee_addr_t;

- total: 0 errors, 41 warnings, 5740 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

Your patch has style problems, please review.

NOTE: Ignored message types: AVOID_EXTERNS BRACES CONFIG_EXPERIMENTAL CONST_STRUCT DATE_TIME FILE_PATH_CHANGES MINMAX MULTISTATEMENT_MACRO_USE_DO_WHILE NETWORKING_BLOCK_COMMENT_STYLE PRINTK_WITHOUT_KERN_LEVEL SPLIT_STRING VOLATILE

NOTE: If any of the errors are false positives, please report
      them to the maintainers.

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

greg-fer

Applied my edits, so I'm approving.
Please check my commit and squash it.

rlubos

Gone through the first commit so far, left some feedback. It seems that checkpatch covered also a good amount of issues, please fix those.

rlubos · 2020-05-08T09:33:40Z

subsys/zigbee/CMakeLists.txt

+zephyr_sources(osif/zb_nrf_transceiver.c)
+zephyr_sources(osif/zb_nrf_crypto.c)
+zephyr_sources(osif/zb_nrf_pwr_mgmt.c)
+zephyr_sources(osif/nrf_ecb_driver.c)


Note: we should soon get this into fw-nrfconnect-zephyr, please remove it from the PR after it's merged.

Can we remove it now? You already have the Zephyr version in the tree. Or do you prefer to leave it as it is for the initial release? @wbober?

@rlubos I would be keen on ditching this. @maciekfabia what do you think? This should be fairly simple?

rlubos · 2020-05-08T09:39:14Z

subsys/zigbee/Kconfig

+
+config ZBOSS_DEFAULT_THREAD_STACK_SIZE
+	int "Stack size of ZBOSS Zephyr task"
+	default 2048    #ToDo: TBD


So, have you found out a proper value yet?

We will use this value. #Todo removed

rlubos · 2020-05-08T09:43:32Z

subsys/zigbee/common/zigbee_helpers.c

+LOG_MODULE_REGISTER(zigbee_helpers, CONFIG_ZIGBEE_HELPERS_LOG_LEVEL);
+
+/* Rejoin-procedure related variables. */
+static zb_uint8_t           stack_initialised             = ZB_FALSE;


Shouldn't this be bool? Also, since these variables do not interface any zboss APIs, why not use native types instead (bool, u8_t etc)?

I changed variables in this group to bool

rlubos · 2020-05-08T09:46:15Z

subsys/zigbee/common/zigbee_helpers.c

+#endif
+}
+
+int to_hex_str(char *out, uint16_t out_size, const uint8_t *in,


General note: we should rather use kernel types in NCS, wherever possible (u16_t etc.), but you probably already noticed from the checkpatch output.

rlubos · 2020-05-08T09:50:47Z

subsys/zigbee/common/zigbee_helpers.c

+			  sizeof(zb_ieee_addr_t), true);
+}
+
+uint8_t parse_hex_digit(const char c)


Zephyr provides some utils for hex string conversion, please consider using them to avoid code duplication:
https://github.com/zephyrproject-rtos/zephyr/blob/master/include/sys/util.h#L153

parse_hex_digit replaced with char2hex from sys/util.h.
Other string-hex functions in this file can also convert in reverse order. They cannot be fully replaced because it's not available in their counterparts in utils.h but they probably could be much shorter if they used util.h functions.
I think it can be postponed to subsequent pull requests

rlubos · 2020-05-08T10:37:32Z

subsys/zigbee/osif/zb_nrf_platform.c

+#include <init.h>
+
+#include "zb_nrf_platform.h"
+#include "zboss_api.h"


#include <zboss_api.h>

rlubos · 2020-05-08T10:37:59Z

subsys/zigbee/osif/zb_nrf_platform.c

+	LOG_ERR("Fatal error occurred");
+	k_fatal_halt(K_ERR_KERNEL_PANIC);
+}
+


Extra empty line.

rlubos · 2020-05-08T10:38:27Z

subsys/zigbee/osif/zb_nrf_platform.c

+	/* Configure event/signals to wait for in wait_for_event function */
+    static struct k_poll_event wait_events[] = {
+		K_POLL_EVENT_INITIALIZER(K_POLL_TYPE_SIGNAL,
+								 K_POLL_MODE_NOTIFY_ONLY,


Looks like the indentation gone wild.

fixed after checkpatch report

rlubos · 2020-05-08T10:40:48Z

subsys/zigbee/osif/zb_nrf_pwr_mgmt.c

+	ZB_SET_TRACE_OFF();
+
+	/* Disable Zigbee stack-related peripherals to save energy. */
+	/*zb_osif_priph_disable();*/


Please remove dead code.

rlubos · 2020-05-08T10:41:14Z

subsys/zigbee/osif/zb_nrf_timer.c

+ * SPDX-License-Identifier: LicenseRef-BSD-5-Clause-Nordic
+ */
+
+/* TODO: Change function names. The 'nrf52' prefix should be removed */


Seems to be done already?

yes, the comment removed

rlubos · 2020-05-08T11:36:47Z

samples/zigbee/light_bulb/boards/nrf52840_pca10056.overlay

@@ -0,0 +1,17 @@
+/* Copyright (c) 2019 Nordic Semiconductor ASA


Also, the file has been renamed to reflect the new board name

rlubos · 2020-05-08T11:37:20Z

samples/zigbee/light_bulb/dts.overlay

@@ -0,0 +1,32 @@
+/* Copyright (c) 2019 Nordic Semiconductor ASA


rlubos · 2020-05-08T11:40:55Z

samples/zigbee/light_bulb/src/main.c

+#include <zboss_api.h>
+#include <addons/zboss_api_addons.h>
+#include <zb_mem_config_med.h>
+#include <zigbee_helpers.h>


If zigbee_helpers.h is supposed to be a public header and used in applications, it should be moved to the include directory then. include/zigbee/zigbee_helpers.h?

We are going to move zigbee_helpers.h to include directory but it requires some cleaning, some functions need to be removed/moved to other files, some need to be rewritten to make use of zephyr utilities.
After this is done the file could be moved and documented in NCS documentation but for this PR I'd prefer it to stay at its current location

rlubos · 2020-05-08T11:44:50Z

samples/zigbee/network_coordinator/dts.overlay

@@ -0,0 +1,32 @@
+/* Copyright (c) 2019 Nordic Semiconductor ASA


rlubos · 2020-05-08T12:31:35Z

Looks good overall, some nits from my side plus you need to sort out the CI failures, otherwise we coul d get it in (once nrfxlib counterpart is merged).

lemrey · 2020-05-14T07:45:21Z

had a very quick look and have two comments:

we should group the header files in their own folder under /include
let's use printk to print from the sample instead of the logger

rlubos · 2020-05-14T09:09:36Z

let's use printk to print from the sample instead of the logger

Why so? I don't think there's a well-established convention for that in Zephyr (nor in NCS). Some samples use printk while others use logger (and some even switched to logger recently , see asset_tracker). IMO, with the flexibility logger gives, it's ok to use it.

tejlmand · 2020-05-14T11:02:17Z

subsys/zigbee/CMakeLists.txt

+zephyr_include_directories(common)
+zephyr_include_directories(osif)
+zephyr_include_directories(zb_error)
+
+# Source files
+zephyr_sources(osif/zb_nrf_platform.c)
+zephyr_sources(osif/zb_nrf_logger.c)
+zephyr_sources(osif/zb_nrf_nvram.c)
+zephyr_sources(osif/zb_nrf_timer.c)
+zephyr_sources(osif/zb_nrf_transceiver.c)
+zephyr_sources(osif/zb_nrf_crypto.c)
+zephyr_sources(osif/zb_nrf_pwr_mgmt.c)
+zephyr_sources(osif/nrf_ecb_driver.c)


This should be in independent lib, please use zephyr_library.
No need that all those files are placed directly in libzephyr.a

tejlmand · 2020-05-14T11:06:00Z

subsys/zigbee/Kconfig

+	bool "Include functions for CLI logging"
+	default y
+	help
+	  Include functions for CLI logging.


This is not including log functions for the user.

Instead, it enables logging inside the Zigbee subsys, correct ?
Help text is not clear on this.

This is probably due to historical reasons. This controls the eprxzcl component from nRF5 for T&Z SDK.
This dumps all received ZCL packets: eprxzcl == EndPoint R(x)eceive ZCL - packet dump.
By default, it was included in the Zigbee CLI example.

tejlmand · 2020-05-14T11:10:24Z

tests/subsys/zigbee/zboss_api/callback_api/CMakeLists.txt

+add_definitions(-Wno-packed-bitfield-compat)
+add_definitions(-DTIMER_DEFAULT_CONFIG_IRQ_PRIORITY=6)
+
+include($ENV{ZEPHYR_BASE}/cmake/app/boilerplate.cmake NO_POLICY_SCOPE)


Please update to use find_package(Zephyr ...)

tejlmand · 2020-05-14T11:16:49Z

tests/subsys/zigbee/zboss_api/callback_api/CMakeLists.txt

+
+cmake_minimum_required(VERSION 3.8)
+
+add_definitions(-Wno-packed-bitfield-compat)


Why is this needed here.
In Zephyr, those warnings are handled in the toolchain setup:
https://github.com/NordicPlayground/fw-nrfconnect-zephyr/blob/master/cmake/compiler/gcc/target_warnings.cmake
and can be controlled using -DW=[1|2|3].

This warning is generated by ZBOSS headers. The same definition is inside ZBOSS's CMakeLists.txt file. I'd remove it here and move the discussion to the nrfxlib's PR.

Line removed.
This warning suppression is not specified in a sample or test but propagated from ZBOSS CMakeLists.txt

tejlmand · 2020-05-14T11:23:24Z

tests/subsys/zigbee/zboss_api/callback_api/CMakeLists.txt

+cmake_minimum_required(VERSION 3.8)
+
+add_definitions(-Wno-packed-bitfield-compat)
+add_definitions(-DTIMER_DEFAULT_CONFIG_IRQ_PRIORITY=6)


This looks like something that should have been in a configuration file.
Kconfig ?
Secondly, I can't find the place where it is used.

It's not needed any more. Removed

tejlmand · 2020-05-14T11:33:06Z

samples/zigbee/light_bulb/CMakeLists.txt

+# SPDX-License-Identifier: LicenseRef-BSD-5-Clause-Nordic
+#
+
+cmake_minimum_required(VERSION 3.8)


Suggested change

cmake_minimum_required(VERSION 3.8)

cmake_minimum_required(VERSION 3.13)

tejlmand · 2020-05-14T11:35:33Z

samples/zigbee/light_bulb/CMakeLists.txt

+if (EXISTS "${CMAKE_CURRENT_SOURCE_DIR}/boards/${BOARD}.overlay")
+  set(DTC_OVERLAY_FILE "${DTC_OVERLAY_FILE} ${CMAKE_CURRENT_SOURCE_DIR}/boards/${BOARD}.overlay")
+endif()


This will not work with when using deprecated board names nor aliased board names.
Please remove.

Board overlays are handled in Zephyr build system, when named correctly.
See https://github.com/NordicPlayground/fw-nrfconnect-zephyr/blob/master/cmake/app/boilerplate.cmake#L463-L471

The overlay files have been combined into one and renamed to app.overlay

tejlmand · 2020-05-14T11:36:17Z

samples/zigbee/light_bulb/CMakeLists.txt

+endif()
+
+include($ENV{ZEPHYR_BASE}/../nrf/cmake/boilerplate.cmake)
+include($ENV{ZEPHYR_BASE}/cmake/app/boilerplate.cmake NO_POLICY_SCOPE)


Please use: find_package(Zephyr ...)

tejlmand · 2020-05-14T11:37:04Z

samples/zigbee/network_coordinator/CMakeLists.txt

+if (EXISTS "${CMAKE_CURRENT_SOURCE_DIR}/dts.overlay")
+  set(DTC_OVERLAY_FILE "${CMAKE_CURRENT_SOURCE_DIR}/dts.overlay")
+endif()


Remove those lines and rename dts.overlay to app.overlay

tejlmand · 2020-05-14T11:37:26Z

samples/zigbee/network_coordinator/CMakeLists.txt

+endif()
+
+include($ENV{ZEPHYR_BASE}/../nrf/cmake/boilerplate.cmake)
+include($ENV{ZEPHYR_BASE}/cmake/app/boilerplate.cmake NO_POLICY_SCOPE)


Please use: find_package(Zephyr ...)

lemrey · 2020-05-14T11:43:27Z

We (I) have been trying to let samples print to the console using printk instead of the logger in NCS, and there are several reasons for that.

The first is that printk has a predictable and consistent behavior while the logger is a very configurable subsystem and it can behave totally different based on its configuration. It's good that samples print to the console consistently and that would be hard to do if they used the logger (log level/ buffer sizes/ deferred, etc). If one sample used a 4k buf for the logger, for a good reason, that'll be copy-pasted until the end of time in many samples that could have worked perfectly with the default instead; this applies of course to every option in the logger.

A second reason is readability of the output. When logging is enabled, it is enabled system-wide; that can generate a lot of output coming from subsystems and libraries (unless explicitely disabled, one by one). I argue that when running a sample, the user doesn't care much about what's happening in all subsys and libs in use, but more about what the sample is doing. If a sample prints 20 lines after booting, it's harder for the user to see if it is behaving as expected, because they have to "fish" the lines they are interested in among 20. Instead, if logging is not enabled (or enabled only for wrn/err/assert via NCS_SAMPLES_DEFAULT), the sample has full control of the output on the console and it is easy to follow what the sample is doing. It's also easy to document the expected output on the console.

asset_tracker is an application, not a sample /samples vs /application, they are different from samples and also more complex, so they are treated separately.

rlubos · 2020-05-14T14:56:46Z

A little offtop.

@lemrey
It's not that I completely disagree with you (maybe except for the lest paragraph, I don't buy it). printk is simple and it has its use cases. But it also has its flaws - for instance if you enable logger when printk is used, the output will likely get messed up, unless you know that "special" switch to pipe printk via the logger (or use the minimal logger variant). And a lot of samples in NCS start off with logger being enabled anyway so I don't think that log bloat is the case here (you can always lower LOG_DEFAULT_LEVEL if that's a problem, I don't think that error level logs can be considerer as undesired - if they show up, something definintely is not correct).

The point is, we have not written down any "best practicies" guide for the samples, so it's hard to enforce such requests based on reviewer preference. For intstance, recently I was reviewing PR upstream, where it was explicitly requested to use logger in the sample (even though it was only writng down a greeting message), there's no consensus. I'm not saying we should follow upstream Zephyr blindy here. But if we want to have a NCS set of rules, we should at least write them down and get accepted, only then we'll have a basis to enforce them.

ru-fu

This looks really good!
But we're missing a few links within the documentation. And I guess library documentation and the third sample are planned for later?

ru-fu · 2020-05-14T14:52:35Z

doc/nrf/user_guides.rst

@@ -21,3 +21,4 @@ They introduce you to important concepts and guide you through developing your a
   ug_bootloader
   ug_unity_testing
   ug_thread
+   ug_zigbee


This one and the Thread user guide should go under ug_thingy91 (to keep the "Working with" user guides together).

ru-fu · 2020-05-14T14:54:16Z

doc/nrf/ug_zigbee.rst

+
+Zigbee is a portable, low-power software networking protocol that provides connectivity over 802.15.4-based mesh network.
+It also defines an application layer that provides interoperability among all vendors.
+


Can we link somewhere for more information? (Probably the Zigbee Alliance?)

Added, good point.

ru-fu · 2020-05-14T14:54:59Z

doc/nrf/ug_zigbee.rst

+Introduction
+************
+
+Zigbee is a portable, low-power software networking protocol that provides connectivity over 802.15.4-based mesh network.


Suggested change

Zigbee is a portable, low-power software networking protocol that provides connectivity over 802.15.4-based mesh network.

Zigbee is a portable, low-power software networking protocol that provides connectivity over 802.15.4-based mesh networks.

or

Suggested change

Zigbee is a portable, low-power software networking protocol that provides connectivity over 802.15.4-based mesh network.

Zigbee is a portable, low-power software networking protocol that provides connectivity over an 802.15.4-based mesh network.

Went for the 2nd one.

ru-fu · 2020-05-14T14:57:05Z

doc/nrf/ug_zigbee.rst

+Zigbee is a portable, low-power software networking protocol that provides connectivity over 802.15.4-based mesh network.
+It also defines an application layer that provides interoperability among all vendors.
+
+In combination with Zephyr RTOS and |NCS|, Zigbee allows for easy development of low-power connected solutions.


Zephyr is part of the NCS package, thus either just "In combination with |NCS|," or "In combination with |NCS| and the integrated Zephyr RTOS,".

2nd option adopted.

ru-fu · 2020-05-14T14:58:25Z

doc/nrf/ug_zigbee.rst

+
+See :ref:`zigbee_samples` for the list of available Zigbee samples.
+
+.. |zboss_lib| replace:: The |NCS|'s Zigbee protocol uses the ZBOSS library, a third-party precompiled Zigbee stack.


Link to ZBOSS?

Yes, once it's available. Will work on this with @b-gent .

ru-fu · 2020-05-14T15:30:08Z

samples/zigbee/network_coordinator/README.rst

+
+This Zigbee network coordinator sample establishes the Zigbee network and commissions Zigbee devices that want to join it.
+
+You can use this sample together with :ref:`Zigbee light bulb <zigbee_light_bulb_sample>` and Zigbee light switch to set up a basic Zigbee network.


Suggested change

You can use this sample together with :ref:`Zigbee light bulb <zigbee_light_bulb_sample>` and Zigbee light switch to set up a basic Zigbee network.

You can use this sample together with the :ref:`Zigbee light bulb <zigbee_light_bulb_sample>` and Zigbee light switch to set up a basic Zigbee network.

ru-fu · 2020-05-14T15:30:37Z

samples/zigbee/network_coordinator/README.rst

+
+* One or both of the following samples:
+
+  * The :ref:`Zigbee light bulb <zigbee_light_bulb_sample>` application programmed on one or more separate devices.


Suggested change

* The :ref:`Zigbee light bulb <zigbee_light_bulb_sample>` application programmed on one or more separate devices.

* The :ref:`Zigbee light bulb <zigbee_light_bulb_sample>` sample programmed on one or more separate devices.

ru-fu · 2020-05-14T15:31:00Z

samples/zigbee/light_bulb/README.rst

+  * |nRF52840Dongle|
+  * |nRF52833DK|
+
+* The :ref:`Zigbee network coordinator <zigbee_network_coordinator_sample>` application programmed on one separate device.


Suggested change

* The :ref:`Zigbee network coordinator <zigbee_network_coordinator_sample>` application programmed on one separate device.

* The :ref:`Zigbee network coordinator <zigbee_network_coordinator_sample>` sample programmed on one separate device.

ru-fu · 2020-05-14T15:31:49Z

samples/zigbee/network_coordinator/README.rst

+    Indicates whether the network is open or closed.
+
+    .. note::
+         When you use the coordinator on the |nRF52840Dongle|, it is the LED 3 that informs about the successful network joining.


Same question as for the other sample.

ru-fu · 2020-05-14T15:32:24Z

samples/zigbee/network_coordinator/README.rst

+
+After programming the sample to your development kit, test it by performing the following steps:
+
+1. Turn on the coordinator development kit.


same as above ;)

lemrey · 2020-05-14T15:44:21Z

I agree @rlubos; we had a draft for these guidelines in Confluence, perhaps we could make it available so that it is visible for everybody?

wbober · 2020-05-15T09:02:05Z

@ru-fu Light switch sample and ZBOSS lib are in separate PRs.

greg-fer

@ru-fu , thank you for your comments and feedback. I've implemented the changes in a review branch: https://github.com/maciekfabia/fw-nrfconnect-nrf/tree/add_zigbee_subsystem_review
This will be merged into this one.

maciekfabia · 2020-05-18T13:06:46Z

we should group the header files in their own folder under /include

We are going to do so but it requires some cleaning, refinement and documenting. For this PR I'd prefer them to stay at its current location and move them in next pull requests
Also, most of the headers are for internal use only.

maciekfabia · 2020-05-18T13:21:43Z

@tejlmand, we addressed all comments, could you please take a look?

tejlmand

Looking better, but still a couple of comments to be addressed.

tejlmand · 2020-05-18T21:37:00Z

subsys/zigbee/CMakeLists.txt

+zephyr_interface_library_named(zigbee)
+
+target_include_directories(zigbee INTERFACE common)
+target_include_directories(zigbee INTERFACE osif)
+target_include_directories(zigbee INTERFACE zb_error)


~~Why is an interface target needed here ?~~
~~and not simple using zephyr_library_include_directories() on the lib below.~~

Nevermind. Found APP_LINK_WITH_ZIGBEE in Kconfig.
Disregard this comment.

tejlmand · 2020-05-18T21:40:02Z

tests/subsys/zigbee/osif/crypto/CMakeLists.txt

+zephyr_include_directories(${ZEPHYR_BASE}/../nrf/subsys/zigbee/osif)
+zephyr_include_directories(${ZEPHYR_BASE}/../nrfxlib/zboss/include)
+zephyr_include_directories(${ZEPHYR_BASE}/../nrfxlib/zboss/include/osif)


It should be possible to use: ${ZEPHYR_NRFXLIB_MODULE_DIR} here, instead of: ${ZEPHYR_BASE}/../nrfxlib

Replaced with ${NRFXLIB_DIR} for now. Will be replaced with ${ZEPHYR_NRFXLIB_MODULE_DIR} once it's available in fw-nrfconnect-zephyr

tejlmand · 2020-05-18T21:40:48Z

tests/subsys/zigbee/osif/crypto/CMakeLists.txt

+target_sources(app PRIVATE
+  src/main.c
+  ${ZEPHYR_BASE}/../nrf/subsys/zigbee/osif/zb_nrf_crypto.c
+  ${ZEPHYR_BASE}/../nrf/subsys/zigbee/osif/nrf_ecb_driver.c


It should be possible to use: ${ZEPHYR_NRF_MODULE_DIR} here, instead of: ${ZEPHYR_BASE}/../nrf

Replaced with ${NRF_DIR}

tejlmand · 2020-05-18T21:41:26Z

tests/subsys/zigbee/osif/nvram/CMakeLists.txt

+if (EXISTS "${CMAKE_CURRENT_SOURCE_DIR}/dts.overlay")
+  set(DTC_OVERLAY_FILE "${CMAKE_CURRENT_SOURCE_DIR}/dts.overlay")
+endif()


Lines are still present.

rlubos

I have no further comments for now. Let's address the remaining issues (moving the header to public location, switching to printk) in seprate PRs, if time permits, since they're not ciritcal. And it's really cumbersome to deal with such a large PR.

maciekfabia · 2020-05-19T11:27:58Z

if (EXISTS "${CMAKE_CURRENT_SOURCE_DIR}/dts.overlay") set(DTC_OVERLAY_FILE "${CMAKE_CURRENT_SOURCE_DIR}/dts.overlay") endif()

Lines are still present.

sorry, my mistake. Removed

rlubos · 2020-05-19T14:22:23Z

@maciekfabia Please update west.yml with correct SHA: 0b0049b

lemrey · 2020-05-19T14:48:15Z

I'd rather have the public headers under /include in this PR, but if we are short on time it's fine to merge this as is, as long as we agree that they are going to be moved afterwards.

rlubos · 2020-05-19T14:59:42Z

Thanks @lemrey, @maciekfabia promised to fix this later: #2208 (comment)

CLAassistant · 2020-05-21T10:16:24Z

All committers have signed the CLA.

Add zigbee subsystem and related tests Signed-off-by: Maciej Fabia <[email protected]>

Add Zigbee samples: - network coordinator - light bulb Signed-off-by: Maciej Fabia <[email protected]>

Add owners for zigbee subsys, samples and tests Signed-off-by: Maciej Fabia <[email protected]>

wbober · 2020-05-22T06:46:15Z

@carlescufi Can we merge this please?

maciekfabia requested review from carlescufi, ru-fu and tejlmand as code owners May 5, 2020 23:54

ru-fu added the doc-required PR must not be merged without tech writer approval. label May 6, 2020

ru-fu requested a review from greg-fer May 6, 2020 06:47

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

maciekfabia mentioned this pull request May 6, 2020

zboss: Add ZBOSS library nrfconnect/sdk-nrfxlib#166

Merged

maciekfabia force-pushed the add_zigbee_subsystem branch from 460a232 to a3146c7 Compare May 6, 2020 23:40

greg-fer approved these changes May 7, 2020

View reviewed changes

wbober mentioned this pull request May 7, 2020

samples: zigbee: Add overlay to enable 802154 FEM & WiFi Coex #2215

Merged

rlubos reviewed May 8, 2020

View reviewed changes

maciekfabia force-pushed the add_zigbee_subsystem branch from c46d505 to 4a353ef Compare May 11, 2020 01:37

mariuszpos mentioned this pull request May 11, 2020

zigbee: Add light switch sample #2226

Merged

maciekfabia force-pushed the add_zigbee_subsystem branch 4 times, most recently from d088983 to d62fdc1 Compare May 12, 2020 17:02

lemrey requested review from b-gent and lemrey May 13, 2020 12:06

maciekfabia force-pushed the add_zigbee_subsystem branch 2 times, most recently from d0b6251 to dc03d60 Compare May 13, 2020 22:53

tejlmand requested changes May 14, 2020

View reviewed changes

maciekfabia force-pushed the add_zigbee_subsystem branch from dc03d60 to 7e32f54 Compare May 14, 2020 15:26

ru-fu reviewed May 14, 2020

View reviewed changes

maciekfabia force-pushed the add_zigbee_subsystem branch from 7e32f54 to 8cc61dc Compare May 15, 2020 00:27

greg-fer reviewed May 15, 2020

View reviewed changes

maciekfabia force-pushed the add_zigbee_subsystem branch 2 times, most recently from fb61a37 to 05a3247 Compare May 18, 2020 10:04

tejlmand requested changes May 18, 2020

View reviewed changes

rlubos approved these changes May 19, 2020

View reviewed changes

tejlmand approved these changes May 19, 2020

View reviewed changes

maciekfabia force-pushed the add_zigbee_subsystem branch from 7896c95 to b1a97e3 Compare May 19, 2020 14:12

maciekfabia force-pushed the add_zigbee_subsystem branch from b1a97e3 to 7eb99d4 Compare May 19, 2020 14:36

maciekfabia force-pushed the add_zigbee_subsystem branch from 7eb99d4 to 80fc3b6 Compare May 21, 2020 10:16

maciekfabia added 2 commits May 21, 2020 13:17

zigbee: Add zigbee subsystem

b73d926

Add zigbee subsystem and related tests Signed-off-by: Maciej Fabia <[email protected]>

samples: zigbee: Add network coordinator and light bulb samples

3809608

Add Zigbee samples: - network coordinator - light bulb Signed-off-by: Maciej Fabia <[email protected]>

maciekfabia force-pushed the add_zigbee_subsystem branch from 80fc3b6 to ef95492 Compare May 21, 2020 11:18

codeowners: Add owners for zigbee

497d509

Add owners for zigbee subsys, samples and tests Signed-off-by: Maciej Fabia <[email protected]>

maciekfabia force-pushed the add_zigbee_subsystem branch from ef95492 to 497d509 Compare May 21, 2020 13:08

rlubos merged commit bbbd85d into nrfconnect:master May 22, 2020

		@@ -0,0 +1,17 @@
		/* Copyright (c) 2019 Nordic Semiconductor ASA

		@@ -0,0 +1,32 @@
		/* Copyright (c) 2019 Nordic Semiconductor ASA


		cmake_minimum_required(VERSION 3.8)

		add_definitions(-Wno-packed-bitfield-compat)

	cmake_minimum_required(VERSION 3.8)
	cmake_minimum_required(VERSION 3.13)


		Zigbee is a portable, low-power software networking protocol that provides connectivity over 802.15.4-based mesh network.
		It also defines an application layer that provides interoperability among all vendors.


		See :ref:`zigbee_samples` for the list of available Zigbee samples.

		.. \|zboss_lib\| replace:: The \|NCS\|'s Zigbee protocol uses the ZBOSS library, a third-party precompiled Zigbee stack.


		This Zigbee network coordinator sample establishes the Zigbee network and commissions Zigbee devices that want to join it.

		You can use this sample together with :ref:`Zigbee light bulb <zigbee_light_bulb_sample>` and Zigbee light switch to set up a basic Zigbee network.

	You can use this sample together with :ref:`Zigbee light bulb <zigbee_light_bulb_sample>` and Zigbee light switch to set up a basic Zigbee network.
	You can use this sample together with the :ref:`Zigbee light bulb <zigbee_light_bulb_sample>` and Zigbee light switch to set up a basic Zigbee network.


		* One or both of the following samples:

		* The :ref:`Zigbee light bulb <zigbee_light_bulb_sample>` application programmed on one or more separate devices.

	* The :ref:`Zigbee network coordinator <zigbee_network_coordinator_sample>` application programmed on one separate device.
	* The :ref:`Zigbee network coordinator <zigbee_network_coordinator_sample>` sample programmed on one separate device.


		After programming the sample to your development kit, test it by performing the following steps:

		1. Turn on the coordinator development kit.

zigbee: Add zigbee support #2208

zigbee: Add zigbee support #2208

Conversation

maciekfabia commented May 5, 2020

NordicBuilder commented May 5, 2020 • edited Loading

checkpatch (informational only, not a failure)

greg-fer left a comment

Choose a reason for hiding this comment

rlubos 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maciekfabia May 12, 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

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

rlubos commented May 8, 2020

lemrey commented May 14, 2020

rlubos commented May 14, 2020

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lemrey commented May 14, 2020 • edited Loading

rlubos commented May 14, 2020

ru-fu 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

Choose a reason for hiding this comment

greg-fer May 15, 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

lemrey commented May 14, 2020

wbober commented May 15, 2020

NordicBuilder commented May 5, 2020 •

edited

Loading

maciekfabia May 12, 2020 •

edited

Loading

lemrey commented May 14, 2020 •

edited

Loading

greg-fer May 15, 2020 •

edited

Loading

tejlmand May 18, 2020 •

edited

Loading