#751 and #759

[proddy]

No description provided.

[MichaelDvP]

A lot of breaking changes for HA, but there are some thing to think about:

1. The boottime is now send on every ntp connect/disconnect, but on a wifi reconnect it is removed. This will give a HA complain. Better leave it with ntp_connected() and in NTPSettingService. Multiple sending does not hurt, the boottime is corrected by uptime and stays the same. If you want reduce to a single ntp-trigger you can use something like:

static bool firsttime= true;
if (firsttime) {
    emsesp::EMSESP::system_.send_info_mqtt("connected");
   firsttime = false;
}

2. The HA autoconfig is a bit mixed now. As far as i've seen you change a lot of things. The path for config does not contain the basename anymore, eg. homeassistant/number/ems-esp/thermostat_hc2_daytemp/config becomes homeassistant/number/ems-esp_thermostat_hc2_daytemp/config. The removal/cleanup by subscribing to homeassistant/#/ems-esp/* does not work anymore, You can't use a wildcard subscription now without removing non-ems-esp configs.
3. The commands will not work, the basename is doubled in "~/%s" because "~" and uniq_id contains it. Also the replace('_','/') could make trouble if the shortname contains a '_' (not the case for now).
4. Now you have "uniq_id" and "object_id" the same with the shortname, good idea because the shortname always have to be unique for mqtt. The en-fullname is not used anymore and can be removed from this function.
5. In system-status, heartbeat, dallas, analog the "uniq_id" and "object_id" are not the same, this should be in line. Also the basename prefix is missing there, confusing, and give collision for multiple ems-esp in one system.

[proddy]

Thanks for the review

1. The boottime is now send on every ntp connect/disconnect, but on a wifi reconnect it is removed. This will give a HA complain. Better leave it with ntp_connected() and in NTPSettingService. Multiple sending does not hurt, the boottime is corrected by uptime and stays the same. If you want reduce to a single ntp-trigger you can use something like:

I changed it, like you said no harm in sending every hour

2. The HA autoconfig is a bit mixed now. As far as i've seen you change a lot of things. The path for config does not contain the basename anymore, eg. homeassistant/number/ems-esp/thermostat_hc2_daytemp/config becomes homeassistant/number/ems-esp_thermostat_hc2_daytemp/config. The removal/cleanup by subscribing to homeassistant/#/ems-esp/* does not work anymore, You can't use a wildcard subscription now without removing non-ems-esp configs.

I couldn't reproduce that. It worked fine in my setup.

3. The commands will not work, the basename is doubled in "~/%s" because "~" and uniq_id contains it. Also the replace('_','/') could make trouble if the shortname contains a '_' (not the case for now).

fixed that , thanks

4. Now you have "uniq_id" and "object_id" the same with the shortname, good idea because the shortname always have to be unique for mqtt. The en-fullname is not used anymore and can be removed from this function.

en-fullname removed, good catch.

5. In system-status, heartbeat, dallas, analog the "uniq_id" and "object_id" are not the same, this should be in line. Also the basename prefix is missing there, confusing, and give collision for multiple ems-esp in one system.

I've fixed that too.

Now just need some small tests and I'll check in the latest updates to this PR

[MichaelDvP]

I couldn't reproduce that. It worked fine in my setup.

See https://www.home-assistant.io/integrations/mqtt/#mqtt-discovery

The <node_id> level can be used by clients to only subscribe to their own (command) topics by using one wildcard topic like <discovery_prefix>/+/<node_id>/+/set.

The node_id (basename) is skipped in your new implementation, but here

EMS-ESP32/src/mqtt.cpp

Lines 558 to 563 in 7144c74

	// with disabled HA we subscribe and the broker sends all stored HA-emsesp-configs.
	// Around line 272 they are removed (search for "// remove HA topics if we don't use discover")
	// If HA is enabled the subscriptions are removed.
	// As described in the doc (https://emsesp.github.io/docs/#/Troubleshooting?id=home-assistant):
	// disable HA, wait 5 minutes (to allow the broker to send all), than reenable HA again.
	queue_subscribe_message(discovery_prefix_ + "/+/" + mqtt_basename_ + "/#");

we need the basename as node_id in the topic.

Your HA works, but the cleanup if you disable HA discovery not.

Also i think in mqtt (explorer or other viewers) it's better to have all configs of one HA-node together. like this

[proddy]

still not following you 100%

