Strange parsing of MQTT commands about ithowifi HOT 21 CLOSED

Ik heb de wiki aangepast zodat gebruikers niet op verkeerde ideeën worden gebracht ☺.
De parsing zoals je die afhandelt ziet er zo op eerste gezicht goed uit. Ik ben nu aan het kijken naar de ArduinoJSON library. De eerste PR is al onderweg ;).

from ithowifi.

I'm pretty sure it worked in 2.2.3.

from ithowifi.

What platform are you using to send those commands? I tested using node-red but I'm not sure if the node-red does some sanitising before sending it to MQTT
Are you sure about 2.2.3? Seems old

from ithowifi.

mqtt pub is just an alias for the mosquitto publish tool. This ensures the payload is sent exactly as specified.
Sorry about 2.2.3, I meant 2.4.3.

To add some extra information: The problem only exists when the number is a single digit.
This works: mqtt pub -t itho/cmd -m '{ "speed": "5.0" }'
This doesn't: mqtt pub -t itho/cmd -m '{ "speed": "5" }'

From homeassistant I send speed changes in 5% increments, so it is 0, 13, 26, ... It took me a long time to find why all speed changes worked except for 0.

from ithowifi.

And if you try:
mqtt pub -t itho/cmd -m '{ "speed": 5 }'
?

from ithowifi.

That works.

from ithowifi.

The API officially does not support strings as value when the key is "speed", only numbers are supported.

from ithowifi.

Shall we agree that that is not related the bug in the JSON parser?

In general, parsers should either reject all quoted strings when a number is required (strict), or accept all quoted strings that contain a number (loose).

From https://github.com/arjenhiemstra/ithowifi/wiki/MQTT-integration#ithocmd-change-device-settings

{
"speed":"150",
"timer":"15"
}

Also:

curl 'http://nrgitho.squirrel.nl/api.html?speed="5"'
⇒ OK

(alhough it sets the speed to 0 ...)

from ithowifi.

No sorry, you are missing my point. Not supported means undefined behaviour and maybe even more important, it means it is not tested during development.

The wiki is community based, which is great but not guaranteed to be perfect. The API documentation on the add-on itself should be clear on this point.

That can be changed of course but that is the current situation.

from ithowifi.

Well, let's say this is not the way I try to write my software.

Anyway, I've removed the quotes from the MQTT publish so I'm fine.

from ithowifi.

Well, let's say this is not the way I try to write my software.

