Initial commit of MQTT entity management framework. #656

skotl · 2022-02-03T15:22:33Z

Addable extension method AddMqttExtensions()
Rough entity creation through EntityUpdater (which is probably a poor name) that needs more strongly-typed.

Note that I can't push this to a branch on the netdaemon core, which I think it would be better to be in - if you want to create a new branch ("mqtt_extensions" or similar?) then I can update the PR to point to that instead.

That will also allow Eugene and I to collaborate around the branch without upsetting your work.

This has been tested manually within the context of the Create entities from ND discord chat.

Breaking change

Non-breaking change

Proposed change

New extension project called NetDaemon.Extensions.MqttEntities that adds a services extension AddMqttExtensions() that can be called from program.cs

Sample usage:

    public MqttTester(ILogger<MqttTester> logger, IEntityUpdater entityUpdater)
    {
        _logger = logger;
        _entityUpdater = entityUpdater;

        _entityUpdater.CreateAsync("binary_sensor", "motion", "motion_test_1", "mqtt test")
            .GetAwaiter();
    }

Requires additional config in appSettings.json:

  "Mqtt": {
    "Host": "homeassistant.local",
    "Port": "1883",
    "UserId": "mqtt",
    "Password": "secret"
  }

Type of change

Dependency upgrade
Bugfix (non-breaking change which fixes an issue)
New feature (which adds functionality to an existing integration)
Breaking change (fix/feature causing existing functionality to break)
Code quality improvements to existing code or addition of tests

Additional information

This PR fixes or closes issue: fixes #
This PR is related to issue:
Link to documentation pull request:

Checklist

The code change is tested and works locally.
Local tests pass. Your PR cannot be merged unless tests pass
There is no commented out code in this PR.
I have followed the [development checklist][dev-checklist]
The code compiles without warnings (code quality chek)
Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

Documentation added/updated for www.home-assistant.io

Addable extension method `AddMqttExtensions()`, rough entity creation through `EntityUpdater` (which is probably a poor name) that needs more strongly-typed.

NetDaemon.Extensions.MqttEntities/DependencyInjectionSetup.cs

FrankBakkerNl · 2022-02-03T16:04:40Z

NetDaemon.Extensions.MqttEntities/DependencyInjectionSetup.cs

