Codexplosion bei _delay_ms mit ADC Wert

[Michael O. (michi26206)]

Hallo,

ich möchte mit einem Attiny25 eine Spannung, welche durch ein Poti 
einstellbar ist, messen und diese dann mit der funktion _delay_ms(); als 
Wartezeit verwenden.

Funktionieren würde das ganze. Allerdings wenn ich den ADC Wert als 
Parameter an _delay_ms(); übergeben explodiert mein Code. Sprich er 
springt von ca. 250 Bytes auf ca. 4000 Bytes. Da ich aber wie oben 
geschreiben einen Attiny25 verwende hab ich jetzt ein Problem ;)

Code-Snippet:

uint16_t Pause;
Pause = 0x0000;

uint16_t ADC_messen(uint8_t channel)
{
  ADMUX = (ADMUX & ~(0x1F)) | (channel & 0x1F);
  ADCSRA = (1<<ADSC);            // messen
  while (ADCSRA & (1<<ADSC) );    // auf AVR warten
  ADC_Hilfe = (ADCL<<8);  //ADCL um 8 Bit verschieben
  ADC_Hilfe = ADC_Hilfe + ADCH;  //ADCH hinzufügen
  return ADC_Hilfe;                    // ADC Wert zurückgeben
}

Pause = ADC_messen(0x01)
_delay_ms(Pause);

Kann mir jemand sagen, ob es geht den Code zu verkleinern. Um mir dann 
auch nen Tipp geben kann wie ich das anstellen muss. Die beiden Timmer 
sind leider für PWM belegt.

Gruß

[Hc Z. (mizch)]

for (uint16_t i = Pause; i > 0; i--)
    delay_ms(1);

Grund: Aus der Dukumentation von delay.h:

In order for these functions to work as intended, compiler optimizations
 must be enabled, and the delay time must be an expression that is a known
constant at compile-time. If these requirements are not met, the resulting
 delay will be much longer (and basically unpredictable), and applications 
that otherwise do not use floating-point calculations will experience severe 
code bloat by the floating-point library routines linked into the
 application.

[X- R. (x-rocka)]

sieht doch kurz und knackig aus.
nimmt _delay_ms() auch 16 bit integer?
vielleicht selber eine delay funktion bauen mit NOPs und zähler?

Abgesehen davon:
ADC_Hilfe ist woanders deklariert?
ADCH solltest du zuerst lesen und shiften, dann ADCL addieren.

[Hc Z. (mizch)]

  ADC_Hilfe = (ADCL<<8);  //ADCL um 8 Bit verschieben
  ADC_Hilfe = ADC_Hilfe + ADCH;  //ADCH hinzufügen

sieht mir eher nach einer Vertauschung des oberen mit dem unteren Byte 
aus.   Ich glaube nicht, dass das so beabsichtigt ist.

[Roland P. (pram)]

Der GCC kann IMHO auch 16Bit-Register lesen.

Mach einfach ein "return ADC" dann kannst dir das Bitgeshifte sparen.
(ADC entspricht ADCL | ADCH << 8

Außerdem sehe ich gerade, dass du ADCL shiftest. Verwechslung oder 
Absicht?

Gruß
Roland

[Hubert G. (hubertg)]

Wenn du unbedingt ein delay verwenden musst dann schau dir mal das
util/delay_basic.h  an.
Steht in der gcc.lib.referenz.

[Detlev T. (detlevt)]

Michael O. schrieb:
> Kann mir jemand sagen, ob es geht den Code zu verkleinern. Um mir dann
> auch nen Tipp geben kann wie ich das anstellen muss.
Baue eine Schleife mit _delay_ms(1.0) drin, die Pause oft durchlaufen 
wird.

while(Pause--) _delay_ms(1.0);

Gruß, DetlevT

[Michael O. (michi26206)]

danke @ all

auf die For Schleife hätte ich auch kommen können.

Das mit ADCH und ADCL war ein Fehler von mir, da ich ncht aufgepasst 
habe.

Den im Datenbaltt steht ja:
>When ADCL is read, the ADC Data Register is not updated until ADCH is read. 
>Consequently, if
>the result is left adjusted and no more than 8-bit precision is required,
>it is sufficient to read
>ADCH. Otherwise, ADCL must be read first, then ADCH.

Allerdings hab ich nicht auf die Bitwertigkeit geschaut.

ADC_Hilfe war auch deklariert hab ich nur im Snippet vergessen.

Gruß

[Max (Gast)]

Ich möchte noch auf richtige Datentypen hinweisen (den Code selber habe 
ich nicht nachgeschaut). Man MUSS dir Typen für Konstanten auch als 
unit16 machen mit dem Suffix "U". Nach ANSI ist jede Konstante immer 
"signed int"-Datatype und ohnen Suffix wir schreiben einen 
signed-Operand in die unsigned-Variable, was datentypmismatch bedeutet. 
Weiter, "bitwise and shift operations" nur für "unsigned" definiert 
sind. Und leztens, "Integer promotion" Regel achten, d.h. nach ANSI ist 
immer jede Opration auf mind. Integer-Breite durchgeführt und klenere 
Typen werden auf "signed int" im dem Fall hochcastet. Beim AVR kann zum 
Abweichung kommen, aber ANSI spricht klar und MISRA schreibt das vor.

uint16_t Pause;
Pause = 0x0000U;

uint16_t ADC_messen(uint8_t channel)
{
  ADMUX = (ADMUX & ~(0x1FU)) | (channel & 0x1FU);
  ADCSRA = (1U<<ADSC);            // messen
  while (ADCSRA & (1U<<ADSC) );    // auf AVR warten
  ADC_Hilfe = ((uint16_t)ADCL)<<8);  //ADCL um 8 Bit verschieben
  ADC_Hilfe = (uint16_t)ADC_Hilfe + (uint16_t)ADCH;  //ADCH hinzufügen
  return ADC_Hilfe;                    // ADC Wert zurückgeben
}

Pause = ADC_messen(0x01U)
_delay_ms(Pause);

[Michael O. (michi26206)]

Ich stell gerade fest, dass die ADC Messung gar nicht funktioniert. 
Gemessen werden soll mit ADC1 bzw. PB2

Code-Snippet:

uint16_t Pause;
Pause = 0x0000;

ADMUX = (1<<ADLAR);  //"runden"
ADCSRA = (1<<ADATE);
ADCSRA = (1<<ADEN);  //ADC ein

uint16_t ADC_messen(uint8_t channel)
{
  ADMUX = (ADMUX & ~(0x1F)) | (channel & 0x1F);
  ADCSRA = (1<<ADSC);            // messen
  while (ADCSRA & (1<<ADSC) );    // auf AVR warten
  return ADCH;                    // ADC Wert zurückgeben
}

Pause = ADC_messen(1);

Hat jemand nen Fehler entdeckt?

Gruß

[Stefan E. (sternst)]

Michael O. schrieb:
> Hat jemand nen Fehler entdeckt?

ADCSRA = (1<<ADATE);
ADCSRA = (1<<ADEN);  //ADC ein

Die zweite Zeile setzt ADATE wieder auf Null (aber warum setzt du ADATE 
überhaupt?).

  ADCSRA = (1<<ADSC);            // messen

Und beim Starten der Messung den ADC gleichzeitig abzuschalten ist 
sicher auch keine gute Idee.

[Michael O. (michi26206)]

ups....

da hat wohl wern nen | vergessen

danke dir

gruß

[Simon K. (simon)]

Michael O. schrieb:
>

>   return ADCH;                    // ADC Wert zurückgeben
>

->

return ADC;

[Hc Z. (mizch)]

> Nach ANSI ist jede Konstante immer
> "signed int"-Datatype und ohnen Suffix wir schreiben einen
> signed-Operand in die unsigned-Variable, was datentypmismatch bedeutet.

In ANSI C gibt es kein Ding namens "datentypenmismatch", sondern 
"promotion rules".  Es ist also vielmehr genau definiert, zu welchem 
Typen erweitert wird, wenn 2 verschiedene Typen aufeinander treffen.

> Weiter, "bitwise and shift operations" nur für "unsigned" definiert
> sind.

Nein, auch für signed Integers ist Shift definiert, solange sie nicht 
negativ sind:

