Issues in latest commits

[euphi]

I'm trying the latest Homie version and it runs quite stable.

I know that it is heavy work in progress, so please see my comments here as a quick feedback.

New Property Class

I noticed a strange issue that my derived HomieNode register to empty property.

Cause is a dangerous issue in the new HomieInternals::Property class: It stores the propery as "const char*", so it is just a pointer.

In my case, I was creating the property "const char *" as a local variable, so it was overwritten later.

This may be unusual, but there are some good reasons for doing this, especially if you create properties in a loop or similar.

Example code that does not work with the current implementation of the Property class:

class RGWNode : public HomieNode {
    enum RGB_MAP {
        R, G, B, W
    };
    const char rgbw_id[4] = {'r', 'g', 'b', 'w'};
    uint16_t rgbw_values[4] = { 0, 0, 0, 0 };
[...]
};

RGBWNode::RGBWNode() :  HomieNode("LED", "RGBW"), initialized(false) {
    for (uint_fast8_t i=R;i<=W;i++) {
        char cb[2];
        cb[0] = rgbw_id[i];
        cb[1] = '\0';
        this->advertise(cb)->settable();
    }
}

Settings in config.json

• The config is invalid, if there are no settings. I workaround this by returning valid in _validateConfigSettings if there are no settings:

ConfigValidationResult Helpers::_validateConfigSettings(const JsonObject& object) {
  ConfigValidationResult result;
  result.valid = false;

  if (!object.containsKey("settings"))
  {
      result.reason=F("No settings");
      result.valid = true;
      return result;
  }
  if (!object["settings"].is<JsonObject&>()) {
    result.reason = F("settings is not an object");
    return result;
  }

• Furthermore it seems that Homie generates an invalid JSON file, when storing a JSON file received from the configurator app. (Extra semicolons).

[marvinroger]

I did not think about this use case, so there won't be any problems anymore with "dynamic" properties. ;)

I've tried to make the settings optional, but I am not sure it will actually work. Will need some testing.

For the latest point, I'll check. Thanks a lot for this feedback!

[euphi]

One more example where the property is just a number that can receive "ON".

bool RelaisNode::handleInput(const String& property, const String& value) {
    int16_t id = property.toInt();
    if (id <= 0 || id > 8) {
        LN.logf("RelaisNode::handleInput()", LoggerNode::ERROR,
                "Receive unknown property %s with value %s.", property.c_str(),
                value.c_str());
        return false;
    }
    bool on = value.equalsIgnoreCase("ON");
    LN.logf("RelaisNode::handleInput()", LoggerNode::INFO,
            "Receive command to switch %x to %s.", id, on ? "On" : "Off");
    relais_bitset |= (1 << (id-1));
    updateRelais();
    return true;
}

So, up to now my "RelaisNode" class just use "subscribeToAll". It is not yet updated to the new advertise(..) method, but when, I want just to create a for-loop to advertise all 8 properties.

Update: I know my logic to set the relais_bitset is flawed. It is work in progress ... ;-)

[marvinroger]

https://github.com/marvinroger/homie-esp8266/commit/dae5beb3968573a0dc8b8b205e45d270370c4e23 fixes the Property issue, and I just tested, settings also work without the field in config.json since https://github.com/marvinroger/homie-esp8266/commit/e8b195abc85329b4d44f5d4d3ddbe05637fde1ae

Can you explain the extra semicolons issue? The framework stores the raw JSON it receives from the HTTP API, so that's weird.

[euphi]

Sorry, I meant extra comma, not extra semicolon. That comma may be was caused by my poor workaround to just validate the "settings" by returning result.valid = false;

I will try again later this evening.

[euphi]

Regarding the advertise() of properties:

I think the strdup() solution is ok as quick fix, but really needs to be improved. As shown, there are various examples for nodes that have many properties, so it is better to not allocate memory for each property.

My first idea for a solution is to add (again) a subscribeToAll(const char* subscribestring) that sends the parameter subscribestring as $properties

[marvinroger]

This is done in order to allow introspection from the backend, so that a backend could talk to any node without knowing it in advance. This would defeat the purpose of $properties...

[euphi]

Yes, therefore I proposed the string that needs to be provided for subscribeToAll.

I really think that there should be a solution for nodes with a large amount of properties. In automation, there are often devices with many IOs. Usually they only have numbers, so the assignment to a specific signal is done at a central controller, not at the controller in the field; unused IOs are already reserved and wired up to a terminal, so they can be used without any update of software or parameterization in the IO controller.

You could easily build a cheap but powerful IO device with a ESP8266 and some chained PCF8575 and running Homie.

For introspection, some rules could be proposed to create a string for $subscribe, e.g. something like "out[0-255]" means that there are properties "out001", "out002", "out003" etc. till "out255".

[marvinroger]

That's what I was thinking about. :)

node.advertiseRange("out", 1, 255); // If instead of "out" we've had NULL, then there would not be prefixes

$properties : out[1-255]

Is it fine?

[euphi]

Yes, its fine. :+1: How do you want to implement it?

In void BootNormal::_onMqttMessage, will you just search for the prefix or for the complete property?

I fear it may be to much overhead for just calling a GlobalInputHandler or NodeInputHandler that will parse the property again?

[euphi]

Regarding the second bug (invalid config.json):

I tried again with your latest commit (04e65f35e29794ab6de2f5e17694dba7409f2646), deleted my config.json to boot in ConfigurationMode and used the "Homie Setup" App to send my configuration.

To debug it, I added a Serial.println(config); at void Config::write(const char* config) and a Serial.println(buf); at bool Config::load() line 53.

Output now is almost no configuration at all?!

✔ Wi-Fi scan completed
Received config request
****Config*****

