ioBroker / ioBroker.node-red

Instantiate the server with node-red
Apache License 2.0
52 stars 27 forks source link

ioBroker get liefert falsche Werte #258

Closed Bernd9000 closed 2 years ago

Bernd9000 commented 2 years ago

Ich möchte über node-red bestimmte Datenpunkte in ioBroker abfragen ob Sie existieren. Dazu trage ich einfach das Topic in ioBroker-get node ein. Zwei Topic Beispiele:

1.) mqtt.0.shellies.Shelly_25_Reserve.roller.0.command 2.) mqtt.0.shellies.Shelly_25_Reserve.roller.0.command/pos 3.) mqtt.0.shellies.Shelly_25_Reserve.roller.0.test

Alle drei Topic existieren aktuell nicht. Der erste und zweite Topic haben einmal existiert. Als msg.payload erhalte ich beim ersten "null" beim zweiten und dritten NICHTS=keine Ausgabe

Sollte nicht bei allen drei "null" als msg.payload ausgegeben werden? Gibt es eine andere Möglichkeit ein Topic auf Existenz zu prüfen?

Apollon77 commented 2 years ago

Bitte logleel des iobroker adaptera auf debug stellen und log posten

Bernd9000 commented 2 years ago

Alle drei Inject nodes gedrückt, nur beim ersten Topic kommt "null" 1

Debug: 2

Apollon77 commented 2 years ago

Naja es besteht formal ein unterschied zwischen "einem State der den Wert 'null' hat" und "einem state der nicht existiert aka "keinen" Wert hat ... "keiner" ist nicht gleich "null" ... "Null" ist ein erlaubter Wert eines States. Was sollte denn bei einem "nicht existierenden State" als Wert weitergegeben werden?

Bernd9000 commented 2 years ago

Aber alle drei States existieren gar nicht, dann sollte doch als payload bei allen drei das selbe kommen ? Bei einem nicht existierendes Objekt würde ich das nicht im msg.payload ausgeben sondern z.B. auf msg.error ausgeben. Dies könnte man als Anwender dann geziehlt abfragen.

Apollon77 commented 2 years ago

Am Ende wäre meine Vermutung das (aus welchen grund auch immer) bei dem einen zwar vllt das Objekt nicht existsiert aber ein State da ist ... das sollte nicht passieren ... aber :-) Ich überlege mal ob man ggf doch das Objekt zusätzlich prüft

Bernd9000 commented 2 years ago

Okay, danke

Bernd9000 commented 2 years ago

Nachtrag: Der output vom ioBroker-get sollte bei einem nicht vorhandenen Datenpunkt wenigstens den Topic enthalten und als payload "undefined".So kann man darauf reagieren.

mickym2 commented 2 years ago

Ich denke mal die komplette Diskussion ist ja in diesem Thread enthalten. Ich bin hier aber auch der Meinung von @Apollon77, dass es ein unterschied ist, ob man null - also kein Wert in einem existierenden State - zurückgibt oder das Objekt gar nicht existiert. In diesem Fall - sollte die GET Node - die Existenz des Objektes überprüfen. Falls kein Objekt vorhanden ist, sollte man ruhig einen Fehler (error) erzeugen, den man mit einer Catch Node abfangen kann und ggf. darauf reagieren kann und das Objekt über eine iobroker_out node anlegen zu können. Dazu muss natürlich das error Objekt das topic enthalten, dass nicht existiert.

Apollon77 commented 2 years ago

@mickym2 An error feels too hard because in fact this is still a valid case - especially when states have values with expiry or such ... what about returning undefined? Is this allowed in node-red? This would differentiate is to null vs a value?!

Apollon77 commented 2 years ago

My proposal would be to return undefined (if return type is object) or empty string (if valueConvert is true). Only question: Add option to choose? Because in fact it will be "breaking" because now the nodes of such states will send out a content which was not the case before. So add a config for "also send payload for not existing state values" with default false?

mickym2 commented 2 years ago

