Hallo,
hab ein Problem: in folgendem Code-Schnipsel wird current_time in der
ersten Zuweisung offenbar falsch berechnet, denn mit der zweiten
(hartkodierten) Zuweisung funktioniert alles wunderbar. Wer denkt hier
falsch: ich, der cpp oder (avrgc)c?
Übrigens liefert derselbe Code, wenn ich ihn einfach mal ins
Hauptprogramm reinschreibe und 'ne Debugausgabe mache, das richtige
Ergebnis.
Gruß, Mr Ratlos
Ralph K. schrieb:
#if ( F_CPU >= 1.000.000 ) && ( F_CPU <= 10.900.000 )
#elif ( F_CPU >= 8.000.000 ) && ( F_CPU <= 87.200.000 )
Irgendwie leuchtet mir dieser Code nicht recht ein.
Also die Überlappung.
Gerade getestet: auch die folgende Variante schlägt fehl:
uint16_t tempICR1; tempICR1 = ICR1;
current_time = (uint32_t)1000 * tempICR1 / 2000 ; // §§§ error
Also wie (1000 * x / 2000) bei ausreichender Bitbreite ungleich (x / 2)
sein kann, ist mir schleierhaft.
Mr. Ratlos^2
> Irgendwie leuchtet mir dieser Code nicht recht ein.> Also die Überlappung.
Verstehe ich, aber die ist hier ja nicht von Belang.
Bei gegebenem F_CPU ist der Prescaler halt 8.
Probleme, die sich besser mit einem übersetzbaren Beispiel
nachvollziehen lassen, lassen sich ohne übersetzbares Beispiel
schlechter nachvollziehen.
> ist hier ja nicht von Belang.
Korrekt. Darum ging es auch nicht. War nur drüber gestolpert.
Das ist das 16bit-Register ICR1 auf einem ATMega168. Also irgendwas
zwischen 0 und 2^16-1 :)
Hier kommt der Code (anbei), falls jemand willens ist, da reinzuschauen.
Es geht um Zeile 229 ff. in DHT11+22.c
Ähm, Tabweite im Editor auf 4 Zeichen setzen, dann sieht's auch gut aus
;)
Ralph K. schrieb:> Gerade getestet: auch die folgende Variante schlägt fehl:>> uint16_t tempICR1; tempICR1 = ICR1;> current_time = (uint32_t)1000 * tempICR1 / 2000 ; // §§§ error>> Also wie (1000 * x / 2000) bei ausreichender Bitbreite ungleich (x / 2)> sein kann, ist mir schleierhaft.>> Mr. Ratlos^2
Ich kann hier leider nix nachschauen aber schreib mal
uint32-t statt uint16-t
Und klammer mal ((uint32-t) 1000 * tempicr1)/2000
Sobald ICR1 größer 65 wird reicht der 16 Bit Raum nicht mehr aus um die
Multiplikation durchzuführen. Du musst also sowohl die Operanden und
anschließend das Ergebnis nach 32 Bit casten.
Ebenso musst du mal eine Worst-Case Rechnung durchzuführen um zu wissen,
ob current_time das Ergebnis der Division überhaupt aufnehmen kann.
Auf einem ATmega sind das aber teure Rechnungen.
Al schrieb:> Sobald ICR1 größer 65 wird reicht der 16 Bit Raum nicht mehr aus um die> Multiplikation durchzuführen.
1000L * ICR1 wird in 32 Bits berechnet.
> Du musst also sowohl die Operanden und> anschließend das Ergebnis nach 32 Bit casten.
Ein Operand allein reicht.
Ralph K. schrieb:> current_time = (uint32_t)1000 * tempICR1 / 2000 ; // §§§ error
dann schreib mal
tempICR1 = 65;
current_time = (uint32_t)1000 * tempICR1 / 2000 ; // §§§ error
printf("%d", current_time);
ich bin sicher dass das Ergebnis richtig rauskommt.
Allerdings stelle ich mir die Frage warum du nicht einfach durch 2
dividierst?
Ralph K. schrieb:> current_time = 1000L * ICR1 / ( F_CPU / (PRESCALER_VALUE) / 1000 ); //> §§§ -> error> current_time = ICR1 / 2; // §§§ -> okay
die beiden Zeilen sind aber auch definitiv nicht das gleiche, weder für
PRESCALER_VALUE 1, noch für 8
Walter schrieb:> die beiden Zeilen sind aber auch definitiv nicht das gleiche, weder für> PRESCALER_VALUE 1, noch für 8
Und warum?
current_time = 1000L * ICR1 / ( F_CPU / (PRESCALER_VALUE) / 1000 );
Assoziativität von l9nks nach rechts bei / und *. Zudem bindet die
Klammer stärker, also:
1
current_time=1000L*ICR1/(16000000/8/1000);
2
current_time=1000L*ICR1/(2000000/1000);
3
current_time=1000L*ICR1/2000;
4
current_time=1*ICR1/2;
exakt das Selbe.
---
Ralph K. schrieb:> current_time = (uint32_t)1000 * tempICR1 / 2000 ; // §§§ error
Versuch mal anstatt des Zahlenwertes die Variable in 32Bit zu casten:
current_time = 1000 * (uint32_t)tempICR1 / 2000 ; // geht vielleicht
;-)
Roland ... schrieb:> Versuch mal anstatt des Zahlenwertes die Variable in 32Bit zu casten:
Sollte das gleiche Ergebnis haben. Bei Zweifel am Code könnte ebendieser
helfen (.lss beispielsweise).
So, zunächst erstmal ein Dankeschön für alle Beiträge.
Hier sind ja einige Dinge angesprochen worden, die ich kurz
zusammenfassen möchte (alles überprüft und getestet):
a) 1000L -> Operation wird mit 32 ausgeführt
b) der typecast eines Operanden genügt
c) eine Klammer um den Ausdruck vor der Division ändert nichts am
Ergebnis (und sollte es wegen der Operator-Assozitivität auch nicht)
d) die beiden Ausdrücke (also der gewünschte und die einfache Division
SIND identisch)
Das Rätsel scheint aber auf einer ganz anderen Ebene zu liegen. Ich habe
jetzt fast 2 Stunden im Nebel herumgestochert und dabei auch mal
Codezeilen verschoben (wegen temporären Variablen und Zuweisungen an
Debug-Variablen). Dabei hat sich gezeigt, dass die Reihenfolge der
Anweisungen eine Rolle spielt. Ein Beispiel:
Also das ist eindeutig ein Fall einer Verschwörung der Technik gegen
ihren Herrn und Meister :D.
Den Prozessor habe ich inzwischen übrigens auch schon mal getauscht,
hätte ja ein Bitfehler sein können - ist aber leider nicht der Fall.
Hat jemand von Euch schon mal ähnliche Erfahrungen der dritten Art
gemacht? Wäre dankbar für jeden Hinweis der zur Ergreifung des Fehlers
führt ;)
Mr. Ratlos^3
Roland ... schrieb:> exakt das Selbe.
stimmt, habe ich mich verguckt
aber es gilt immer noch, ich bin zuversichtlich dass der Fehler
außerhalb dieser Formel liegt:
Walter schrieb:> dann schreib mal> tempICR1 = 65;> current_time = (uint32_t)1000 * tempICR1 / 2000 ; // §§§ error> printf("%d", current_time);
Das war ja auch zunächst mein Verdacht. Aber meine vorsichtige Antwort
lautet nein. Wenn die #DEBUG-Defines nicht gesetzt sind, gibt es nur ein
Array in dem betreffendem Modul und dessen Indizes sind dem gründlichen
Augenschein nach fehlerfrei. iEdge kann auch nicht größer als 83 werden,
denn das wird getestet und die Schleife beendet, in der die Codestelle
steht, die mir Kopfzerbrechen macht.
Vielleicht schaut hier ja auch ein Assembler-Lesen Könnender rein? (Man
soll die Hoffnung ja nie aufgeben :). Falls ich richtig liege, ist die
entscheidende Stelle die folgende:
funktioniert (Division durch Konstante 2000):
1
ldi r26,lo8(0) ; D.1712,
2
ldi r27,hi8(0) ; D.1712,
3
asr r27 ; D.1712
4
ror r26 ; D.1712
5
ror r25 ; D.1712
6
ror r24 ; D.1712
7
movw r12,r24 ; current_time, D.1712
funktioniert nicht (Division durch eine Variable, der ich den Wert 2000
zugewiesen habe):
1
ldi r24,lo8(0) ; D.1712,
2
ldi r25,hi8(0) ; D.1712,
3
ldi r18,lo8(1000) ; ,
4
ldi r19,hi8(1000) ; ,
5
ldi r20,hlo8(1000) ; ,
6
ldi r21,hhi8(1000) ; ,
7
call __mulsi3
8
ldi r18,lo8(2000) ; ,
9
ldi r19,hi8(2000) ; ,
10
ldi r20,hlo8(2000) ; ,
11
ldi r21,hhi8(2000) ; ,
12
call __divmodsi4
13
movw r12,r18 ; current_time, tmp113
Erkennt hier jemand Fehlerpotenzial?
[Ergänzung: Letzter Akt für heute - habe die disassemblierte Objektdatei
hier mal noch drangehängt.]
Walter schrieb:> aber es gilt immer noch, ich bin zuversichtlich dass der Fehler> außerhalb dieser Formel liegt:>> Walter schrieb:>> dann schreib mal>> tempICR1 = 65;>> current_time = (uint32_t)1000 * tempICR1 / 2000 ; // §§§ error>> printf("%d", current_time);
noch Mal, was kommt bei dieser Ausgabe raus!
Versteif dich doch nicht auf die blöde Formel wenn du sie noch nicht
wirklich als Fehler isoliert hast.
Die "schwierigen" Fehler sind immer die bei denen man betriebsblind auf
die faslsche Stelle starrt
Hallo Walter, wenn ich diesen Ausdruck berechne, kommt halt 32 raus. Das
geht soweit okay. Ich hatte gehofft, das in
Beitrag "Re: Fehler bei einfacher Integer-Division?" schon klar genug
beschrieben zu haben.
Inzwischen habe ich eine praktikable Variante gefunden, um das Problem
zu umgehen: den Divisor
( F_CPU / (PRESCALER_VALUE) / 1000 ) in eine C-Konstante
zu packen.
Der Compiler erzeugt dann Code ohne call von __mulsi3, so wie er es auch
macht, wenn da eine 2000 als Literal steht. Es läuft dann alles wie man
es halt erwartet und auch diese mysteriöse Abhängigkeit von der
Code-Reihenfolge löst sich in Luft auf.
Bleibt trotzdem der bittere Nachgeschmack, was da eigentlich passiert
ist. An einer Aufklärung der Sache bin ich von daher immer noch sehr
interessiert ;) Wenn jemand den Code in bestimmter Weise
zurechtgestutzt/aufbereitet haben möchte, um damit dann bei der
Fehlersuche zu helfen, mache ich das gerne.
Grüße, Mr. Ratlos^0
Zeig halt mal den kompletten C-Source Code.
Auch wäre es hilfreich, mal ein paar Werte für ICR1 zu kennen. Dann
könnte man den Code auch mal auf potentielle arithmetische Overflows
abklappern.
Ich mach den Code mal hübsch (werfe das ganze Debugging-Gedöns raus) und
stell ihn nochmal hier ein. Die hier als erstes gepostete Variante
glänzt halt nicht gerade mit Übersichtlichkeit. So in einer Stunde
etwa...
Ralph K. schrieb:> Ich mach den Code mal hübsch (werfe das ganze Debugging-Gedöns raus) und> stell ihn nochmal hier ein.
So weit reduzieren, dass das Problem gerade noch auftritt, aber nichts
Unnötiges mehr drin ist.
Ralph K. schrieb:> Ich mach den Code mal hübsch (werfe das ganze Debugging-Gedöns> raus) und> stell ihn nochmal hier ein. Die hier als erstes gepostete Variante> glänzt halt nicht gerade mit Übersichtlichkeit. So in einer Stunde> etwa...
Schön. Bis jetzt nur 1 Sache (aber bei mir ist die Formatierung auch
nicht gut, d.h. bin nicht sicher):
- iByte = (iEdge-5) 2 8; Ist getestet dass IEdge>=5 und <=84?
Ersetz auch mal
if (iEdge == 83)
durch
if (iEdge >= 83)
und:
int8_t get_humidity( void)
{
if ( dataIsValid )
return data[0];
else
return -128;
}
ist eine Konversion von data[0] nach int8_t. Ist es klar und getestet,
dass data[0] nur von 0 bis 127 geht?
Danke für das Drüberschauen und die Tipps.
lalala schrieb:> iByte = (iEdge-5) 2 8; Ist getestet dass IEdge>=5 und <=84?
Ja, das ist sicher.
> Ersetz auch mal> if (iEdge == 83)> durch> if (iEdge >= 83)
Definitiv unnötig.
> ist eine Konversion von data[0] nach int8_t, Ist es klar und getestet,> dass data[0] nur von 0 bis 127 geht?
Auch das ist vom Wertebereich her korrekt, wobei es im Falle von
get_humidity() ein klarer Schönheitsfehler ist, schließlich ist die
Luftfeuchtigkeit positiv definit.
Bin dann auch gleich fertig mit dem Code säubern...
Bis gleich
Hier kommt Code - ohne Helm und ohne Mode :)
Ich habe mich entschieden, funktionell alles unverändert zu lassen. So
lang ist der Quelltext ja nicht.
Für die Beschreibung des Problems bitte ich Euch, den Thread nochmal zu
überfliegen. Fakt ist:
Zeile 169 im Code aktiv -> der Wert von current_time ist nach allen
Regeln des heuristischen Suchens = 0
Zeile 169 auskommentiert (168 übernimmt) -> alles bestens.
Wenn noch was benötigt wird, nur zu, ich stehe zu Euren Diensten :D
//---------------------------------------------------------------------
Ich habe gerade noch den Quelltext von 4er-Tabs befreit, somit sollte
ihn jeder angenehm lesen können. Siehe zweite Datei im Anhang.
Assembler ist nicht gerade meine Spezialität ;)
Anbei die disassemblierte ELF-Datei in der Variante mit der
fehlerverursachenden Code-Zeile. Im Listing wird es in Zeile 346
interessant.
Soweit ich das sehe wird folgender Code erzeugt:
1000 * x / 2000, wobei zunächst die Multiplikation als 16u*16u -> 32u
ausgeführt wird und danach eine 32-Bit unsigned Division.
Das ist doch das, was du willst?
Wie ich am Disassembly sehe verwendest du eine recht neue Version von
avr-gcc. Welche genau?
Ändert sich das Verhalten mit -fwrapv ? mit -fno-strict-aliasing?
Die Positionierung von memset() im Disassembly von acquire_DHT() ist
unklar, entspricht das er erwarteten Stelle? Hier scheint der Compiler
den Code rumzuschieben, was zusammen mit dem weggecasteten volatile
evtl. zu Problemen führt (evtl. auch nur dann, wenn sich an ganz
anderer Stelle im Code was ändert).
Johann L. schrieb:> 1000 * x / 2000, wobei zunächst die Multiplikation als 16u*16u -> 32u> ausgeführt wird und danach eine 32-Bit unsigned Division.>> Das ist doch das, was du willst?
Ja, genau das sollte passieren. Nur sollte nach der Zuweisung an die
Variable current_time ein Zugriff darauf auch diesen Wert liefern.
> Wie ich am Disassembly sehe verwendest du eine recht neue Version von> avr-gcc. Welche genau?
avr-gcc (AVR_8_bit_GNU_Toolchain_3.4.3_1072) 4.8.1
Hatte bis vor kurzem noch die WinAVR2010-Installation, aber um zu
prüfen, ob da möglicherweise die Probleme liegen, habe ich auf die
gcc/libc aus der aktuellen ATMEL Toolchain gewechselt.
> Ändert sich das Verhalten mit -fwrapv ? mit -fno-strict-aliasing?
Sorry, aber heute kann ich das nicht mehr überprüfen, klappt vielleicht
morgen. Ich melde mich wieder, sobald ich was habe.
Johann L. schrieb:> Die Positionierung von memset() im Disassembly von acquire_DHT() ist> unklar, entspricht das er erwarteten Stelle? Hier scheint der Compiler> den Code rumzuschieben, was zusammen mit dem weggecasteten volatile> evtl. zu Problemen führt (evtl. auch nur dann, wenn sich an ganz> anderer Stelle im Code was ändert).
Ich glaube nicht, dass hier das Problem liegt. Zum einen scheint mir das
Assemblerlisting da gut auszusehen. Zum anderen hatte ich schon mal
testweise das memset() durch explizite Zuweisungen ersetzt ohne dass
sich dadurch am Fehlerverhalten etwas geändert hatte. Zum anderen: dass
data[] als volatile deklariert ist, ist ein historisches Überbleibsel
und insofern ohne Belang, als das es keine Zugriffe auf das array aus
einer ISR mehr gibt. Ich werde die Deklaration und das memset trotzdem
noch mal ersetzen, bei so einer Suche im Dunkeln ist jede noch so kleine
Unklarheit möglicherweise eine zuviel.
Johann L. schrieb:> Ändert sich das Verhalten mit -fwrapv ? mit -fno-strict-aliasing?
Nein, es ändert sich nicht.
Ich kann's kaum glauben, dass das Problem so ungewöhnlich ist, dass
niemand eine Lösung/Antwort parat hat. Nun gut, wenn hier keine Ideen
mehr kommen, lege ich das Problem vorerst zu den Akten - bis ich
fließend Assembler lesen kann :)
Ralph K. schrieb:> Johann L. schrieb:>> Ändert sich das Verhalten mit -fwrapv ? mit -fno-strict-aliasing?>> Nein, es ändert sich nicht.>> Ich kann's kaum glauben, dass das Problem so ungewöhnlich ist, dass> niemand eine Lösung/Antwort parat hat. Nun gut, wenn hier keine Ideen> mehr kommen, lege ich das Problem vorerst zu den Akten - bis ich> fließend Assembler lesen kann :)
Wie überprüfst Du genau, dass der Wert verkehrt ist? Der Fehler liegt
vermutlich nicht an der von Dir vermuteten Stelle.
lalala schrieb:> Wie überprüfst Du genau, dass der Wert verkehrt ist? Der Fehler liegt> vermutlich nicht an der von Dir vermuteten Stelle.
Zum einen (schnelle Prüfung):
Durch Prüfen von data[] nach Ende der Schleife. Stimmt der Wert des in
Frage stehenden Teilers (bzw. das Ergebnis der Division) nicht, dann
stimmt das dadurch bestimmte Timing des Sensorsignal-Lesens nicht, die
Schleife bricht ab und liefert (quasi als Error-Flag) ein ungültiges
Ergebnis.
Zum anderen (DEBUG-Variante):
Durch eine Speicherung von current_time zu jedem Schleifendurchlauf in
einem Array, welches dann (wegen des möglicherweise zeitkritischen
Verhaltens der Schleife) nach Schleifenende per UART ausgegeben wird.
Johann L. schrieb:> Wenn du sicher bist, daß du die böse Zeile gefunden hast, kannst du ja> für beide Varianten Code erzeugen und miteinander vergleichen.
Mache ich heute abend und stelle es hier mit rein.
Noch eine Frage dazu: wie sollte ich idealerweise das Assemblerlisting
erzeugen, damit beide Varianten gut diffbar sind?
Nächster Schuss ins blaue: vielleicht verbraucht die Fehlerhafte Version
zu viel Zeit. Test: nimm die ok Fassung rechne noch eine andere Variable
aus (schau dass das nicht wegoptimiert wirs)
Das ist einfach zu viel Code, 1000 Zeilen Disassembly, sorry, das wird
sich hier keiner anschauen.
Und oben war der Code noch ganz anders, dort wurde durch 2000 geteilt
und jetzt durch eine Variable, die 2000(?) enthält. Was denn nun?
Schreib mal ein kleines Testprogramm, einmal mit Version A und einmal
mit Version B, z.b.
Und lasse das für verschiedene X durchlaufen und vergleiche die
Ergebnisse. Kommt das gleiche raus?
Den Fehler innerhalt 1000 anderer Zeilen zu suchen ist nicht so
sinnvoll, zumindest wenn du dir sicher bist, daß die Division der
Übeltäter ist.
Und wie zeitkritisch ist all das? Stört es denn garnicht, wenn die
Division ewig Zeit verbraucht? test_A oben brauch rund 20 Ticks, test_B
locker mehr als 500.
Du kannst auch die test_A und test_B in dein Programm einbauen, kommt
der Fehler dann immer nocht? Dann ist es ein Timing-Problem. Kommt er
nicht, steht der Aufruf von test_B wohl an anderer Stelle als die
Division, nämlich dort, wo auch test_A aufgerufen wird.
Und wie ich bereits oben schrieb, befindet sich die Division im asm an
anderer Stelle, als der C-Code vermuten lässt.
...und falls es dich beruhigt, ich habe folgendes Programm
1
volatileunsignedX;
2
3
// Test A
4
unsigned__attribute__((noinline,noclone))
5
test_A(void)
6
{
7
return1000L*X/2000;
8
}
9
10
// Test B
11
unsigned__attribute__((noinline,noclone))
12
test_B(void)
13
{
14
volatileintd=2000;
15
return1000L*X/d;
16
}
17
18
#include<stdio.h>
19
20
intmain(void)
21
{
22
printf("Start\n");
23
do
24
{
25
if(test_A()!=test_B())
26
{
27
printf("X = %u\n",X);
28
}
29
}while(++X);
30
31
printf("Ready\n");
32
return0;
33
}
mit avrtest durchnudeln lassen für
* -O0
* -O1
* -O2
* -Os
für folgende Compiler
* avr-gcc 4.7.2
* avr-gcc 4.8.2
* WinAVR-20100110 (avr-gcc 4.3.3)
Nun rate mal, was das Ergebnis war! Genau: Die Ausgabe war immer
Start
Ready
[Geklimper von avrtest]
Vielen Dank Johann.
Johann L. schrieb:> Und wie zeitkritisch ist all das? Stört es denn garnicht, wenn die> Division ewig Zeit verbraucht? test_A oben brauch rund 20 Ticks, test_B> locker mehr als 500.
500 :-O - das ist zu viel! Die Division+drumrum hat gesichert
16MHz*20us=320 Takte Zeit, alles drüber kann (und ab 400 wird) zu einem
Fehler führen.
Das einzige was mich dann noch wundert ist, dass meine Debugausgabe im
Fehlerfall - zumindest immer wenn ich das überprüft hatte - 'ne saubere
Null für das Ergebnis der Division geliefert hatte. Aber das schaue ich
mir noch mal im Detail an.
Dann werde ich mich also mal an meine Hausaufgaben machen...
Mal nachgemessen für X=1000:
Selbst bei optimiertem Code (Verwendung von __umulhisi3) braucht
Variante test_B 1333 Ticks mehr als Variante test_A! Die Division
wird ja mit 32 Bits durchgeführt. Aber selbst mit 16 Bits wäre es immer
noch ziemlich langsam!
Thread beobachten
Seitenaufteilung abschalten