MQTT Improvements / Autodiscovery

[torbennehmer]

Good evening together,

I've been thinking (and working) on the MQTT interface of BSB-LAN, #332 is a first attempt on moving MQTT further along. The reasoning behind this is the flexibiliity MQTT gives you. I've been using it to integrate with Home Assistant as the existing integration doesn't work for me as I don't have a room thermostat in my BSB.

The idea behind all this is to move the async MQTT interface (much better than polling anyway) to a point where it gets more transparent and feature rich. For example:

• Forwarding informational messages (which are already parsed to debug out) to MQTT
• Listening to the bus on messages applicable to MQTT logging topics and forwarding them to MQTT on match
• Providing a Home Aasistant auto-discovery topic based on the data in the data available to extract

I'm still working my way into the codebase, but I believe this should be possible at least with the resources a Due provides (which I am using for myself here).

What I would like to know if this is something you can make use of or if it is outside the scope of what you want to achive. I cannot promise you how quickly I can build any further changes - as a family father my time is limited. But I'd like to contribute a bit, since what you're doing here is really cool (thanks!!!).

Best regards, Torben

[fredlcore]

Thanks a lot for your contribution, I have already asked @dukess and @Luposoft63 to have a look at your PR because they have been writing most of the MQTT code so far and I'm not actively using it so I'm not an expert when it comes to MQTT and its best use cases.
However, I certainly think that it's a great way to interface with other home automation systems, so we should enhance this feature if possible and the feature/size relation is good enough.
It's perfectly understandable if your family comes first (so does mine), the only thing I would ask for (generally) is that PRs with new features are not "half-ready" so that we would end up with large areas of orphaned and barely useful code in case the contributor is no longer able to contribute. But what you have submitted so far does not give me this impression, so I'd say let's go for it.

[torbennehmer]

@fredlcore I agree, that MQTT is really valuable, especially, since it is a push-based, not polling all along. I've got good experience with a few shelly switches via MQTT as well.

Regarding half-ready features: Don't worry, I've been in the business for over 20 years now and I had my share of these things. I'll do my best and try to get one feature-complete change at a time into the codebase. I won't send you anything incomplete, wouldn't be my style anyway ;-)

What I wonder: Is there a specific (arduino?) reason that leads to the whole code being in a single file and not separated into various C-files for better managebility? I'm having a hard time when merging upstream changes in that big file. Also, having the code separated a bit problably will make it easier tracking changes. Do I miss something here? If yes, please just point me into the right direction (y'know, RTFM and such).

[fredlcore]

Ok, good to hear :).
As for the whole code in a single file: The code is grown historically and organically over the last five years. Yes, from today's point of view, one would do things differently, but since it's basically two people who effectively work on the code and we know the places we need to edit in an out, it's gonna stay that way, sorry ;)...

[torbennehmer]

@fredlcore Would you be interested if I'd do a bit of refactorisation at least for MQTT to break this down into more manageable parts? This might also make it easier for others to jump into development. The more parallel development there is, the bigger the advantage at this point would be.

There is no need to do this in a single big action, it could be done individually and step by step where ther's time for it or where the code in question is touched anyway.

What do you think?

[fredlcore]

