Refactor of DigitalStrom MQTT bridge to make the code more readable and have more functionalities

[albertomontesg]

This change performs some "from the ground up" changes that completely refactor the current implementation. The idea behind these changes have been to learn on best practices for Go and how to create good and scalable interfaces. This has been a learning experience that I believe could be merge back if you like how it looks like. Otherwise I am completely fine with keeping this as a fork.

Directory structure has been changed to implement it as different modules under a pkg directory as most of Go projects do. Also gives more structure and differentiate between clients, configs, utils and business logic.

Architecture has been change to have the following structure:

DigitalStrom API Client: a client that keeps track of the connection to the DS server, the endpoint internals and the event loop. This way we make the DigitalStrom API component pluggable and completely abstract how the different endpoints should be called. Also the API has been converted into structures and therefore are strongly typed which bring us more safety on the business logic side. The implementation is inspired the same way MQTT client keeps the connection to the server. Subscribing to events requires now to pass a callback that gets executed on new events the same way as it happens with MQTT.

Controller: now a controller is responsible to maintain both clients (MQTT and DS) and then execute different modules in parallel. Those modules are different pieces of business logic that are completely independent and require pass information between the clients:

• Devices: responsible for receiving input from MQTT and sending it back to DS, the same time is responsible for getting the update scene events and update the MQTT topic.
• Circuits: responsible for periodically poll DS to get the latest values and publish then in MQTT
• Scenes: receiving DS events and updated the MQTT. As new feature, now you can activate a scene through the MQTT. It also gets exported into Home Assistant which was requested by a user.
• Buttons (NEW): responsible to read Joker push button events from DS and publish it to MQTT. This also gets exported to Home Assistant.

Also the controller is responsible to fetch the Home Assistant configs from the different modules to export them at start up. The controller is using the registry pattern to initialise the different modules, and makes a good us of Go interfaces to run the modules and fetch the Home Assistant configs.

Some additional changes are:

• Now most of the things are cached (e.g. given scene and group, which are the devices to update, the scene names,...) which should simplify the logic when new events received.
• Config is using easier apis from Viper and has better support for the file config.

The final code structure is:

• digitalstrom: Client to communicate with DS. this could be for any other project if required or could be updated in case there are new API available without the need to change the rest of the code.
• mqtt: small wrapper around the original MQTT client in order to set topic prefix and have the connection to the broker given the config.
• config: module to read the config using Viper library.
• home_assistant: module to have all logic regarding Home Assistant: the config structures and to publish them into MQTT
• controller: responsible to glue all the logic above.
• controller/modules: different business logic we need to bridge different entities between DS and MQTT.

The reasons behind these changes:

• API logic was spread across almost all modules. Now the client is the only one responsible for passing the parameters and then parse the responses.
• The current implementation uses too many channels for synchronisation between modules which was difficult to follow. Now we have two clients and different modules that are responsible for passing information between those which encapsulates the business logic.
• Now there is in a single file the business logic regarding those entities. For example in the scene module contains all the information on:
  • MQTT topic structure and message
  • Calling DS Client
  • Generating the Home Assistant configs

If you want to also have more information, feel free to check the commits messages where I have provided as detailed explanation as possible for all the different changes I have implemented.

Feel free to contact me if you have any further doubts and I will be happy to clarify anything.

[gaetancollaud]

@albertomontesg any update on this ? Do you still have time to continue fixing it ?

[albertomontesg]

@gaetancollaud Sorry for the long period without answering, but I went back to work (after paternity leave) and my free time to spend on this type of things got significantly reduced.

I still need to address some of your comments, but I have some comments on your overall conclusions for this PR:

configuration is not retro compatible, the soft was complaining that I didn't provide a mqtt_username

It could be. Something that surprised me from the previous implementation is that you expected a .yaml file with the syntax of an .env file. Commonly, YAML files have the field names in snake case while originally here it was expected all in uppercase. I believe some consistency could be the way forward even if this means having some backward incompatible change.

it's super slow to load the initial state of all the devices (see video). For reference, I have 30-40 devices

The logic for loading the initial state for all the devices is the same as the previous implementation. At Start() launch a goroutine to iterate through all the devices and fetch their state values. Even more, this change makes use of an endpoint on DS API that allows you to fetch the values for multiple channels in the same device in a single API call, while previously 1 API call was made for each device-channel pair. With the original implementation it was taking a lot for me to get all devices updated (I have ~70 devices) and the same is happening with the current implementation. Note that most of the changes are architectural about the code rather than changes in the business logic.

already mentioned but the energy update does a lot of queries

Same here, the logic hasn't changed. For each meter it performs 2 queries for power and energy consumption. No change of logic at this end either.

[gaetancollaud]

Sorry for not answering earlier, i've also been busy. Congrats on your new born.

The logic for loading the initial state for all the devices is the same as the previous implementation. At Start() launch a goroutine to iterate through all the devices and fetch their state values. Even more, this change makes use of an endpoint on DS API that allows you to fetch the values for multiple channels in the same device in a single API call, while previously 1 API call was made for each device-channel pair. With the original implementation it was taking a lot for me to get all devices updated (I have ~70 devices) and the same is happening with the current implementation. Note that most of the changes are architectural about the code rather than changes in the business logic.

I'm sure my implementation was faster because I used the "property tree" and I neved requested the device value directly. Because those calls take 2-3s per device. You can have a look here: https://github.com/gaetancollaud/digitalstrom-mqtt/blob/master/digitalstrom/devices.go#L163

already mentioned but the energy update does a lot of queries

Same here, the logic hasn't changed. For each meter it performs 2 queries for power and energy consumption. No change of logic at this end either.

yeah possible, sorry about this comment.

[BentEngbers]

Is there an update on this pull request? I am experiencing some bugs with this mqtt bridge, and i wanted to look into these issues after this refactor was completed

Thanks for the great work!

[gaetancollaud]

Is there an update on this pull request? I am experiencing some bugs with this mqtt bridge, and i wanted to look into these issues after this refactor was completed

Thanks for the great work!

regardless of the status of this PR, don't hesitate to open an issue with what you found.

For me this PR is not yet ready and I'm not sure if @albertomontesg has time to work on it.

[gaetancollaud]

closed in favor of #45