the node is our mqtt base, and it follows the same folder structure as your screenshot.

I made some changes, but haven't tested as I don't have HA right now (on a plane). From the output, it looks ok. I did a quick test earlier and found out the subscribe wasn't working, like for example sending a value to the topic ems-esp/thermostat/hc1/seltemp doesn't work for some reason. Haven't found out why.

Also I couldn't get the send command to work either.

I'll pick it up again on the weekend

Wish I never started stupid #759 !!

[MichaelDvP]

still not following you 100%

You're right, sorry, i dont know what i have seen, but now i've tested and it's ok.

A few last things:

• analogsensors: object_id and uniq_id should use gpio and analogsensoras in path (not analog_sensor)
• dallassensor: sync path/object_id/uniq_id to dallassensor (not temperature_sensor)
• system_status: sync path/object_id/uniq_id to system_status (path is now only system and object_is is different to uniq_id)

And a question:
in dallassensor the sensorid in path is marked with // use '_' as HA doesn't like '-' in the topic name, but the basename is default ems-esp and seems to work in path? What is right? Has it changed with HA versions?
https://www.home-assistant.io/integrations/mqtt/#mqtt-discovery
says:

<discovery_prefix>/<component>/[<node_id>/]<object_id>/config
<object_id>: The ID of the device. This is only to allow for separate topics for each device and is not used for the entity_id. The ID of the device must only consist of characters from the character class [a-zA-Z0-9_-] (alphanumerics, underscore and hyphen).

[proddy]

@MichaelDvP I need your help. With this PR (version b11) the MQTT subscribe is not working anymore and I can't figure out why. It works in b10 and if I compare this PR against it I see nothing that would break the subscribe. This is without HA. For example, sending a publish to ems-esp/system/test is picked up in b10 but not in b11. I can see the MQTT broker receives the subscribe request so on the server everything looks solid. Also no library updates. I've spent 2hrs trying to figure this out, and I'm sure it's something stupid I've overlooked. very frustrating! If you have some time could you take a look please?

[MichaelDvP]

I'll take a look when i'm back home, tomorrow evening or mo morning.

[proddy]

I'll take a look when i'm back home, tomorrow evening or mo morning.

Thanks, it's driving me nuts and just can't figure it out. To reproduce

1. Build this PR (EMS-ESP b11) (building with EMSESP_DEBUG in pio_local.ini gives more info as you know)
2. Turn off HA discovery in MQTT Settings so it's plain vanilla MQTT
3. Use MQTTExplorer to publish anything to one of the subscribed topics like ems-esp/system or ems-esp/analogsensor
4. you should see it being picked up on the log and console as a MQTT topic received with the contents of the package

only with b11 (this PR) step 4 doesn't comes up with nothing I don't know why since most of the changes are HA related, except for the NTP time in the info topic.

[proddy]

I'll take a look when i'm back home, tomorrow evening or mo morning.

Thanks, it's driving me nuts and just can't figure it out. To reproduce

1. Build this PR (EMS-ESP b11) (building with EMSESP_DEBUG in pio_local.ini gives more info as you know)
2. Turn off HA discovery in MQTT Settings so it's plain vanilla MQTT
3. Use MQTTExplorer to publish anything to one of the subscribed topics like ems-esp/system or ems-esp/analogsensor
4. you should see it being picked up on the log and console as a MQTT topic received with the contents of the package

only with b11 (this PR) step 4 doesn't comes up with nothing I don't know why since most of the changes are HA related, except for the NTP time in the info topic.

@MichaelDvP I found the error. I had removed maxTopicLength from MqttSettingsService, now I've added it back and hardcoded to the default.

[proddy]

merging...

[MichaelDvP]

I've tested now the latest dev, the maxTopicLength is only used here:

EMS-ESP32/lib/framework/MqttSettingsService.cpp

Line 140 in d080f5d

_mqttClient.setMaxTopicLength(_state.maxTopicLength);

We can only set this line to FACTORY_MQTT_MAX_TOPIC_LENGTH (also tested)

I'm not happy with changing the command log to INFO, With Thomas' iobroker-adater getting device values every10 sec the log is flooded.

I'll make a few changes in a PR.

[proddy]

I'm not happy with changing the command log to INFO, With Thomas' iobroker-adater getting device values every10 sec the log is flooded.

good point, forgot about that. Can you change it back to LOG_DEBUG(("Calling command.... then and add a comment so I remember in case I decide to change it again :-)