rtl_433 icon indicating copy to clipboard operation
rtl_433 copied to clipboard

Read rtl_433_mqtt_hass.py configuration from rtl_433.conf

Open klattimer opened this issue 1 year ago • 12 comments

As the title suggests, a reasonably self contained change to load the configuration for HASS from the rtl_433.conf file if it exists.

klattimer avatar Feb 18 '24 12:02 klattimer

Currently, rtl_433_mqtt_hass is an example. Even if it were promoted to first class and supported, I think it's a mistake to blur the programs together. rtl_433_mqtt_hass should have its own config file, and it can be next to rtl_433.conf.

So I object to this PR being merged, at least for now.

What problem are you trying to solve?

gdt avatar Feb 20 '24 12:02 gdt

That seems like drawing an unnecessary line in the sand considering that the hass integration depends on the existing configuration, which duplicates the information required for execution.

klattimer avatar Feb 20 '24 14:02 klattimer

I don't really object to the hass program reading the config to rind out about rtl-433 config, but I don't think that any further hass/mqtt config belongs in the main config file. We have so far not crossed into configuring examples in the main config, and I don't think we should do that.

One might ask instead why the rtl_433 config is needed, view that as a bug, and try to resolve it. I personally run rtl_433 without a config file.

gdt avatar Feb 20 '24 14:02 gdt

I run rtl_433 as a service which reads the config file from /etc/ and hass reading the same config file makes sense. I don't think any further configuration is required for hass so I don't see why you wouldn't want to allow hass users to use this feature. It seems like an artificial inhibitor to having a simpler system configuration.

klattimer avatar Feb 20 '24 16:02 klattimer

One might ask instead why the rtl_433 config is needed, view that as a bug, and try to resolve it. I personally run rtl_433 without a config file

That’s a somewhat unhelpful statement in this context. I’m prone to saying things like that too. But let’s take a step back for some reflection on this.

——

This project has got to be getting close to 10 years old by now if it isn’t already. I’m pretty sure I have data files and maybe commits from 2015. Things have changed since then.

The user community for rtl_433 has grown since the addition of Home Assistant and its add-ons with rtl_433 and the auto discovery script packaged in somewhat ready to run containers. It would be nice if there were some usage metrics to help measure this.

For this segment of the user community, for better or worse rtl_433 and the auto discovery script are conflated into one service. This comes up often when people ask for help. The technical barriers to entry have been lowered. There is some expectation of an experience similar to using Zigbee networks.

From an rtl_433 developer’s perspective, the auto discovery script is just one of several examples and is a second or third class concern. While this is somewhat understandable, it can be frustrating to a segment of the user population. It might be time for some of these examples to be considered more like a companion app rather than an example.

So breaking the questions and concerns down:

Example/Companion apps reading rtl_433 configuration

I think it is pretty easy to agree that it makes sense, where possible, to avoid redundant configuration like MQTT connection information, topic strings, also possibly frequency, what protocols are enabled/disabled, etc.

Example/Companion apps storing their configuration information in the same place as rtl_433

This is where any disagreement starts. While the title of this PR is “read”, the objection is on sharing the config file.

From an rtl_433 developer’s perspective, it is easy to see an objection to having other configuration data stored together that might at some point cause some sort of conflict.

However, from a user’s perspective, it is hard to understand and explain why there are two configuration files and how to figure out which one to put configuration information in.

If there was a design for the configuration syntax/grammar to allow clear delineation (namespaces?) then it might be possible to store the union of both sets of configuration data in one place and avoid conflict.

I think it would be useful to get @merbanan and @zuckschwerdt views on this.

Another alternative for apps that use/embed rtl_433

An application that manages rtl_433 and other processes could just have it’s own single configuration source that is used to generate what ever files or command line arguments are needed for rtl_433 and the other processes.

Problems with the hass auto discovery script

It is probably worth mentioning here that some of these issues arise because the hass auto discovery script is somewhat of a quick hack rather than a well thought out application. It winds up being useful enough that it doesn’t get replaced. It probably works OK in 60-80% of the common cases.

Also because it is primarily used when getting started, once things are working well enough it tends to be forgotten. It’s no longer needed if the configuration is retained and no new devices are added.

rct avatar Feb 20 '24 16:02 rct

Probably we should think about "is our stance that these are examples reasonable". They are more than that, and I think if we elevate them to first-class, then we have a different situation.

What I don't like is merging things while out of the other side of our mouths saying that examples are 2nd/3rd class.

As for mqtt config, I sort of view it as a bug that rtl_433 directly does mqtt, vs using a helper program. Probably there should be a single program that receives json over udp and does all the mqtt including the config topics, optionally.

So I think we're already not in a great place and it would be better to dig out of it instead of blurring the primary program and examples. I am not sure it would be that hard.

gdt avatar Feb 20 '24 16:02 gdt

As for mqtt config, I sort of view it as a but that rtl_433 directly does mqtt, vs using a helper program. Probably there should be a single program that receives json over udp and does all the mqtt including the config topics, optionally.

Given that this PR just adds the ability to read the MQTT configuration from the rtl_433 config file to eliminate redundant configuration that seems to be looking for a whole different battle.