Sorry for the delayed response, I would not be against a bit of refactorisation, but that should be coordinated with the people who have mostly written these parts (which wasn't me except for the very beginning, but rather @dukess and @Luposoft63, among others). Could you also outline what you would plan to do and what the benefits would be?

[torbennehmer]

Hi @fredlcore, @dukess, @Luposoft63

I'd suggest a gradual refactoring, something I've done a lot over the years with grown codebases. The idea would be to take a single feature (e.g. MQTT) and extract it into its own source files. This often results at least in a number of additional functions, that have to be created. The most important thing here is that there are no changes in the code - it would have to be taken as is, so that the chance of any side-effect is as small as possible. Any further cleanup should not be taken.

The advantages I see are these:

1. It would be easier for newcomers to get a hold of the source base. Keeping together a 10.000 line source file is just about impossible
2. Handling the code in PRs etc. is easier as well, as there are less changes per source file (always a good thing when doing QA)
3. On the long run it would allow central methods like loop() to be much more compact, basically a sequence of function calls relaying into their specific modules.
4. In sum, this will greatly increase the maintainability of the source base, as you'll have separate modules, one for each purpose. The amount of code from intermixed features will be reduced.
5. Following the previous point, this should also make it easier to increase the overall quality of the code - the less code you have to overlook, the easier it is to spot mistakes.
6. If there is code, that needs to be modernized (for example, if you want to move to an OO-like codebase), splitting up the code per module also makes things easier to handle.

Ultimately these suggestions arose from my first experience with your codebase.

Please take it as a (quite subjective) opinion from an outsider. Also: I do not want to suggest, that your code is bad, you are very far from it. The quality of the code itself is quite good - I have seen much much worse in my 25 years of coding. I even wrote much worse code myself in my past ;-).

I'm talking about it, because I was in this situation on my job quite often, where we had to refactor grown code to move on with it.

I'd offer my services and experience to do so, if you wish; as I say, it would be a gradual step-by-step approach, always focusing on single parts of the code. I'd never try a big-bang approach (been there, done that :-)). Also, I'd suggest doing sort of a proof of concept with a single part of the code, so that you can get a better feeling for it. Often its easier to judge something like this when seeing an example as compared to discussing it in dry theory.

Best regards
Torben

[torbennehmer]

On a side note: More elaborate changes like #346 would also take advantage of it, once MQTT is factored out, dissolving it further into individual MQTT command patterns is also possible.

[fredlcore]

Thank you, @torbennehmer!
I understand the points you have raised, and one of the first things I did when I took over the code in 2017 was to split the code into BSB_LAN_defs.h, BSB_LAN_config.h, html_strings.h and the main BSB_LAN.ino. However - and that is also a very subjective perspective - I never understood how to navigate dozens of code files where it is not clear where a (sub-)function is located. I just experienced this when trying to debug the SD_MMC library for the ESP32. Yes, IDEs like PlatformIO can bring me to the source code of a given function (however this does not work well if you work a lot with ifdefs and various platforms). But trying to find out which functions access the SPI bus or serial port has in my experience been a pain in the back. In the current situation, I can just search through the whole BSB_LAN.ino and find the code that I'm looking for.

Maybe with a more professional/experienced background, it is more obvious where to look for certain kinds of code and then the benefits of your suggestion certainly outweigh anything else. Even though several people have contributed valuable pieces of code to the project, in the end very few of them stayed on contributing regularly. That's why so far I have addressed the programming needs based on my own preferences, and for reasons stated above, I prefer the actual code to stay in one place.

That does not mean that the code doesn't need cleanup, of course! The way functions are scattered across the code has no longer any reasonable order, and once we have dropped Mega support, quite a bit of code could go for good and be rewritten in a more readable way.

Having said that, the MQTT and JSON code are the two sections of code which have advanced so much that my original approaches are hardly visible anymore (which is good :)!). So I'd agree to move these two parts of the code to new files if those people mainly working with it would want that, too (@dukess and others).

[torbennehmer]

I understand the points you have raised, and one of the first things I did when I took over the code in 2017 was to split the code into BSB_LAN_defs.h, BSB_LAN_config.h, html_strings.h and the main BSB_LAN.ino. However - and that is also a very subjective perspective - I never understood how to navigate dozens of code files where it is not clear where a (sub-)function is located. I just experienced this when trying to debug the SD_MMC library for the ESP32.

Oh, I can feel your problems, definitely. Good code structure is more of an art, then a science. Also, to be honest, the current Arduino IDE isn't exactly powerful in this respect, when compared to the big ones or even VS Code. The main point here, is that a structure is only good if there's an agreement on it.

