Skip to content
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

Draft
wants to merge 40 commits into
base: v1dev
Choose a base branch
from

Conversation

cornelius-koepp
Copy link
Member

@cornelius-koepp cornelius-koepp commented Nov 4, 2024

TODOs

  • Concept
  • Implementation
  • UI/UX ETS
    • Precheck
    • User-Acceptance-Test
    • Context Help
  • Test
    • Development-Tests
    • User-Test
  • Documentation
  • Optimization / Cleanup

Neue Blocktypen

"Anzahl"

  • Zählt 1-Bit-Eingänge (bis zu 9) mit Wert 1/true
  • Ansonsten Konfiguration wie für UND/ODER

"Statistische Aggregation"

  • Verschiedene Funktionen zur Aggregation:
    • Summe
    • Mittelwert
    • Minimum
    • Maximum
    • Spannbreite (Maximum - Minimum)
  • Bis zu 9 nutzbare Eingänge
  • Verschiedene (numerische) Wertetypen für Eingänge und Ausgänge
    • DPT 5.* 8-Bit vorzeichenlos
    • DPT 5.001 Prozent (0..100%)
    • DPT 6.* 8-Bit vorzeichenbehaftet
    • DPT 7.* 2-Byte vorzeichenlos
    • DPT 8.* 2-Byte vorzeichenbehaftet
    • DPT 9.* 2-Byte Gleitkommawert
    • DPT 12.* 4-Byte vorzeichenlos
    • DPT 13.* 4-Byte vorzeichenbehaftet
    • DPT 14.* 4-Byte Gleitkommawert
  • Einstellbares Verhalten für

* 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
@cornelius-koepp cornelius-koepp added the enhancement New feature or request label Nov 4, 2024
@cornelius-koepp cornelius-koepp changed the base branch from v1 to v1dev November 4, 2024 19:52
@@ -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")),
Copy link
Member

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

Copy link
Member Author

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)
Copy link
Member

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.

Copy link
Member Author

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 ;-)

Copy link
Member

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.

Copy link
Member

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.

Copy link
Member Author

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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New Feature/FunctionBlock: Aggregation New Feature/FunctionBlock: Boolean Input Counter
2 participants