-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
base: master
Are you sure you want to change the base?
Conversation
…void breaking old code. Retain must be an integer.
…rectly, replaced partially, but it requires more love.
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? |
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. |
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. |
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. |
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. |
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") |
There was a problem hiding this comment.
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") |
There was a problem hiding this comment.
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") |
There was a problem hiding this comment.
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.
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) |
There was a problem hiding this comment.
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.
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. |
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. |
I haven't done a line-by-line review, but at a high level:
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 - 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:
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. |
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. |
As the title suggests, a reasonably self contained change to load the configuration for HASS from the rtl_433.conf file if it exists.