@zuckschwerdt added MQTT, HTTP, InfluxDB, etc. a while ago. If you want to change that, it's not going to happen by rejecting this PR.

If you consider the auto discovery script to be just part of the collection of examples, this PR should stand on it's own merits against that example. It doesn't seem like it is going to negatively impact rtl_433 architecturally.

rct avatar Feb 20 '24 18:02 rct

As it was mentioned here, rethinking/modularizing the architecture towards (external) modules that reformat/aggregate/... data before passing it out (e.g. by MQTT) is a goal. It seems useful with the complexities of matching MQTT to some eco-systems, or e.g. with problems in representation like Influx, where we now need to decide wether something is "data" or "label" and can easily configure it.

zuckschwerdt avatar Feb 20 '24 18:02 zuckschwerdt

Note: the commit https://github.com/merbanan/rtl_433/pull/2841/commits/10e5ea9ece5bf2e73a7e035d0fec8d89bca33886 from this PR should fix issue #2840.

Right now it looks like there are two fix related commits in this PR, and two commits for adding the feature of pulling the MQTT connection details from the rtl_433 config file: https://github.com/merbanan/rtl_433/pull/2841/commits/aa313f5d15c8d8ad76a6b346541ce0f0ce50eff8 (and the logic for finding the config file https://github.com/merbanan/rtl_433/pull/2841/commits/c3601900bf5c6c3b59753d724d58520681b88846 )

With regards to the fix related commits:

@klattimer do you have an example/test case of what https://github.com/merbanan/rtl_433/pull/2841/commits/f109051b984dedc32a870cef3e5c4a66a2d1eada fixes?

@deviantintegral - if you get a chance, it would be good if you could look over that change.

With regards to the rest of the discussion here about coupling between rtl_433, the auto discovery script, and MQTT:

This PR probably would have generated less notice if it had been clearer that the goal is read the MQTT connection information from rtl_433's config file and not any larger coupling between the two.

(I'd suggest renaming the PR to rtl_433_mqtt_hass.py: Read MQTT connection configuration from rtl_433.conf. )

Further discussion about architectural direction should probably move to #1964 and/or some other appropriate discussion.

rct avatar Feb 20 '24 21:02 rct

I haven't done a line-by-line review, but at a high level:

  1. It would be great to get the mqtt library changes in a separate PR. I think that could be quickly reviewed and merged.
  2. In the current HA addon, we assume most users are running an MQTT broker inside of Home Assistant. If you're not running MQTT as an HA addon, then you're probably running with a manual docker-based setup in which case you don't need the HA addons at all - just use docker containers directly. Since we can make that assumption, by default we grab the mqtt connection details from Home Assistant's API. So, we wouldn't get much benefit from this feature, but since it's optional it wouldn't hurt us either (beyond any long-term code maintenance concerns).

I'll also say that I've been thinking a bit about what it would look like to connect to rtl_433 over a websocket instead of mqtt. zwavejs does that, and it's pretty great especially for new users. I'm nowhere near close enough to actually doing anything about it, but it's a thought!

deviantintegral avatar Feb 24 '24 00:02 deviantintegral

@deviantintegral - Thanks for the info. I'd like to see this PR merged at least for the fixes and I don't see it forcing some implicit architectural change.

(I don't use the add-ons, my rtl-sdrs are attached to RPIs that are separate from my Home Assistant installation, and the discovery script only needs to be run on demand for new devices. I'm using the MQTT add on on my Hass instances, but have to configure connection details manually without access to the API.)

The rest of this belongs in a separate discussion:

I've been thinking a bit about what it would look like to connect to rtl_433 over a websocket instead of mqtt.

So what are you envisioning here in terms of connection topology for the various components and what would the benefits be?

Are you thinking web socket just for the auto-discovery script or also the data path from rtl_433 into Home Assistant?

FWIW, I run the ZwaveJS UI add on (Formerly known as ZwaveJS2MQTT). I'm only using the web socket data path for Home Assistant, but also have ZwaveJS UI publish to MQTT so I get access to some of the bits that the Home Assistant integration doesn't (or didn't) expose well (at the time) like Zwave lock events and parameters (user codes, etc.)

I also run Zigbee2MQTT rather than ZHA. I mention this because I think for rtl_433 there is probably more in common withZigbee2MQTT--It has to deal with a wider range of devices don't adhere to standards well. It uses a library of "converters" for adding device support and can more easily handle new/ad-hoc devices. https://www.zigbee2mqtt.io/advanced/support-new-devices/01_support_new_devices.html#_2-2-adding-converter-s-for-your-device

Also ZwaveJS depends on the ZwaveJS integration running inside of Home Assistant. Zigbee2MQTT relies completely on MQTT devices. Both are used by HA platforms other than just Home Assistant.

If I were a NodeJS developer, I'd be taking a look at how much of Zigbee2MQTT could be used to solve the gaps/desires around getting rtl_433 data into home automation platforms.

rct avatar Feb 24 '24 17:02 rct

I should have pointed out that I don't use the builtin mqtt support.

See also #3013. Having thought more I'm ok with trying to find config in the rtl_433 config file, as long as there is a configf file possibel for the hass bridge, for things that don't belong in the rtl_433 file, or for people that want them separate.

gdt avatar Jul 29 '24 15:07 gdt