+    public static IServiceCollection AddMqttExtensions(this IServiceCollection services)
+    {
+        services.AddSingleton<IMqttFactory, MqttFactory>();
+        services.AddScoped<IEntityUpdater, EntityUpdater>();


Scoped is only usefull if you have state in tje class that should be scoped (per app in case of ND) I think thats not the case here.

My concern is the connection to mqtt, which times out after 10 seconds.
But actually... I remake the connection each time when a message is called, so that's probably not an issue.

OK - will update - thanks

helto4real · 2022-02-03T16:05:28Z

@skotl I will add you to the dev team here. You can push to branch here your self and make it easier for you guys to collaborate

skotl · 2022-02-03T16:22:09Z

Changed DI scope to Singleton for the local services, changed target branch to mqtt_extensions.
Just need to check that @FrankBakkerNl is happy with the comment on MqttFactory

NetDaemon.Extensions.MqttEntities/IEntityUpdater.cs

NetDaemon.Extensions.MqttEntities/IMessageSender.cs

BeeHiveJava · 2022-02-03T17:03:16Z

NetDaemon.Extensions.MqttEntities/MessageSender.cs

+        _logger = logger;
+        _mqttFactory = mqttFactory;
+
+        _mqttConfig = configuration.GetSection("Mqtt")


Should use the options framework instead here.

NetDaemon.Extensions.MqttEntities/MessageSender.cs

BeeHiveJava · 2022-02-03T17:04:46Z

NetDaemon.Extensions.MqttEntities/MqttConfiguration.cs

+
+internal class MqttConfiguration
+{
+    public string? Host { get; set; }


Are these properties really nullable?

Probably not - I'll update - thanks :)

You can probably use a record here with a primary constructor and have the properties auto generated. That way you can make then non nullabe without getting warnings. Username and password might actually be optional

FrankBakkerNl · 2022-02-03T17:45:28Z

NetDaemon.Extensions.MqttEntities/MessageSender.cs

+
+    public async Task SendMessageAsync(string topic, string payload)
+    {
+        using (var mqttClient = _mqttFactory.CreateMqttClient())


Maybe cache this client in a field so it is not recreated each time and then dispose in Dispose if this class.

FrankBakkerNl · 2022-02-03T17:50:27Z

NetDaemon.Extensions.MqttEntities/DependencyInjectionSetup.cs

+    {
+        services.AddSingleton<IMqttFactory, MqttFactory>();
+        services.AddSingleton<IEntityUpdater, EntityUpdater>();
+        services.AddSingleton<IMessageSender, MessageSender>();


The MessageSender does not have any state so it does not have to be a singleton. I read its recimended to use transient in those cases (creating a new instance is relatively cheap)

helto4real

I am ok for this to be merged to the feature branch so @eugeneniemand and @skotl can work on it. I save my comments to the real PR. You can use these comments as guidance for your work. Thanks for the contribution!

helto4real · 2022-02-03T16:12:04Z

NetDaemon.Extensions.MqttEntities/DependencyInjectionSetup.cs

+    ///     Adds scheduling capabilities through dependency injection
+    /// </summary>
+    /// <param name="services">Provided service collection</param>
+    public static IServiceCollection AddMqttExtensions(this IServiceCollection services)


AddNetDaemonMqttEntityManagement or something like that. In the other extensions we hade NetDaemon name here

helto4real · 2022-02-03T16:14:09Z

NetDaemon.Extensions.MqttEntities/EntityUpdater.cs

+            name = name, device_class = deviceClass, state_topic = statePath, json_attributes_topic = attrsPath
+        });
+
+        await _messageSender.SendMessageAsync( topicPath, payload);


Add a ConfigureAawait(false). Even if chances are slim this is used by a UI app we did this in the rest of assemblies

helto4real · 2022-02-03T16:17:22Z

NetDaemon.Extensions.MqttEntities/IMessageSender.cs

@@ -0,0 +1,6 @@
+namespace NetDaemon.Extensions.MqttEntities;
+
+public interface IMessageSender


Is this intended to be exposed to users or internal interface? If internal make it internal. If it is inteneded to exposed to users. I would think we can have one public API/interface being injected

Yes - definitely should be internal!

helto4real · 2022-02-03T17:00:37Z

NetDaemon.Extensions.MqttEntities/MessageSender.cs

+
+        _logger.LogDebug($"Sending to {message.Topic}:\r\n   {message.ConvertPayloadToString()}");
+
+        var publishResult = await client.PublishAsync(message, CancellationToken.None);


ConfigureAwait (same reason as before, consistency)

… PR #656

* Initial commit of MQTT entity management framework. (#656) * Initial commit of MQTT entity management framework. Addable extension method `AddMqttExtensions()`, rough entity creation through `EntityUpdater` (which is probably a poor name) that needs more strongly-typed. * DI scope update from PR comment * Added Update and Remove methods and resolved some of the comments for PR #656 * Changing names to be less generic * Moved to src/extensions, renamed folder and fixed warning * Added example app and fixed IOptions for mqtt config * Fix CI Build error * Add support for nuget and default host (#660) * Added Discovery prefix * MQTT connect and publish can throw exceptions as well as return error codes, so handle each case in a consistent manner by throwing a new MqttConnection or MqttPublish exception. Add try...catch to the test app so that we can see when exceptions are thrown in called methods. * 1. Add XML comments 2. Remove unused IConfiguration arg from MessageSender constructor * Resolving PR comments * Merged Skotl's code and fixed hard coded retain flag * Removing throw and fixed warning. Added comment for Delay * address PR comments * Internalising... Co-authored-by: Scott Leckie <scott.leckie@kwolo.com> Co-authored-by: Tomas Hellström <tomas.hellstrom@yahoo.se>

* Initial commit of MQTT entity management framework. (net-daemon#656) * Initial commit of MQTT entity management framework. Addable extension method `AddMqttExtensions()`, rough entity creation through `EntityUpdater` (which is probably a poor name) that needs more strongly-typed. * DI scope update from PR comment * Added Update and Remove methods and resolved some of the comments for PR net-daemon#656 * Changing names to be less generic * Moved to src/extensions, renamed folder and fixed warning * Added example app and fixed IOptions for mqtt config * Fix CI Build error * Add support for nuget and default host (net-daemon#660) * Added Discovery prefix * MQTT connect and publish can throw exceptions as well as return error codes, so handle each case in a consistent manner by throwing a new MqttConnection or MqttPublish exception. Add try...catch to the test app so that we can see when exceptions are thrown in called methods. * 1. Add XML comments 2. Remove unused IConfiguration arg from MessageSender constructor * Resolving PR comments * Merged Skotl's code and fixed hard coded retain flag * Removing throw and fixed warning. Added comment for Delay * address PR comments * Internalising... Co-authored-by: Scott Leckie <scott.leckie@kwolo.com> Co-authored-by: Tomas Hellström <tomas.hellstrom@yahoo.se>

Initial commit of MQTT entity management framework.

b3930db

Addable extension method `AddMqttExtensions()`, rough entity creation through `EntityUpdater` (which is probably a poor name) that needs more strongly-typed.

FrankBakkerNl reviewed Feb 3, 2022

View reviewed changes

NetDaemon.Extensions.MqttEntities/DependencyInjectionSetup.cs Show resolved Hide resolved

FrankBakkerNl reviewed Feb 3, 2022

View reviewed changes

DI scope update from PR comment

ed9d30a

skotl changed the base branch from dev to mqtt_extensions February 3, 2022 16:20

BeeHiveJava reviewed Feb 3, 2022

View reviewed changes

FrankBakkerNl reviewed Feb 3, 2022

View reviewed changes

helto4real approved these changes Feb 3, 2022

View reviewed changes

eugeneniemand added the pr: new-feature label Feb 3, 2022

eugeneniemand merged commit a651058 into net-daemon:mqtt_extensions Feb 3, 2022

eugeneniemand added a commit that referenced this pull request Feb 4, 2022

Added Update and Remove methods and resolved some of the comments for…

228e3fd

… PR #656

		@@ -0,0 +1,6 @@
		namespace NetDaemon.Extensions.MqttEntities;

		public interface IMessageSender


		_logger.LogDebug($"Sending to {message.Topic}:\r\n {message.ConvertPayloadToString()}");

		var publishResult = await client.PublishAsync(message, CancellationToken.None);

Uh oh!

Initial commit of MQTT entity management framework. #656

Initial commit of MQTT entity management framework. #656

Uh oh!

Conversation

skotl commented Feb 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Breaking change

Proposed change

Type of change

Additional information

Checklist

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

helto4real commented Feb 3, 2022

Uh oh!

skotl commented Feb 3, 2022

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

helto4real left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

skotl commented Feb 3, 2022 •

edited

Loading