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

Fix HA sensor discovery with MQTT #1957

Closed
wants to merge 1 commit into from

Conversation

kiddouk
Copy link

@kiddouk kiddouk commented Oct 23, 2019

Before this patch, the config object (and its JSON) was reused across the sendSwitch and sendMagnitude. This led to a situation that advertising for the sensors includes some old keys (state_command, payload_on, ...) to be sent. Which leads to HA ignoring the sensor advertisement with

Exception in async_discover_sensor when dispatching 'mqtt_discovery_new_sensor_mqtt': ({'name': 'switch_fridge_reactive', 'state_topic': 'switch-fridge/reactive', 'payload_on': '1', 'payload_off': '0', 'availability_topic': 'switch-fridge/status', 'payload_available': '1', 'payload_not_available': '0', 'unique_id': 'ESPURNA-XXXXX_reactive_3', 'device': {'identifiers': ['ESPURNA-XXXXX'], 'name': 'Switch that monitors the fridge', 'sw_version': 'ESPURNA 1.13.6-dev (2.3.0)', 'manufacturer': 'TECKIN', 'model': 'SP22_V14'}, 'unit_of_measurement': 'W', 'platform': 'mqtt'},)
Traceback (most recent call last):
  File "/usr/lib/python3.6/site-packages/homeassistant/components/mqtt/sensor.py", line 81, in async_discover_sensor
    config = PLATFORM_SCHEMA(discovery_payload)
  File "/usr/lib/python3.6/site-packages/voluptuous/schema_builder.py", line 272, in __call__
    return self._compiled([], data)
  File "/usr/lib/python3.6/site-packages/voluptuous/schema_builder.py", line 594, in validate_dict
    return base_validate(path, iteritems(data), out)
  File "/usr/lib/python3.6/site-packages/voluptuous/schema_builder.py", line 432, in validate_mapping
    raise er.MultipleInvalid(errors)
voluptuous.error.MultipleInvalid: extra keys not allowed @ data['payload_on']

This patch is intended to clean that. Since no data has be shared between sendSwitch and sendMagnitude except the common info, each function now uses it own config object (at the price of 2 inits when advertising in mqtt).

Impact: RAM and CPU.

Rational of this patch: I could have removed the Json keys that we didn't need, but I found this kind of manipulation rather dirty and unwelcoming for new developers that will have to understand the logic.

@mcspr
Copy link
Collaborator

mcspr commented Oct 24, 2019

As already mentioned in gitter, we should probably just re-use some key as the root instead of using the global one.

Note that directly removing keys is probably a bad idea memory-wise. For example:
https://gist.github.com/mcspr/901a431696b5e207bf33a0bd1246a91f#file-runme2-txt
We keep the same structured data with the same exact keys, overwriting in the loop. Buffer does not allocate any new space during this process.

But, if the object is re-created under the same key:
https://gist.github.com/mcspr/901a431696b5e207bf33a0bd1246a91f#file-runme3-diff
https://gist.github.com/mcspr/901a431696b5e207bf33a0bd1246a91f#file-runme3-txt
It will bump the size every time

@mcspr
Copy link
Collaborator

mcspr commented Oct 24, 2019

And I already had basic draft of structs for switch & sensors, which I now have here rebased on the recent dev:
dev...mcspr:ha/ext-structs
The end goal was to do the same thing that shared "config" struct does and keep everything outside of jsonbuffer, while the jsonbuffer itself only stores pointers to data or basic types with constant size requirements.
While it does achieve the intended reduction of jsonbuffer usage, I wonder how it would be readable after a month or so :) I do, however, like the structs instead of json objects.

@kiddouk
Copy link
Author

kiddouk commented Oct 26, 2019

I got to review your branch and I do agree with the above comment. Nice to move out of JsonBuffer but the increased complexity makes it hard to sell.

Your implementation shows that need to recompute name and unique_id (hard to avoid) which was part of the reason why I thought that a super simple implementation would just do the trick.

I could change my patch to meet half way. We have a shared device object that gets injected in sensor_root and switch_root and we decode the appropriate object when we advertise.

What do you think? I'd be happy to implement that if you give your green light.

@mcspr
Copy link
Collaborator

mcspr commented Oct 26, 2019

Maybe. I can think on my changes for some time, but we can still apply the specific fix using separate roots for 1.13.6.

Instead of directly using pre-defined root object as storage, haSendMagnitudes/haSendSwitches can either create an another temporary object in the config.jsonBuffer (since we send everything at once, just before loop) or use subkey.
Buffer will still grow with each relay or magnitude addition, since the things we store are always copied, but it should not exceed 2KB size limit in most cases

The main reason why I also want that change with generic map is to also use it in yaml generation as-is. And also name ha_config_t -> ha_ctx_t (since this is the thing I made it do)
yaml config is still a useful thing to have, fwiw it is what i use to run some general tests with development install of hass

@kiddouk
Copy link
Author

kiddouk commented Oct 28, 2019

ok, that makes sense and I understand your intent regarding the yaml generation. I agree that giving a super simple yaml instructions are the way to help new-comers to integrate Espurna with HA.

mcspr added a commit to mcspr/espurna that referenced this pull request Nov 1, 2019
- stack-like discovery struct to store pending mqtt topic and
message
- use separate json objects for sensor and switch data (different solution for the xoseperez#1957)
@mcspr mcspr mentioned this pull request Nov 1, 2019
@mcspr
Copy link
Collaborator

mcspr commented Nov 9, 2019

Closing via #1969
When you have time, please see if it broke anything important :)

@mcspr mcspr closed this Nov 9, 2019
@mcspr mcspr mentioned this pull request Feb 14, 2020
34 tasks
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.

2 participants