-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
New Block-Types Count (Boolean Inputs) and Aggregation (Sum, Mean, Min, Max, Range) #3
base: v1dev
Are you sure you want to change the base?
Conversation
* WIP: Statistic Aggregation - ETS-App-Part * WIP / WIP - Input KOs * WIP - Firmware * WIP - Draft Complete for Test * Separate Types for Inputs and Output * Extend Output Config * Output Same as Input * Include Aggr Block in Module * Fix Build: `readInputKos()` `initMissingInputValues()` from FunctionBlock * Fix: Use Output Type Same as Input * ETS-App Reorder (Aggr): Function, Inputs, Output * ETS-App (Aggr): Cleanup ComObject Definitions * ETS-App (Aggr): Use Unit-Less DPTs 9/14 * Use Assign for Output Type Same as Input Type
… Feedback * Rounding for Non-Float Outputs * Small ETS UI Improvements based on Feedback * Function Selection * Indent Output Type Specific Config * ETS-UI: Show Rounding for output==int && (input==float || fkt=avg) * Implementation for Limiting Output Values to Configured DPT * Value Range Limit * Reduce Range Handling * Extract `_limitToOutputDptRange(result)` * Use int64_t * fixup! Rounding -- crop for int output * Flag for int Output Type
* Extract _dptType * Handling of Output-DPT
@@ -2,7 +2,7 @@ | |||
#include "knxprod.h" | |||
|
|||
LogicFunctionBlock::LogicFunctionBlock(uint8_t channelIndex, LogicFunctionBlockType type) | |||
: FunctionBlock(channelIndex, type == LogicFunctionBlockType::LogicOR ? "OR" : "AND"), | |||
: FunctionBlock(channelIndex, type == LogicFunctionBlockType::LogicOR ? "OR" : (type == LogicFunctionBlockType::LogicAND ? "AND" : "COUNT")), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Das sollten wir nun in eine statische Hilfsfunktion auslagern, vielleicht kommen da ja noch mehr dazu
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ja, diese Form skaliert nicht gut mit größerer Anzahl von (Sub)Typen. Wird beim Blocktyp Aggregation mit 5 Typen noch deutlicher. Hatte es dort erst einmal in ein Makro ausgelagert für bessere Lesbarkeit als in nur einer Zeile. Siehe https://github.com/OpenKNX/OFM-FunctionBlocks/pull/3/files#diff-21a11a6bb4060f3d236ba08e39da2f0541da66dde2f07820c1be33a038e34f95R4-R15
Wir sollten da vor allem eine Lösung finden die möglichst einheitlich ist über alle Funktionsblöcke.
if (inputKo == 2) | ||
inputValue = !inputValue; | ||
|
||
if (_type == LogicFunctionBlockType::LogicAND) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hier würde ich nun auch auf ein switch statt der if elseif Variante gehen. Alternative überlegung wäre sonst, dass LogicFunctionBlock abstract wird, und wir die Typespezifischen Dinge in den Ableitungen machen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Switch wird in Kombination mit der aktuellen Nutzung des break
(mit dem Du die Schleife verlässt wenn das Ergebnis fest steht) unschön. Sonst hätte ich das auch schon direkt mit umgestellt ;-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Man könnte statt dem break den Schleifenzähler auf einen großen Wert stellen, aber ob das schöner ist weiß ich auch nicht.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wahrscheinlich wäre am Besten mit Ableitungen zu Arbeiten und dann eben die Schleife mehrfach implementiert zu haben.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Man könnte statt dem break den Schleifenzähler auf einen großen Wert stellen, aber ob das schöner ist weiß ich auch nicht.
Das wird vom Kontrollfluss dann eher unübersichtlicher.
[…] mit Ableitungen zu Arbeiten und dann eben die Schleife mehrfach implementiert zu haben.
Mehrfach Implementieren der Schleife würde ich vermeiden wollen. Dafür steckt zu viel in der Vorverarbeitung drin. Bei Vervielfältigung der Implementierung läuft das absehbar irgendwann auseinander. Template- oder Strategy-Pattern (Idee: bool canBreak = aggregate(value)
) wären ggf. eine Option, würde aber den Code-Umfang/Dateianzahl an dieser Stelle auch deutlich erhöhen. Eine Verbesserung dadurch wäre daher aus meiner Sicht fragwürdig.
Alternativ könnten wir auch ganz auf die Fallunterscheidung (und Optimierung für AND/OR) verzichten und nur die Eingänge==1 zählen:
AND :<=> (anzahl1 == anzahlGenutzt)
OR :<=> (anzahl1 > 0)
COUNT := anzahl1
* Improve Input Description for AND/OR * Improve Description for Block-Bezeichnung
* ETS-App: Add Multi-Line-Comment HelpContext (to AND/OR/Prio) * ETS-App: Add Multi-Line-Comment HelpContext (to Count) * ETS-App: Add Multi-Line-Comment HelpContext (to Aggregation)
* for Aggregation * Set all Flags to Disabled as Default
* Fix: Rounding Needed for Weighted Sum * Refactor: getParamInputWeight
TODOs
Neue Blocktypen
"Anzahl"
"Statistische Aggregation"