Bug im AVR Embedded bei ISR - wo melden?

[Timm Thaler]

Ich hab jetzt Stunden zugebracht, um ein "komisches" Verhalten im AVR Embedded zu analysieren. Kurz:

Beim Aufruf einer ISR wird R1 nicht mit gesichert. R1 wird als Null angenommen und damit gerechnet. Ist R1 nicht Null, gibt es Rechenfehler.

Der Code

Code: Alles auswählen

procedure timer0_interrupt; alias: 'TIMER0_COMPA_ISR'; interrupt; public;
begin
  inc(sseccnt);
  if sseccnt >= 25000 then begin  // 1 Sekunde
    sseccnt := 0;

wird umgesetzt zu

Code: Alles auswählen

TIMER0_COMPA_ISR:
	push	r26
	push	r20
	push	r19
	push	r18
	push	r0
	in	r0,63
	push	r0
# [25] inc(sseccnt);
	lds	r20,(U_sEM_INT_ss_SSECCNT)
	lds	r19,(U_sEM_INT_ss_SSECCNT+1)
	ldi	r18,1
	add	r20,r18
	adc	r19,r1 <= hier wird mit r1 = 0 gerechnet
	sts	(U_sEM_INT_ss_SSECCNT),r20
	sts	(U_sEM_INT_ss_SSECCNT+1),r19

R1 ist aber nicht zwingend Null, denn eine Multiplikation mul oder muls nutzt r1:r0 als Rückgaberegister.

Eine Multiplikation irgendwo im Programm gibt

Code: Alles auswählen

# [50] mean := (15 * mean + vval) div 16;
	lds	r18,(U_sEM_DEFINE_ss_MEAN)
	lds	r19,(U_sEM_DEFINE_ss_MEAN+1)
	ldi	r22,15
	mov	r21,r1
	movw	r2,r18
	mul	r2,r22
	movw	r18,r0 <= ab hier enthalten R1:R0 die Ergebnisse von mul
	mul	r3,r22
	add	r19,r0
	mul	r2,r21
	add	r19,r0
	clr	r1 <= hier erst wird R1 wieder sicher Null
	mov	r2,r19
	mov	r21,r1
 

Schlägt der Interrupt in einer Multiplikation zu, ist R1 nicht sicher Null.

[kupferstecher]

wo melden?

Im Bugtracker?
bugs.freepascal.org

Mir ist auch schon aufgefallen, dass r1 als 'Nullregister' verwendet wird. In ASM hab ich immer r10 fuer die Null und r11 fuer die Eins genommen.

Bin gespannt wie schnell der Bug gefixt werden kann.

[Timm Thaler]

Habs gefunden und gemeldet https://bugs.freepascal.org/view.php?id=33165

Hoffe da geht zeitnah jemand ran. So ist das erstmal nicht verwendbar.

[siro]

Hallo Timm Thaler,

so wie ich den Assembler Code verstehe stimme ich Dir voll zu, das kann wohl nicht funktionieren.
Ich habe aber eben merkwürdige Dinge gelesen:

"I noticed that a "readonly-r1" option wasn't compatible with use of MUL."

http://www.avrfreaks.net/forum/why-r1-s ... ro-avr-gcc
http://www.avrfreaks.net/forum/gcc-nonusage-r0

Siro

[Timm Thaler]

Mir ist auch schon aufgefallen, dass r1 als 'Nullregister' verwendet wird. In ASM hab ich immer r10 fuer die Null und r11 fuer die Eins genommen.

Ich hab auch schon öfter gedacht: Eigentlich eine blöde Idee, R2 wäre besser. Macht der C-Compiler aber auch nicht besser, der verwendet afaik R0, welches auch noch für lpm verwendet wird.

Nur fällt der Bug kaum auf, weil er wirklich nur auftritt, wenn man aus einer Hardware-Multiplikation in eine ISR springt und in dieser irgendwo Null verwendet wird. (Addition mit Übertrag, Variable rücksetzen). Genau diese Konstellation habe ich nun: Eine 25.000mal pro Sekunde aufgerufene ISR und im Hauptprogramm ein Mittelwertfilter, welcher seeehr viele Multiplikationen macht.

Fieserweise wurde der Fehler erst merklich, als ich eine stinknormale for-loop eingefügt habe. Selbst mit leerem Rumpf brachte die meinen Timer durcheinander. Ich hab stundenlang nach einem Fehler in der for-loop gesucht, bis ich gemerkt habe, dass sie einfach nur das Timing im Hauptprogramm so verschiebt, dass sich ISR und Multiplikation öfter treffen und das schon vorhandene Problem offensichtlich wurde.

[Timm Thaler]

Workaround bis der Bug gefixed ist:

Code: Alles auswählen

procedure timer0_interrupt; alias: 'TIMER0_COMPA_ISR'; interrupt; public;
begin
  asm
    clr r1
  end['r1'];  // Workaround für R1 Bug
  inc(sseccnt);
  if sseccnt >= 25000 then begin  // 1 Sekunde
    sseccnt := 0;
 

Ein clr r1 am Anfang jeder ISR zwingt den Compiler R1 zu pushen, in der ISR verwendet der Compiler dann R1 ganz normal als Null, und poppt R1 am Ende auch wieder brav. Schließlich wollen wir das Ergebnis unserer Multiplikation in der Hauptroutine auch wiederhaben.

