Fehler in ProofAndSetValue bei Gleitkommawerten #1058
Comments
|
In der Tat ist diese ganze ProofAndSetValue Methode/Funktion kompletter Müll IMHO :) Schon beim letzten Fix hätte ich das ganze am liebsten auf den Müll geworfen und hätte es komplett neu geschrieben. Aber das würde zuviele Änderungen reinbringen die dann sicherlich niemals zum originalen CCU3 Image zurückwandern würden. Trotzdem gebe ich die Hoffnung nicht auf das wir vielleicht im Rahmen von RaspberryMatic hier die Funktion zumindest zu repariert bekommen das man sie bedienen kann. Wenn du also einen PR bzw. Änderungsvorschlag hast dann bitte her damit und wir schauen das wir da zumindest eine kleine Verbesserung reinbringen können. |
jens-maus
added
🐛 bug-report
Aber was ist an der Methode, wie sie die ganzen Jahre bis zu 3.55 eingebaut war, verkehrt?
Ich möchte eigentlich auch mal wieder was anderes machen, als nur WebUI bauen/fixen. In der Hoffnung, dass eQ-3 bald eine neue, reparierte CCU FW Version raus bringt, verwende ich erstmal weiter die alte abgehangene |
|
Bin nun heute auch drüber gestolpert. |
|
Hab auch das Problem, kann die Hysterese bei der DV eines HM-TC-IT-WM-W-EU mit einem HM-LC-Sw1-FM nicht mehr auf einen Gleitkommawert setzten. |
|
Ich was mal so frei und hab das eQ-3 gemeldet. |
jens-maus
added
🙏 help wanted
Kannst du nochmal exakt die Version von |
|
aus 3.53.30 ProofAndSetValue = function(srcid, dstid, min, max, dstValueFactor, event)
{
// Falls das Tasten-Event nicht mit ¸bergeben wurde ....
var keyCode = 0,
finalVal;
if (event) {
keyCode = event.keyCode;
}
var ok = true;
if (! min) min = 0;
if (! max) max = 100;
if (! dstValueFactor) dstValueFactor = 0.01;//dstValue = value/100
value = $F(srcid);
//replace , by .
var idx = value.indexOf(',');
if (idx >= 0)
{
var tokens = value.split(",");
value = "";
if (tokens[0]) value += tokens[0];
value += '.';
if (tokens[1]) value += tokens[1];
$(srcid).value = value;
}
//User is already editing?
if (value.charAt(value.length-1) == '.') return;
if (! value)
{
//alert("Keine Zahl.");
//value = 0;
//$(dstid).value = min;
finalVal = min;
ok = false;
}
else if (isNaN(value))
{
//alert("Keine Zahl.");
//value = min;
finalVal = min;
ok = false;
}
else if (value < min)
{
//alert("Der kleinste Wert ist 0.");
//value = min;
finalVal = min;
ok = false;
}
else if (value > max)
{
//alert("Der grˆflte Wert ist 100.");
//value = max;
finalVal = max;
ok = false;
}
if (ok)
{
//$(srcid).style.backgroundColor = "white"; // problem with firefox 69.0.3 (64-bit)
$(srcid).style.backgroundColor = "#fffffe";
$(dstid).value = value * dstValueFactor;
$(srcid).setAttribute("valvalid", "true");
// Cursortasten abfangen, ansonsten springt der Cursor im Texteingabefeld
// beim IE (Version 8 u. 9) mit jedem Druck auf eine Cursortaste ans Ende des Wertes.
// Man kann nicht mittels Cursor-Links nach links wandern, da der Cursor immer ans Ende springt.
// [HM-1293]
if ((keyCode) < 37 && (keyCode > 40) ) {
$(srcid).value = value;
}
}
else
{
$(srcid).setAttribute("valvalid", "false");
$(srcid).style.backgroundColor = "red";
$(dstid).value = finalVal * dstValueFactor;
window.setTimeout(function(){$(srcid).style.backgroundColor = "white";},1000);
}
}; |
|
Aber scheint ja nicht mehr lange zu dauern, bis die neue Version kommt |
|
@jp112sdl Ja, halt die Augen mal auf das eq3 OCCU repo in den nächsten Tagen, würde ich sagen :) |
|
Hab mal die von @jp112sdl gepostete ProofAndSetValue auf eine aktuelle Nightly losgelassen. Angegebener Wertebereich ist (5.00 - 30.00).
Da klemmt es wohl noch an anderer Stelle. |
BrowserCa... Ach nee, war ja nicht mehr. Ja wie gesagt, das ist die Funktion so wie sie bisher erfolgreich zum Einsatz kam. Vielleicht wurde für HmIP aber auch noch an anderen Stellen was umgebaut, so dass für diese Geräte die alte Funktion nicht mehr geht. |
Ja, bei einer HM-Heizgruppe lässt sich der Wert wie gewünscht einstellen. Das gute ist ja, man braucht gar keine HmIP-Geräte um eine IP-Heizgruppe anzulegen. <--- "wink mit dem Zaunpfahl" ;-) |
Da wird Bei HM Geräten sind es 2 verschiedene IDs. Aber ich würd jetzt hier eh nix mehr fummeln. Vertane Zeit. Kommt ja eh ein Fix. |
|
Sag bloß du hast eine IP-Gruppe angelegt? :-D Ich denke auch wir warten auf einen "offiziellen" Fix. |
Zuviel würde ich von dem "Fix" jetzt aber ehrlich gesagt nicht erwarten... |
Erschreckende Einschätzung. Nachdem sich so viele Nutzer beschwert haben, sollte zumindest die vorherige Funktionalität (meinetwegen auch inkl. HmIP ^^) wiederhergestellt werden. Warten wir mal ab. |
|
Wirst du denn nun noch eine 3.55.5.2021xxxx rausbringen und den Fix aus 3.55.10 ( Oder gibt es gleich eine 3.55.10.2021xxxx? |
|
Es wird dann gleich eine 3.55.10.xxxxx geben. |
|
Ohne das jetzt aber genauer angeschaut zu haben scheint es mir so zu sein das hier immer noch probleme geben wird, denn die "neue" ProofAndSetValue() der 3.55.10 scheint in großen teilen der aktuellen in den Testversionen gleich zu sein. Mal sehen. |
|
So, das Update zur OCCU 3.55.10 is durch. Bitte dann mal das problem mit dem nächsten Snapshot noch einmal genauer testen und berichten ob es nun dadurch behoben ist. Die Änderungen durch eQ3 sehen zwar zu 99% gleich aus mit denen hier, aber es könnte natürlich eine gewisse Abhängigkeit geben die trotzdem nun das korrekte Editieren der Geräteparameter zulässt. Ansonsten müssten wir hierfür wieder einen entsprechenden WebUI Patch nachliefern, versteht sich. |
|
CCU3 3.55.10 |
Ja, ist wenn dann ein neues 🥚 Was sich bei der 3.55.10 geändert hat, sieht man z.B. hier:
|
Einer? ;-) Aber wir müssen das hier nicht unbedingt weiter verfolgen. Mit dem "Workaround" geht es ja. Da ich ja gerade eine blitzsaubere CCU3 aktiv habe könnte ich ja mal den Erstkontakt mit eQ-3 wagen... |
|
Interessant ist auch, dass man Temperaturen, beim Thermostat direkt gesetzt, auf .5 oder .0 gerundet bekommt (nach dem Übertragen zum Gerät) - so wie es sein soll: aber in einer Gruppe ist jeder Dezimalwert bei der Temperatur erlaubt. 🥴 Keine Ahnung, ob das hier mit diesem Issue direkt was zu tun hat. Ich glaub da liegt noch jede Menge mehr im Argen. |
|
Das zeigt nur wie fehlerhaft die Funktion angelegt wurde. Man müsste die Anzahl der Nachkommastellen und die Rundungsregeln übergeben. Ohne diesen input wird es immer kleine Fehler geben. Das würde aber einen gravierenden Umbau nötig machen, der ja eher nicht gewünscht wird. Also besser die Fehler minimieren. Ich kann morgen mal meine oben skizzierte Idee ausprobieren und vorstellen, wenn das gewünscht ist. |
Du kannst gerne ein Beispiel erstellen, persönlich weiß ich aber nicht, wie du basierend auf "falsch getrimmten" Min/Max Werten dennoch die korrekte Anzahl Nachkommastellen erkennen möchtest. Solange die Aufrufe an |
|
Also ich habe den SourceCode der Original CCU FW 3.57.4 im Pfad www nach ProofAndSetValue durchsucht. Daraus ergeben sich folgende Fundstellen:
Typische Aufrufe sind: Sehr häufig erfolgt die Übergabe also nicht als String, sondern als (Ganz)Zahl. Ob das in all diesen Fällen korrekt ist, kann ich nicht prüfen. Notfalls müsste man überall Ich habe nur eine einzige Stelle gefunden, wo versucht wird Fließkommazahlen als Zahl zu übergeben:
Diese Stelle muss also auf jeden Fall korrigiert werden. |
|
Hier ist meine Lösung mit einem Workaround für "falsch" als Zahl übergebene Limits. Die Anzahl der Nachkommastellen wird dann anhand des eingegebenen Werts ermittelt. Damit ist zumindest eine Rückwärtskompatibilität sichergestellt. Den von @jp112sdl beschriebene automatisch Rundung auf .0 / .5 kann ich allerdings nicht nachvollziehen. Wird da überhaupt ProofAndSetValue aufgerufen? ProofAndSetValue-2021-03-12-a.js.txt Ich bekomm das leider nicht ordentlich formatiert hier rein... Änderung in fett ProofAndSetValue = function(srcid, dstid, min, max, dstValueFactor, event) // feststellen ob String oder Zahl übergeben wurde // Falls das Tasten-Event nicht mit uebergeben wurde .... if (event) { var ok = true; if (! min) min = 0; var value = $F(srcid); //replace , by . } var parsedValue; var minSplit = min.toString().split("."); if(minSplit.length === 2) { // !0 would result in true so explicitly check if it is not 0 if (ok) |
Habe ich hier mal probiert. Bei mir war es Zeile 1433 in der /www/config/ic_common.tcl geändert in... Das "repariert" zumindest alle betroffenen Stellen des oben beschriebenen CUxD-Thermostates inklusive korrekter Rundung auf die Nachkommastelle. Müsste man mal gucken ob das vielleicht "die EINE Stelle" ist die korrigiert werden muss. |
Tut mir leid wenn das vielleicht wieder etwas ablehnend rüberkommt. Aber für sowas gibt es das "diff" bzw. "patch" format. Schau es dir an und dann bereite deinen vorgeschlagenen änderungen entsprechend vor und präsentiere die so. Dieses unformatierte Kauderwelsch kann man sich wirklich nicht antun. Und noch besser: Mach einen PullRequest! |
|
Sorry aber dann bin ich raus. Die Zeit auch noch einen Github Lehrgang zu machen habe ich dann doch nicht. |
|
Kann dich zwar verstehen, Jens aber auch... Denn ich kann den Spaghetti-Haufen auch nicht überblicken. |
Wenn du dir diese Zeit nicht nimmst oder die Zeit gerne für etwas anderes nutzen willst, dann bist du (so hart es klingt) hier in der Tat leider am falschen Platz. Nur durch geordnetes Vorgehen hier auf GitHub und in diesem Issue Ticket kommt man hier weiter. Andere bringen die Zeit auch auf um das Handwerkszeug zu erlernen das man braucht um gemeinschaftlich zu arbeiten. |
|
Kein Problem. Ihr macht die Spielregeln und ich entscheide ob ich mitspielen will. Das ist in Ordnung für mich. |
Sorry @MichaelN0815 aber das stellt für mich definitiv keine Lösung dar. Folglich ist es sehr riskant einfach eine Eingabe von 5.75 zu erlauben, nur weil der Anwender es eingibt. |
|
In der ic_common.tcl habe ich übrigens auch den Grund gefunden, warum mein Cuxd-Device 0,001 und x,001 als Limits an die Funktion übergibt:
Macht das irgendeinen Sinn? Oder sollte man das korrigieren zu:
EDIT: dieser Quatsch steht nur in der OCCU 3.57.4 drin. Das sollte/muss EQ-3 korrigieren. |
|
Lt. https://homematic-forum.de/forum/viewtopic.php?f=65&t=66446&start=120#p653562
Aufruf der Funktion erfolgt mit: Die Funktion macht also was von ihr erwartet wird. Wieso da Nachkommastellen übergeben werden, wenn gar keine zulässig sind, kann ich nicht beurteilen. HM-CC-RT-DN scheint für die "VALVE_ERROR_RUN_POSITION" einen Integerwert zu erwarten. Der Aufruf erfolgt vermutlich in der Der Programmierer wollte aber wohl wirklich float haben: HM-Sec-Win: Kraft für den Kippantrieb, jede eingegebene Zahl entweder zu "0" oder zu "1".
Der Faktor wird mit 0 übergeben! |
Des Rätsels Lösung ist am Anfang der Funktion versteckt: Mit Thiemos's Testfunktion sowie der ProofAndSetValue aus der RM 3.57.5.20210424 sehe ich einen korrekten Wert bei der Übergabe. |
Ja, das habe ich mittlerweileauch herausgefunden.irgendwo im anderen Thread |
|
Und das bleibt so: https://homematic-forum.de/forum/viewtopic.php?p=659447#p659435 ? |
|
Was übergibst du denn da an Limits, das so viele digits dabei raus kommen? |
|
Sieht man auf Seite 1 des Threads. Deshalb schrieb ich da auch:
@jens-maus hat ja auch bei einigen Aufrufen die Nachkommastellen optimiert. |
|
Ich hab keinen Plan. Hier sind es 2 Stellen bzw. 1 Stelle getMinValue macht gar nichts explizit mit Kommastellen Wenn es jemanden stört, soll er dann bei meinem Addon einen PR machen. Hat ja dann hier nix zu suchen. |
Genau das solltest du ändern. Schau dir mal meine folgenden Änderungen von gestern an: D.h. du solltest da definitiv die Floating Point precision von getMinValue/getMaxValue so anpassen, das es dann nur die anzahl von nachkommastellen zurückgibt die der aktor auch verträgt, oder eben keine damit es ein integer wird wenn ProofAndSetValue() aufgerufen wird. Das hat selbst eQ3 an einigen Stellen versäumt, deshalb ja auch das WirrWarr das ich gerade selbst versuche zu entzwirren - deshalb die Commits bzgl. |
|
Wo ich mir grad andere Hobbies gesucht hab... Na dann warte ich mal auf das was da noch kommt und schaue es mir im nächsten Winter an ^^ Funktion ist ja gegeben 👍 |
|
Da es hier seit geraumer Zeit sehr ruhig geworden ist und in den aktuellen RaspberryMatic Versionen wohl keinen nennenswerten Probleme mehr mit der "ProofAndSetValue" Funktion mehr gibt und auch in aktuellen Testversionen sich keine neue Probleme mehr aufgetan haben, beachte ich dieses Problem erst einmal als gelöst. Falls wieder erwarten es wieder Probleme mit der ProofAndSetValue Funktion geben sollte, dann bitte am besten dazu ein neues separates Ticket aufmachen. |
Describe the bug
Wenn ein Geräteparameter als
min/maxGrenze einen Gleitkommawert besitzt, der nicht auf.0endet, kommt es zu Problemen in der WebUI.Ich habe 3 Konsolenmeldungen eingebaut.
An denen kann man gut sehen, dass die Eingabe in einem Feld, das eigentlich Float zulässt (Wertebereich 0.00 - 7.00) funktioniert - auch wenn in den "float is not allowed"-Zweig eingestiegen wird.
Die Eingabe in einem anderen Feld (0.50 - 15.50) ist kaum bis schwer möglich.
NaN".." wird sofort entfernt (sieht man in dem Screencast glaub ich nicht, weil es zu schnell passiert).Das Problem betrifft nicht nur den Rollladenaktor
System information:
Additional context
...
The text was updated successfully, but these errors were encountered: