dvdgeisler / DirigeraClient

Dirigera Client API: Java written client API to interface IKEA's new smarthome hub DIRIGERA
MIT License
74 stars 9 forks source link

What else do we need for a 0.1.0 release? #53

Closed TheMrBooyah closed 1 year ago

daveabbott007 commented 1 year ago

My little wishlist, Device names in log entries Direction of log entries (i.e from Dirigera or to Dirigera)

dvdgeisler commented 1 year ago

I would like to implement the websockets in the MQTT bridge first. Right now its polling the Dirigera endpoint, which is kind of hacky.

dvdgeisler commented 1 year ago

Device names in log entries

I thought about changing the logging to JSON format. This is less readable, but its straight to log more contextual information. It would also enable easy integrate into ELK. What do you guys think about this?

TheMrBooyah commented 1 year ago

The reason I'm asking is so I can update the add-on in home assistant with a more traceable version. Now it just uses the "latest" available every time I run an update.

It's not the highest priority atm but it would make it easier to keep track of what bugs have been implemented and are in the add-on

TheMrBooyah commented 1 year ago

I thought about changing the logging to JSON format. This is less readable, but its straight to log more contextual information. It would also enable easy integrate into ELK. What do you guys think about this?

How about a toggle? Like you can toggle having the JSON format with more information for integration with ELK or even Elastic or a normal log for people that don't need the extensive information. This normal log would only display basic stuff like what message goes where with a basic set of information (like we have now)

dvdgeisler commented 1 year ago

I thought about changing the logging to JSON format. This is less readable, but its straight to log more contextual information. It would also enable easy integrate into ELK. What do you guys think about this?

How about a toggle? Like you can toggle having the JSON format with more information for integration with ELK or even Elastic or a normal log for people that don't need the extensive information. This normal log would only display basic stuff like what message goes where with a basic set of information (like we have now)

A reasonable idea. Log4J is very flexible in this terms. Its also possible to log classic messages on stdout and write json to a log file.

TheMrBooyah commented 1 year ago

A reasonable idea. Log4J is very flexible in this terms. Its also possible to log classic messages on stdout and write json to a log file.

In terms of compatibility with the docker add-on I rather not have anything being written to a file. However, if you go for log4j I can just overwrite the configuration for the logging with a custom one for docker that sends those directly to an ELK instance. This obviously would only happen if the toggle is turned on and the default would be normal stdout for the add-on

dvdgeisler commented 1 year ago

In this case, i would argue keeping it simple and using the protocol format we already have. We can certainly refine the messages. I don't think ELK is a thing right now (except for myself). Docker/Hass should work with less overhead as possible, though. I expect the largest user base there.

TheMrBooyah commented 1 year ago

I would like to implement the websockets in the MQTT bridge first. Right now its polling the Dirigera endpoint, which is kind of hacky.

In case you need a bit more info about the websocket events: https://pastebin.com/7qG2LRzh

dvdgeisler commented 1 year ago

Websockets on the API is already working. Also, the data model is already available: dirigera-client-api/src/main/java/de/dvdgeisler/iot/dirigera/client/api/model/events. However, Websockets are not yet used in the MQTT module. The only thing missing is the change from polling to an event-driven approach.

If nothing comes up, I'll get to work on it tomorrow.

dvdgeisler commented 1 year ago

It's done. i replaced the polling with websocket events. However, while doing so, i faced a problem that i don't know how to solve properly: If the MQTT connection is interrupted for any reason, all subscriptions are lost after reconnecting. I have found a hook that notifies that the connection was lost, but none that notifies that the reconnect was successful. Therefore, i have no trigger to restore the subscriptions. As a result, the communication from Dirigera to HA works, but not vice versa, after a reconnect.

As a workaround, I implemented it to exit the application if the connection to the MQTT broker is lost. At least in docker, you have the option to restart the container automatically if it goes offline.

TheMrBooyah commented 1 year ago

Isn't there some sort of heartbeat that could be sent? Like with the websocket you've the ping and pong events

dvdgeisler commented 1 year ago

The MQTT client has a keep-alive option, which is also enabled. But it is handled under the hood. The only callback I have found is triggered when it detects that the broker is no longer responding. However, I would need a callback indicating that the connection is restored to resubscribe to the topics.

TheMrBooyah commented 1 year ago

Shall we do the first release? Especially now that we had a major increase in usability and updated to websockets.

This would create a nice baseline for the home assistant add-on :)

dvdgeisler commented 1 year ago

I am just writing the release notes :)

TheMrBooyah commented 1 year ago

It should do it automatically when you push a version like "v0.1.0" as tag :)

dvdgeisler commented 1 year ago

Damn, I used the github UI to do this.... I think I have screwed up now. The lates tag should point to the same commit as the 0.1.0, right?

TheMrBooyah commented 1 year ago

Latest takes the latest master without a tag. (Pre-release)

The other release build actually builds from a tag. You can remove a release and then push a v0.1.0 tag and it'll create a release with release notes being the commit messages

dvdgeisler commented 1 year ago

Im not sure, i created / pushed the tag 0.1.0, but i cant see any action running

dvdgeisler commented 1 year ago
on:
  push:
    tags:
      - "v*"

i guess i have to prefix the tag by "v"

TheMrBooyah commented 1 year ago
on:
  push:
    tags:
      - "v*"

i guess i have to prefix the tag by "v"

Yeah it needs to be "v0.1.0"

dvdgeisler commented 1 year ago

Works like a charm 👍

TheMrBooyah commented 1 year ago

Perfect! Will update the add-on.

Anyone with future improvements/ideas please create a new issue