So considering all options - I came to the following solution:

I think to get a clear output for all cases would be:

  1. Non existing states ==> error (configurable as option to have backward compatibility), but can be handled in the flow with a catch node.
  2. Uninitialized state ==> undefined
  3. Empty state only for empty strings.

In my opinion it should be avoided in any case to have a different behavior of the node returning an object or value.

Therefore in summary I agree to add a config for "also raise error for not existing state values" with default false - but not sending a payload instead raising an error ( to be able to differentiate between non existing states and uninitialized states).

Apollon77 commented 2 years ago

GitHub updated

Bernd9000 commented 2 years ago

Danke, aber ich glaube das funktioniert noch nicht richtig ?

In den Eigenschaften vom ioBroker-get Node wird die Option wohl nicht gespeichert. Stelle ich auf "Return state value as undefined" und öffne erneut steht es wieder auf "Return error on non existing Object" 1

Stelle ich als Topic ein nicht existierendes Objekt ein wird nur ein Leerstring übergeben. Node-status wird aber gelb angezeigt 2

mickym2 commented 2 years ago

Nun das undefined finde ich nicht so gut - aber funktioniert denn der Error - sprich kannst Du dann mit einer Catch-Node den Fehler abfangen und dann einen neuen Flow anstoßen?

Bernd9000 commented 2 years ago

Nein, der catch Node liefert gar nichts.

mickym2 commented 2 years ago

Nein, der catch Node liefert gar nichts.

Du musst eine Debug-Node mit Objekt nehmen - es kommt ein err-Objekt und keine payload raus.

Falls aber gar nichts kommt, dann stimmt was noch nicht. ;)

Bernd9000 commented 2 years ago

Wie stelle ich den Debug-Node auf Objekt um ? Wenn ich den ioBroker-get auf Objekt umstelle wird übrigens (null) geliefert. 1

Bernd9000 commented 2 years ago

Okay, ich vermute mal hier wie im Bild, aber da kommt auch nichts 1

mickym2 commented 2 years ago

OK - I confirm - the error option does not work.

But I get always undefined for non existing states back.

image

Apollon77 commented 2 years ago

Please check again ... seems I forgot to define default in the iobroker.html ...

Apollon77 commented 2 years ago

Stelle ich als Topic ein nicht existierendes Objekt ein wird nur ein Leerstring übergeben.

Das ist "wie die bisherige Logik", da wollte ich nicht breaken. Wenn man "value" einstellt dann wird undefined und null auf einen leerstring gemapped ... "War schon immer so" ... das undefined kommt zurück wenn man "objekt" wählt anstelle value

Bernd9000 commented 2 years ago

Hab jetzt nochmal getestet, ioBroker-get ist auf Object eingestellt. Ergebnis bei einem nicht existierenden Objekt:

1.) "Return state value as undefined" = null 2.) "Return error on non existing Object" = null

Bei beiden kommt das gleiche Ergebnis. Ein error kommt nicht, weder beim debug-node noch beim catch-node Oder mache ich was falsch?

Apollon77 commented 2 years ago

Ok, try again please ... seems a new settings needed even another line of logic

mickym2 commented 2 years ago

Na wenn man die bisherige Logik nicht brechen will muss man null (also nichts) zurückgeben, dann wird aber entweder nichts zurückgegeben oder ein Fehler erzeugt.. Undefined eigentlich nie (was in meinen Augen richtig ist und weiterhin nur existierenden, nicht initialisierten States vorbehalten bleibt). Und um einen Fehler zu erzeugen muss man die node.error mit einer Fehlerbeschreibung und einem msg Objekt aufrufen. Um es kompatible zu machen - gibt man eine Beschreibung mit.

node.error("Dies ist ein Fehler",{description:"Function get kann state xxxx nicht ermitteln"});

https://nodered.org/docs/user-guide/writing-functions#logging-events Über die catch Node kann man dann so was abfangen und reagieren:

image

