uint32_t division atmega328p

[Christian G. (Firma: GDV-IT) (gdv-it)]

Hallo Forum,

ich habe hier ein kleines Verständnisproblem zur 32-Bit Division.
Ab und zu, in vieleicht 5% der Fälle, kommt es zu fehlerhaften 
Berechnungen wenn ich 2 UINT32_T Zahlen miteinander dividiere. Hier mein 
Auszug aus Code und Log, vieleicht fällt jemanden was dazu ein. 
Dankeschön.

CODE:

declaration:

// ADC
static uint32_t AdcSamples[] = {0,0,0,0,0,0};
static uint32_t AdcSum[] = {0,0,0,0,0,0};
static bool CalculationInProgress = false;

interrupt:

ISR(TIMER0_OVF_vect) {
        // dont count on calculation
        if(!CalculationInProgress) {
                // read ADC for each sensor input
                // and sumarize ADC values
                // PINCx = ADCx
                for(int t = 0; t < 6; t++) {
                        if(SwitchLayout[t] == 3) {
                                AdcSamples[t]++;
                                AdcSum[t] = AdcSum[t] + (uint32_t) ReadADC(t);
                        }
                }
        }
}

main:

                        // every minute (64x60)
                        if (AdcSamples[t] >= (64*60)) {

                                CalculationInProgress = true;

                                uint32_t roundedVal = AdcSum[t] / AdcSamples[t];
                                sendGenericUint32Message(9634, AdcSum[t]);
                                sendGenericUint32Message(9635, AdcSamples[t]);
                                sendGenericUint32Message(9636, roundedVal);

                                AdcSamples[t] = 0;
                                AdcSum[t] = 0;

                                CalculationInProgress = false;

                      }

ERKLÄRUNG:
Ein Timer führt alle paar millisekunden die ISR durch und summiert Werte 
u.
Anzahl Zählungen um am ende einen Mittelwert bilden zu können.

RICHTIG:

9634 with len 4  0x00 0x19 0x8f 0x02 
9635 with len 4  0x00 0x00 0x0f 0x00 
9636 with len 4  0x00 0x00 0x01 0xb4 

1675010 / 3840 = 436,2005 => 436

FALSCH:

9634 with len 4  0x00 0x19 0x90 0x4c 
9635 with len 4  0x00 0x00 0x0f 0x00 
9636 with len 4  0x00 0x00 0x01 0x99 

1675340 / 3840 = 436,2864 => 436
aber berechnet wird 409

[Franz (Gast)]

"CalculationInProgress" sollte "volatile" sein.

[Walter T. (nicolas)]

Christian G. schrieb:
> Hier mein
> Auszug aus Code und Log, vieleicht fällt jemanden was dazu ein.

Wenn bei einer Division ein unerwartetes Ergebnis herauskommt, ist es in 
90% der Fälle so, dass nicht die Zahlen durcheinander verrechnet werden, 
von denen man es glaubt. In den restlichen 10% ist das Ergebnis, das man 
sich ansieht, nicht das Ergebnis der genannten Operation.

Sprich: Guck, ob wirklich im Array in dem Moment das drin steht, was 
erwartet wird.

Franz schrieb:
> "CalculationInProgress" sollte "volatile" sein.

Und mindestens AdcSamples[] oder AdcSum[], ansonsten finden die 
Operationen auf den Arrays nicht unbedingt zwischen den beiden 
Operationen auf das bool statt.

Ansonsten ist der Code äquivalent zu:

                                CalculationInProgress = true;
                                CalculationInProgress = false;
                                uint32_t roundedVal = AdcSum[t] / AdcSamples[t];
                                sendGenericUint32Message(9634, AdcSum[t]);
                                sendGenericUint32Message(9635, AdcSamples[t]);
                                sendGenericUint32Message(9636, roundedVal);
                                AdcSamples[t] = 0;
                                AdcSum[t] = 0;

[mitlesa (Gast)]

Christian G. schrieb:
> AdcSum[t] = AdcSum[t] + (uint32_t) ReadADC(t);

ReadADC () ist hier nicht zu sehen, könnte bisschen länger
dauern als das was man in der ISR tun sollte.

[Christian G. (Firma: GDV-IT) (gdv-it)]

War mir garnicht bewusst, ich werde es mal so umsetzen und beobachten.

@mitlesa : ich war davon ausgegangen,dass es keine Rolle spielt, da die 
Zahlen für die Division ja die "richtigen" sind

uint16_t ReadADC(uint8_t ADCchannel) {
  //select ADC channel with safety mask
  ADMUX = (ADMUX & 0xF0) | (ADCchannel & 0x0F);
  //single conversion mode
  ADCSRA |= (1<<ADSC);
  // wait until ADC conversion is complete
  while( ADCSRA & (1<<ADSC) );
  return ADC;
}

und natürlich werde ich das auslagern in die main :)

[QQ (Gast)]

mitlesa schrieb:
> Christian G. schrieb:
>> AdcSum[t] = AdcSum[t] + (uint32_t) ReadADC(t);
>
> ReadADC () ist hier nicht zu sehen, könnte bisschen länger
> dauern als das was man in der ISR tun sollte.

vor Allem, da das ja 6x ausgeführt wird.

Christian G. schrieb:
> while( ADCSRA & (1<<ADSC) );

Funktionsaufrufe können lange dauern, vor Allem, wenn viel auf den Stack 
muss. Außerdem sind solche Schleifen im Interrupt ein no-go.

Schaue dir mal den "ADC conversion complete" interrupt an!

Verkleinere mal die Datentypen, wo es möglich ist. Ist uint32_t wirklich 
nötig???

Außerdem sorgt "CalculationInProgress" dafür, dass sicherlich nicht 
jeder timer interrupt den ADC bedient. Gegebenenfalls kann das auch zu 
Problemen führen.

[Christian G. (Firma: GDV-IT) (gdv-it)]

Vielen Dank für eure Mühe,

CalculationInProgress hatte ich nur testhalber eingebaut um genau das zu 
erreichen was volatile scheinbar macht. Dass die ISR nicht in den Werten 
herumschreibt. Bis jetzt scheint es zu laufen, mir waren die Eigenheiten 
von volatile in C nicht bekannt. Ich und C werden eh keine Freunde mehr 
;)

uint32_t ist leider aufgrund der mess- und kalkulationsgenauigkeit 
nötig.

Ich beobachte mal obs noch ausreißer gibt, bis dahin hab ich erstmal 
viel anderen Code zu fixen ...

[Christian G. (Firma: GDV-IT) (gdv-it)]

kurze Rückmeldung, keine Fehler mehr, DANKESCHÖN!

[(prx) A. K. (prx)]

"volatile" ist keine Memory Barrier. Die Zugriffe auf dieses Flag sind 
per Sprachdefinition zwar in sich geordnet, müssen das aber nicht 
gegenüber den Zugriffen auf die Arrays sein. Auch wenn das nun 
wahrscheinlich so rauskommt.

https://www.nongnu.org/avr-libc/user-manual/group__avr__cpufunc.html