I'm merely stating that it is currently undefined behaviour. Undefined behaviour is undefined behaviour, whatever you do not define in software is undefined with an uncertain outcome.
You are without a doubt a far better developer than I am (let me remind you of my statement on this repo; I'm not one at all) but if you manage to prevent undefined behaviour you are truly king.

If you wish to change it into defined behaviour; great! let's talk about that. If you want to criticise what I created, good I can handle that! but be straight.
If you want to tell me how not to write software, please do it in another way than just telling how it is not the way how you would do it, that's not just not nice and not educational for anybody.

from ithowifi.

Hey Arjan,
Sorry dat mijn bedoelingen niet goed overkwamen. Het is niet bedoeld als negatieve kritiek op jouw software. Ik ben er heel blij mee (eindelijk doet dat kreng wat ik wil) en je support is top! En je diplomatieke vaardigheden zijn in elk geval beter dan de mijne.

Persoonlijk ben ik een aanhanger van "be conservative in what you send, be liberal in what you accept" (het Robustness Principle). Ja, dat kost tijd en energie en het is dus een voortdurende afweging hoe ver je daarin wil gaan.

Wat betreft "undefined behaviour" -- inderdaad, undefined is undefined. Maar "undefined" is software-technisch de else van een if ... then. Vroeger returnde ik dan wel eens de waarde 42, of een sterretje, maar daar ben ik snel van teruggekomen. Uiteindelijk blijkt het toch vaak beter om te kijken wat je ervan kunt maken (in ons geval interpreteer "5" als 5) of een foutmelding te geven.

Ik heb nog niet naar de parsing gekeken, maar gebruik je een standaard JSON library of parse je zelf?

from ithowifi.

Kritiek is helemaal prima, welkom zelfs, daar leer ik van. De toon kwam mij echter vervelend over maar heel fijn om je reactie te lezen!

Volgens mij zijn we het ook met elkaar eens.
Ik heb geprobeerd de API zo resilient mogelijk te maken, elk soort input moet erop afgevuurd kunnen worden. Ik heb daarbij alleen geen rekening gehouden met het parsen van strings als het nummers moeten zijn. Prima om dat mogelijk te maken maar dan moeten er wat extra checks ingebouwd worden denk ik. Ik maak gebruik van de ArduinoJSON lib (https://github.com/bblanchon/ArduinoJson). Ik vermoed dat daar wat wijzigingen in doorgevoerd zijn waardoor we het veranderde gedrag zien.

from ithowifi.

Eri is iets raars aan de hand maar ik kan er niet echt de vinger opleggen. Dit is het stukje code

        if (!(const char *)root["speed"].isNull())
        {
          jsonCmd = true;
          if (!(const char *)root["timer"].isNull())
          {
            ithoSetSpeedTimer(root["speed"].as<uint16_t>(), root["timer"].as<uint16_t>(), MQTTAPI);
          }
          else
          {
            ithoSetSpeed(root["speed"].as<uint16_t>(), MQTTAPI);
          }
        }

Als ik { "speed" : "5" } stuur dan wordt dit stukje code overgeslagen. Maar zodra ik er iets in verander:

        if (!(const char *)root["speed"].isNull())
        {
          jsonCmd = true;
          D_LOG("X");
          if (!(const char *)root["timer"].isNull())
          {
            ithoSetSpeedTimer(root["speed"].as<uint16_t>(), root["timer"].as<uint16_t>(), MQTTAPI);
          }
          else
          {
            ithoSetSpeed(root["speed"].as<uint16_t>(), MQTTAPI);
          }
        }

wordt het stukje wel goed uitgevoerd (en treed de parsing anomalie niet meer op).

Als ik het herschrijf als:

       if (root["speed"])
        {
          jsonCmd = true;
          if (root["timer"])
          {
            ithoSetSpeedTimer(root["speed"].as<uint16_t>(), root["timer"].as<uint16_t>(), MQTTAPI);
          }
          else
          {
            ithoSetSpeed(root["speed"].as<uint16_t>(), MQTTAPI);
          }
        }

gaat het altijd goed. De test if (root["speed"]) is een snellere (en kortere) versie van if (!(const char *)root["speed"].isNull()) (zie https://arduinojson.org/v6/api/jsonobject/isnull/ ).

Overigens zit er nog een fout in het stukje code voor "timer": in de if/if/else wordt ithoSetSpeed(root["timer"].as<uint16_t>(), MQTTAPI); aangeroepen, dat moet natuurlijk ithoSetTimer zijn.

Zie PR.

from ithowifi.

Bedankt en heel interessant! Ik heb deze aanpak eerder geprobeerd maar had toen issues meen ik mij te herinneren. Ik moet nog even testen maar ik gok dat jij dat al gedaan hebt:
if (root["speed"]) test dit of er een key "speed" aanwezig is of dat de value van speed niet resulteert in false. In het laatste geval zou je namelijk een issue hebben als bijv. value == 0.

from ithowifi.

Overigens zit er nog een fout in het stukje code voor "timer": in de if/if/else wordt ithoSetSpeed(root["timer"].as<uint16_t>(), MQTTAPI); aangeroepen, dat moet natuurlijk ithoSetTimer zijn.

Zeker! Fijn dat je deze gevonden hebt!

from ithowifi.

Zoals ik ook al bij de PR schreef, "5" is een string, niet een int.

from ithowifi.

if (root["speed"])` test dit of er een key "speed" aanwezig is of dat de value van speed niet resulteert in false. In het laatste geval zou je namelijk een issue hebben als bijv. value == 0.

root["speed"] is een object, dus als het er is test de if true.

from ithowifi.

Zoals ik ook al bij de PR schreef, "5" is een string, niet een int.

Lang leve .as<uint16_t>() :)

from ithowifi.

Een andere, vreemde constructie in de code:

[1]     if (root["speed"])
        {
          jsonCmd = true;
[2]       if (root["timer"])
          {
[3]         ithoSetSpeedTimer(root["speed"].as<uint16_t>(), root["timer"].as<uint16_t>(), MQTTAPI);
          }
          else
          {
            ithoSetSpeed(root["speed"].as<uint16_t>(), MQTTAPI);
          }
        }
[4]     if (root["timer"])
        {
          jsonCmd = true;
[5]       if (root["speed"])
          {
[6]         ithoSetSpeedTimer(root["speed"].as<uint16_t>(), root["timer"].as<uint16_t>(), MQTTAPI);
          }
          else
          {
            ithoSetTimer(root["timer"].as<uint16_t>(), MQTTAPI);
          }
        }

Hier test je op speed [1] en dan op timer [2], als ze beide zijn gezet roep je ithoSetSpeedTimer [3] aan.
Vervolgens test je op [4] timer, dan nogmaals op [5] speed, als ze beide zijn gezet roep je nogmaals [6] ithoSetSpeedTimer aan. Je zou daar iets van kunnen maken als:

        if ( root["speed"] && root["timer"] )
        {
          jsonCmd = true;
          ithoSetSpeedTimer(root["speed"].as<uint16_t>(), root["timer"].as<uint16_t>(), MQTTAPI);
        }
        else
        if (root["speed"])
        {
          jsonCmd = true;
          ithoSetSpeed(root["speed"].as<uint16_t>(), MQTTAPI);
        }
        else
        if (root["timer"])
        {
          jsonCmd = true;
          ithoSetTimer(root["timer"].as<uint16_t>(), MQTTAPI);
        }

from ithowifi.

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

from ithowifi.