Der Vorteil eines Fehlers ist außerdem, dass er in diesem Fall, wenn man keine Catch Node verwendet - dieser Fehler im Log des iobrokers auftaucht.

image

mickym2 commented 2 years ago

Im Moment bekomme ich weiterhin nur undefined zurück (egal welche Option - die eingestellte Option bleibt auch nicht im Dialog erhalten):

image

Kein Rückschluss auf eingestellte Option und Auswahlfenster müsste größer sein

image

Apollon77 commented 2 years ago

Ich kapiers nicht :-( Er ist im korrekten Code-Block, sonst wäre die Markierung am Node nicht "gelb". kann also nur heissen das das setting nicht ankommt

https://github.com/ioBroker/ioBroker.node-red/blob/f7a073e8ee02c2f85a99e849a48dc65e60f62418/nodes/ioBroker.js#L579-L592

Das kann nur heissen, dass der Wert nicht gespeichert wird oder nicht durchgereicht wird :-( kannst du ggf mal eine Logzeile vor dem Block einbauen und node.errOnInvalidState loggen ?

Apollon77 commented 2 years ago

Na wenn man die bisherige Logik nicht brechen will muss man null (also nichts) zurückgeben,

Nicht ganz richtig weil die ganze zeit schon für "also payload "value" zurückgeben" ein leerer String zurückgegeben wird

Siehe: https://github.com/ioBroker/ioBroker.node-red/blob/f7a073e8ee02c2f85a99e849a48dc65e60f62418/nodes/ioBroker.js#L565

(unchanged zu vorher). "null" wird nur zurückgegeben wenn als payload "object" gewählt wurde

Apollon77 commented 2 years ago

PS: Ich hab auch das Feld mal "breiter "gemacht ... kannst bitte nochmal GitHub installieren

mickym2 commented 2 years ago

Nun arbeitet es richtig . Aber ich bestreite, dass vorher ein undefined zurückgegeben wurde.

image

Aber wie gesagt backward komptibel ist es nicht. Denn vorher wurde gar nichts zurückgegeben, siehe erstes Posting:

Alle drei Topic existieren aktuell nicht. Der erste und zweite Topic haben einmal existiert. Als msg.payload erhalte ich beim ersten "null" beim zweiten und dritten NICHTS=keine Ausgabe

Tschuldige wenn ich Dir also widerspreche.

Apollon77 commented 2 years ago

Aber ich bestreite, dass vorher ein undefined zurückgegeben wurde.

Da stimme ich zu. Vorher wurde NICHTS zurückgegeben sondern nur was geloggt.

Aber wie gesagt backward komptibel ist es nicht. Denn vorher wurde gar nichts zurückgegeben, siehe erstes Posting:

korrekt - hatte dich aber oben so verstanden. :-) ... Wenn wir es backward compatible haben wollen muss ich noch ein "return states without value" true/false setting machen. Kann ich tun. Sag an :-) Andererseits ist es ein Major increase was wir hier machen, wir könnten also hier breaken!

mickym2 commented 2 years ago

Wenn es nicht zuviel Aufwand bedeutet - eine 3. Option rein zu machen (in der nichts zurückgegeben wird - ignore non existing states) - dann hätte man backward Kompatibilität und die Leute müssten in ihren bisherigen Flows ggf. keine Switch-Node zum herausfiltern anhängen.

Ich glaube es wird ein tolles Release!!! - Endlich mal alle Dinge bereinigen. Ich danke schon mal dafür.

Die get und list Nodes werde eh oft ohne topic in der Editor UI verwendet - sondern über msg.topic gesetzt.

Apollon77 commented 2 years ago

Have fun :-)) https://github.com/ioBroker/ioBroker.node-red/commit/38ee79157b5203c7a03e6a96eec382c9e1a4714e

Default ist jetzt das "backward compatible" und das loggt jetzt je nachdem ob objekt oder state empty ist was sinnvolles aber sendet nichts raus

mickym2 commented 2 years ago

So das arbeitet perfekt

Bernd9000 commented 2 years ago

Super, Danke