dawidchyrzynski / arduino-home-assistant

ArduinoHA allows to integrate an Arduino/ESP based device with Home Assistant using MQTT.
https://dawidchyrzynski.github.io/arduino-home-assistant/
GNU Affero General Public License v3.0
480 stars 116 forks source link

Concatenate MQTT device id with each sub-device id #186

Closed daniloc closed 7 months ago

daniloc commented 1 year ago

Thanks as always for this great library!

This PR transparently concatenates the MQTT device identifier into device IDs for each sub-device. This allows automatic device discovery and publishing for multiple devices running the same code.

The change is made in the HABaseDeviceType constructor:

HABaseDeviceType::HABaseDeviceType(
    const __FlashStringHelper *componentName,
    const char *uniqueIdSuffix) : _componentName(componentName),
                                  _uniqueId(nullptr), // initialize to nullptr
                                  _name(nullptr),
                                  _serializer(nullptr),
                                  _availability(AvailabilityDefault)
{
    HADevice const *device = HAMqtt::instance()->getDevice();
    if (device)
    {
        const char *mainUniqueId = device->getUniqueId();
        if (mainUniqueId && uniqueIdSuffix)
        {
            size_t totalLength = strlen(mainUniqueId) + strlen(uniqueIdSuffix) + 2;
            char *concatenatedId = new char[totalLength];
            snprintf(concatenatedId, totalLength, "%s_%s", mainUniqueId, uniqueIdSuffix);
            _uniqueId = concatenatedId;
        }
        else
        {
            _uniqueId = uniqueIdSuffix;
        }
    }
    else
    {
        _uniqueId = uniqueIdSuffix;
    }

    if (mqtt())
    {
        mqtt()->addDeviceType(this);
    }
}

C++ is not my everyday language, so please point out if I've done something boneheaded here.

daniloc commented 1 year ago

btw, this restores functionality lost from v.1.3.0, which did the same work at the serializer level. Working in that code was beyond my abilities:

https://github.com/dawidchyrzynski/arduino-home-assistant/blob/a4e50b7d048373d1254fef2e3e48f5e9a5fd51fc/src/device-types/DeviceTypeSerializer.cpp#L228C1-L250C2

erkr commented 9 months ago

In my humble option this is not the best way to solve it. This approach extends also the topic names. The topics now contain the unique device id twice, what increases the payloads(not needed). I think the unique 'device_id' (166864 in my example) shall only prefix the unique_id in the config payload:

config = {"name":"Relay 1","uniq_id":"166864_R1"...

BUT this fix also extend the topics:

aha/RELAIS-CTRL_166864/166864_R1/stat_t

they shall remain like this:

aha/RELAIS-CTRL_166864/R1/stat_t

These topics were already unique!

My proposal: Put the concatenated id in a new variable. Use that new variable in all derived classes to render the unique_id in the config part.

erkr commented 9 months ago

And that works! the HABaseDeviceType constructor sets an extra variable _configUniqueID (basically your code)

{
    HADevice const *device = HAMqtt::instance()->getDevice();
    _configUniqueID = uniqueId;
    if (device)
    {
        const char *mainUniqueId = device->getUniqueId();
        if (mainUniqueId && uniqueId)
        {
            size_t totalLength = strlen(mainUniqueId) + strlen(uniqueId) + 2;
            char *concatenatedId = new char[totalLength];
            snprintf(concatenatedId, totalLength, "%s_%s", mainUniqueId, uniqueId);
        _configUniqueID = concatenatedId;
        }
    }       
    if (mqtt()) {
        mqtt()->addDeviceType(this);
    }
}

added this new variable _configUniqueID to HABaseDeviceType.h:

    /// The unique ID that was assigned via the constructor.
    const char* _uniqueId;

    /// The extended unique ID used for publish Config
   const char* _configUniqueID;

And we need to modify one line for device types method buildSerializer() , in order to use the new variable instead. I.e. in HASWitch.cpp:

    _serializer->set(AHATOFSTR(HAUniqueIdProperty), _configUniqueID);

A little more work, but the result is now that the config topic looks like:

{"name":"Relay 1","uniq_id":"RCTRL_198e31_R1","ic":"mdi:pipe-valve", ...

and the AHA topics are still without the extra prefix:

aha/RCTRL_198e31/R1/cmd_t

PS: I didn't change two device types as they don't include the unique ID in the config:

daniloc commented 9 months ago

@erkr This is so cool! Great catch on the topic names, that's definitely inelegant. Thanks for digging in on this. Do you have a working fork you'd like to use for a PR to supersede mine, or should I update mine? If you wanted to attach the changed files, happy to drop those in.

erkr commented 9 months ago

Nice you like it. Yes I have a working version, but I'm not that into working on GitHub, Here is a zip file with the modified device types. You are free to use it, and I really hope you can get it ending up in the official Arduino lib someday :-) device-types.zip

dawidchyrzynski commented 7 months ago

This feature will be implemented soon. It has to be backward compatible with previous approach for generating unique IDs.

daniloc commented 7 months ago

Thank you for your wise stewardship, @dawidchyrzynski !

dawidchyrzynski commented 7 months ago

217