Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Read rtl_433_mqtt_hass.py configuration from rtl_433.conf #2841

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

klattimer
Copy link
Contributor

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

@gdt
Copy link
Collaborator

gdt commented Feb 20, 2024

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?

@klattimer
Copy link
Contributor Author

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.

@gdt
Copy link
Collaborator

gdt commented Feb 20, 2024

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.

@klattimer
Copy link
Contributor Author

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.

@rct
Copy link
Contributor

rct commented Feb 20, 2024

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.

@gdt
Copy link
Collaborator

gdt commented Feb 20, 2024

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.

found_config = True
break
except FileNotFoundError:
logging.warning("Config file not found")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be helpful to be less ambiguous about what config file you are talking about in this and the other log messages.

I would explicitly mention that it is rtl_433's config file and/or the path.

Some of the Home Assistant add-ons have their own config files for this script.

except FileNotFoundError:
logging.warning("Config file not found")
except Exception as e:
logging.exception("Error reading config file")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above, be explicit about the file.


if found_config:
if host:
logging.info(f"Using host {host} from config")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as about, be more explicit about the source, "from config" is a bit ambiguous.

@rct
Copy link
Contributor

rct commented Feb 20, 2024

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.

@@ -943,6 +964,8 @@ def bridge_event_to_hass(mqttc, topic_prefix, data):
published_keys = []

base_topic, device_id = rtl_433_device_info(data, topic_prefix)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor nit, but it feels a bit unclean that you didn't remove the old assignment to base_topic.

I know this whole script needs work, but If you don't want to refactor rtl_433_device_info, then just use:

_, device_id = rtl_433_device_info(data, topic_prefix).

The underscore seems to be the prevailing convention for a value you don't care about when unpacking a tuple during assignment.

@zuckschwerdt
Copy link
Collaborator

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.

@rct
Copy link
Contributor

rct commented Feb 20, 2024

Note: the commit 10e5ea9 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: aa313f5 (and the logic for finding the config file c360190 )

With regards to the fix related commits:

@klattimer do you have an example/test case of what f109051 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.

@deviantintegral
Copy link
Contributor

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!

@rct
Copy link
Contributor

rct commented Feb 24, 2024

@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.

@gdt
Copy link
Collaborator

gdt commented Jul 29, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants