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

Feature/ha discovery #18

Merged
merged 12 commits into from
Jun 16, 2024
Merged

Feature/ha discovery #18

merged 12 commits into from
Jun 16, 2024

Conversation

DaSchaef
Copy link
Contributor

untested, may not work

@DaSchaef
Copy link
Contributor Author

It is now working on my empty, local HA test setup.
There is a major change:

GDOOR now sends:
"BUS_IDLE" after each bus message, so that the sensor falls back to "BUS_IDLE"

Feedback needed, as I am not a HA user.

@DaSchaef DaSchaef marked this pull request as ready for review May 15, 2024 19:07
@jschroeter
Copy link
Collaborator

I gave it a try and it works also for me, nice!

Can you please explain why you did the BUS_IDLE change? I do see some downsides of this, e.g. it's harder to read anything meaningful from the HA history view.
history

Further, the HA Logbook is a bit polluted by the BUS_IDLE which I think doesn't add any value?
logbook

@DaSchaef
Copy link
Contributor Author

DaSchaef commented Jun 8, 2024

I implemented BUS_IDLE because otherwise HA shows a wrong state. E.g. the bus is not ringing, but the web interface shows "ringing".

@DaSchaef
Copy link
Contributor Author

DaSchaef commented Jun 8, 2024

Maybe there are other states similar "online"/"offline" we could use? I'm really not into HA 😅

@nholloh
Copy link

nholloh commented Jun 8, 2024

I think BUS_IDLE is just fine. The issue is just that with the logbook its quite hard to catch the DOOR_OPEN event, which is barely a second long. I think it might be best to leave it as is for now, since we can find out about the bus data through a serial connection, which is only needed once for setup. You need to establish a serial connection anyway for the initial flash, so the issue shouldn't be too big.

To make it more convenient in the long run, we could add an mqtt topic to set the adapter into some sort of learning mode, which would allow it to record and save the next door open event it captures. You could then use another mqtt topic to directly trigger the door open using the previously recorded bus data. Not sure if this needs to be part of this feature or if we should put it on a separate branch.

I can look into the coding tomorrow. Let me know what you think :)

@DaSchaef
Copy link
Contributor Author

DaSchaef commented Jun 9, 2024

We could just send two MQTT topics?

  • status: The behavior of this PR, with BUS_IDLE
  • last_message: The behavior of the current main branch, without BUS_IDLE

?

https://community.home-assistant.io/t/mqtt-message-filtering-with-template/42961/2
We could even use templating in the discovery message and announce two entities, where one filters out the BUS_IDLE. No code change in the ESP needed besides the discovery message?

@DaSchaef
Copy link
Contributor Author

DaSchaef commented Jun 9, 2024

To make it more convenient in the long run, we could add an mqtt topic to set the adapter into some sort of learning mode, which would allow it to record and save the next door open event it captures. You could then use another mqtt topic to directly trigger the door open using the previously recorded bus data. Not sure if this needs to be part of this feature or if we should put it on a separate branch.

I would say it would need a new PR (Changelog, Discussion, Tracking, Bugs etc is easier).
During the HA coding I also thought of this,
but was not sure:

  • How many Door Openers to learn? One? Two?
  • What is with other events, like e.g. Light Button?
  • How to represent it? Button? Sensor?

My problem was the lacl of a good idea about usability.

@sim-san
Copy link

sim-san commented Jun 9, 2024

We could just send two MQTT topics?

  • status: The behavior of this PR, with BUS_IDLE
  • last_message: The behavior of the current main branch, without BUS_IDLE

?

https://community.home-assistant.io/t/mqtt-message-filtering-with-template/42961/2
We could even use templating in the discovery message and announce two entities, where one filters out the BUS_IDLE. No code change in the ESP needed besides the discovery message?

I like the idea of filtering the Messages. So we have one ground truth and different views on this data.

The last 'last_message' could be an entity of the entity category 'diagnostics'. So it will not be added to the dashboard automatically but will still show up in the device page in a card for the category.

See: https://www.home-assistant.io/integrations/sensor.mqtt/#entity_category 16

@jschroeter
Copy link
Collaborator

jschroeter commented Jun 9, 2024

I do understand that it's a bit weird that a previously action stays visible until the next occurs but I still think it's better without BUS_IDLE. HA shows when the value was last updated, so you see when the action happened. It's basically the same as with other sensors, e.g. I have an outdoor temperature sensor which only updates once per hour to save battery. The UI also shows an "old" value between the updates, but I can see when it was last updated.
Afaik there is no real "event" thing in HA and we have to use a sensor for it?

What exact value to the user does BUS_IDLE bring? I don't really get it yet :-) For me it's rather annoying, as said when looking into the logbook, history and for automations, which will generally trigger twice (action + BUS_IDLE; but probably no big issue since usually you'd filter for specific actions/parameters anyway).
The online/offline case should be covered already I think.

The issue is just that with the logbook its quite hard to catch the DOOR_OPEN event, which is barely a second long.

If there is no BUS_IDLE, it's easy to see those in the logbook and sensor attributes?!

...since we can find out about the bus data through a serial connection, which is only needed once for setup. You need to establish a serial connection anyway for the initial flash, so the issue shouldn't be too big.

True, and the web flasher does include a serial terminal where you can see it directly (no need to open a terminal). We could promote this instead of e.g. the HA logbook.

@DaSchaef
Copy link
Contributor Author

DaSchaef commented Jun 9, 2024

You HA user need to comment/decide.

I also found this:
https://www.home-assistant.io/integrations/event.mqtt/

@nholloh
Copy link

nholloh commented Jun 9, 2024

I created a separate issue to how to integrate deeper into home assistant, e.g. by using esphome: #25. I believe with esphome we could more easily address some of the conceptual challenges we are facing here. Needs some validation with openHAB users, though, because any solution we come up with ultimately should work for them as well.

Also, another issue for learning bus messages: #26

@nholloh nholloh mentioned this pull request Jun 9, 2024
@nholloh
Copy link

nholloh commented Jun 9, 2024

To discuss bus_idle I created a separate issue here: #27

@jschroeter
Copy link
Collaborator

I guess we could merge and close this, it's not ideal yet but it works. And improvements are on their way :-)

@DaSchaef DaSchaef merged commit 4a99174 into main Jun 16, 2024
1 check passed
@DaSchaef DaSchaef mentioned this pull request Jun 16, 2024
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.

4 participants