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

Conform to Home Assistant Discovery Spec #475

Closed
wants to merge 1 commit into from

Conversation

ccutrer
Copy link

@ccutrer ccutrer commented Sep 20, 2024

Description:

availability is documented as a list (of objects), not a single object. Just use the simpler availability_topic instead.

Fixes openhab/openhab-addons#17375

Checklist:

  • The pull request is done against the latest master branch
  • Only relevant files were touched
  • Only one feature/fix was added per PR and the code change compiles without warnings
  • The code change is tested and works
  • I accept the CLA.

NOTE: The code change must pass CI tests. Your PR cannot be merged unless tests pass

`availability` is documented as a list (of objects), not a single object. Just use the simpler availability_topic instead.
@alexdelprete
Copy link
Collaborator

alexdelprete commented Sep 21, 2024

The implementation perfectly conforms to HA documentation: a one item list is a list.

This is the code right now:

 json["avty"]["t"] = _lockPath + mqtt_topic_mqtt_connection_state;

and it generates the following json in the config topic:

  "avty": {
    "t": "~/maintenance/mqttConnectionState"
  },

in yaml config format it's equivalent to:

mqtt:
  - lock:
      name: Door
      availability:
        - topic: "~/maintenance/mqttConnectionState"

as you can see, avty is a list, and t is the first string object of that list, so perfectly conformant to the documentation, and in fact Home Assistant perfectly recognizes the availability of the lock (it would throw errors otherwise).

There are many examples in HA docs with that one item example. I'd suggest you correct the openhab parser instead, because if it throws an error, it's not conformant to HA documentation.

@ccutrer
Copy link
Author

ccutrer commented Sep 22, 2024

Respectfully, that is not the equivalent yaml (example written in Python, since Home Assistant is Python, and the reference implementation):

#!/usr/bin/env python3

import json
import yaml

# Sample JSON string
json_string = '''
{
  "avty": {
    "t": "~/maintenance/mqttConnectionState"
  }
}
'''

# Parse the JSON string
data = json.loads(json_string)

# Convert to YAML
yaml_output = yaml.dump(data, default_flow_style=False)

# Print the YAML output
print(yaml_output)

Output:

avty:
  t: ~/maintenance/mqttConnectionState

Notice that there is no - before the t? The - introduces a list. Without, it's simply an object. I agree a list of a single item is still a list, but the JSON produced by nuki_hub is not a list (technically called an array in JavaScript, and a sequence in YAML; it's Python that calls it a list), it's simply an object. Which I've not seen an example of in the Home Assistant documentation - can you point me towards one? I'll also see if I can dig through Home Assistant's source to see if/where it treats the full availability object as a single object or a list.

@ccutrer
Copy link
Author

ccutrer commented Sep 22, 2024

@ccutrer
Copy link
Author

ccutrer commented Sep 22, 2024

Feel free to close this if you feel that strongly about a single line change. I plan on updating openHAB to accept a non-list. It would still be nice if you could accept this change to get @adielsa going right away (I suspect your project is more agile at releases than openHAB). I'll also see if I can get a PR accepted to HA's documentation to clarify that an immediate object rather than a list is also acceptable. I've had good luck with similar clarifying PRs in the past, but this one will be a bit bigger since it's something repeated across all MQTT components.

Alternately, @adielsa, perhaps you could compile your own image of nuki_hub, with this PR, to get yourself going right away? I haven't looked too much into this project beyond finding its discovery code, so I'm not sure if compiling it yourself is common.

Copy link
Collaborator

@alexdelprete alexdelprete left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with your analysis, and I apologize for my mistake.

After checking the ArduinoJson Assistant I'd modify like this, so we keep using availability:

json["avty"][0]["t"] = availabilityTopic;

It will produce the following json structure:

{
  "avty": [
    {
      "t": "~/maintenance/mqttConnectionState"
    }
  ]
}

Which is a list. :)

@alexdelprete
Copy link
Collaborator

I found another small bug a few lines after the availability topic line. Closing this in favor of #476.

Thanks for the heads-up.

@ccutrer
Copy link
Author

ccutrer commented Sep 22, 2024

👍

I appreciate the discussion. I’m always glad when we can improve compatibility and ease-of-use for users of any home automation platform.

@technyon
Copy link
Owner

#476 is merged

@alexdelprete
Copy link
Collaborator

I appreciate the discussion.

I did too. And sorry for the initial misunderstanding.

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.

[mqtt.homeassistant] availability doesn't have to be a list
3 participants