dib0 / elro_connects

The ELRO Connects uses a propriety protocol on the local network over UDP. This is the reversed engineered python implementation of the protocol.
MIT License
16 stars 7 forks source link

Home assistant auto discovery #17

Closed Skons closed 2 years ago

Skons commented 2 years ago

This PR adds the ability for ELRO Connects to add the ELRO devices automatically to Home Assistant. Users can decide if they want this by only providing -a on the commandline. This makes it easy to use this tool with HA. I am also planning to create a HA addon to run ELRO Connects, it makes this tool more user friendly IMO.

To better support Home Assistant, the property State is now used to show the Alarm state instead of only using "alarm" as MQTT payload. Home Assistant is unable to know the state if the payload differs.

I hope the implementation has been done correctly.

14 is also solved within hub.py line 17. To find the issue, I added some debug logging. To have better insights in what is happening when, the logging now shows timestamps.

depuits commented 2 years ago

Looks pretty good, I do have 2 questions.

  1. Are devices automatically created in HA or do you need to approve them? Because a device might be reporting on its id first and then change topics to the device name. Maybe this should be fixed by always letting devices report to their id instead of the name and only use the name as display and debug info.
  2. While the fix for issue #14 will work won't it create a potential alarm miss if the device was not created yet? (I think you meant that the fix is on line 170)
Skons commented 2 years ago
  1. They are created automatically (it is pretty awesome if you'll ask me). Fair point in regards to id vs name, but that is I think a design problem of ELRO Connects itself. Because if it has no name it will report to base_topic/elro/[id] and as soon as it gets a name, that will change to base_topic/elro/[name]. It would be better that ELRO Connects would always report to [id], so all communication is done on a "static" topic. The properties send by ELRO Connects will eventually contain the name. A problem of the HA auto discover is that the friendly name cannot be changed afterwards. With this de device will be known by its id mostly, and the friendly name can be found in the attributes.
  2. True that, but that is because a device that is not created cannot be reported to (which is why it crashes now, it tries to create a device with data that has no device information). What i can add is that the discovery in HA must have taken place and if the device has not been created yet (due to a reboot or maybe crash of ELRO Connects) the data is sent to the [id] topic (just like above) with only {"state":"alarm"}. If the device is known, all data get to be sent. I do not have enough knowledge of the Hub itself, but if it is possible a device should always be created with a command to the k1 requesting device information of a specific device. In this case the device will always be known. Maybe @dib0 knows if this is possible?

Last but not least, after publishing the code i thought that the discovery topic and data should be more ELRO Connects specific. The data is now sent to sensor.[name], and with all the above it would be better if it is something like sensor.elrok1[id]. All device specific information will show up as attributes. This will make sure a sensor is always the same device, and an alarm can be sent to that sensor even if ELRO Connects has not instantiated the device properly.

hildensia commented 2 years ago

Hi, awesome. Thanks for the contribution. A few thoughts (and you might object or say you won't do it because it's too much change):

  1. We can change to id always as the topic name, I wouldn't object. If you want you can do the change yourself. I'm somewhat busy atm, but I'm happy to review and merge.
  2. Can you add tests for your functionality?
  3. Generally I would love to have some sort of templating (jinja?) for the responses we send. That way everybody could configure their reponses the way they want. We could have an initial publish task and move the HA specific format to a HA-init config file. That way we can integrate similar features for other systems very easily. That would take a) provide a set of useful configs for all responses b) add a function that reads those configs for each response and renders it through a templating eninge (e.g. jinja) c) instead of publishing fixed responses, read a specific config for each response and call above mentioned function d) refactor your auto discovery task to a "initial_publishing_task", that takes a specific response config and publishes it

Let me know if you would be interested in writing that piece, if not we can move on with your change.

Skons commented 2 years ago
  1. Because my PR depends on it, I would be happy to take that on
  2. If i would know how, I would do it. I already had difficulty making it run, because I am not used to the bin structure, and the way it uses pip. Normally I would run the scripts directly and maybe run a debugger on it. The way it is structured, I could not do this. Can you help me pointing me in the right directions? My tests now where done by adding a elro.py into the elro folder and removing "elro." from all imports.
  3. That is a bit much for what I am implementing right now, I even do not have the right knowledge now to pull this off. So if you don't mind I will not add that...
hildensia commented 2 years ago
  1. Cool
  2. The tests use py.test (https://docs.pytest.org/en/6.2.x/, to install run pip install pytest), then you simply run $ pytest in the project folder. It automatically detects the files using their names. If you look at the tests folder and follow the tests in there it should be simple to understand. Especially the test under https://github.com/dib0/elro_connects/blob/a27a881bf4bc57034a08a8e315e8a5a04645e82e/tests/test_mqtt.py#L32 should be a good starting point (you can probably copy paste it and change the necessary parts to call the right function and change the expected output)
  3. Totally fine. Maybe I do it at some point.
Skons commented 2 years ago

Updated the code, with the following improvements

  1. The ELRO device is now always published on the id, not on the name anymore (base_topic/elro/[id])
  2. When a device is not yet known and an alarm is triggered, ELRO Connects will no longer crash. It will even create the device. For Home Assistant the device needs to be discovered for the data to show up, but the data will always be published on base_topic/elro/[id]
  3. I added the property device_name to the mqtt payload. In Home Assistant the property name is already reserved, and because the name is not used in the topic anymore, there was no way to know the friendly name of the device. I did not remove the property name.
  4. Sometimes the socket will not connect (on Windows) with error "[WinError 10022] An invalid argument was supplied". This has been resolved with a retry. A maximum of 3 retries will occur before it really quits (hub.py line 105).
  5. Test routines have been updated

For the latter one, I followed your directions @hildensia but I sill needed to install ELRO Connects. Which means to test ELRO Connects, it needs to be installed after every change (am I correct?). The tests itself also generated errors on my side:

FAILED tests/test_device.py::test_calling_update_fires_updated_event - AttributeError: 'Event' object attribute 'set' is read-only
FAILED tests/test_device.py::test_setting_device_state_fires_updated_event - AttributeError: 'Event' object attribute 'set' is read-only
FAILED tests/test_device.py::test_setting_battery_level_fires_updated_event - AttributeError: 'Event' object attribute 'set' is read-only
ERROR tests/test_mqtt.py::test_handle_device_alarm_sends_alarm_message - AttributeError: 'Event' object attribute 'wait' is read-only
ERROR tests/test_mqtt.py::test_handle_device_update_sends_update_message - AttributeError: 'Event' object attribute 'wait' is read-only
hildensia commented 2 years ago

you can install it in an 'editable' way. That only soft-links all the files and all changes you make are immediately available without reinstalling:

$ pip install -e .