search

pbkhrv / rtl_433-hass-addons

Collection of Home Assistant add-ons that use rtl_433
217 stars 102 forks source link

Update Dockerfile #160

Closed catduckgnaf closed 8 months ago

catduckgnaf commented 9 months ago

This is a link to a separate contained discovery script, with more additions. It should work with next and non next.

Summary

Alternatives Considered

Testing Steps

  1. First...
  2. Then...
  3. You should see... (screenshots or console text are helpful)
deviantintegral commented 8 months ago

I'm a little hesitant to completely diverge the autodiscovery script from the upstream rtl_433 repository and pull in "with more additions" without clarity on what those are. The upstream rtl_433 has many more eyes and overall activity, and I think we benefit from their work. On diffing the current master of your version of the autodiscovery script addon, I didn't see exactly where this is fixed. If this is fixed by changing one of our files, I'd like to just get it committed here. And if it's in the python script, it would be good if we can file an issue or PR upstream.

Ninja1283 commented 8 months ago

@deviantintegral reviewing against the upstream rtl_433_mqtt_hass.py file, there was some documentation cleanup/clarification and a few minor mapping edits, but the main edits were at line 868 reversing the if-else logic, line 966 adding the discovery_ids , and line 996, removing run().

Agreed that this should be a PR upstream, if they fix the issues.

catduckgnaf commented 8 months ago

Since "upstream" doesn't care about breaking things for us, and I didn't see the problem with cloning the script, checking changes, and making sure it doesn't break things.

My head hurts looking at their requirements for a PR with such a big project. The script has so many issues most never use it.

Anyway, I'm gonna close my request. I also think its silly to leave a non working add-on in place for months, you could always submit it back when/if they fix it.