name
✔ Configured
↻ Rebooting into normal mode...

 ets Jan  8 2013,rst cause:2, boot mode:(3,6)

load 0x4010f000, len 1384, room 16 
tail 8
chksum 0x2d
csum 0x2d
v3ffe8430
~ld
�Setup
name������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������
✖ Invalid JSON in the config file
Triggering CONFIGURATION_MODE event...

[euphi]

It seems that JsonObject& parsedJson = parseJsonBuffer.parseObject(bodyCharArray); (BootConfig.cpp:317) also writes at the buffer (char * bodyCharArray) and strips "unneeded" characters.

I solved this by printing the parsedJson to the config file:

  String fileoutput;
  parsedJson.printTo(fileoutput);

  _interface->config->write(fileoutput.c_str());

I noticed another bug with the "Homie Setup" app, but I will file a separate issue because it affects the app in general (and thus 1.5.0, too).

[marvinroger]

I implemented properties range. Note I did not test either (I don't have an ESP8266 right now, so it probably won't work :confused:). But it works this way: you send properties to the property topic with _ as a separator between the property name and the index ("led_0"). The handlers were changed to add an HomieRange parameters that contain isRange and index fields, so you don't have to parse anything yourself.

Yes you're right, ArduinoJson changes the char array... I implemented too much things, so I missed it, thanks again! :+1:

I will fix when I come back from vacations. ;)

[euphi]

Yes, your progress was great the last weeks! :+1:

First test worked fine:

devices/labctrl/$homie 2.0.0
devices/labctrl/$implementation esp8266
devices/labctrl/Relais/$type switch8
devices/labctrl/Relais/$properties in[1-8]:settable
devices/labctrl/$name Labcontroller
devices/labctrl/$localip 192.168.0.119
devices/labctrl/$uptime/interval 120
devices/labctrl/$fw/name undefined
devices/labctrl/$fw/version undefined
devices/labctrl/$implementation/config {"name":"Labcontroller","device_id":"labctrl","wifi":{"ssid":"KDG"},"mqtt":{"host":"192.168.0.220","port":1883,"base_topic":"devices/","auth":false},"ota":{"enabled":false}}
devices/labctrl/$implementation/version 2.0.0
devices/labctrl/$implementation/ota/enabled false
devices/labctrl/$online true
devices/labctrl/Log/DEBUG/RelaisNode Ready
devices/labctrl/Log/DEBUG/RelaisNode::updateRelais() Value: 55
devices/labctrl/Relais/in_1 On
devices/labctrl/Relais/in_2 Off
devices/labctrl/Relais/in_3 On
devices/labctrl/Relais/in_4 Off
devices/labctrl/Relais/in_5 On
devices/labctrl/Relais/in_6 Off
devices/labctrl/Relais/in_7 On
devices/labctrl/Relais/in_8 Off
devices/labctrl/$signal 34
devices/labctrl/$uptime/value 13
devices/labctrl/Relais/in_1/set On
devices/labctrl/Log/INFO/RelaisNode::handleInput() Receive command to switch 1 to On.
devices/labctrl/Log/DEBUG/RelaisNode::updateRelais() Value: 55
devices/labctrl/Relais/in_1 On
devices/labctrl/Relais/in_1/set Off
devices/labctrl/Log/INFO/RelaisNode::handleInput() Receive command to switch 1 to Off.
devices/labctrl/Log/DEBUG/RelaisNode::updateRelais() Value: 54
devices/labctrl/Relais/in_1 Off

(Source code https://github.com/euphi/HomieNodeCollection/blob/master/src/RelaisNode.cpp)

Many thanks and have a nice vacation! :-)

[euphi]

There is another subtle problem with the changes for Homie 2.0.0:

To add MQTT QoS, you changed the signature of

bool setNodeProperty(const HomieNode& node, const String& property, const String& value, bool retained = true); to void setNodeProperty(const HomieNode& node, const char* property, const char* value, uint8_t qos = 1, bool retained = true);

If you make use of the retained flag in 1.5.0 to set it to false and upgrade to 2.0.0, this does not result in a compiler error. Instead the false is implicitly casted to uint8_t '0' and retained becomes true by default.

You should add a clear warning in an Upgrade Guide 1.5.0 to 2.0.0.

[marvinroger]

I'll definitely add an upgrade guide. :) and there's a way to workaround this issue ( http://stackoverflow.com/questions/18463937/how-to-prevent-implicit-conversion-from-char-array-to-bool )

Le 21 août 2016 1:04 PM, "Ian Hubbertz" notifications@github.com a écrit :

There is another subtle problem with the changes for Homie 2.0.0:

To add MQTT QoS, you changed the signature of

bool setNodeProperty(const HomieNode& node, const String& property, const String& value, bool retained = true); to void setNodeProperty(const HomieNode& node, const char* property, const char* value, uint8_t qos = 1, bool retained = true);

If you make use of the retained flag in 1.5.0 to set it to false and upgrade to 2.0.0, this does not result in a compiler error. Instead the false is implicitly casted to uint8_t '0' and retained becomes true by default.

You should add a clear warning in an Upgrade Guide 1.5.0 to 2.0.0.

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/marvinroger/homie-esp8266/issues/130#issuecomment-241251279, or mute the thread https://github.com/notifications/unsubscribe-auth/AA8eNTq4zytF-AoC6Bfiw2Fvzm2j2ArAks5qiDCkgaJpZM4JpF0L .

[marvinroger]

The writing of config.json through API should be fixed, the MQTT QoS signature problem is not a problem anymore, since the signature completely changed (see the updated examples). This was needed to add a clean way to send a range property value through setNodeProperty(). Note, once again, I haven't been able to test, so chances are it's not stable.

I also added back the LedStrip example with the new range properties and documented range properties.

[marvinroger]

Everything here was resolved, thanks for the heads up!