High-level infrastructure for MQTT based integrations

OnFreund commented 4 years ago

Context

Following up on the discussion in https://github.com/home-assistant/core/pull/36531.

The MQTT integration covers quite a lot (optimistic updates, availability, json parsing, and discovery to name a few. But it's also a cliff - if you need some custom behavior (e.g. custom discovery), you're off the cliff and have to start from the ground up, based on the low level infrastructure (basically subscribe and publish).

As we get more MQTT based integrations (e.g. OZW, MySensors), there's a lot of code repetition between those integrations, to implement all of the features that the MQTT integration already provides.

Proposal

I propose that we increase the scope of the infrastructure so that it allows integrations to take advantage of all of these higher level building blocks that the MQTT integration provides. That will also allow us to simplify the MQTT integration, as it will basically be just another integration built on top of this infrastructure.

Consequences

MQTT based integrations will be easier to build, and will be less repetitive.

frenck commented 4 years ago

I do not see the HA side of this? Logic like that should go into the python package of the integration published on PyPi.

Sharing the building blocks has a high impact on the maintainability of the core itself. It means that changes to the MQTT integration impacts a whole tree of other integrations, we should not allow for that.

Jc2k commented 4 years ago

@OnFreund Don't know if a useful data point or not, but the Dyson Fan integration is also MQTT based under the hood. It's support library (libpurecool) uses paho. It's blocking and needs threads to do its job. I want to contribute an async version (if i ever have time). Do you think there are parts of the main MQTT integration that the Dyson Fan integration could/should use?

There's a lot of non-mqtt stuff around discovery and authentication that would still have to come from the support library, does that mean this integration isn't a good fit for your idea? Or if you think it would work, how would you structure it? Like if the support library didn't do the MQTT part it wouldn't be very useful on its own and it couldn't really be tested on its own.

OnFreund commented 4 years ago

@Jc2k I'll have to dig into that integration to know more, but I think that specific integrations would still need a PyPi package for sure (even if it's just to get topic and payload definitions) - it's just that they'll have higher level building blocks.

Looking at some of the existing integrations, it seems that most of them could be implemented in terms of the existing MQTT entities, but they have a different discovery scheme. With that in mind, I would envision an API that's somewhat similar to that. Something like:

sensor = new MqttSensor(state_config ={...}, availability_config={...}, device_info={...}, ...)

but there are other alternatives, like exposing the existing mixins (e.g. MqttAvailability).

@frenck Many of these are features that are tied to the HA implementation. Additionally, the MQTT integration already does all of this, while relying only on the low level interface, not an elaborate PyPi package. I think the implementation for this could live in a PyPi package, but I think the API should be accessible from HA. Eventually, even the MQTT integration would be implemented using this package.

balloob commented 4 years ago

The MQTT integration should not offer this.

The MQTT integration allows the user to specify the protocol via configuration (or discovery) and build on top. But then the user is also responsible for removing devices and cleaning up things. Anything else like services etc is not possible.

Any integration needs to rely on their own classes for entity logic. This is not something we will change.

Wwe just implemented OZW using MQTT and an external PyPI package and it is working wonderful. The bases classes used for OZW rely on a simple mechanism of feeding messages (topic+payload) and it then build up the whole network based on it. You can then define relations etc.

Base classes are here https://github.com/cgarwood/python-openzwave-mqtt/blob/master/openzwavemqtt/base.py

If it helps we can consider extracting those base classes so that any MQTT based lib can rely on it.

OnFreund commented 4 years ago

Thanks @balloob. Took me some time to gather my thoughts here.

I agree that the MQTT integration should not offer this, but HA also has a layer of HA infrastructure that is available to other integrations (basically, the publish and subscribe functionality).
I agree that the integration is limited to user configuration, and there are certain limitations that require a full blown integration (two examples that come to mind - custom services and a custom discovery schema).
HA has a few integrations already built on top of MQTT (OZW, MySensors, and I believe Dyson too), and there are a few additional candidates (Shelly, Tasmota, etc...)
Currently, such integrations have a lot of code duplication (and a lot of it is shared with the user configured MQTT entities). I think we can all agree that ideally we'd like to reduce that duplication, and rely on common code.
Having a common PyPI package providing that infrastructure makes sense - the code doesn't have to necessarily live inside the core repo.
To really avoid duplication, and to ensure that integrations get access to the same set of features, I would recommend that the user-configured MQTT entities also rely on that package.
Ideally, in most cases, I think the integration-specific packages, built on top of that common package, should be relatively thin - there's no reason to reinvent the wheel in each integration. I think the OZW base classes are a bit too low-level for this, and another layer on top of them might be needed.
We will probably still have HA specific behavior that's repeated in every integration. Hopefully it'll be minimal.

To recap - I think that a common package makes sense, but I think it needs to be a higher level one. To me the test is whether we can completely reimplement the existing user-configured MQTT entities using this package with minimal code, apart from parsing the configuration and implementing the HA interface.

Hypfer commented 4 years ago

I'd like to add Valetudo to the list of additional integration candidates which might be built on top of MQTT, since it has pretty much reached the limits of what is possible with the generic vacuum.mqtt.

However there doesn't seem to be a simple way to scale beyond that without reinventing everything from scratch

emontnemery commented 3 years ago

This has been discussed several times before, and I think @balloob is right, there is nothing to gain by offering this in Home Assistant. The existing MQTT protocols implemented by the MQTT integration are flexible to make them possible to connect with devices with varying functionality.

An external package with shared bits and pieces to base a custom MQTT protocol on sounds like a good idea but it's not the right topic for an architecture discussion.

I suggest to close this issue.

OnFreund commented 3 years ago

