Unexpected float behaviour in C with AVR atmega8

Question

I've been trying to figure out why I cannot get a sensible value from multiplying an unsigned int with a float value.

Doing something like 65535*0.1 works as expected but multiplying a float with a uint from memory creates mad values. I have a function that reads an ADC and returns an uin16_t. With this value I am printing it to a 4-digit led-display, which is working fine.
Multiplying the same value with 1.0 returns something different entirely (it's too large for my display so I don't really know what it is).

My code is below but the area of contention is at the bottom in main(). Any help would be great. Thanks

main.c:

#include <avr/io.h>
#include <util/delay.h>
#include <avr/interrupt.h>
#include <stdint.h>
#define BAUD 9600
#include <util/setbaud.h>
#define DISP_BRIGHT_CMD     'z'
#define DISP_RESET          'v'

#define ADC_AVG            3 

volatile uint8_t  hi,lo;
volatile uint16_t result; 

ISR(ADC_vect)
{ 
    lo = ADCL; 
    hi = ADCH; 
    MCUCR &= ~_BV(SE); //Clear enable sleep 
} 


void initSerial(void)
{
    // set baud rate
    UBRR0H = UBRRH_VALUE;
    UBRR0L = UBRRL_VALUE;
    // set frame format
    UCSR0C |= (0x3 << UCSZ00); // 8n1
    // set enable tx/rx
    UCSR0B = _BV(RXEN0) | _BV(TXEN0);
}

void initADC(void)
{
    // AVCC and ADC0
    ADMUX   = _BV(REFS0); 
    // Enable, div128, + 1st setup
    ADCSRA  |= _BV(ADEN)|_BV(ADSC)|_BV(ADPS2)|_BV(ADPS1)|_BV(ADPS0)|_BV(ADIE);
}

uint16_t readADC(void)
{
    uint16_t average=0;
    // Start Conversion
    ADCSRA |= _BV(ADSC);

    for (char i=0;i<ADC_AVG;i++) {
        MCUCR   |= _BV(SE);
        ADCSRA  |= _BV(ADSC);
        __asm volatile("sleep");
        MCUCR   &= ~_BV(SE);
        result  = (hi<<8);
        result  |= lo;
        average += result;
    }
    average /= ADC_AVG;
    return average;    
}

void sendByte(char val)
{
    while (! (UCSR0A & (1<<UDRE0)) ); //wait until tx is complete
    UDR0 = val;
}

/*
 * Convert voltage to temperature based on a negative coefficient for MAX6613
 */
uint16_t analogToTemp(uint16_t val)
{
  uint16_t temp;
  //v     = 5 * (val/1023.0);
  //temp  = (1.8455 - (5.0*(val/1023.0)))/0.01123;
  temp  = (1.8455 - (5.0*(val/1023.0)))*89;
  //temp = val * M_PI;
  //v     = 5 * ( val/1024);
  //temp  = (2 - v) * 89;

  return temp;
}

void initDisplay()
{
    sendByte(DISP_RESET);
    sendByte(DISP_BRIGHT_CMD);
    sendByte(0);
}

void serialSegments(uint16_t val) 
{  
  // 4 digit display
  sendByte(val / 1000);
  sendByte((val / 100) % 10);
  sendByte((val / 10) % 10);
  sendByte(val % 10);  
}

int main(void)
{
    uint16_t calc=0,sense=0;

    DDRB    |= _BV(DDB5);
    PORTB   |= _BV(PORTB5);
    initSerial();
    initADC();
    initDisplay();
    sei();
    MCUCR   |= (1 << SM0); // Setting sleep mode to "ADC Noise Reduction" 
    MCUCR   |= (1 << SE);  // Sleep enable 
    for(;;) {
        //PORTB   ^= _BV(PORTB5);
        if (calc>=9999){ // I can't see the real value. Max val on display is 9999
        //if (sense>=330){
            PORTB |= _BV(PORTB5);
        } else {
            PORTB &= ~_BV(PORTB5);
        }

        sense   = readADC();
        //calc    = sense*1.0;      // refuses to calculate properly
    calc    = analogToTemp(sense);  // a bunch of zeroes
        //calc = 65535*0.1;         // a-ok

        serialSegments(calc);
        _delay_ms(500);
        serialSegments(sense);
        _delay_ms(500);
    }
    return 0;
}

