Comments (21)
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.
Related Issues (20)
- Rounding errors in statusvalues HOT 11
- [Feature] provide FW update available of add-on via MQTT HOT 2
- Frost valve opening and worrying sound coming from it after connecting module HOT 2
- Adding a RF remote: ID error, please use HEX notation separated by ',' (ie. 'A1,34,7F') HOT 3
- Domoticz Python Plugin HOT 2
- trigger status update when changing settings via API/MQTT HOT 8
- [Feature request] Add spider remote HOT 8
- MQTT settings are reset to defaults when module restarts HOT 3
- CVE-S ECO SP: init failed - please power cycle the Itho unit - HOT 2
- Is it possible to enable additional cooling for heat pumps using the API? HOT 1
- [Minor] Opening left menu in new tab shows JSON or 404 HOT 3
- Losing System status (2401) counters after a while HOT 4
- Low ventilation setpoint setting results in '-1%' setpoint HOT 1
- Same SSID on multiple access points in a cluster is seen as multiple different wifi networks. HOT 1
- HA MQTT: received on topic itho/ithostatus. 'high' is not a valid preset mode HOT 7
- Homey fanspeed insights HOT 1
- Link to Domoticz does not work when using Publish topic other than flat
- NON-CVE 1 Software Update not working HOT 1
- unit suddenly flipped outdoor and room temperatures HOT 1
- itho RFT-RV commands are always being send to the itho unit. HOT 1
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from ithowifi.