Zigbee improvements

[oleo65]

• Fixed battery cluster reporting (bug).
• Made some configuration properties configurable through Kconfig.
• Made log level configurable in zigbee and core parasite lib.
• Split debug build config from production.

I contributed to a zigbee firmware fork in the past and tried to migrate some concepts / bugfixes I experienced there. I personally really like the idea here, that the core libraries are reused no matter what protocol stack is used. This seems to me the right approach and I am happy to push the zigbee firmware forward.

In the long run, I believe even more configuration could be centralized. The Zephyr configuration system is quite powerful. Let me know what you think. 😃

[rbaron]

Hey @oleo65 that's fantastic. Thank you! I was looking forward to discuss this PR with someone. I've made some comments mostly about naming. Looking forward to your thoughts.

BTW out of curiosity, did you manage to test this with either Zigbee2MQTT or something else?

[rbaron]

Nice 👌. I'd suggest PRSTLIB_DEFAULT_LOG_LEVEL to be more descriptive. Do you know if we can set the default to one of the enum values? E.g.: LOG_LEVEL_WRN. Looking at 2 I'm not sure what the default is. If not, I'd leave a comment that 2 corresponds to WRN.

[rbaron]

I'm also wondering where this config should live. It seems less of property of each board and more of a property of the project that uses the different boards.
What do you think of moving these to the project's Kconfig? And I'm not sure how, but it would be great if there's an easy way to provide a default for prstlib.

[oleo65]

I don't know about the enum values but would guess not because the Kconfig is resolved first before any C code is compiled. But I did not research this further...

I thought about the config hierarchy and did not come up with a nice solution without duplicating code, since there is no real common ancestor in the device tree. I don't know if it is maybe possible to have more than one ancestor (like a mixin) and maybe have a common b-parasite mixin with common configuration flags for both MCU variants. I would really like that as it might help to reduce configuration errors and feature drifts...

[rbaron]

I did a quick test:

config PRSTLIB_DEFAULT_LOG_LEVEL
	int "Default log level for prstlib"
	default LOG_LEVEL_INF

And unfortunately that doesn't work :( (LOG_LEVEL_INF is undefined at this point). It seems like Zephyr implements the following strategy for subsystem logging:

Each subsystem defines their own choices. I'm not sure if this is overkill for us, but it would perfect if we could just add a Kconfig file to prstlib and do it there.

[oleo65]

Changed the naming of the config flag as discussed.

[Hedda]

I personally really like the idea here, that the core libraries are reused no matter what protocol stack is used. This seems to me the right approach and I am happy to push the zigbee firmware forward.

+1 The same core application for Zigbee should in theory be reusable for Matter standard over Thread (Project CHIP), see -> #87

[oleo65]

@rbaron I set the sensor up via nativ Home Assistant ZHA integration. as all my other devices. It basically worked straight out of the box.

If you already run a Zigbee2MQTT gateway, that might also be a valid solution, but I like to keep my setup simpler without relying on an additional (MQTT) component. I got away with it so far... 😉

As for the naming of variables etc. I am quite torn in half. To me it felt almost all the time redundant to name something with PRST prefix or something similar. The code is for a firmware for the parasite and I believe it is not necessary to distinct "your own custom" code from "framework" code. But that is also a highly debatable topic on how to name your stuff in your code. (We often have that discussion at work, but usually resolve it with clear and simple names...)

I am fine to rename any variables in this pull request so that it complies with the rest of the project to move this forward. But I would love to start a general refactoring branch with more separations of concerns within the different code files to make it easier for new people (including me) to comprehend which code part does what. 😃

[oleo65]

I am pretty sure that I will eventually move to Matter over Thread. And I am happy that all the improvements to zigbee and the move to zephyr pay directly towards a Matter/Thread implementation.

At the moment I simply do not have the infrastructure to test any of this myself...

[rbaron]

As for the naming of variables etc. I am quite torn in half. To me it felt almost all the time redundant to name something with PRST prefix or something similar. The code is for a firmware for the parasite and I believe it is not necessary to distinct "your own custom" code from "framework" code. But that is also a highly debatable topic on how to name your stuff in your code. (We often have that discussion at work, but usually resolve it with clear and simple names...)

Agreed, naming is subjective and personal and I also like it as simple and short as possible, but no simpler. I think even in our simple setup there's already room for confusion: does CORE_LOG_LEVEL control the log level of the prstlib or the application or Zephyr core/kernel? Or all of them? If some other app includes prstlib as one of its few external libraries, does the name CORE still make sense? I would think it's clearer to have separate PRSTLIB_DEFAULT_LOG_LEVEL for the lib and PRST_ZB_DEFAULT_LOG_LEVEL for the application, and it's easier to grep them and see where they're modified.

[oleo65]

I pushed changes with the discussed config namings and some slight improvements to the power cluster. This should resolve the discussions.

I did not change the CONFIG_LOG_DEFAULT_LEVEL as this is a default zephyr config flag I reused in the main.c which should be fine for the main entry point.

[rbaron]

LGTM. Let me know if you have any thoughts on the conversation I just unresolved:

• Board definitions don't seem like a good place for a LOG config for prstlib
• I'm not sure what 2 is in the default. How does it relate to LOG_{DBG,INF,WRN,ERR}? What should I set this to if I want to see LOG_DBG calls? If no great solution, please leave a comment with the mapping

Also okay to merge as is and I'll try some ideas. Tks!

[oleo65]

For the latter: 2 equals WRN. I found the values in the docstring of the zephyr library and I will add a comment to the config value and a reference where to find more. 😉

The problem with the location of the prstlib log flag is tricky. If you put it within the sample Kconfig and reference it in the common prstlib, you need to "duplicate" the value in all samples (ble, blinky, etc.) otherwise they won't compile. We can definately do that but it feels also clunky to me.

Edit: I might have found a specific solution via the macros.h file. I am not sure if this really works, e.g. for the ble or blinky sample, but the code compiles even if I don't specifically define that config flag in the specific Kconfig file. What's your take on that? 🤷

[rbaron]

I think that's better, thanks. I will take a stab at setting up a proper Kconfig for prstlib. I think that would be the ideal solution. If the build system doesn't like it, I will at least set up a prstlib/config.h and move the config there, away from macros.h.

Tks once again!