there is nothing to gain by offering this in Home Assistant.

Looking at existing/potential MQTT based integrations and the amount of code duplication between them easily proves that there's much to gain.

emontnemery commented 3 years ago

@OnFreund I'm not questioning that it may be useful with a library to deduplicate common code, I'm just repeating that it's not a good fit for the Home Assistant core repo, and that there's no need to keep this architecture issue open.

Hypfer commented 3 years ago

I'm not particularily happy about people with autogenerated profile pictures trying to silence relevant conversations.

Cease

Hypfer commented 3 years ago

Wow. You can't be serious

Hypfer commented 3 years ago

Btw I'm pretty sure that this way of not only silencing the discussion but also any mention of silencing happening here by flagging the comment is not only abuse of power but a clear violation of the Home Assistant Code of Conduct.

I strongly urge you to read it and act accordingly.

https://github.com/home-assistant/core/blob/dev/CODE_OF_CONDUCT.md

OnFreund commented 3 years ago

Why was this closed?? There are several people here who believe this topic merits a discussion.

frenck commented 3 years ago

@Hypfer I'm sorry, your original mention violated the CoC you are throwing at others. That comment was not called for, unneeded, and did not contribute to the subject in here either. Furthermore, was targeted against a respected member of the project. Consider this an official warning.

frenck commented 3 years ago

@OnFreund I'm not sure, although I do agree ok the closing myself.

@emontnemery Could you elaborate on this?

Edit: Honestly, I think the reasoning is summed up before he closed it, which I think sensible.

OnFreund commented 3 years ago

It seems pretty arbitrary that several people believe this has merit, and then one person does not and simply closes it. Additionally, the reasoning dismisses without addressing my previous comment in which I detail why I think an external package is not enough.

frenck commented 3 years ago

one person does not and simply closes it

Sorry to say, but at some point, a single person has to close it... (that is how issues work, it isn't closable by multiple people).

I do see your points, nevertheless, sharing that part of the code base with integrations, makes maintenance and innovation harder in the longer run, as more things are built on top of it. If Home Assistant ever wants to go the route of the splitting of integrations from the core, having integrations building on top of each other is a bad thing and should, therefore, be limited to building on top of entity components.

Just that, makes the rest of the discussion for me irrelevant. If MQTT should be abstracted into a package: Sure, maybe. Start creating one, maybe it can serve as a base for all. Who knows 🤷

Hypfer commented 3 years ago

@frenck How else should one react to a dismissal of a valid discussion by an account which doesn't appear to be affiliated with the project in any way nor looks like it's a real account at all? I'm sorry for misjudging this - although I don't see how I could've judged this better - however, this still doesn't void my criticism.

frenck commented 3 years ago

@Hypfer Decisions are made at times, one can agree or disagree. That is fine. Your comment wasn't related to that discussion at all, nor was it criticism towards the actual contents of the discussion. Instead, it was targeted against a responding person because of his avatar, despite a "member" status shown by his account and apparently without checking the profile of the person you commented on.

At this point, I suggest you either comment on the topic matters or reframe from commenting. Thanks for understanding.

Hypfer commented 3 years ago

@frenck

despite a "member" status shown by his account

This is wrong.

There is no indication whatsoever

My criticism wasn't targetet at this person. My criticism was targeted as a random looking account appearing to dismiss a valid technical discussion.

I am aware that there are a few commits in home-assistant related repos. This however doesn't prove any affiliation with the project and therefore right to dismiss views of the community.

emontnemery commented 3 years ago

@OnFreund in the comment which you linked to you write:

I agree that the MQTT integration should not offer this, but HA also has a layer of HA infrastructure that is available to other integrations (basically, the publish and subscribe functionality).

Having a common PyPI package providing that infrastructure makes sense - the code doesn't have to necessarily live inside the core repo.

Hence, it seemed already clear and agreed that this proposal, good or not, is not for Home Assistant core and there's no benefit of keeping an architecture issue open.

To really avoid duplication, and to ensure that integrations get access to the same set of features, I would recommend that the user-configured MQTT entities also rely on that package.

If such a library is written, and the existing components in the MQTT integration can be refactored to use it I'm all for it.

OnFreund commented 3 years ago

@frenck

If Home Assistant ever wants to go the route of the splitting of integrations from the core, having integrations building on top of each other is a bad thing and should, therefore, be limited to building on top of entity components.

That's why I specifically differentiated between the MQTT integration, and the MQTT infrastructure. In my proposal, we also refactor the integration to rely on the common infrastructure.

@emontnemery that is assuming such a package could be built outside of HA yet provide everything discussed. I'm a bit skeptical.

frenck commented 3 years ago

In my proposal, we also refactor the integration to rely on the common infrastructure.

That is a potential second step.

that is assuming such a package could be built outside of HA yet provide everything discussed

That is a correct assumption, as we prefer libraries for integrations outside of HA.

OnFreund commented 3 years ago

That is a potential second step.

That doesn't matter. The point is that the common code lives outside the integration.

That is a correct assumption, as we prefer libraries for integrations outside of HA.

I'm not convinced - there are a lot of HA specific things currently happening in the MQTT integration.

emontnemery commented 3 years ago

I just want to add one thing, devices can be identifed by a connection, e.g. a MAC. In this case entities from multiple integrations belonging to the same connection will be collected under one device.

This means a custom integration can rely on existing MQTT integrations for some or all of its entities, and use custom entities for some entities or only to manage the device itself.

OnFreund commented 3 years ago

Thanks @emontnemery. That's a great idea when MQTT discovery is available, but in most cases I've seen, the main driver for a custom implementation is a different discovery mechanism.

home-assistant / architecture