4. The result of E1 << E2 is E1 left-shifted E2 bit positions: vacated
bits are filled with zeros. ... If E1 has a signed type and a
nonnegative value, and E1x2^32 is representable in the result type,
then that is the resulting value; otherwise, the behavior is undefined.

> aber ANSI spricht klar und MISRA schreibt das vor.

Ich weiß nicht, was MISRA schreibt, aber ANSI sagt für den gezeigten 
Code klar aus, dass die Korrektur überflüssige Mühe ist.

[Roland P. (pram)]

lies dir mal das Datenblatt zum ADC durch. Ich glaube mich zu erinnern, 
dass man beim Umschalten von ADMUX den ersten Wert unter bestimmten 
Umständen verwerfen musss, weil er falsch ist.

Gruß
Roland

[Max (Gast)]

4. The result of E1 << E2 is E1 left-shifted E2 bit positions: vacated
bits are filled with zeros. ... If E1 has a signed type and a
nonnegative value, and E1x2^32 is representable in the result type,
then that is the resulting value; otherwise, the behavior is undefined.

Das ist aber klare Problembeschreibung! Wenn ich 0xFF (also signed) 
8-mal nach links shifte, E1x2^16 (ich habe 16-Bit integer, deswegen nur 
16), E1 nicht mehr "representable" ist. 0xff00 (signed) bedeute -32512 
anstatt 65280 unsigned. Die ASNI-Zeile oben beschreibt nur die Werte, 
die maximal 0x7f haben. Für die Werte 0x80 bis 0xff ist "the behavior is 
undefined". Wer sicherheitsrelevente Systeme programiert, kennt diese 
Problematik und verwendet explizit Casting.

[Jörg W. (dl8dtl) (Moderator)]

Du hast mit deiner Erklärung Recht, aber das "2x2^32" bzw. "2x2^16"
da im Text ist falsch: im Standard steht "2 x 2^E2".  E2 ist hier
gleich 8.  Ändert nur nichts dran, dass 0xff00 nicht als signed int
darstellbar ist.

Allerdings trifft das auf das ADC-Ergebnis nicht zu: die ganzen
IO-Register sind vom Typ uint8_t, damit wird der Teil "If E1 has
an unsigned type, [...]" wirksam, für den das Ergebnis definiert
ist.

[DERLEVELER (Gast)]

ADC_Hilfe = ADC_Hilfe + ADCH;  //ADCH hinzufügen

würde ich lieber so schreiben:

ADC_Hilfe += ADCH;  //ADCH hinzufügen

[Simon K. (simon)]

DERLEVELER schrieb:
>

ADC_Hilfe = ADC_Hilfe + ADCH;  //ADCH hinzufügen
>

>
> würde ich lieber so schreiben:
>

ADC_Hilfe += ADCH;  //ADCH hinzufügen

Nein, ADCH und ADCL sollte man nicht direkt auslesen, sondern ADC 
benutzen, damit diese in der richtigen Reihenfolge ausgelesen werden.
Einfach mal lesen, was schon geschrieben wurde, kann nicht schaden!

[Falk B. (falk)]

@  Simon K. (simon) Benutzerseite

>Nein, ADCH und ADCL sollte man nicht direkt auslesen, sondern ADC
>benutzen, damit diese in der richtigen Reihenfolge ausgelesen werden.

Ich würde lieber ADCW nutzen ;-)

MFG
Falk

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

Falk Brunner schrieb:

> Ich würde lieber ADCW nutzen ;-)

Kein Unterschied.

[Falk B. (falk)]

@  A. K. (prx)

>> Ich würde lieber ADCW nutzen ;-)

>Kein Unterschied.

Hmm, stimmt, im C. Aber im ASM geht das nicht, weil ADC ein Befehl ist. 
Ist auch so mit Fallunterscheidung in den Include Files drin ;-)

MfG
Falk

[Jörg W. (dl8dtl) (Moderator)]

Falk Brunner schrieb:
> Aber im ASM geht das nicht, weil ADC ein Befehl ist.

Könnte schon, da der C-Präprozessor ja nur ADCW erseten würde,
während adcw immer noch für den Befehl benutzbar ist.

Aber: wer sich für Assembler entschieden hat, kann sowieso keine
16-bit-IO-Register automatisch lesen oder schreiben lassen. ;-)