gdoor-org / gdoor

Wifi adapter and bus protocol documentation for the Gira TKS Door System
https://gdoor-org.github.io/
GNU General Public License v3.0
14 stars 4 forks source link

Refactor JSON and high level parsing #6

Closed DaSchaef closed 2 months ago

DaSchaef commented 2 months ago

This branch should:

  1. Fix JSON compliance #4
  2. Add a high level bus message #5

Needs testing, as I currently do not have a working bus test setup.

jschroeter commented 2 months ago

Thanks! I fixed a few minors, now the output is valid JSON.

DaSchaef commented 2 months ago

Thanks! I just pushed

jschroeter commented 2 months ago

Great, it works now! But it outputs two lines with JSON for each action, one low and one high level. I still think it would be better to combine both into one JSON, so each action triggers exactly one JSON output. But we could still leave it as separate logic.

jschroeter commented 2 months ago

After playing a bit with using the high level output in Home Assistant I think we should convert the parameters, source and destination to one numeric value for each instead of an array with hex values. So far I couldn't get it working with an array of hex values. Could you do that or do you have a different idea? Thanks.

jschroeter commented 2 months ago

I think we should convert the parameters, source and destination to one numeric value for each instead of an array with hex values.

Or we just concat the hex values to a string like the hexvalue field. Actually I think this is better, most importantly it should be one field to easily filter for. I just pushed this change.

DaSchaef commented 2 months ago

What about the following commits, combining both json on high level?

jschroeter commented 2 months ago

Combining is good, but just from a Home Assistant point of view it should be all flat on one level. Processing nested structures is possible but then it gets difficult. I pushed a proposal were everything is on the same level.

Unfortunately I had to disable the raw field as for some reason it breaks the HA integration. Not sure why, I guess it's just reaching a size limit. But one question on this: what value do I get out of the fields data, raw and valid? Honestly, so far, I personally only see a use case for the hexvalue field, this is very useful to copy the command to then send the exact same one back to the bus. But the others? So far I don't check the valid field, as it always returned true. Maybe we could omit data, raw and valid by default, but provide an configuration option to enable a "debug" mode where these fields are sent?

And on that note: I'm thinking about if we really do need to support JSON input. I guess it's fine to say the input always has to be in the hexvalue format since typically you just want to repeat a command that you received before.

DaSchaef commented 2 months ago

Combining is good, but just from a Home Assistant point of view it should be all flat on one level. Processing nested structures is possible but then it gets difficult. I pushed a proposal were everything is on the same level.

Ok, i think we can do this, I have some implementation ideas for it, see below.

Unfortunately I had to disable the raw field as for some reason it breaks the HA integration. Not sure why, I guess it's just reaching a size limit. But one question on this: what value do I get out of the fields data, raw and valid? Honestly, so far, I personally only see a use case for the hexvalue field, this is very useful to copy the command to then send the exact same one back to the bus. But the others? So far I don't check the valid field, as it always returned true. Maybe we could omit data, raw and valid by default, but provide an configuration option to enable a "debug" mode where these fields are sent?

  • data: array of bus bytes (content is duplicated with hexvalue)
  • raw: raw bit counts, only useful for debugging
  • valid: It's good that you always get true ;-) This field indicate that the checksum and parity bit in the raw bit stream were correct. You can get a false, if e.g. the checksum or any of the parity bits failed.

We can filter out invalid frames and the raw fields for HA. But these fields are very useful for debugging or (very) low level playing with the bus. In principle:

For MQTT I hope we just can send both to different topics, as it is up to the user to subscribe.

And on that note: I'm thinking about if we really do need to support JSON input. I guess it's fine to say the input always has to be in the hexvalue format since typically you just want to repeat a command that you received before.

If it is OK for novice users to use hexvalue I would like to skip JSON input. Just because we can skip a library dependency :)

jschroeter commented 2 months ago

Sounds great! Feel free to implement it, I probably won't have any more time until the weekend.

MQTT: yes, I also think there we can easily provide both.

Agreed with the JSON input - let's not plan it for now; we can always add it later if really needed.

jschroeter commented 2 months ago

Just noticed one issue with my current simple setup (Home Assistant serial sensor): the HA entity (sensor) stores the last received action from the bus as its state. If the same action is received again, it won't update nor trigger any automations, since the state is unchanged. This is usually a nice feature of HA, but in this case not the wanted behaviour. Not sure how this is handled on other platforms; I don't really want to implement HA-specific behaviour in the adapter. Currently I do see 3 options:

  1. solve this somehow in HA, e.g. reset the state in the automation after an action is processed. Should work but I couldn't get it running yet.
  2. send a unique value in each JSON, e.g. a random id. Might be acceptable I guess.
  3. send e.g. an empty JSON right after each action, which seems weird. (this was why it worked before as we were still sending the debug output between the JSONs)

@DaSchaef @ravenst0ne do you know how updates of identical values are handled in OpenHAB and ioBroker?

DaSchaef commented 2 months ago

In Openhab you can decide if you want to listen to all or changes only. I'll merge this branch, as I think it will be compatible to HA now.

See firmware/README for details (if needed, we can fine tune or change further of course).

jschroeter commented 2 months ago

Works perfectly, thanks!