vintlabs / fauxmoESP

Add voice control of your ESP32 and ESP8266 devices using Amazon Alexa
MIT License
379 stars 70 forks source link

Improve UniqueID for devices #126

Closed pvint closed 3 years ago

pvint commented 3 years ago

@pvint Is there any way to make the device id random or to set that myself? I think I figured out why my 2nd successful discovery got the last 5 of my devices. And that is because the ids of the 5 before those were the same ids as the first 5 devices that were discovered. I know this sounds confusing, but it's a problem with the device ids being set programmatically. I'd rather set the device id myself to something I know hasn't been already used.

Originally posted by @jenniferlee1818 in https://github.com/vintlabs/fauxmoESP/issues/120#issuecomment-726451454

pvint commented 3 years ago

Moved to a separate issue.

I've been thinking about better ways to handle the UniqueID, and I think the two things that are most important is for it to be (obviously), unique, but also repeatable.

My current idea is to use a hash of the device MAC along with the device's name. Something like this pseudo-code:

uniqueid = md5(mac + deviceName);
jenniferlee1818 commented 3 years ago

fauxmoESP.cpp

#include <MD5Builder.h>

String byte2hex(byte Zahl){
    String Hstring = String(Zahl, HEX);
    if (Zahl < 16){Hstring = "0" + Hstring;}
    return Hstring;
}

String makeMD5 (String text){
    byte bbuff[16];
    String hash = "";
        MD5Builder md5; 
        md5.begin();
        md5.add(text); 
        md5.calculate(); 
        md5.getBytes(bbuff);
        for ( byte i = 0; i < 16; i++) hash += byte2hex(bbuff[i]);
        return hash;   
}

String fauxmoESP::_deviceJson(unsigned char id) {

    if (id >= _devices.size()) return "{}";

    String mac = WiFi.macAddress();
        mac.replace(":", "");
        mac.toLowerCase();

    fauxmoesp_device_t device = _devices[id];

    mac.concat(device.name);

    String hash = makeMD5(mac).substring(0,16);  

    char buffer[strlen_P(FAUXMO_DEVICE_JSON_TEMPLATE) + 64];
    snprintf_P(
        buffer, sizeof(buffer),
        FAUXMO_DEVICE_JSON_TEMPLATE,
        device.name, hash.c_str(),
        device.state ? "true": "false",
        device.value
    );
    //free(hash);
    return String(buffer);

}
unsigned char fauxmoESP::addDevice(const char * device_name) {

    String mac = WiFi.macAddress();
        mac.replace(":", "");
        mac.toLowerCase();

    fauxmoesp_device_t device;
    unsigned int device_id = _devices.size();

    // init properties
        device.name = strdup(device_name);
    device.state = false;
    device.value = 0;

    // Attach
    _devices.push_back(device);

        mac.concat(device_name);
    String hash = makeMD5(mac).substring(0,16);

        DEBUG_MSG_FAUXMO("[FAUXMO] Device '%s' added as #%d, unique id is %s\n", device_name, device_id, hash.c_str());

    return device_id;

}

templates.h

// Working with gen1 and gen3, ON/OFF/%, gen3 requires TCP port 80
PROGMEM const char FAUXMO_DEVICE_JSON_TEMPLATE[] = "{"
    "\"type\":\"Extended color light\","
    "\"name\":\"%s\","
    "\"uniqueid\":\"%s\","
    "\"modelid\":\"LCT015\","
    "\"state\":{"
        "\"on\":%s,\"bri\":%d,\"xy\":[0,0],\"reachable\": true"
    "},"
    "\"capabilities\":{"
        "\"certified\":false,"
        "\"streaming\":{\"renderer\":true,\"proxy\":false}"
    "},"
    "\"swversion\":\"5.105.0.21169\""
"}";

MD5.zip

pvint commented 3 years ago

Excellent! I spent a bunch of time on Friday thinking about this and looking into different methods to create a good hash to use for this and about any potential issues that might arise, and have been thinking about it over the weekend, and in the meantime it looks like you did it for me, thanks!

I'll test it out in a little bit... I presume it's working for you?

pvint commented 3 years ago

Also, @jenniferlee1818 - if you have a minute or two, would you mind doing this in a pull request on the dev branch? (I can do it if you want)

jenniferlee1818 commented 3 years ago

Yes, it works and solves so many issues with discovery. It's awesome because the unique id for each device is always the same. I don't even know how to do a pull request, or what that means, but would love to learn. Maybe I'll just try...

pvint commented 3 years ago

Yeah, give it a go... I requested to do it on the dev branch, but don't worry about that if it might get in your way.

My basic definition of a "Pull Request" is this: You have code changes that are useful to the project, and you can submit it (as a PR), and then I can choose to merge it in, and then it's "on record" that it was you who did that bit. Github actually makes it quite easy (but perhaps not always obvious)

pvint commented 3 years ago

Also, something I want to note: I'm a bit worried that we might not want it to always be the same... I cannot come up with a situation off the top of my head where it would be bad to have a device with the same name running from the same ESP device having the same uniqueid, but there's a hint of paranoia there that I want to keep in mind.

jenniferlee1818 commented 3 years ago

I can't figure out the pull request thingy. Sorry!

jenniferlee1818 commented 3 years ago

And I think the md5 in addDevice function isn't necessary. I just wanted to see that there in the debug output.

pvint commented 3 years ago

That's ok - I can do it (I was just partially being lazy...). I might not do it until morning though (I'm in Canada, GMT-5).

ddweber456 commented 3 years ago

@jenniferlee1818 great job, I'm looking forward to giving it a try. Were you able to discover all 15 of your devices in a single pass?

jenniferlee1818 commented 3 years ago

@ddweber456 no, I can only get 5 before watchdog reset.

pvint commented 3 years ago

@jenniferlee1818 - I have added your code and committed it to the master branch. Many thanks!

Also, referring to only being able to add 5 devices before it resets - I'm hoping to have a resolution for that soon.

pvint commented 3 years ago

Thinking about this more, I think I will reopen this. I think with the current method there's a couple potential pitfalls, the biggest being that if a device is renamed it will get a new uniqueid, and this may not always be desirable.

pvint commented 3 years ago

In commit 73ff88e I've added ability to manually set the device's uniqueid:

void fauxmoESP::setDeviceUniqueId(unsigned char id, const char *uniqueid)

This can be run any time after a device is created, and this should take care of cases where you want to change a device name or even the host that a device runs on without changing the unique ID.

jenniferlee1818 commented 3 years ago

@pvint More devices on one mcu would be great! I have 3 nodemcu's piggybacking power to operate one little IR led. But, it works!

pvint commented 3 years ago

@pvint More devices on one mcu would be great! I have 3 nodemcu's piggybacking power to operate one little IR led. But, it works!

@jenniferlee1818 I'm curious about what you mean exactly, but I think it's a bit outside of the UniqueID topic. Really need a forum for general discussion stuff.... If you think it's related to FauxmoESP, feel free to open an issue here (but it doesn't sound like it is... you can send me an email about it if you wish, maybe I can help)