Makefile:

# AVR-GCC Makefile
PROJECT=Temp_Display
SOURCES=main.c
CC=avr-gcc
OBJCOPY=avr-objcopy
MMCU=atmega328p
OSC_HZ=16000000UL
OPTIMISATION=2
PORT=/dev/ttyUSB0

CFLAGS=-mmcu=${MMCU} -std=gnu99 -Wall -O${OPTIMISATION}  -DF_CPU=${OSC_HZ} -lm -lc 

${PROJECT}.hex: ${PROJECT}.out
    ${OBJCOPY} -j .text -O ihex ${PROJECT}.out ${PROJECT}.hex
    avr-size ${PROJECT}.out

$(PROJECT).out: $(SOURCES)
    ${CC} ${CFLAGS} -I./ -o ${PROJECT}.out ${SOURCES}

program: ${PROJECT}.hex
    stty -F ${PORT} hupcl
    avrdude -V -F -c arduino -p m168 -b 57600 -P ${PORT} -U flash:w:${PROJECT}.hex

clean:
    rm -f ${PROJECT}.out
    rm -f ${PROJECT}.hex

EDIT: OK, i've simplified the code somewhat

#include <avr/io.h>
#include <util/delay.h>
#include <stdint.h>
#define BAUD 9600
#include <util/setbaud.h>
#define DISP_BRIGHT_CMD     'z'
#define DISP_RESET          'v'


void initSerial(void)
{
    // set baud rate
    UBRR0H = UBRRH_VALUE;
    UBRR0L = UBRRL_VALUE;
    // set frame format
    UCSR0C |= (0x3 << UCSZ00); // 8n1
    // set enable tx/rx
    UCSR0B = _BV(TXEN0);
}


void sendByte(char val)
{
    while (! (UCSR0A & (1<<UDRE0)) ); //wait until tx is complete
    UDR0 = val;
}


void initDisplay()
{
    sendByte(DISP_RESET);
    sendByte(DISP_BRIGHT_CMD);
    sendByte(0);
}

void serialSegments(uint16_t val) {  
  // 4 digit display
  sendByte(val / 1000);
  sendByte((val / 100) % 10);
  sendByte((val / 10) % 10);
  sendByte(val % 10);  
}

int main(void)
{
    uint16_t i=0,val;

    DDRB    |= _BV(DDB5);
    initSerial();
    initDisplay();
    for(;;) {
        val = (uint16_t)(i++ * 1.5);
        serialSegments(i);
        _delay_ms(500);
        serialSegments(val);
        _delay_ms(500);
        if (val > 9999){
            PORTB |= _BV(PORTB5);
        } else {
            PORTB &= ~_BV(PORTB5);
        }
    }
    return 0;
}

Solution 1

Unsuffixed floating point constant are of type double not float.

Use f suffix to have a float literal, e.g., 0.1f

This can make a huge overhead as MCUs like atmega8 don't have a floating point unit and all floating point operations have to be implemented in firmware by the implementation.

With small devices like atmega8 one usually try to avoid using float operations as without a FPU they are very expensive in CPU cycles.

Now there is no reason an implementation would no correctly translate an expression like:

calc = sense * 1.0;

when calc and sense are of uint16_t type.

Solution 2

Not exactly your code, maybe close enough, maybe not.

First off when I display the output and compare these to lines:

val = (unsigned int)(i++ * 1.5);
...
val = i+(i>>1); i++;

the result is the same. Disassembling shows a few things as well. First off avr-gcc

avr-gcc --version
avr-gcc (GCC) 4.3.4
Copyright (C) 2008 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

