Top S. schrieb:> Das ganze läuft auf einem AtMega328PB. Würdet ihr das auch so machen> oder gibt es da bessere Wege?
Sicher. Man kann eine binäre Suche machen, wenn die Kurve monoton ist,
damit ist man schneller. Man kann auch Speicherplatz gegen
GEschwindigkeit eintauschen, wenn man mehr Werte in die Tabelle
schreibt, dann reicht ein einzelner Zugriff.
Aber die allererste Frage lautet. MUSS es WIRKLICH schneller sein? Wie
oft wird die Funktion aufgerufen?
https://www.mikrocontroller.net/articles/AVR-GCC-Codeoptimierung#Prinzipien_der_Optimierung
Mögliche Maßnahmen:
* Anzahl der Datenpunkte des Kennfeldes (eine Zweierpotenz + 1) machen.
* Input-Range auf 0...(Zweierpotenz - 1) oder -(Zweierpotenz -
1)...+(Zweierpotenz - 1) normieren.
* Input in äquidistanten Punkten vorgeben. Kann auch mit Offset sein,
der dann zuvor abgezogen wird.
Folgen davon:
* Index der Tabelle kann durch schieben des Input nach Rechts gefunden
werden
* Lineare Interpolation kann durch Multiplikation des Nachkommateils
(das was zuvor durch Rechtsschieben für Index herunter gefallen ist) und
schieben gewonnen werden.
Contra Optimierung:
Für eine Tankanzeige läuft deine Rechnung wohl schnell genug, dass man
die nicht optimieren muss.
mfg mf
Thema bitte schließen, sonst artet das aus in einen sinnlosen
Wettbewerb.
Solange das verzögerungsfrei ausgeführt wird, der Speicher nicht voll
ist, und fehlerfrei läuft, Finger weg.
Wie genau soll das sein?
Guck dir erstmal die Werte an, das sieht nicht sonderlich toll aus. Der
Wert bei 72 % passt nicht.
Man könnte da in erster Näherung eine Gerade durchlegen ...
y = 1 - (adc_value - 432)/(972 - 432 + 1)
Hallo,
bis jetzt läuft das alles ziemlich rund. Es ging nur um die
grundsätzliche Richtung da es ein paar mehr Anzeigen sind. Wäre blöd am
Ende festzustellen das ich an die hälfte des Codes noch mal ran muß.
Optimiert wird natürlich nur wenn es eng wird. Bis jetzt ist das kein
Problem.
>Guck dir erstmal die Werte an, das sieht nicht sonderlich toll aus. Der>Wert bei 72 % passt nicht.
Danke, da ist was schief gelaufen.
Gruß Rene
R R schrieb:> Wäre blöd am Ende festzustellen das ich an die hälfte des Codes noch mal> ran muß.
Dann wird da noch mehr per Kennfeld umgerechnet? Wie wäre es, aus der
Linearen Interpolation eine Funktion zu machen, die nicht für jede
Anzeige, die es braucht, Copy-Paste-Legasthenie enthält?
mfg mf
R R schrieb:> Wäre blöd am Ende festzustellen das ich an die hälfte des Codes noch> mal ran muß.
Der Code ist so unwartbar und fehlerhaft.
Versuch den Code für Dich einfach und kurz zu halten. Wobei kurz nicht
meint, einfache Anweisungen durch komplexere zusammenzufassen.
R R schrieb:> oder gibt es da bessere Wege?
Wenn du dein Kennfeld davor und dahinter mit Grenzwerten befüllst,
kannst du dir die beiden if sparen und bekommst immer ein
Interpolationsergebnis.
Unwartbar: kennfeld_len und kennfeld müssen gemeinsam geändert werden,
Abhilfe ofsetof
Der Code enthält Daten der Kennlinie, das ist ebenfalls redundant
(unwartbar). Die ersten zwei if können entfallen. Und nur max am Ende
testen.
Eine Funktion für jede Kennlinie, Abhilfe: Array als Pointer
Fehler: Off by one.
Wenn return im if, dann kein else if.
Danach brauchst Du nur noch per binärer Suche nach Deinem adc Wert
suchen. Die neuen Elemente am Anfang und Ende sorgen dafür, das Du immer
im Array bleibst.
R R schrieb:> Würdet ihr das auch so machen> oder gibt es da bessere Wege?
Ist vollkommen o.k., da muß nichts optimiert werden.
Das ist >1000* schneller, als der langsame Mensch ablesen kann.
Bruno V. schrieb:> Rolf M. schrieb:>> Du meinst wahrscheinlich sizeof>> Dann aber zweimal :-)!
Uups, sorry: ich meinte natürlich countof. Also sizeof(a)/sizeof(a[0])
Und die Prozent braucht es wirklich ? Fuer eine Anzeige ?
Anzeigen sind immer langsam, mehr wie 3 updates pro Sekunde werden als
nervoes wahrgenommen.
Als Optimierung koennte zb ein Fehler zugelassen werden. Sind zB 3% in
der Anzeige zulaessig, dann koennte man die Funktion approximieren. zB
als Polynom, oder stueckweise stetig.
Als weitere Optimierung koennte zB mit 1/128 gearbeitet werden. Ein
Stellglied arbeitet auch besser mit 1/128 wie mit 1/100
Wenn höchste Performance wichtig ist, würde ich das konstante Array
(kennfeld) nicht in den Flash Speicher legen, sondern ins RAM. Weil die
CPU darauf schneller zugreifen kann.
Das letzte Element befindet sich bei Index kennfeld_len - 1. Außerdem
kann man "100" aus dem Array lesen; __flash ist ja transparent für
avr-gcc, daher geht das ohne Overhead:
Die ersten beiden Vergleiche + returns brauchen weniger als 15
Instruktionen, da kann eh nix mehr gespart werden.
Die eigentliche Berechnung würd ich so notieren, dass besser zu sehen
ist, was abgeht (und auch da braucht man kein "\" als Zeilentrenner /
-fortsetzung.
Dividend und Divisor sind int, daher stellt sich auch die Frage nach der
Genauigkeit der Berechnung. Falls die Genauigkeit ungenügend ist, kann
man die ADC-Werte einfach hochskalieren und genauere Werte verwenden,
zum Beispiel indem man am unteren Ende der .adc_value 4 Bit dranhängt,
was immer noch locker in 16-Bit Integer passt. Zu Beginn der Routine
muss dann natürlich auch adc_value angepasst werden per adc_value <<= 4.
Und warum wird die Berechnung signed gemacht? Man kann die Berechnung so
hinschreiben, dass nur positive Werte auftreten, weil: .adc_value ist
positiv und monoton (abgesehen von dem Fehler bei 72), .prozent ist
positiv und monoton.
Johann L. schrieb:> Die eigentliche Berechnung würd ich so notieren, dass besser zu sehen> ist, was abgeht (und auch da braucht man kein "\" als Zeilentrenner /> -fortsetzung.
Zumal sie ja auch einfach falsch ist. Kurze Bezeichner (wie in der
Mathematik) erleichtern das Lesen und verstehen ungemein. Beispiel:
adc_value = a, kennfeld = k, prozent = v (für value) und max = i. Dann
steht da:
R R schrieb:> return (adc_value - kennfeld[max].adc_value) *> (kennfeld[max-1].prozent - kennfeld[max].prozent)\> / (kennfeld[max-1].adc_value - kennfeld[max].adc_value) +> kennfeld[max].prozent;
soweit so gut. Nur ist a nicht zwischen k[i].a und k[i-1].a, sondern
"dahinter".
Es liegt daher an dem TO, mal ein update zu geben, was er verstanden
hat.
Dass Anfang ODER Ende nicht explizit getestet werden müssen ist ja
bereits mehrfach geschrieben (oder beide nicht mit einem weiteren
Eintrag)
Bruno V. schrieb:> Kurze Bezeichner (wie in der> Mathematik) erleichtern das Lesen und verstehen ungemein
Für dich mag das ja gut lesbar sein, aber für Andere evtl. nicht. Ich
finds grausam.
Ingo L. schrieb:>> Kurze Bezeichner (wie in der>> Mathematik) erleichtern das Lesen und verstehen ungemein> Für dich mag das ja gut lesbar sein, aber für Andere evtl. nicht. Ich> finds grausam.
In der Tat. Das verstehen nur Autisten und Freaks.
Was an:
Bruno V. schrieb:> r = (a-k[i].a) * (k[i-1].v-k[i].v)/(k[i-1].a-k[i].a)+k[i].v;
leichter zu lesen und zu verstehen sein soll, erschließt sich mir nicht.
Das Projekt, drei Monate in der Schublade, und selbst der Entwickler
wird grübeln, was denn nun a ist, was k usw. Variablen dürfen heute echt
länger sein als ein-zwei Buchstaben, daran sollte man nicht sparen. Die
Zeiten, wo man eine Variable mit lediglich einem Buchstaben abkürzen
musste, sind schon lange vorbei, auch im Embedded-Bereich.
Damit hieht man direkt die Gleichung für eine Gerade, d.h. Linearität in
adc_value, und dass
adc_value = x2 auf y2 abbildet, und adc_value = x1 auf y1 abbildet.
Tatsächlich kann man ein Kennfeld einfach als Abbildung x → ƒ(x) = y
betrachten. Ob da nun Bananen auf Tomaten abgebildet werden oder
ADC-Werte auf Prozente spielt keine Rolle für die Berechnungen. Das
einzige was zählt sind Wertebereich und Bildbereich (hier int16_t und
int8_t).
Der Vorteil der aktuellen Implementierung ist dann nur, dass die Länge
des Arrays zur Compilezeit bekannt ist, was ein paar Bytes Code einspart
verglichen mit einem verallgemeinerten Ansatz, der die Array-Länge als
Parameter bekommen würde.
Schwieriger ist hingegen die Korrektheit, d.h. dass es innerhalb der
Berechnung nicht zu einem Überlauf kommt. Es wird ja int16_t * int8_t
gerechnet und davon ausgegangen, dass das Ergebnis noch in int16_t
passt.
Keks F. schrieb:> Thema bitte schließen, sonst artet das aus in einen sinnlosen> Wettbewerb.> Solange das verzögerungsfrei ausgeführt wird, der Speicher nicht voll> ist, und fehlerfrei läuft, Finger weg.
Joa, Thema ist wie erwartet gelaufen. Dafür bitte nochmal 5 Downvotes.
Hallo,
Danke für die rege Teilnahme an der Diskusion. War diese Woche bischen
ausgelastet. Deswegen erst jetzte eine Rückmeldung. Ich habe jetzt mal
diese Änderungen eingefügt (der Fehler im ersten Kennfeld ist immer noch
drin):
R R schrieb:> Das ist noch nicht fertig,
Wow, dass sieht sehr gut aus, Du scheinst alle verschiedenen Tipps
wirklich überdacht und abgewogen zu haben.
Die einzige Frage, die sich mir noch stellt: Wenn nur 8 Bit
zurückkommen, würde ich den cast (und ggf. Grenzchecks) in der Funktion
machen.
Ich vermeide casten soweit es geht, da man damit Compiler-Warnungen
verliert (Überlauf, Pointer statt int, ..)
R R schrieb:> humen_value
Hat das was mit Humus zu tun? Oder Homus? ;-)
>adc_value_2_human
value liest man zwar oft in Software und Quellcode, ist aber in den
allermeisten Fällen Unfug, denn es ist ja immer ein Wert! Das muss man
nicht extra schreiben.
R R schrieb:> int16_t adc_value_2_human(int16_t adc_value, const __flash _kf* kf){> uint8_t max = 0;> while (kf[max].adc_value > adc_value){> max++;> }
...
> }
Ist adc_value < 0, terminiert die Iteration nicht, Du läufst aus dem
Array und hast UB.
Warum ist adc_value ein vorzeichenbehafteter Typ?
Falls das so sein soll, gehört da mindestens eine Assertion für den
Wertebereich hin. Oder ändere Dein Sentinel.
>Ist adc_value < 0, terminiert die Iteration nicht, Du läufst aus dem
Array und hast UB.
adc_value kommt direct aus ADCL / ADCH und kann somit nicht kleiner 0
werden.
>Warum ist adc_value ein vorzeichenbehafteter Typ?
Damit in der Funktion adc_value_2_human kein Typwechsel erforderlich
wird.
Ein uint16_t führt zu einem falschen Ergebnis.
>Hat das was mit Humus zu tun? Oder Homus? ;-)
Ups :-)
>value liest man zwar oft in Software und Quellcode, ist aber in den>allermeisten Fällen Unfug, denn es ist ja immer ein Wert! Das muss man>nicht extra schreiben.
Kann ich nachvollziehen. Ist mir aber so lieber, also bleibt es wie es
ist.
>Falls das so sein soll, gehört da mindestens eine Assertion für den>Wertebereich hin. Oder ändere Dein Sentinel.
Meinst du das so:
R R schrieb:
Typedefs nennt man typisch _t, also kf_t.
Die binäre Suche hast du auch nicht. Wenn dein Wert am Ende der
Kennlinie liegt, musst du 32 mal die Schleife abklappern. Mit der
binären Suche reichen 5 Durchläufe. Und der restliche Krümelkram. Eher
so, siehe Anhang.
Falk B. schrieb:> Mit der binären Suche reichen 5 Durchläufe.
Der TO hat oben eine Code mit implizit bekannte Feldgröße präsentiert.
Ein wirklicher CodeSmell. Dann hat er das deutlich verbessert, auch wenn
Grenzen-Checks noch fehlen und es ähnlich fragil (aber üblich) ist wie
ein String mit terminierender Null.
Und jetzt soll er eine binäre Suche implementieren, die die Qualität auf
Anfang rückt? Als Fingerübung OK, aber in Produktionscode läuft das
regelmäßig in noch schwerer zu findende Fehler.
Falk B. schrieb:> R R schrieb:>> Typedefs nennt man typisch _t, also kf_t.> Die binäre Suche hast du auch nicht. Wenn dein Wert am Ende der> Kennlinie liegt, musst du 32 mal die Schleife abklappern. Mit der> binären Suche reichen 5 Durchläufe. Und der restliche Krümelkram. Eher> so, siehe Anhang.
Das mit den signed DT ist immer noch Mist. Nimm unsigned und Du kannst
wieder eine Abfrage sparen. Zudem drückt der Code besser das aus, was
wirklich gemeint ist.
Es wird nirgends sichergestellt, dass die Tabellen sortiert sind. Das
ist ganz großer Mist.
R R schrieb:>>Ist adc_value < 0, terminiert die Iteration nicht, Du läufst aus dem> Array und hast UB.>> adc_value kommt direct aus ADCL / ADCH und kann somit nicht kleiner 0> werden.
Genau. Deswegen nimmt man unsigned.
>>>Warum ist adc_value ein vorzeichenbehafteter Typ?>> Damit in der Funktion adc_value_2_human kein Typwechsel erforderlich> wird.
Dann passe _kennfeld entsprechend an.
>>Falls das so sein soll, gehört da mindestens eine Assertion für den>>Wertebereich hin. Oder ändere Dein Sentinel.>> Meinst du das so:>>
Du scheinst ja schon mal was von Vor- und Nachbedingungen gehört zu
haben ;-)
Die Vorbedingung ist bzgl. adc_value ok, wobei Du das wie gesagt mit
unsigend verbessern kannst. Es fehlt eine Vorbedingung bzgl. kf.
Und zusätzlich sollten die Parametervariablen const sein.
Wilhelm M. schrieb:> Das mit den signed DT ist immer noch Mist. Nimm unsigned und Du kannst> wieder eine Abfrage sparen. Zudem drückt der Code besser das aus, was> wirklich gemeint ist.
OK.
> Es wird nirgends sichergestellt, dass die Tabellen sortiert sind. Das> ist ganz großer Mist.
Wer sagt das? Ich sagte, daß die binäre Suche auf montone Kennlinien
angewiesen ist. Und wenn man nicht total behämmert ist, speichert man
die Kennlinie ganz normal, wie man es in einer Exeltabelle tun würde. In
aufsteigender Reihenfolge, sodaß man einen Graphen darstellen kann.
Wilhelm M. schrieb:> Und zusätzlich sollten die Parametervariablen const sein.
uint8_t adc_value_2_human(int16_t adc_value, const __flash _kf* kf)
Der Zeiger auf des Kennfeld ist ein Zeiger auf const. Bei adc_value ist
es egal, denn das ist immer eine Kopie eines Werts. Selbst wenn man da
drauf schreiben würde, würde das keinerlei Nebenwirkungen im Programm
haben. Ein const ist dort reine Kosmetik.
Falk B. schrieb:> Wilhelm M. schrieb:>> Und zusätzlich sollten die Parametervariablen const sein.>> uint8_t adc_value_2_human(int16_t adc_value, const __flash _kf* kf)>> Der Zeiger auf des Kennfeld ist ein Zeiger auf const.
Korrekt, damit ein Input-Parameter. Das meinte ich aber nicht, s.u.
> Bei adc_value ist> es egal, denn das ist immer eine Kopie eines Werts.
Für den Aufrufer ist das egal. Aber nicht in der Funktion selbst.
> Selbst wenn man da> drauf schreiben würde, würde das keinerlei Nebenwirkungen im Programm> haben. Ein const ist dort reine Kosmetik.
Nein.
uint8_t adc_value_2_human(const int16_t adc_value, const __flash _kf*
const kf);
Variablen sollten wenn möglich read-only sein, so auch
Parametervariablen. Die Bedeutung von adc_value ist eben der Wert des
ADC, wenn ich dummerweise irgendwo in(!) der Funktion adc_value /= 2
machen würde, wäre das Paar (Name, Wert) nicht mehr korrekt.
Bei kf ist es sogar noch schlimmer: kf ist der Zeiger auf eine Tabelle.
Ein kf -= 42 referenziert wahrlich nichts sinnvolles und man hat UB.
Wilhelm M. schrieb:> Falk B. schrieb:>> Wilhelm M. schrieb:>>> Und zusätzlich sollten die Parametervariablen const sein.>>>> uint8_t adc_value_2_human(int16_t adc_value, const __flash _kf* kf)>>>> Der Zeiger auf des Kennfeld ist ein Zeiger auf const.>> Korrekt, damit ein Input-Parameter. Das meinte ich aber nicht, s.u.>>> Bei adc_value ist>> es egal, denn das ist immer eine Kopie eines Werts.>> Für den Aufrufer ist das egal. Aber nicht in der Funktion selbst.
Wieso? Was passiert denn, wenn man versehentlich adc_value beschreibt?
Expoldiert dann der Stack?
>> Selbst wenn man da>> drauf schreiben würde, würde das keinerlei Nebenwirkungen im Programm>> haben. Ein const ist dort reine Kosmetik.>> Nein.>> uint8_t adc_value_2_human(const int16_t adc_value, const __flash _kf*> const kf);>> Variablen sollten wenn möglich read-only sein, so auch> Parametervariablen.
Jaja, man kann alles übertreiben. Eine einfache Parametervariable kann
keinen Schaden außerhalb der Funktion anrichten, das können nur Pointer.
> Die Bedeutung von adc_value ist eben der Wert des> ADC, wenn ich dummerweise irgendwo in(!) der Funktion adc_value /= 2> machen würde, wäre das Paar (Name, Wert) nicht mehr korrekt.
Ohje, die Oberkorrekten.
> Bei kf ist es sogar noch schlimmer: kf ist der Zeiger auf eine Tabelle.> Ein kf -= 42 referenziert wahrlich nichts sinnvolles und man hat UB.
Und schlimmen AbKüFi.
Falk B. schrieb:> Wilhelm M. schrieb:>> Falk B. schrieb:>>> Wilhelm M. schrieb:>>>> Und zusätzlich sollten die Parametervariablen const sein.>>>>>> uint8_t adc_value_2_human(int16_t adc_value, const __flash _kf* kf)>>>>>> Der Zeiger auf des Kennfeld ist ein Zeiger auf const.>>>> Korrekt, damit ein Input-Parameter. Das meinte ich aber nicht, s.u.>>>>> Bei adc_value ist>>> es egal, denn das ist immer eine Kopie eines Werts.>>>> Für den Aufrufer ist das egal. Aber nicht in der Funktion selbst.>> Wieso? Was passiert denn, wenn man versehentlich adc_value beschreibt?
Das hatte ich in meinem Beitrag beschrieben.
> Expoldiert dann der Stack?
Das habe ich nicht geschrieben ;-)
>>> Selbst wenn man da>>> drauf schreiben würde, würde das keinerlei Nebenwirkungen im Programm>>> haben. Ein const ist dort reine Kosmetik.>>>> Nein.>>>> uint8_t adc_value_2_human(const int16_t adc_value, const __flash _kf*>> const kf);>>>> Variablen sollten wenn möglich read-only sein, so auch>> Parametervariablen.>> Jaja, man kann alles übertreiben. Eine einfache Parametervariable kann> keinen Schaden außerhalb der Funktion anrichten, das können nur Pointer.
In diesem Fall ja nicht, weil es ein Input-Parameter ist. Hatte ich auch
geschrieben.
>>> Die Bedeutung von adc_value ist eben der Wert des>> ADC, wenn ich dummerweise irgendwo in(!) der Funktion adc_value /= 2>> machen würde, wäre das Paar (Name, Wert) nicht mehr korrekt.>> Ohje, die Oberkorrekten.
Nun, der TO hatte nach Optimierung gefragt. Und optimieren kann man in
verschiedene Richtungen. Auch das sollte Dir klar sein.
Ich denke, dass auch Du Dich manchmal fragst, was der Wert in einer
Variablen für eine Bedeutung hat. Und es ist gut, wenn der Name darüber
Auskunft gibt.
Ansonsten braüchte man die Variable auch nicht adc_value zu nennen,
sondern könnte einfach x als Name nehmen. Ich denke, das hast auch Du
verstanden.
>> Bei kf ist es sogar noch schlimmer: kf ist der Zeiger auf eine Tabelle.>> Ein kf -= 42 referenziert wahrlich nichts sinnvolles und man hat UB.>> Und schlimmen AbKüFi.
Dir scheint entgangen zu sein, dass kf der Name der Parametervariablen
ist. Also nix AbKüFi, jedenfalls nicht von mir. Denn ich habe nur die
Namen des TO übernommen.
Thread beobachten
Seitenaufteilung abschalten