search

rbaron / b-parasite

🌱💧 An open source DIY soil moisture sensor
1.85k stars 143 forks source link

Zigbee improvements #84

Closed oleo65 closed 1 year ago

oleo65 commented 1 year ago

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. 😃

Hedda commented 1 year ago

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 -> https://github.com/rbaron/b-parasite/issues/87

oleo65 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

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

oleo65 commented 1 year ago

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 commented 1 year ago

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!