uses floats not doubles, so the comments about 1.5F vs 1.5 are quite valid in general but not relevant here. Second it is producing floating point values, single precision, and is doing floating point math, the compiler did not take a shortcut there, it converts to float, does the multiply then converts back.

Using my hex display routine and your decimal display routine (modified to output on a serial terminal), here again it produces the same output, the floating point math does not appear to be the problem.

I started this task to see if the floating point performance was a killer, and it is, but the timing changes depending on how I test it. it took as much as 157 times longer for the code with the floating point compared to fixed point. If I leave in the call to serialSegments(), but have that call a dummy routine instead of the serial port it is 3 times slower for the float. Also I built two different ways and pulled in a libc/m which used a different set of floating point routines, the floating point routines chosen by the C compiler were 7 times slower than the libc/libm.a sitting in the /usr/lib64/avr/lib/ directory. Once you add the waiting on serial port and other delays you may not notice the difference in timing so this experiment while demonstrating that the float is quite painful, is probably not the smoking gun even if your code is timing sensitive, we are talking a few milliseconds.

In addition to the code below I tried this as well:

for(i=0;i<9999;i++) { vala = (unsigned int)(i * 1.5); valb = i+(i>>1); i++; if(vala!=valb) { hexstring16(i); hexstring16(vala); hexstring16(valb); } }

No fail. I limited to 9999 because serialSegments() only chops up decimals from 0 to 9999. Now your loop goes beyond that to 65535, but you would see that cause problems without the float though, right?

avr.c

#define UCSRA (*((volatile unsigned char *)(0xC0)))
#define UDR   (*((volatile unsigned char *)(0xC6)))
#define TCCR0A  (*((volatile unsigned char *)(0x44)))
#define TCCR0B  (*((volatile unsigned char *)(0x45)))
#define TCNT0   (*((volatile unsigned char *)(0x46)))

#define TCCR1A  (*((volatile unsigned char *)(0x80)))
#define TCCR1B  (*((volatile unsigned char *)(0x81)))
#define TCNT1L  (*((volatile unsigned char *)(0x84)))
#define TCNT1H  (*((volatile unsigned char *)(0x85)))

void dummy ( unsigned int );
void uart_putc ( unsigned char c )
{
    while(1) if(UCSRA&0x20) break;
    UDR=c;
}
void hexstring16 ( unsigned int d )
{
    unsigned int rb;
    unsigned int rc;

    rb=16;
    while(1)
    {
        rb-=4;
        rc=(d>>rb)&0xF;
        if(rc>9) rc+=0x37; else rc+=0x30;
        uart_putc(rc);
        if(rb==0) break;
    }
    uart_putc(0x0D);
    uart_putc(0x0A);
}

#ifdef SEGMENTS

void sendByte(char val)
{
    uart_putc(0x30+val);
}


void serialSegments(unsigned int val) {
  // 4 digit display
  dummy(val / 1000);
  dummy((val / 100) % 10);
  dummy((val / 10) % 10);
  dummy(val % 10);
}

//void serialSegments(unsigned int val) {
  //// 4 digit display
  //sendByte(val / 1000);
  //sendByte((val / 100) % 10);
  //sendByte((val / 10) % 10);
  //sendByte(val % 10);
  //uart_putc(0x0D);
  //uart_putc(0x0A);
//}



#else

void serialSegments(unsigned int val)
{
    dummy(val);
}

//void serialSegments(unsigned int val)
//{
    //hexstring(val);
//}


#endif

int main(void)
{
    unsigned int i,val;
    volatile unsigned int xal,xbl,xcl;
    volatile unsigned int xah,xbh,xch;

    hexstring16(0x1234);

    TCCR1A = 0x00;
    TCCR1B = 0x05;

    xal=TCNT1L;
    xah=TCNT1H;
    for(i=0;i<9999;)
    {
        val = (unsigned int)(i++ * 1.5);
        //serialSegments(val);
        //hexstring16(val);
        dummy(val);
    }
    xbl=TCNT1L;
    xbh=TCNT1H;
    for(i=0;i<9999;)
    {
        val = i+(i>>1); i++;
        //serialSegments(val);
        //hexstring16(val);
        dummy(val);
    }
    xcl=TCNT1L;
    xch=TCNT1H;
    xal|=xah<<8;
    xbl|=xbh<<8;
    xcl|=xch<<8;
    hexstring16(xal);
    hexstring16(xbl);
    hexstring16(xcl);
    hexstring16(xbl-xal);
    hexstring16(xcl-xbl);
    return 0;
}