Yes, IDEs like PlatformIO can bring me to the source code of a given function (however this does not work well if you work a lot with ifdefs and various platforms). But trying to find out which functions access the SPI bus or serial port has in my experience been a pain in the back. In the current situation, I can just search through the whole BSB_LAN.ino and find the code that I'm looking for. [...]

Yeah, this is difficult at least with Arduino IDE, it is unable to track references forward. What works in this case is searching over the entire directory subtree with your own code. Something I do quite often. One of the requirements then is, that your own sources are not intermixed with external dependencies.

The normal approach of having a file per module / part of the application also works well, as it pulls everything together that is responsible for dealing with MQTT (to stay at the current example).

I myself come from an OOP heavy background, there you've got one file per class, which isn't too bad if the class hierarchy is sound. If not, it can be like hell :)

Even though several people have contributed valuable pieces of code to the project, in the end very few of them stayed on contributing regularly.

Guilty as charged :-) Back 15 years ago when I was quite active in the CMS open source world, I expereinced the same thing.

That's why so far I have addressed the programming needs based on my own preferences, and for reasons stated above, I prefer the actual code to stay in one place.

👍

That does not mean that the code doesn't need cleanup, of course! The way functions are scattered across the code has no longer any reasonable order, and once we have dropped Mega support, quite a bit of code could go for good and be rewritten in a more readable way.

Agreed. Also, this is one of the things that would need to be looked at anyway. So no hazzle to do this in separated steps which I did with my MQTT PR as it was opportune there.

Having said that, the MQTT and JSON code are the two sections of code which have advanced so much that my original approaches are hardly visible anymore (which is good :)!). So I'd agree to move these two parts of the code to new files if those people mainly working with it would want that, too (@dukess and others).

That would be a good idea, especially because it could give you an idea, what options there are in terms of code organisation. To be honest - most of the time the visions, that could be done with any codebase heavily exceed the time available to do so. So much things to do and so little time...

So:

@dukess @Luposoft63 Would you be willing to consider further reogranization of the MQTT parts of the code? much as it was done in the way of the last PR? What's your opinion about it?

[dukess]

I will stay aside: my improvements were purely cosmetic, since I do not use MQTT and I do not even have anything to test it on.

[fredlcore]

@torbennehmer: Good to see we're generally on the same page :) - @dukess, I think you are underestimating your contribution :), but if you have no objections, then I'd say go ahead, @torbennehmer!

[Luposoft63]

@torbennehmer
Thanks for the idea and thanks for asking me.
I use MQTT in an own way in custom-file while using the mqtt-functions stored in the ino-file.
The only thing I must know just for sure:
The accessing to this functions from the custom-files are farther possible after reorganizing?
If yes (and I think it can't be no!) I will have an eye on it and see forward for testing.

[nielssch]

@torbennehmer I am pleased to read to read your ideas on this matter and would love to see a more advanced MQTT functionality in BSB-LAN to integrate in Homeassistant with auto discovery. Did you progress during the last three years?

[fredlcore]

Most of the refactoring work has been done already, the question of auto-discovery, however, is discussed in a different discussion/issue.

[torbennehmer]

I'm sorry to admit that Covid basically threw me out of BSB-LAN development, also, I underestimated the complexity of what I could do in my spare time. Right now, real life with two kids growing up mostly prevents any work for my to put into this project, unfortunately. This may change again in the future, I'll try at least, but can't promise you anything here. Sorry.

[fredlcore]

Totally understandable, and thank you for your contributions so far!

[fredlcore]

Since these points originally raised by @torbennehmer...

• Forwarding informational messages (which are already parsed to debug out) to MQTT
• Listening to the bus on messages applicable to MQTT logging topics and forwarding them to MQTT on match
• Providing a Home Aasistant auto-discovery topic based on the data in the data available to extract

...have now all been implemented, I guess we can close this topic now. Thanks everyone for all the contributions and ideas!

[fredlcore]

BTW, I have now created a tutorial video on how to integrate BSB-LAN into Home Assistant using auto-discovery:
https://youtu.be/DbHEiWm5nBs