[Timm Thaler]

Im Bugreport wird jetzt behauptet, das wäre schon länger gefixed.

Ich habe hier Lazarus 1.9.0 von 2018-02-04 mit FPC 3.3.1 SVN Revision 57250. Das ist das was mir fpupdeluxe als trunk anbietet. Ist das die neueste Version?

[FPK]

3.1.1 ist trunk, dass Du 3.3.1 hast, ist unwahrscheinlich, das gibt es nämlich noch nicht

Die Lazarus-SVN-Version hat nichts mit der FPC SVN-Version zu tun.

[Timm Thaler]

Ja Schreibfehler.

Ich scheitere gerade daran, die FPC Revision rauszubekommen. Warum wird die in Lazarus nicht gleich mit angezeigt?

Allerdings wird sie in die Assembler-Sources mit gepackt, und da steht jetzt endlich auch "FPC 3.1.1-r38235 [2018/02/13] for avr - embedded".

Dummerweise ist der Bug jetzt - anders: clr r1 erfolgt vor Sichern des Statusregisters - und verändert dieses. Das kann ich auch durch Inline-Assembler nicht workarounden. Wird hoffentlich zeitnah angepasst.

[kupferstecher]

Dummerweise ist der Bug jetzt - anders: clr r1 erfolgt vor Sichern des Statusregisters - und verändert dieses.

Irgendwie wird durch ein veränderliches Register der Sinn des Ganzen torpediert. Eigentlich müsste der Kompiler doch dann das Register in jeder Funktion wo null verwendet wird erneut nullen und vorher pushen. D.h. nicht nur in der ISR, da der Kompiler vermutlich nicht alle Pfade inklusive Funktionen durchlaufen/simulieren wird, um zu schauen, wo welches Register verändert wird. Bzw. kann er oft gar nicht, da Objektdateien, also kompilierte Units, auch unabhängig vom Gesamtwerk funktionieren müssen.

Dann kann man für den Wert 0 widerum gleich ein beliebiges Register wählen und an Ort und Stelle nullen.

Oder?

[Timm Thaler]

Naja, die Idee dahinter ist, dass man ein Register hat, welches man explizit nur dafür verwendet mit Null zu rechnen. Und nicht anders benutzt.

Prinzipiell ist das schon eine brauchbare Idee, schließlich hat der AVR genug Register. Und ldi geht nicht mit allen Registern, add+adc geht nicht mit Konstanten, Vergleiche gehen nur bedingt mit Konstanten. Somit vereinfacht so eine vordefinierte "Null" schon den Code.

Dummerweise hat man sich halt für R1 entschieden, und genau dieses (und R0) verwendet der AVR intern für die Rückgabe einer Hardware-Multiplikation mit mul, muls... So wie früher R0 immer für die Rückgabe eines mit lpm gelesenen Flash-Wertes war (heute geht auch jedes andere Register).

Beim GCC-Compiler fiel die Entscheidung für R1 anscheinend, bevor Atmel mul einführte. Bei FPC hätte man aber auch R2 nehmen können - und hätte diese Probleme nicht.

Auf jeden Fall ist die ISR jetzt erstmal ganz kaputt. Durch das clr VOR dem Sichern des Statusregisters bleibt der Controller ziemlich zuverlässig hängen.

[kupferstecher]

Ich wuerde den Bug-Report schliessen und einen neuen aufmachen. Auf den alten kannst du ja als Ursache verweisen.

Das CLR in der ISR halte ich aber auch fuer Murks. Weil gerade eine ISR ist ja oft zeitkritisch und dann das CLR auch dann aufzurufen, wenn R1 gar nicht verwendet wird... Klar, als ersten Schritt, dass der Kompiler wieder laeuft, passt es schon.

Wie gesagt, ich sehe keinen Sinn in einem "Nullregister", wenn das Register nicht immer auf 0 bleibt. Dann besser das "Feature" abschalten.

Beim GCC-Compiler fiel die Entscheidung für R1 anscheinend, bevor Atmel mul einführte. Bei FPC hätte man aber auch R2 nehmen können - und hätte diese Probleme nicht.

Beim FPC fuer AVR ist Rueckwertskompatibilitaet vermutlich noch nicht so wichtig...

[Timm Thaler]

Naja, das Problem ist momentan, dass der Compiler die ISR jetzt kaputtmacht. Durch das clr wird das SREG zerstört.

[FPK]

Beim GCC-Compiler fiel die Entscheidung für R1 anscheinend, bevor Atmel mul einführte. Bei FPC hätte man aber auch R2 nehmen können - und hätte diese Probleme nicht.

Beim FPC fuer AVR ist Rueckwertskompatibilitaet vermutlich noch nicht so wichtig...

Die Kompatibilität zu gcc ist wichtig, es gibt sicherlich genügend Leute, die mit gcc erzeugte Libraries/Object files linken wollen und dann macht das Erfinden eigener Konventionen nur Probleme.

[Timm Thaler]

Naja, das Problem ist momentan, dass der Compiler die ISR jetzt kaputtmacht. Durch das clr wird das SREG zerstört.

Gefixed in FPC 3.1.1-r38241 [2018/02/14] for avr - embedded.