dummy.s

.globl dummy
dummy:
    ret

vectors.s

.globl _start
_start:
    rjmp reset


reset:
    rcall main
1:
    rjmp 1b

.globl dummy
dummy:
    ret

Makefile

all : avrone.hex avrtwo.hex

avrone.hex : avr.c dummy.s
    avr-as dummy.s -o dummy.o
    avr-gcc avr.c dummy.o -o avrone.elf -mmcu=atmega328p -std=gnu99 -Wall -O2 -DSEGMENTS
    avr-objdump -D avrone.elf > avrone.list 
    avr-objcopy avrone.elf -O ihex avrone.hex

a    vrtwo.hex : avr.c vectors.s
    avr-as vectors.s -o vectors.o
    avr-as dummy.s -o dummy.o
    avr-gcc -c avr.c -o avrtwo.o -mmcu=atmega328p -std=gnu99 -Wall -O2 -nostartfiles 
    avr-ld vectors.o avrtwo.o -o avrtwo.elf  libc.a libm.a 
    avr-objdump -D avrtwo.elf > avrtwo.list 
    avr-objcopy avrtwo.elf -O ihex avrtwo.hex



clean :
    rm -f *.hex
    rm -f *.elf

This was all run on an arduino pro mini (atmega328p).

Comments

[regomodo]

So it's a case of scaling up the numbers to remove the fractions then and do integer maths? I swear i've seen people do float-ops in atmega8s. So the reason I can do a simple 65535*0.1 and not (uint16_t)val*1.0 is because the former was actually done in compilation?

[regomodo]

calc = sense * 1.0 is exactly why i'm getting quite perplexed here. I've tried it on 2 different compilers created via differing means (Ubuntu and my own script) on differing machines.

[ouah]

65535 * 0.1 will be done at compilation time but not (uint16_t) val * 1.0 which will be executed at runtime on the atmega. Otherwise floating point operations are supported by avr-gcc but using fixed-point operations is better.

[regomodo]

That's what I was doing initially. I had 2 vars, v&temp, as type float. Those were used for calculations then casted as an uint16_t. You can see from the commented out sections within analogToTemp() that I was trying to get anything to work.

[avra]

You are wrong with float<=>int conversion and casting. Drop the whole project, make new test one and use simple basic conversion and casting commands with step by step debugging in AVR Studio and you will find your point of misunderstanding the concept. You also have a wrong concept about Stackoverflow too. This is not a discussion forum, and people help for rewards. So far I don't see that you gave +1 to anyone. You don't have to, but I see that some people gave a lot of effort for nothing. Read the FAQ.

[regomodo]

I appreciate that SO uses a points based system but much like Reddit's karma it seems all a bit silly, especially once you've got basic permissions. Getting OT, i've already cut out any float-ops and scaled out my values to perform integer math. I think i'll keep my float-ops on my Arm projects.

[regomodo]

Thanks for having a look. Just out of interest, what is all the assignments & shifts of x(a/b/c)l's for?

[old_timer]

was taking snapshots of the 16 bit timer, then at the end combined the two 8 bit parts into a 16 bit count.

[old_timer]

I am not here for the points, here to help folks out sharing what I know and learning from what others know. If the problem requires then a discussion can be useful, maybe at the end it all gets edited into a concise question and answer for long term preservation. Sometimes the poster needs as much help asking their question as they do with the question itself and that can require some back and forth. The ability to add comments under questions or answers would not be here if discussion was not allowed.

[old_timer]

I was using the timer to time the difference between the float solution and fixed point solution for that particular multiplier, which is where the 157 times longer to use float came from.